diff options
Diffstat (limited to 'docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md')
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 417 |
1 files changed, 340 insertions, 77 deletions
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 7b44e7e..944dfee 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 @@ -131,8 +131,8 @@ taking that too literally. | 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 | **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. **One case unverified by hand:** the notes also report a single-message `id:` query whose card would not open, which is the same empty-`MessageIdRole` failure and should be gone; confirmed 2026-08-15 as a SEPARATE defect with a different cause, see item 96 | -| 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 | +| 87 | Auto mark-read marks a whole thread, including replies never displayed | defect | S | **done** 2026-08-16, unreleased. Built on 108, which is why it stayed small: the timer tracks a MESSAGE id now, and arms for a reply too, which it never did before | +| 88 | `threadAt(current.row())` answers about the wrong thread for a reply row | defect | M | **done** 2026-08-16, unreleased. The audit found FOUR live sites, not one. `ThreadListModel::threadFor(index)` resolves a reply through its parent; every caller holding a selected index converted, and no `.row()` on a selected index remains in `mainwindow.cpp`. Unblocks 87 | | 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 | @@ -162,6 +162,20 @@ taking that too literally. | 94 | `pinned` has nothing left to decide once the buttons are built-in | maintenance | S | open; **blocked on 93**, and deliberately not part of it. A user-visible removal: the row becomes built-ins only and every saved query lives in the menu | | 96 | A query returning the thread already on display opens onto the placeholder | defect | S | **done** 2026-08-15, unreleased. Split from 66's unverified half, which had a different cause. Reproduced from two screenshots after four measured eliminations | | 97 | An edit made during a sync is reverted in the list when the sync ends | defect | S | **done** 2026-08-15, unreleased. Found by hand-testing item 89's fix. The sync-end refresh ran BEFORE the held-edit flush, so it read a database that still carried the old tag | +| 98 | "Important" adds the tag but cannot remove it, unlike every other toggle | defect | XS | open, found 2026-08-16 in the notes reconciliation | +| 99 | The unread action is labelled "Toggle unread" whichever way it will go | presentation | S | open; depends on 98's toggle shape, and the label is harder than it looks | +| 100 | The message pane offers Back, Forward, Reload and Save page, none of which mean anything | defect | XS | open, found 2026-08-16. Chromium's standard menu is added wholesale | +| 101 | Sync is account-aware for edits but not for the account the user is looking at | workflow | S | open; item 49 built the edit half deliberately. Needs a decision, see the entry | +| 102 | The rules table shows no note, so the field explaining a rule is invisible until it is opened | workflow | XS | open, found 2026-08-16 | +| 103 | What Delete does to mail on the server is undocumented and unverified | clarification | S | open; a question first, possibly no code at all | +| 104 | Mail visible in Thunderbird never reaches qtmaildir | defect | ? | open, reported 2026-08-16, cause NOT established. Most likely outside this repo; see the entry before writing code | +| 109 | A root card's own message is invisible to a message-scoped write | defect | S | **done** 2026-08-16, unreleased. Found by hand-testing 108. `applyMessageTagChange` and `messageById` searched only the loaded replies, and a root's message is never among them, so the ORDINARY gesture repainted nothing and wiped the pane's chip row | +| 110 | A card and the message pane show tags belonging to a message's siblings | defect | S | **done** 2026-08-16, unreleased. Found by hand-testing 109 against a real 4-message thread. `ThreadSummary::tags` is notmuch's UNION; a card standing for one message drew it. Also the reason a root card could not repaint at all | +| 111 | A card should show its siblings' tags smaller, not drop them | presentation | S | **done** 2026-08-16, unreleased. The user's own design, from looking at 110's result: own tags full size, the thread's others smaller and muted, so nothing appears to vanish on selection | +| 105 | Acting on a reply changes the counter and nothing on screen | defect | M | **done** 2026-08-16, unreleased. Found by hand-testing 88, and took three passes. FOUR causes: no optimistic update for a message-scoped write, no doomed cue on a reply row, both toggles reading the reply's THREAD state so they were one-way, and the message pane's strip not following a message edit. Also bolds an unread reply, at the user's request | +| 106 | A tag change made on one message during a sync is silently lost | defect | XS | **done** 2026-08-16, unreleased. Found by READING while fixing 105, never reported. `flushHeldEdits` re-sent only thread-scoped edits, so a message-scoped one was shown, counted as pending, and never written | +| 107 | A thread-scoped write leaves the loaded replies showing their old tags | defect | XS | **done** 2026-08-16, unreleased. `applyTagChange` updated the summary only, so marking a thread read left its expanded replies bold | +| 108 | Acting on a thread root means the whole thread, though it displays one message | workflow | M | **done** 2026-08-16, unreleased. `messageScopeFor()` beside `scopeFor()`; five `*_thread` actions in a "Whole thread" submenu on `Ctrl+Alt+<key>`. User-visible: minor bump, `### Upgrading` written | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -255,81 +269,6 @@ recorded. **Size: `?`, unspecified.** Do not propose a design for this; ask. -## 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 **Observed (user, from the notes):** "passed tag should appear when subject is @@ -494,6 +433,330 @@ bump either way: an ignored optional field is not a breaking change. **Size: S.** Removing a field, two UI affordances and their tests. +## 98. "Important" adds the tag but cannot remove it, unlike every other toggle + +**Observed (user, from the notes):** "the add 'Important' action should be a +toggle (like unread)." + +**Cause (verified in the code).** `src/mainwindow.cpp:863` registers `flag` as a +one-way add: + +```cpp +addAction(QStringLiteral("flag"), tr("&Important"), + tr("Mark the selected threads as important"), [this]() { + tagSelected({ QStringLiteral("flagged") }, {}, tr("Mark important")); +}); +``` + +Adding a tag that is already there is a no-op the user cannot see, so pressing +the key or the button on an already-important thread appears to do nothing at +all. Nothing in the UI removes `flagged` except the general tag dialog. + +**The two neighbouring actions are already toggles**, so this is an +inconsistency rather than a missing feature. `delete` +(`src/mainwindow.cpp:825`) and `toggle_unread` (`:867`) both read the current +state and choose a direction, and `delete`'s comment states the rule this should +follow: one direction for the WHOLE selection, flipping only when every selected +thread is already in the target state, because a single keystroke that leaves a +selection in two states is worse than either outcome. + +**Everything needed is already loaded.** `ThreadSummary::isFlagged()` +(`src/types.h:64`) reads the tag off the summary, so the direction can be +decided without a worker round trip, exactly as `isDeleted()` is. + +**Approach, now a two-line change.** Item 105 extracted +`MainWindow::everySelectedRowHasTag()`, which is the whole of the direction +logic: +`everySelectedRowHasTag("flagged") ? tagSelected({}, {"flagged"}, tr("Unmark important")) : the current add`. +The undo stack needs nothing new, since `TagChange::inverted()` already covers +both directions. + +**Constraints.** + +- Call `everySelectedRowHasTag()`, never a hand-rolled loop. Two separate fixes + went into that logic on 2026-08-16 (items 88 and 105) and both were bugs a + copy of the then-current `delete` loop would have inherited: resolving a + reply's row number to the wrong thread, and asking a reply's thread instead + of the reply. +- The action's tooltip says "Mark the selected threads as important" and would + become wrong. Item 99 is the same problem for `toggle_unread` and the two + should be decided together. +- The label question belongs to item 99, not here. This item is the behaviour + only: the key stops being a no-op. + +**Size: XS.** + +## 99. The unread action is labelled "Toggle unread" whichever way it will go + +**Observed (user, from the notes):** "the label for 'toggle unread' should be +dynamic: on an 'unread' message it should be 'Mark as read', on a 'read' +message it should be 'Mark as unread'." + +**Cause (verified in the code).** `src/mainwindow.cpp:867` registers one static +label, `tr("Toggle &unread")`, and the lambda decides the direction at +invocation time from the current row. The action carries that text in three +places at once: the Message menu (`:1060`), the thread context menu (`:1167`) +and the toolbar (`:1122`, with the `mail-mark-unread` icon). Nothing updates it +when the selection changes. + +**Not as simple as reading the current row**, which is why this is S and not XS. + +- The action applies to the WHOLE selection and picks one direction from the + current row, so with a mixed selection any label naming a single outcome is + either wrong for some rows or has to describe the rule ("Mark all as read"). +- A menu action's text is read when the menu opens, but a TOOLBAR button's text + is on screen continuously, so it has to track `selectionChanged` rather than + being computed at popup time. `currentRowChanged` is the wrong signal for + anything selection-shaped, per `CLAUDE.md`. +- The accelerator is inside the word (`Toggle &unread`). Two different labels + need two accelerators chosen so neither collides in the Message menu, which + already holds "Mark &spam" and "&Important". +- The shortcut list (Help > Keyboard shortcuts) and the config's `[keys]` + section both name the action `toggle_unread`. The action NAME must not change + with the label, or every user's config breaks. Same rule as item 57, which + changed "Flag" to "Important" on screen and left the action and tag alone. + +**Approach.** Compute the label from the same state the lambda already uses, +which since item 105 is `MainWindow::everySelectedRowHasTag("unread")`, update +it on `selectionChanged`, and keep a neutral fallback for an empty or mixed +selection. Decide with item 98, which raises the identical question for +"Important". + +**Use that helper rather than re-deriving the state**, or the label and the +action can disagree. It already encodes the two things this gets wrong on its +own: a reply answers about its MESSAGE, not its thread, and the answer is over +the whole selection rather than the current row. + +**Constraints.** Every label is user-facing and needs `tr()`. Since the strings +are chosen at runtime rather than written once, all of them must exist as +literals `lupdate` can see; a string built by concatenation is not translatable. +`ctest -R translations` is the check. + +**Size: S.** Mostly the mixed-selection and toolbar decisions, not the code. + +## 100. The message pane offers Back, Forward, Reload and Save page, none of which mean anything + +**Observed (user, from the notes):** "back/forward/save page in the right pane +don't make sense, shouldn't be visible." + +**Cause (verified in the code).** `MessageView::showBodyContextMenu` +(`src/messageview.cpp:619`) starts from Chromium's own menu: + +```cpp +QMenu *menu = m_view->createStandardContextMenu(); +``` + +That menu is built for a browser and carries the navigation and page actions +whole. The pane is not a browser: every document arrives through `setHtml()` +with a fixed base URL, so there is no history to go back to, nothing to reload +from, and the request interceptor blocks everything by default anyway. The +entries are inert as well as meaningless. + +**Deliberate as far as it goes.** The comment above the call says the page's own +menu comes first so "copy, select all and the rest stay exactly as they were", +which is right for the editing actions and wrong for the navigation ones. The +item is that the filter was never applied, not that the base menu was a mistake. + +**Approach.** Keep the menu, drop the actions that cannot apply. Qt names them +as `QWebEnginePage::WebAction` values (`Back`, `Forward`, `Reload`, +`SavePage`, and `ViewSource` is worth the same look), and each has a +`pageAction()` whose pointer can be matched against the standard menu's entries +and removed. Removing by matching the action pointer is safer than matching by +text, which is translated. + +**Constraints.** + +- Do not rebuild the menu from scratch. Copy, Copy link address and Select all + are the reason the standard menu is used, and item 85's search entries are + appended to it. +- `Save page` is not the attachment save. Attachments have their own bar and + their own path-traversal checks (see the web view security notes in + `CLAUDE.md`); nothing here should grow a second way to write a file. +- Verify against a real right-click on a real message. The offscreen platform + builds the menu but a screenshot of it proves nothing, and the entry list + depends on what the page reports as available at that moment. + +**Size: XS.** + +## 101. Sync is account-aware for edits but not for the account the user is looking at + +**Observed (user, from the notes):** "sync button should be account-aware." + +**Cause (verified in the code).** `MainWindow::pendingSyncChannels()` +(`src/mainwindow.cpp:3550`) resolves channels from `m_editedAccounts`, the set of +accounts the user has made EDITS in, and from nothing else. The account dropdown +is not consulted. With nothing pending it returns empty on purpose, and +`mailsync.sh` turns that into `mbsync -a`, every channel. + +**Item 49 built exactly this and the reasoning still holds.** The comment states +it: with nothing pending the run is a FETCH, and narrowing a fetch to wherever +the last edit happened would "quietly stop collecting mail everywhere else". +Fetching is global by nature; carrying edits is not. + +**So this needs a decision, not a fix.** The note does not say which of two +things the user means, and they are different features: + +*Sync only the selected account, on demand.* A deliberate "sync this account" +that ignores the pending set, presumably beside the existing Sync rather than +replacing it. Useful when one account is slow and the user wants their mail from +another one now. The risk is the one item 49 named: a button that looks like +Sync and quietly does not collect the rest of the mail. + +*Show which accounts a sync will cover.* No behaviour change at all, just making +the existing account-awareness visible, since today the user cannot tell whether +a run is narrowed or full. The status bar already names each channel as mbsync +reaches it (item 42), so most of this exists. + +**Constraints.** + +- The account dropdown is a VIEW filter. Making it also steer sync couples two + things the user may reasonably want apart: looking at one account while + fetching all of them is the normal case, not an edge case. +- Whatever narrows a run must still carry every pending edit, or an edit is + stranded with nothing on screen to say so. `pendingSyncChannels()` already + falls back to a full sync when it cannot resolve a channel for an edited + account, and that safety must survive. +- An account with no `[account.<key>]` section, or one whose section names no + channel, has no channel to sync. The fallback covers it today. + +**Size: S** for the on-demand button, XS for the visibility half. Ask which. + +## 102. The rules table shows no note, so the field explaining a rule is invisible until it is opened + +**Observed (user, from the notes):** "add 'notes' column to the filters table." + +**Cause (verified in the code).** `TagRule` carries a `note` field +(`src/tagrules.h:37`, "Why the rule is shaped this way. Shown in the dialog"), +and the editor below the table edits it, but the table itself lists five columns +and none of them is the note (`src/tagrulesdialog.cpp:116`): + +```cpp +m_list->setHeaderLabels({ tr("On"), tr("Stage"), tr("Rule"), tr("Tags"), + tr("Matches") }); +``` + +So the one field written specifically to explain a rule can only be read one rule +at a time, by selecting it. With several rules the note is exactly the thing that +would let the user pick the right one without opening each. + +**Approach.** A sixth column. The column widths already persist (item 75), so a +new column needs a sensible default width and nothing else in the way of state. + +**Constraints.** + +- The note is free text of any length and would stretch the column. Elide it and + put the full text in the tooltip; the `Rule` column already faces the same + problem with a long query and is the pattern to match. +- `ColumnCount + 1` in `setColumnCount` is load-bearing: the enum drives the + column indices and there is a spare. Add the enum value rather than hardcoding + 5, and check every place that indexes a column by number. +- Notes are the user's own words and can be empty. An empty cell is correct + here; do not substitute a placeholder. + +**Size: XS.** + +## 103. What Delete does to mail on the server is undocumented and unverified + +**Observed (user, from the notes):** "verify how 'delete' works", with two +sub-questions of their own: "trash bin (?)" and "delete from server (?)". + +**This is a question first.** The user is not reporting a defect; they are saying +they do not know what the button does to their mail, which for a destructive +action is its own problem regardless of the answer. + +**What the code does (verified).** `src/mainwindow.cpp:825` adds and removes the +`deleted` tag, and nothing else. It is a toggle, it goes through the undo stack, +and it writes a notmuch tag. + +**What that means downstream is what needs verifying, and it is NOT in this +repo.** `maildir.synchronize_flags` is true, so notmuch maps certain tags to +Maildir filename flags, and mbsync carries filename flags to the server. Whether +`deleted` is one of those, whether the user's `~/.mbsyncrc` has `Expunge Both` +(it does, on every channel), and what each provider does with a message flagged +deleted, together decide whether this button is reversible. The undo stack makes +the TAG reversible; it says nothing about what a sync did with it in between. + +**Approach.** Measure before designing anything: the notmuch config's +`maildir.synchronize_flags` and its tag-to-flag mapping, one real message tagged +and synced in a test account, and what the server shows afterwards. Then decide +whether the UI needs to say what it does, whether "Delete" is even the right +word for it, and whether a trash view is wanted. + +**Constraints.** + +- **This is the one place the no-confirmation rule should be re-examined rather + than assumed.** `CLAUDE.md` records that a human at a GUI gets undo instead of + confirmation dialogs, and that is right for tags. If the measurement shows the + next sync expunges mail from the server, then undo does not in fact cover this + action, and the premise the rule rests on does not hold for it. +- Do not test this against the user's real accounts. A message that is expunged + to prove that it is expunged is still gone. +- Any answer that involves a trash view is a much larger item and should be + split out rather than folded in here. + +**Size: S** for the investigation and whatever the UI needs to say. Unknown +beyond that, and deliberately not sized further until the measurement exists. + +## 104. Mail visible in Thunderbird never reaches qtmaildir + +**Observed (user, from the notes):** "sync doesn't work compared to thunderbird. +New mail received on thunderbird did not appear in qtmaildir. Need to investigate +further." + +**Cause: NOT established.** Recorded because it is a defect report about mail +going missing, which is the most serious kind this backlog carries, and it has +been sitting in the notes unrecorded. What follows is one measured mechanism that +would produce exactly this symptom, not a diagnosis. + +**qtmaildir cannot show what mbsync did not fetch, and mbsync fetches folders by +pattern.** Three of the five channels in the user's `~/.mbsyncrc` name their +folders explicitly: + +``` +Patterns "INBOX" "[Gmail]/Posta inviata" "[Gmail]/Bozze" "[Gmail]/Speciali" +``` + +and one names only `"INBOX"`. The two non-Gmail channels use `Patterns *`. +Gmail applies labels, and a message whose label is not one of those four is in a +folder mbsync never asks for. Thunderbird speaks IMAP directly and sees every +folder, so the same message is visible there and absent locally. This is a +configuration property of the user's mbsyncrc, outside this repository entirely. + +**One inconsistency worth reporting regardless**, found while checking the +above: one of the Gmail accounts is configured in `qtmaildir.conf` with +`sent = [Gmail]/Posta inviata` and `drafts = [Gmail]/Bozze`, while its mbsync +channel has `Patterns "INBOX"` and fetches neither. The Sent and Drafts filters +for that account can therefore only ever be empty. That is real, and it is +independent of whatever this item turns out to be. + +**Approach.** Reproduce before anything else, and the reproduction has to +distinguish three layers, because the fix lives in a different place for each: + +1. Is the message on disk? `find` in the Maildir, or `notmuch count` on a term + from it. If not, this is mbsync or `.mbsyncrc`, and there is nothing to + change here. +2. If it is on disk, is it indexed? `notmuch new` and count again. If not, this + is notmuch config, `new.ignore` or the hook. +3. Only if it is indexed and still not shown is this qtmaildir's defect, and + then the question is which query hid it: the account scope, the built-in + filter, or a rule that tagged it out of the inbox. + +**Constraints.** + +- Ask the user for one concrete example before investigating: which account, + roughly when, and what Thunderbird shows for it. A general "sync doesn't work" + cannot be reproduced, and the last four defects in this backlog were all found + from a specific message. +- The `post-new` hook from mailctl tags mail unattended. A rule that removes + `inbox` would make a correctly fetched, correctly indexed message vanish from + the default view, which looks identical to a sync failure from the outside. + `notmuch search` without a filter is what tells them apart. +- Do not change `.mbsyncrc` as part of this. It is the user's, it is outside the + repo, and a Patterns change refetches folders. + +**Size: `?`** until reproduced. Most likely not a code change here at all. + + ## Deferred, unsized, or split out Items noted while triaging but not part of the original list. Same numbering |