diff options
| -rw-r--r-- | CHANGELOG.md | 24 | ||||
| -rw-r--r-- | CLAUDE.md | 31 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md | 47 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 140 |
4 files changed, 175 insertions, 67 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index ec08727..e869346 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,17 @@ point at which they are stable. nothing to exclude from and running it would mean the whole Maildir minus one value. +### Removed + +- **The conversation view.** Selecting a thread used to render the whole + conversation in the message pane, earlier messages as unexpandable stubs with + the last two opened. Selecting any row, a thread root or a reply, now renders + exactly one message. + + It was also inconsistent: a thread root rendered the conversation only until + the thread had been expanded once, after which the identical click rendered a + single message. Read a thread by expanding it and walking the reply rows. + ### Changed - The dialog reporting configuration problems at startup now appears over the @@ -30,6 +41,19 @@ point at which they are stable. unchanged: a keybinding that is being ignored does, a notice such as "no sync command configured" does not. +### Upgrading + +Reading a thread now means expanding it and clicking down its replies. Nothing +in your config changes, and no habit built on the reply rows is affected, but +the pane will show one message where it used to show a conversation. + +Automatic mark-read still applies to the **whole thread**, so opening a thread +root marks its replies read as well, including ones you have not displayed. +That was consistent while a root rendered the conversation and is not any more. +Set `mark_read_delay_ms` to a negative value under `[general]` to turn the +behaviour off entirely if that matters to you; narrowing it to one message is +open work. + ## [0.20.0] - 2026-08-14 Anything on screen in the message pane can now be searched for by right-clicking @@ -398,6 +398,26 @@ standalone program containing none of this project's code. A size assertion there passes against both, and a mutation putting the bug back leaves the suite green. Assert on the stored value, and leave the frame to a hand test. +**`ThreadListModel::threadAt(int)` takes a ROW and is wrong for any index that +might be a reply.** A tree numbers rows per parent, so a reply's `row()` indexes +its siblings and `threadAt(current.row())` on the first reply of any thread +returns the FIRST THREAD IN THE LIST. This shipped in `markCurrentThreadRead`, +was mostly masked while the write it guarded was thread-wide, and became "a +random message was marked read" the moment a fix scoped that write to one +message (items 87 and 88). Reach the thread through the INDEX, never through a +row number, unless you have already established the index is a thread row. + +The general rule is in the item 20 note further down, "anything keyed on a row +NUMBER did not survive the port to a tree". This is a surviving instance, found +by corrupting real mail rather than by reading, so treat every remaining +`.row()` in `mainwindow.cpp` as suspect until checked. + +**A test for a mutation on a data-writing path must exercise the REPLY case, +not only the root.** The reverted fix above was mutation-checked and green: it +asserted on a root selection, which is the one case where `current.row()` is +correct. A green mutation check proves the test can fail, not that it covers the +case that matters. + **`test_mainwindow` can now drive a real worker, and three things about it will waste a session each.** `WorkerBackedWindow` builds a throwaway notmuch database and writes a `qtmaildir.conf` pointing at it; `wireWorker()` reads @@ -487,10 +507,13 @@ loosen any of these without an explicit decision. scheme-wide allow would let a hostile body reference `qtmaildir://anything` and be trusted. Consequence: `MessageView` **must** call `setDocumentUrl()` with the same URL it gives `setHtml()`, or nothing renders. -- A whole thread is one HTML document in one web view (a `QWebEngineView` per message would - spawn a Chromium render process each). That makes `cid:` ids collide across messages, so - every reference is rewritten to `cid:<prefix>!<id>`. **A `cidPrefix` must never contain - `!`** — it is the namespace separator. +- The pane renders a LIST of messages into one HTML document in one web view (a + `QWebEngineView` per message would spawn a Chromium render process each). That makes `cid:` + ids collide across messages, so every reference is rewritten to `cid:<prefix>!<id>`. + **A `cidPrefix` must never contain `!`** — it is the namespace separator. + Since item 66 removed the conversation view every caller passes exactly ONE message, so the + collision cannot currently arise; the prefixing stays because the list-rendering path does, + and a security property must not rest on every caller happening to pass one item. - Remote content grants are per-render and never sticky. - **Attachment filenames are untrusted input.** Reduce to basename, strip separators, resolve against the chosen directory, and refuse anything escaping it. Compare resolved paths as 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 ed88693..ff2bbba 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 @@ -4937,3 +4937,50 @@ means something. **Item 66 did not reproduce**, which is recorded in that item rather than worked around here. The test was not widened until it failed; the negative result and what it narrows to are in item 66's own entry. + +## 66. Selecting a thread root leaves the message pane blank until a reply has been selected + +**Done 2026-08-14, unreleased.** Closed by removing the conversation view, at +the user's request. Not the defect it was filed as. + +**What it actually was.** The original note described a blank pane. Two probes +against a real database failed to reproduce that, and the user's own step-by-step +account on 2026-08-14 showed the pane was never blank: clicking an unexpanded +thread root rendered the whole CONVERSATION, stubs plus the last two messages +expanded. Clicking a reply then rendered one message, and clicking back to the +root rendered one message from then on. + +**The cause was timing, not a race.** `onThreadSelected` already preferred to +load the root's own first message, but the model learned that message id only +when the replies arrived (`ThreadNode::first`, populated in the +`threadTreeLoaded` handler). A row that had never been expanded therefore had no +id and fell through to a whole-thread render. The identical click behaved +differently once the thread had been opened, which is what the user was +reporting as inconsistency. + +**Fixed by carrying the id in the query.** `ThreadSummary::firstMessageId` comes +from `notmuch_thread_get_toplevel_messages` during the query walk, so it is +known before any expansion and the fallback is unreachable. Free: measured +indistinguishable from not collecting it over a 36,615-thread database, because +it reads the index rather than the message files. The Sent view takes the first +message the query MATCHED instead, since a Sent row stands for what the user +sent and not for whoever opened the thread; libnotmuch has no matched-messages +iterator, only a count, so that branch walks to the first +`NOTMUCH_MESSAGE_FLAG_MATCH` and stops. + +**The conversation view is gone, on the user's decision.** Asked whether to keep +it reachable another way, the user declined: "I have no use for a view that +shows the first n messages as stubs I can't expand and lets me see only the last +two replies." Worth recording that the stubs not expanding was itself a defect, +so the feature was judged in a broken state; the user chose removal anyway when +told. `NotmuchWorker::loadThread` survives with no UI caller, documented as such, +because it is a tested way to read a thread's messages with the match set +resolved. + +**Two defects came out of this and are open as items 87 and 88.** Auto mark-read +still marks the whole thread, which was coherent while a root rendered the +conversation and is not any more; and `threadAt(current.row())` answers about +the wrong thread for a reply row. The second was found the hard way: a fix for +87 was written, mutation-checked, shipped and reverted the same evening after it +marked an unrelated message read. The test that passed had asserted on a ROOT +selection, the one case where `current.row()` is correct. diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md index 2594890..63c192c 100644 --- a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md +++ b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md @@ -130,7 +130,9 @@ taking that too literally. | 63 | No way to see sent mail, and no filter for it | workflow | M | **done** 2026-08-11; see `specs/2026-08-11-sent-mail-design.md` | | 64 | The Sync button carries a mailbox icon, not a refresh one | presentation | XS | **done** 2026-08-11 | | 65 | No full code review and optimization pass | correctness | ? | open, unspecified | -| 66 | Selecting a thread root leaves the message pane blank until a reply has been selected | defect | S | open; the simple case is RULED OUT, measured 2026-08-14 with the item 36 fixture. Needs a condition nobody has named yet, see the section | +| 66 | Selecting a thread root leaves the message pane blank until a reply has been selected | defect | S | **done** 2026-08-14, unreleased. Not the blank pane it was filed as: the root rendered the CONVERSATION until the thread had been expanded once, then one message. Now always one message, and the conversation view is removed at the user's request | +| 87 | Auto mark-read marks a whole thread, including replies never displayed | defect | S | open; measured 2026-08-14. Reachable only after 66 removed the conversation view. Blocked on 88 | +| 88 | `threadAt(current.row())` answers about the wrong thread for a reply row | defect | S | open; found 2026-08-14 by shipping a fix that marked an unrelated message read. Row numbers are per parent in a tree | | 67 | The placeholder pane counts unread, flagged and inbox, but not sent or drafts | information | XS | **done** 2026-08-11, shipped in 0.15.0 | | 68 | A forwarded subject gets no `passed` tag | workflow | S | open; no subject rule exists, measured 2026-08-11. Decision needed: display mark (XS) or write the flag (S, syncs out) | | 69 | `passed` and `replied` read as words where every other state is a glyph | presentation | S | **done** 2026-08-11, inside item 70 | @@ -286,68 +288,80 @@ recorded. **Size: `?`, unspecified.** Do not propose a design for this; ask. -## 66. Selecting a thread root leaves the message pane blank until a reply has been selected - -**Observed (user, from the notes):** clicking the main message of a thread shows -nothing in the right pane. After expanding the thread and selecting a reply, -clicking the main message again renders it correctly. The user filed a second -observation as a possible duplicate: clicking the main message shows the whole -thread with stubs and the last two replies expanded, and after clicking a reply -and going back to the root, only the root's own message is shown. - -**Cause (partly verified, and the two halves are not the same fault).** - -The second half is not a defect at all. A thread row renders the conversation and -a message row renders one message; `onThreadSelected` (`src/mainwindow.cpp:1847`) -branches on `isMessageRow` for exactly that. What breaks the user's expectation is -that going back to the root should return to the conversation view, and it does -not always: `m_currentMessageId` is set by the message branch -(`src/mainwindow.cpp:1860`) and `onMessageLoaded` refuses to render when it is -empty (`src/mainwindow.cpp:1924`), so the two paths are ordered by which reply -lands first, not by which row is current. This is the "a queued load can outlive -the state that started it" class already recorded in CLAUDE.md. - -The first half, a blank pane on the very first click, has a candidate and only a -candidate: `onThreadSelected` returns without loading anything when the index is -not yet selected (`src/mainwindow.cpp:1804`), a guard added so that a view taking -focus does not open and mark-read a message the user never looked at. If a click -can deliver `currentRowChanged` before the selection model is updated, which -CLAUDE.md records as this signal's documented behaviour, that guard drops the -load and `onSelectionChanged` only reloads when the row CHANGED -(`src/mainwindow.cpp:1721`), which it did not. - -**Approach:** do not fix from this reading. The candidate above is exactly the -kind of plausible cause the project has been burned by twice; instrument the -running application and watch the order of `currentRowChanged`, -`selectionChanged` and the worker reply for one first click on a cold view. - -**Ruled out 2026-08-14: the simple case does not reproduce.** Item 36 shipped -the fixture this entry said was missing, and -`selectingAThreadRootShowsItInTheMessagePane` in `test_mainwindow` now builds a -real database, runs a real query and selects an unexpanded thread root with a -real `threadLoaded` arriving. The pane renders. The test is mutation-confirmed -to fail when `onThreadLoaded()` is disabled, so it is a probe with teeth rather -than a green that proves nothing. - -**What that leaves, and it sharpens this entry's own candidate.** The test uses -`setCurrentIndex()`, which updates the selection model SYNCHRONOUSLY. A real -mouse click does not, and the guard at `src/mainwindow.cpp:1804` returns without -loading when the index is not yet selected. So the negative result is consistent -with the candidate above rather than against it: it says the defect is in the -CLICK path, not in the load path. Reproducing it needs `QTest::mouseClick` on -the viewport at the row's `visualRect`, not a programmatic selection. - -Two other conditions remain unexcluded and are cheaper to try than a click -harness: a thread of more than two messages, and a cold page cache, which item -74 measures as seconds on this machine and which is exactly the window in which -an ordering fault would show. - -**Constraints:** the mark-read guard at 1804 must survive whatever is done, it is -item 35b's fix and removing it re-opens marking mail read without the user having -seen it. A fix that only re-orders the two guards will look correct and will still -be timing-dependent. - -**Size: S**, and a defect rather than an enhancement. +## 87. Auto mark-read marks a whole thread, including replies never displayed + +**Observed (user, 2026-08-14):** "with the first message in a thread selected +(not expanded), the 2s delay that marks it read applies to the whole thread, so +all answers are marked read as well." + +**Cause (verified in the code).** `markCurrentThreadRead` +(`src/mainwindow.cpp`) sends `sendThreadTagChange` with the thread id, so +`unread` is removed from every message in the thread. + +**This was coherent until item 66 and is not any more.** While a thread root +rendered the whole conversation, everything marked read HAD been displayed. +Item 66 made a root render one message; the thread-wide write stayed. + +**Not cosmetic.** `maildir.synchronize_flags` is on, so removing `unread` +rewrites Maildir filenames and the next sync carries it to the server. Mail the +user never opened stops being unread everywhere, and nothing here can put it +back except reading the messages again by hand. + +**Approach: blocked on item 88, and that ordering is the lesson.** The obvious +fix is to send `sendMessageTagChange` for the message on screen. That was +written, tested, mutation-checked, shipped and reverted the same evening, +because it exposed item 88: the guards above the write read +`threadAt(current.row())`, which answers about an unrelated thread when a reply +row is current, and a fix that narrows the write turns that mismatch into "a +random message is now read". Fix 88 first. + +**Constraints for the second attempt.** + +- **Test selecting a REPLY, not only a root.** The reverted work asserted on a + root selection, which is the one case where `current.row()` is correct, so a + mutation-checked test passed against a bug that corrupted mail. +- The card must repaint. `sendMessageTagChange` deliberately makes no + optimistic model update, since `applyTagChange` is thread-keyed; without a + message-scoped equivalent the write lands, the unsynced count rises, and the + card stays bold, which the user reported as a separate fault. +- A thread reads as unread while ANY message does, so the card can only stop + looking unread when nothing else in it carries the tag. For an unexpanded + thread the per-message tags are not loaded, so the honest answer needs + per-message counts in `ThreadSummary` or nothing at all. + +**Size: S**, after 88. + +## 88. `threadAt(current.row())` answers about the wrong thread for a reply row + +**Observed (user, 2026-08-14):** "I clicked on a message and another random +message was marked read." + +**Cause (verified).** `markCurrentThreadRead` calls +`m_model->threadAt(current.row())`. A `QTreeView` numbers rows PER PARENT, so a +reply's `row()` indexes its siblings: `threadAt(0)` on the first reply of any +thread returns the FIRST THREAD IN THE LIST. The guards then compare correct ids +against the wrong thread and let a write through. + +**`CLAUDE.md` already records this trap**, from item 20: "what did NOT survive +that port is anything keyed on a row NUMBER". This is a surviving instance, not +a new discovery, which is the uncomfortable part. + +**Mostly masked before item 87's attempted fix.** With a thread-wide write the +mismatch usually resolved to the same thread or to a no-op; scoping the write to +one message made it visible immediately, as unrelated mail being marked read. + +**Approach.** `threadAt(int)` takes a row and cannot be safe here. Every caller +that has a `QModelIndex` should reach the thread through the index: for a +message row, `ThreadListModel::threadIdForMessage()` or the parent index; for a +thread row, `index.row()` is correct. Audit every `threadAt(` and every +`.row()` in `mainwindow.cpp` rather than fixing this one site, since the same +shape almost certainly appears elsewhere. + +**Consider deleting the row-taking overload** once the callers are converted, so +the trap cannot be re-entered. That is the change that makes this permanent +rather than fixed once. + +**Size: S** for the one site, **M** if the audit finds several. ## 68. A forwarded subject gets no `passed` tag |