From 38b9466aea7daa0ddde91c1de7d8082ac000c238 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Fri, 14 Aug 2026 20:12:59 +0200 Subject: docs: close item 66, open 87 and 88, record the row-number trap Item 66 turns out not to have been the defect it was filed as. The pane was never blank: an unexpanded thread root rendered the CONVERSATION, and the same click rendered one message once the thread had been opened, because the model learned the root's message id only when the replies arrived. The user's step-by-step account is what separated the two halves; two probes against a real database had failed to reproduce the blank pane because there was none. Closed by carrying firstMessageId in the query and removing the conversation view, which the user asked for after being told the stubs not expanding was itself a defect and that the feature was being judged in a broken state. Two defects came out of it and are open. 87: auto mark-read still marks a whole thread, coherent while a root rendered the conversation and not any more. 88: threadAt(current.row()) answers about the wrong thread for a reply row, because a tree numbers rows per parent. 87 is blocked on 88 and the entry says why: a fix for 87 was written, mutation-checked, shipped and reverted the same evening after it marked an unrelated message read. CLAUDE.md gains the row-number trap as its own entry rather than leaving it implied by the item 20 note, plus the rule that a test for a write path must exercise the reply case: the reverted fix was green because it asserted on a root selection, the one case where row() is correct. The cid-prefixing note is corrected to say every caller now passes one message while explaining why the prefixing stays. The changelog carries a Removed entry and an Upgrading note, including that mark_read_delay_ms accepts a negative value to disable auto mark-read entirely, verified against config.h. Co-Authored-By: Claude Opus 5 --- .../2026-08-03-post-0.1.0-usability-closed.md | 47 +++++++ .../plans/2026-08-03-post-0.1.0-usability.md | 140 +++++++++++---------- 2 files changed, 124 insertions(+), 63 deletions(-) (limited to 'docs') 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 -- cgit v1.2.3