diff options
Diffstat (limited to 'docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md')
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md | 158 |
1 files changed, 158 insertions, 0 deletions
diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md index af1de0c..8de26f2 100644 --- a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md +++ b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md @@ -9941,3 +9941,161 @@ sorts. **Still open and separate:** the four autosave revisions of that reply sitting in `Drafts/` with distinct Message-IDs, which is item 165 and is what puts a `draft` chip on the conversation. + +--- + +## 165. A draft gets a new Message-ID on every autosave + +**Observed (developer, 2026-08-25), while hand-testing items 163 and 164.** +Four saved drafts produced four distinct Message-IDs, and one reopen-and-edit +turned one id into another. A draft therefore has no stable identity across +its own revisions. + +**Cause (verified in the code, not inferred).** `MessageBuilder::build()` +calls `g_mime_utils_generate_message_id()` unconditionally on every call +(`messagebuilder.cpp:311`), and every autosave calls `build()`. +`OutgoingMessage` has no field to carry an existing id in, and +`ComposeContext` has no field for the draft's OWN id either: it carries +`inReplyTo` and `references`, which are the ORIGINAL's id when replying, and +`ComposeContextBuilder::forDraft()` never reads the draft's Message-ID back +out of the file it parses. So this is not a changed call site; it needs a +field that does not exist yet, threaded from `forDraft()` through +`ComposeContext` and `OutgoingMessage` into `build()`. + +**Why it matters, and why it is NOT urgent.** To notmuch and to the server, +each revision is a different MESSAGE, not a new version of one. While the +file is replaced correctly this is invisible: one file in, one file out. It +becomes visible whenever a revision is NOT replaced, and item 163 is the +proof, where a stale path forked a draft into two files that were also two +messages and that nothing will ever collapse. Item 163's fix removes the +known way to reach that state; this entry is about the property that turned a +one-file mistake into a two-message one. + +An interrupted save is the remaining route: `DraftStore::write()` unlinks the +previous revision only AFTER the new file is safely in place (deliberately, +so a failed write cannot lose the draft), so a crash between the two leaves +two files, and with two ids they are two drafts rather than one duplicated. + +**Decided by the user on 2026-09-06: a stable id while drafting, DISCARDED at +send.** The second of the three positions below. A draft keeps one identity +across all its revisions, so each save replaces the previous message rather +than adding one; the sent copy is minted fresh and is a different item from +the draft, which is the user's own framing. The draft is DELETED on a +successful send, as it already is today, and the sent message carries no +`draft` tag. + +The three positions, kept because the reasoning is what makes the choice +reviewable: + +- A stable id reused at send time makes the draft and the sent message one + message, which is what a user means by "my draft became this email". It + also means the id was in a file mbsync uploaded to the drafts folder before + the message was ever sent, and the server has seen it. **Not chosen.** +- A stable id DISCARDED at send time keeps revisions collapsed while drafting + and mints a fresh id for the sent copy. Two identities, and the sent one is + the one that threads. **Chosen.** +- The current behaviour is a third position, and its only virtue is that no + id is ever reused for two different things. + +Whichever is chosen must be checked against `In-Reply-To`/`References` on the +eventual send, since `referencesForReply()` builds those from the ORIGINAL's +id and a draft of a reply carries both. + +**What is already built, measured 2026-09-06 rather than assumed.** Two of the +three things the decision describes exist: + +- `DraftStore::write()` removes the previous revision after the new file is in + place (`draftstore.cpp:78`), so "discard what is left after a successful + save" is done LOCALLY. +- `ComposeWindow` removes the draft on a successful send + (`composewindow.cpp:1620`, through `resolveRenamed()` since item 163), so + "delete the draft on send" is done, and the user confirmed that is the + wanted behaviour rather than keeping the last revision. +- The sent copy is built fresh and filed to the Sent folder, so it never + carries `draft`. The tag the user saw on a Sent row came from the orphan + revisions threading into the conversation, not from the sent message. **No + work is needed for that clause**; it was already true. + +**So the whole remaining defect is the id, and the mechanism is the server.** +Measured on the user's own mail: four drafts of one reply, four distinct +Message-IDs, and every one of the four files carrying a `,U=` infix. mbsync +had uploaded each revision to the drafts folder BEFORE the next save removed +the local file, so the server holds four independent messages and syncs all +four back down. The local cleanup is correct and cannot help: the damage +happens between a save and the next sync, and deleting a local file does not +retract an uploaded one. + +This is what raises the item above "invisible while the file is replaced +correctly". The file IS replaced correctly and four messages still exist. +A stable id gives the server one message being revised rather than four +unrelated ones. + +**Constraints.** + +- **A Message-ID reaches the server and every recipient**, so a reused id is + not a local matter. Two different messages sharing an id is worse than two + ids for one draft, which is what makes the current behaviour defensible as + a default rather than simply wrong. +- `MessageBuilder::build()` is on the SEND path as well as the autosave path. + A change that makes ids stable must not make two different sent messages + share one. +- The comment at `messagebuilder.cpp:297` records that GMime generates + neither Date nor Message-ID unless asked, and that a message without one + cannot be threaded by anything receiving it, this application's own index + of the sent copy included. Any "just omit it while drafting" variant has to + answer that. +- Item 163's fix stands on its own and this does not block it: the file is + replaced correctly now, so the fork this would have mitigated no longer + happens by that route. + +**Built 2026-09-06.** Three links, each one a place the id had to travel and +each with its own test: + +- `OutgoingMessage::messageId` is the field that did not exist. Empty means + "mint a fresh one", so the SEND path is unchanged by construction rather + than by remembering to clear it. +- `MessageBuilder::build()` uses a supplied id when there is one and generates + otherwise. `g_mime_message_set_message_id` takes the id WITHOUT angle + brackets and adds them, which is the form `generate_message_id` returns, so + no unwrapping is needed. +- `ComposeWindow::m_draftMessageId` holds the identity between revisions, + assigned from `built.messageId` after a successful save so the FIRST save + adopts the id GMime just generated. `ComposeContext::draftMessageId` carries + it across a reopen, read in `forDraft()` from `ParsedMessage::messageId`, + which the parser already provided and nothing had ever used. + +**One comment had to be corrected rather than left alone**, and it is the kind +that rots into a false claim: the autosave's dirty check explains why it +compares the MESSAGE rather than the built bytes, and its reason was "GMime is +given a fresh Date and Message-ID on every build". Half of that is no longer +true. The Date still is, so the conclusion stands and only the reasoning +needed fixing. + +**Tests, and why there are four.** The property spans three objects, and a test +at any one of them passes while another link is broken: + +- `aSuppliedMessageIdIsUsedRatherThanAFreshOne` builds TWICE, because a single + build cannot distinguish a reused id from a freshly generated one. +- `twoBuildsWithNoSuppliedIdStillDiffer` is the safety constraint: a field + defaulting to some fixed value would satisfy the test above and make two + sent messages share an id. +- `everyRevisionOfADraftKeepsOneMessageId` is the end-to-end property, over + the real files. It changes the body between saves deliberately: without a + real change the dirty check short-circuits, no second file is written, and + the test would pass while proving nothing. +- `aResumedDraftKeepsTheMessageIdItWasSavedUnder` covers the reopen, which the + composer-only tests cannot reach, and asserts on the file written AFTER the + reopen rather than only on the context, since the context carrying the id is + half a chain. +- `aSentMessageDoesNotInheritTheDraftsMessageId` pins the constraint at the + place a future edit would break it, using the already-public + `currentMessage()` rather than adding a test accessor. + +Both wiring changes were mutation-checked: removing the autosave's assignment +fails the revision test, removing `forDraft()`'s read fails the reopen test. + +**What this does NOT do, deliberately.** It does not clean up revisions already +on the server. Those are existing messages in the drafts folder, and nothing +here retracts them; the four found on the user's own mail were deleted by hand +as a separate act. + |