diff options
Diffstat (limited to 'docs/superpowers/plans')
| -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 |
2 files changed, 124 insertions, 63 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 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 |