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 | 134 |
1 files changed, 134 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 26f90fa..af1de0c 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 @@ -9807,3 +9807,137 @@ where it is unless the user overrules the reasoning above. have their own. **Closed 2026-08-29**, unreleased. Star and Archive added to the ordinary branch; Archive left the main toolbar; `mark_all_read` stays there by the user's decision, and the assertion that it does is now in the suite. + +--- + +## 191. The Sent view collapses two messages you sent in one conversation into one row + +**Observed (user, 2026-09-06, by hand against real mail).** The Sent view for +one account showed a row dated 12:42 today whose subject was right, but whose +message pane rendered a message from three weeks earlier. The message actually +sent at 12:42 appeared nowhere in the view. Thunderbird, pointed at the same +IMAP folder, showed both. The user's own statement of what the view owes them +settled the fix: "the sent view is for my sent messages, if I sent 2 messages in +a thread, I want to see both, not threaded (as the view is not threaded), +correctly tagged and dated." + +**Two wrong diagnoses came first and are worth recording**, because both were +plausible and both were contradicted by measurement. The first read the `draft` +chip on the row and blamed item 165; the chip is real and unrelated. The second +read the row's date, found the message in `Sent/`, and concluded the row was +correct and the user was comparing a thread count against a message count; the +counts do differ for that reason, but that was not what they were reporting. The +user's correction, that the pane opened a different message from the one the row +claimed, is what located the defect. **A row's date and the message its pane +opens are two separate reads, and a defect can sit precisely in the gap.** + +**Cause, verified in the code and against the real index.** +`NotmuchWorker::walkThreads()` (`notmuchworker.cpp`) is built on +`notmuch_query_search_threads`, so it emitted exactly one `ThreadSummary` per +thread. The Sent branch then chose ONE message to stand for that thread: + +``` +if (matched) { summary.firstMessageId = ...; break; } +``` + +`notmuch_thread_get_messages` walks oldest-first, so a thread the user had +replied to twice took their OLDEST reply and stopped. The row's `date` and +`subject` came from the thread (`notmuch_thread_get_newest_date`, +`notmuch_thread_get_subject`) while `firstMessageId` named that older message, +which is why the two disagreed on screen. + +Measured on the developer's database: the conversation reported +`[2/9]` — two matched messages of nine — and `notmuch search --output=messages +--sort=oldest-first` over the sent folder returned the 17 Aug message before the +6 Sep one. The view is already flat (`setFlatMode(m_sentView)`, +`mainwindow.cpp`, and `Config::generatorIsFlat` marks `sent` and `drafts`), so +the model was right about being unthreaded and only the worker was wrong. + +**This was also a data-safety defect, which the visible symptom hid.** +`firstMessagePath`, `firstMessageTags` and `firstMessageSender` were all read +from the same wrongly chosen message. `moveMessages` composes its destination +from that path, so Delete or Archive on such a row would have moved the OLDER +message's file, silently, and mbsync would have carried it to the server. The +row the user was looking at named a file they were not looking at. + +**Fix.** The Sent branch emits one `ThreadSummary` per matched message rather +than one per thread: it no longer breaks, and for each matched message it copies +the thread-wide summary and overrides the row's identity — +`firstMessageId`, `firstMessageTags`, `firstMessageSender`, `firstMessagePath`, +and now `date` and `subject`, read from the message with +`notmuch_message_get_date` and `notmuch_message_get_header`. Batching and the +`total` counter move inside that loop, and the thread iteration `continue`s +past the single-summary append below. + +`withRecipients` continues to select the branch, so Sent and Drafts both get +this and no new flag can disagree with the flat-mode flag: the two questions are +answered by one value, which is why the original branch keyed on it. + +**One consequence needed a second change, and it is the part that would have +broken quietly.** Two rows now share a `threadId`, and +`ThreadListModel::reconcile()` keyed its `QHash<QString,int> present` on exactly +that. Two rows mapping to one key means the second looks like a thread that has +vanished, so a sync would have dropped one of the user's sent messages from the +view again, by a different route. `ThreadListModel::rowKeyFor()` is the single +answer to "what makes a row unique": the message id in flat mode, the thread id +otherwise, falling back to the thread id when a flat row carries no message id. +All four keyed sites in `reconcile()` use it. + +**Verification.** Two tests in `test_notmuchworker.cpp`, over a new fixture +thread F (`f1` sent, `f2` received, `f3` sent) which is the shape the defect +needs and which no existing fixture had: + +- `aSentQueryEmitsOneRowPerMatchedMessage` asserts both ids are present, that + the two rows share a thread id (the property that made the reconcile change + necessary), and that the SAME messages under a non-Sent query still fold into + one row, so this is the flat branch's contract and not a change of meaning + for threaded views. +- `aSentRowCarriesItsOwnMessagesDateAndSubject` asserts each row's date and + subject are its own, which is the half the user saw first. + +`aSentQueryCarriesTheMatchedMessageNotTheThreadsFirst` was RETARGETED rather +than retired: its assertion still holds, but it filtered rows by the thread's +subject, and a flat row is now titled by its own message, so it matched nothing. +It filters on the message id instead and asserts the subject. + +Eight other failures were the new fixture's arithmetic, not the change: three +messages and one thread added, so hardcoded totals moved 5→6 threads and 6→9 +messages. + +**The ordering was a second defect under the same item, found by hand after +the first fix shipped to the user's screen.** With both rows present, the 17 +August one drew ABOVE the 6 September one. The cause is that +`notmuch_query_set_sort` is a THREAD sort: it orders the threads the walk +visits and says nothing about the messages inside one, so every row of a thread +inherits that thread's single position. Sorting each thread's own rows in place +is NOT enough either, and the fixture proved it: a message from another thread +dated between two of a thread's replies still cannot land between them. A flat +view is a list of messages, so `walkThreads()` collects flat rows in `flatRows` +and sorts the whole result once, by each row's own date, before emitting. +`std::stable_sort`, so rows sharing a timestamp keep the walk's order rather +than swapping between identical queries. + +`sentRowsAreOrderedByTheirOwnDate` covers it, in both sort directions and +across threads. **The cross-thread half of it passed for the wrong reason at +first**: fixture threads D, E and F happen not to interleave, so per-thread +sorting satisfied a whole-view order check. Fixture thread G exists purely to +break that, dated between thread F's two messages, and the assertion failed +the moment it was added. A mutation check confirms the within-thread half fails +without the sort. + +**One existing test had to be corrected rather than satisfied, and the +correction is a fact about notmuch worth keeping.** +`oldestFirstReversesTheOrder` asserted the two sort directions produce exactly +reversed lists. They do not: `NOTMUCH_SORT_OLDEST_FIRST` orders threads by +their OLDEST message while `NEWEST_FIRST` orders them by their NEWEST, so the +lists mirror each other only while no thread's date span contains another's. +Thread F starts before thread G and ends after it, which is the first fixture +data to violate that. The test keeps the assertion that does hold +(`oldest.first()` is `newest.last()`) and replaces the mirror with a +monotonicity check. It passed on master and fails here for a reason that is not +a regression: the fixture finally contains the shape that distinguishes the two +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. |