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 | 957 |
1 files changed, 957 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 2e941ec..7261bd9 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 @@ -7751,3 +7751,960 @@ new action and the existing ones gathered under it rather than duplicated. - **Item 160 unblocked this** on 2026-08-25: the status bar exists, so a manual save reports through `refreshDraftStatus()` like an autosave. Route `Ctrl+S` through `saveDraftNow()` and the reporting is already done. + +## 162. Delete fails while a sync is renaming the file underneath it + +**Observed (user, 2026-08-25):** deleting a draft reported `Cannot move +<file> to <account>/Trash`. + +**Cause (verified against the live Maildir, not read):** a stale path, and +neither Delete nor item 158 is at fault. + +1. The composer autosaves a draft as `<name>:2,D` and item 158 indexes it + under exactly that filename. +2. **mbsync uploads it and RENAMES it** to `<name>,U=<uid>:2,D`, recording the + server UID in the filename. +3. notmuch still holds the pre-`U=` name until that sync's `notmuch new` runs. +4. `moveMessages()` reads the filename from notmuch and calls + `QFile::rename()` on a path that no longer exists. It fails, the error is + emitted, and the message is skipped. + +Measured, in this order: `notmuch search --output=files` named a file that was +not on disk while `Background sync running...` was up, and the same query was +clean once the sync finished, with the file present under its new `,U=4` name. +That is why it reads as intermittent, and why it heals itself. + +**It is truthful and it loses nothing.** The move is skipped, no wrong folder +is created, no file is destroyed, and the next sync reconciles. The defect is +that the message blames a folder for a timing problem, and that the action +silently does nothing when the user asked for something. + +**This is `CLAUDE.md`'s `,U=` trap from the other side.** `MaildirName::fresh()` +exists because CARRYING that infix across a folder boundary produced +`Maildir error: duplicate UID` on real mail. Here mbsync is ADDING it and the +index lags; the same infix, the opposite direction. + +**Approach, and it needs a decision.** Two candidates: + +- **Refuse the move while a sync holds the lock.** `SyncMonitor` already + reports this, and the held-edit machinery from items 97 and 106 already + exists for exactly this shape: a tag edit made during a sync is held and + flushed when it ends. Delete would join it rather than inventing anything. + This is the likelier right answer, since it matches what every other + mutation already does. +- **Re-resolve the filename** from notmuch immediately before the rename and + re-query the message if the path is gone. Smaller, but it races the same + window it is trying to close, and a second lookup can be stale by the time + it is used. + +**Constraints.** + +- **Delete reaches the real mail server.** Read `CLAUDE.md`'s item 103 notes + before touching `moveMessages()`: a wrong folder name is created, adopted by + mbsync, and propagated to every other client. +- Whatever is built, **the message must say a sync is running**, not name a + folder. The current wording sent the user looking for a broken folder + configuration, which was correct and configured. +- A test cannot see this in the ordinary fixture layout, where nothing renames + a file underneath the index. Driving it means renaming the file between the + index write and the move, which is what the reproducer has to do. + +**Fixed 2026-08-25.** `moveMessages()` re-resolves by MESSAGE ID when the +recorded path is gone: one `reindexFolder()` of that directory, then the +filename from `notmuch_message_get_filenames()` that exists on disk. Bounded +to a single retry, so a genuinely missing file still reports rather than +becoming a silent no-op. + +**The hold candidate above was investigated and rejected**, and the reason +matters more than the fix. `sendMove()` ALREADY refuses while a sync holds the +lock and queues onto `m_heldMoves` (items 97 and 106 built it), yet the defect +still fired. `aSyncHoldsTheWriteLock()` tracks notmuch's write lock, while this +window sits between mbsync's RENAME and that sync's `notmuch new`; mbsync +renames throughout its run without touching that lock, so the damaging window +is open when there is nothing to observe. Refusing on the lock guards the wrong +resource. That refusal is correct for its own purpose and was left alone. + +Covered by `moveMessagesRecoversWhenASyncRenamedTheFile()` and +`moveMessagesStillReportsAMessageThatIsReallyGone()`, the second pinning the +bounded half. The test renames without reindexing, which is the window mbsync +opens, and asserts the database still names the old path so it cannot pass +against a fixture that quietly reindexed. Mutation-checked. + +## 163. The message pane shows a stale path, and the composer forks the draft + +**Observed (user, 2026-08-25):** selecting a draft filled the pane with +`(unreadable message)` and `This message could not be parsed.`, naming a file +under the account's drafts folder. + +**Cause (verified against the live Maildir):** the path in the pane ended +`.dnx:2,D`, and the only file on disk ended `.dnx,U=4:2,D`. Same mechanism as +item 162: mbsync renames an uploaded file to record its server UID, and the +name the application is holding stops existing. + +**The site is different, and so is the fix.** Item 162 is the WRITE path, +`moveMessages()` reading a filename from notmuch. This is the READ path, and +by the time it fires notmuch is already CORRECT: measured, the index named the +`,U=4` file while the pane still named the pre-`U=` one. The stale path is the +MODEL's, cached when the row was loaded, so refusing to act while a sync runs +(162's likely fix) would not help here at all. + +**The report is honest, which is why it is confusing.** `MimeParser` opened a +path that did not exist and said so. Nothing is lost and the next query +repairs it. + +**Approach.** The read path should RECOVER rather than refuse: on a failed +parse, re-resolve the message id through notmuch and retry once before +reporting. `recoverStaleThread()` already exists for the neighbouring problem +(item 91 reuses it) and is the shape to follow. + +**A SECOND site, found 2026-08-25 while hand-testing item 164, and its +consequence is worse than the pane's.** `MainWindow::openComposerFor()` passes +`ref.filePath` to `ComposeContextBuilder::forDraft()`, and `MessageRef::filePath` +is `notmuch_message_get_filename()` captured when the QUERY ran. After mbsync +renames an uploaded draft, a row loaded before that sync names a file that no +longer exists, `MimeParser::parse` fails, `forDraft` returns an empty context +and the composer reports "That draft could not be read". + +Measured, in order: draft written 10:42, sync at 10:50:43 logged "Detected 2 +file renames" and the file became `,U=5`, the user reopened it at 10:52 and +saw the error. + +**The pane's version is cosmetic and self-repairing. This one silently forks +the draft.** The refusal happens BEFORE `openComposer()`, so the user is left +with no composer for that draft. Composing again opens a FRESH one with +`m_draftPath` empty, so its first autosave has no previous path to unlink: +it writes a new file, `DraftStore` unlinks nothing, and the old revision +stays. Both are then uploaded and both reach the server. + +Two properties make this worse than it first reads. Each save mints a NEW +Message-ID (four saves in the hand test produced four ids), so the revisions +are distinct MESSAGES to notmuch rather than two files of one, and no dedup +anywhere will collapse them. And the unlink machinery is entirely correct +throughout, which is why the code reads fine: `forDraft` sets `draftPath`, +the composer seeds `m_draftPath`, the autosave passes it as `previousPath`, +and `DraftStore::write` unlinks it. The comment at `composecontext.cpp:501` +already names this exact failure ("one message becomes two"). None of it runs, +because the reopen was refused before any of it was reached. + +So the fix below covers this site too, and fixing only the pane would leave +the draft-forking half in place. + +**Constraints.** + +- **A retry must be bounded.** A message that genuinely cannot be parsed + (item 41's territory) must still report, or a real defect becomes an + infinite loop. +- Re-resolving by id is what makes this safe; re-scanning the folder is not, + since two files can carry the same id. +- The placeholder wording is correct and should stay for the genuine case. +- **The composer site must recover, not merely report better.** A clearer + error still leaves the user composing a second copy of their own draft. +- Whether a draft should keep a STABLE Message-ID across revisions is a + separate question this entry does not decide. It is what turns a stale path + into two server-side messages rather than one replaced file, and it wants + its own item; note that a draft's id is not yet the sent message's id, so + changing it is not obviously free. + +**Fixed 2026-08-25.** `MaildirName::resolveRenamed()` answers the filesystem +question: the path unchanged when it still exists, otherwise the file in that +same directory whose unique stem matches. mbsync preserves the stem +(`<stem>:2,D` becomes `<stem>,U=5:2,D`), which is what makes resolving by +filename safe here at all. It never recurses and never crosses a folder +boundary; a file that changed FOLDERS is a different question that only the +message id can answer, and `moveMessages()` re-resolves that way for item 162. + +Two refusals in it are deliberate and both have tests. An ambiguous match (two +files sharing a stem, which a correct Maildir cannot produce) yields nothing +rather than a guess, because opening or moving the wrong message is worse than +reporting none. And a genuinely missing file yields nothing too, so a real +deletion still reports instead of becoming a wrong answer. + +**Three call sites, not the one this was filed for.** The pane +(`renderMessages`), Reply and Forward (`openComposerFor`), and the draft reopen +(`forDraft`). The third is the one that cost data, and its shape is worth +keeping: the refusal happened BEFORE any composer existed, so the user composed +again into a fresh window whose autosave had no `previousPath` to unlink. The +unlink machinery was correct at every step and simply never ran. + +`forDraft()` seeds `draftPath` from the RESOLVED path. Seeding the caller's +would let the reopen succeed and the unlink still miss, which is the same fork +arriving one step later; the integration test asserts on `draftPath` for +exactly that reason. + +Covered by five unit tests on the resolver and by +`aDraftRenamedByASyncStillReopensAndReplacesItsFile()`, which renames the draft +the way mbsync does and asserts the file COUNT, the shape the fork actually +takes. Mutation-checked: removing the resolution fails it with the reported +symptom. + +**Left undecided, deliberately:** each save mints a NEW Message-ID, which is +what turns a stale path into two server-side MESSAGES rather than one replaced +file. That wants its own item. A draft's id is not yet the sent message's id, +so changing it is not obviously free. + + +## 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: ESTABLISHED 2026-08-25, and it was this repository after all.** This +entry previously named mbsync's folder `Patterns` as the leading theory and +concluded "most likely not a code change here at all". That was wrong; the +superseded reasoning is kept at the bottom because the reproduction is what +overturned it. + +**`NotmuchWorker` opened one read-only notmuch handle and kept it for the +process lifetime.** A read-only handle is a Xapian SNAPSHOT taken when it is +opened; it never observes a write made by another process afterwards. The sync +script's `notmuch new` is exactly such a process, so every query the worker +answered after startup was served from the index as it stood when the +application launched. `openReadOnly()` returned early on `if (m_db) return +true;` and there was no `notmuch_database_reopen` anywhere in the tree. + +This accounts for every symptom, including the ones that defeated three earlier +theories during the diagnosis: + +- The post-sync refresh found nothing, so `refreshCurrentQuery()` and + `ThreadListModel::reconcile()` were each suspected in turn. Both are correct. +- A query the user typed BY HAND also found nothing. That is what rules out the + model, the generation counter and the account scope together: a fresh query + clears the model and re-runs from scratch, and it still hits the same stale + handle. +- A restart showed the mail instantly, with no sync in between. +- Tag WRITES were never affected, which is why the defect reads as "reading is + broken" rather than "notmuch is broken". `applyTags` opens its own read-write + handle per call, so it always sees current data. + +**Why it survived from 2026-08-16 to 2026-08-25.** The symptom needs mail to +arrive from outside the process while the window stays open, which is the +ordinary way this application is used and the one thing no test did: every +fixture opens a worker, queries it, and drops it. `TestNotmuchWorker::runQuery()` +builds a FRESH worker per call, so the suite was structurally incapable of +reproducing it, and a test written through that helper passes against the bug. + +**Fixed** in `NotmuchWorker::openReadOnly()`: when a handle already exists, +`notmuch_database_reopen(m_db, NOTMUCH_DATABASE_MODE_READ_ONLY)` before +returning it. Every read path begins by asking for the handle, so one call +covers all of them; putting it at the call sites instead would be one more +place to forget. A reopen failure is deliberately NOT fatal, since the existing +handle is still usable and answering from a slightly stale index beats refusing +to answer at all. + +Covered by `aQuerySeesMailIndexedAfterTheWorkerOpened`, which holds ONE worker +across two queries and runs `notmuch new` in a second process between them. +Mutation-checked: `after.size()` is 0 without the fix, 1 with it. The first +query asserts zero results before the message is written, so "found nothing" +cannot mean "the query was malformed". + +**The reproduction, kept because this entry's Approach section asked for exactly +this and it took three wrong turns to get there.** Four messages sent to one +account on 2026-08-25, viewed in that account's Inbox, synced with the app's own +Sync button. The three layers resolved as: on disk (yes), indexed (yes), shown +(no), which is layer 3 and therefore this repository. Two of the four matched +the running view's exact query (`path:"<account>/**" and (tag:inbox)`, 2 results +from the shell) and were absent from a window that had been open across the +sync. + +Two measurement errors made during that diagnosis, both worth repeating because +each produced a confident wrong answer: + +- `notmuch count 'inbox and path:...'` was used to check the view's contents. A + bare `inbox` is a FREE-TEXT term, not a tag term; the app generates + `tag:inbox`. The bare form returned 0 where the real query returns 2, which + briefly made the defect look like a tagging problem. +- The messages' tags were first read across every file matching the subject, + including the sender-side Sent copies in other accounts. That mixed three + accounts' messages into one answer. + +**Superseded theory, kept for the record.** mbsync fetches Gmail folders by +pattern and three of the five channels name their folders explicitly, so a +message labelled anything else is in a folder mbsync never asks for while +Thunderbird, speaking IMAP directly, sees it. That mechanism is real and would +produce a similar symptom, but it is not what was happening here: the mail was +on disk and indexed. It remains a plausible cause of any FUTURE report of this +shape, so check layer 1 before assuming this fix covers it. + +**One inconsistency worth reporting regardless**, found while checking the above +and still true: one of the Gmail accounts is configured in `qtmaildir.conf` with +a sent and a drafts folder, 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, independent of this item, and outside this +repository. + +**Size: XS.** Done. + +## 167. No way to tell one build of an unreleased version from another + +**Observed (user, from the notes):** "we should add a dev build number to be +pushed everytime we rebuild after a fix, so that I can verify if I'm in the +correct app version." The note has sat unrecorded through several sessions; +the 2026-08-25 reconciliation is the first to pick it up. + +**Cause (verified in the code, 2026-08-25.)** The version lives in exactly one +place, `project(qtmaildir VERSION ...)`, and `src/version.h.in` interpolates +`@PROJECT_VERSION@` and nothing else. That is correct for a release and says +nothing between two of them: the string moves only when the release procedure +bumps it, so every rebuild of `0.27.0` reports `0.27.0`. The status table above +shows why it bites in practice, since most closed items since 0.27.0 read +"unreleased" and the user hand-tests each one against a binary they rebuilt +themselves. + +Both surfaces that show the version take it from the same macro, so whatever is +added reaches them at once: the window title (`mainwindow.cpp:939`), the About +dialog (`mainwindow.cpp:2449`), the placeholder pane (`messageview.cpp:547`), +`--version` and `--help` (`main.cpp`). + +**Approach.** Needs a DECISION before any code, because the two candidates fail +in opposite directions. + +A git description (`git describe --always --dirty`, or the short hash) is +accurate and self-explaining: it names the commit the binary was built from, and +a reviewer can check out exactly that. Its cost is that CMake computes it at +CONFIGURE time, so a build after a new commit reports the previous hash unless +the configure step is made to re-run, which is a custom command with a dependency +on `.git/HEAD` and the packed refs, and is the part that usually ships subtly +wrong. + +A monotonic counter always moves and needs no git, but it means nothing on its +own: build 412 does not say which fix is in it, and it differs between the user's +machine and any other, so it cannot be quoted in a report. + +**Constraints.** A release build must keep printing a clean `X.Y.Z`, since the +SlackBuild in the `my-slackbuilds` repo builds from the release tarball where +there is no git checkout at all, and the release procedure checks +`./build/src/qtmaildir --version`. Whatever is added is therefore an addition to +the string in a dev build and absent in a release one, not a change to the +version itself. + +**Decision (user, 2026-08-25): the counter.** The git description was +offered as the recommendation and was not chosen; what the user wants is to +know a rebuild happened, not which commit it was. + +**Built 2026-08-25, unreleased.** `QTMAILDIR_BUILD_NUMBER`, a cmake option ON +by default, runs `cmake/BuildNumber.cmake` as a build step: it increments a +counter and writes `buildnumber.h`, which `version.h` includes. +`QTMAILDIR_VERSION_DISPLAY` is `X.Y.Z build N` when that macro is defined and +plain `X.Y.Z` when it is not. + +Two macros, not one, and the split is the load-bearing part. +`QTMAILDIR_VERSION` stays clean and keeps the window title, `applicationVersion` +and anything that might ever compare versions; `QTMAILDIR_VERSION_DISPLAY` goes +to the three surfaces the user picked: `--version`, `--help`, the About dialog +and the placeholder pane. The window title was offered and declined, since the +number would then sit in every screenshot. + +**The counter had to be a BUILD step, not `configure_file`.** That is the whole +reason this is not two lines: `configure_file` runs once per cmake run, so a +counter interpolated into `version.h.in` sits still across exactly the rebuilds +this item exists to distinguish. `version.h.in` therefore includes a second +generated header rather than carrying the number itself. + +The counter file lives in the build directory and is not tracked, so it cannot +conflict on a pull or dirty the tree; a fresh build directory restarts at 1, +which is honest, because it is a different build tree. A release build passes +`-DQTMAILDIR_BUILD_NUMBER=OFF` and the header is written empty. + +**Verified by running it**, since none of this is reachable from a C++ test: +three consecutive builds reported `build 2`, `build 3`, `build 4`, and a +separate Release configure with the option OFF reported a clean `0.27.0`. The +suite is 37 of 38, the one failure being item 136 on an unrelated path. + +**Size: XS**, as sized. + +## 166. Mail you send to your own other account loses `inbox` + +**Observed (agent, 2026-08-25, while setting up msmtp.)** Four test messages +were sent to one of the user's own accounts, one from each configured sending +account. All four were delivered and indexed. The two sent from accounts whose +Sent folder is fetched locally arrived in the recipient account's Inbox +**without the `inbox` tag**, so they were absent from that account's Inbox view. +The two sent from an account whose Sent folder is not fetched kept `inbox` +normally. + +**Cause: established, and it is the `post-new` hook, not this binary.** +`strip_inbox_from_sent()` in `assets/hooks/post-new` removes `inbox` from any +message matching a configured sent folder's PATH. Its docstring states the +assumption exactly: "the provenance is the file's own path: a message inside a +configured sent folder is one this system sent, and `inbox` was never true of +it." + +That holds for one file. It fails for one MESSAGE, because **notmuch +deduplicates by Message-ID and a message can have several files**. When the +sender and the recipient are both the user's own accounts, mbsync fetches two +copies: the sender's Sent copy and the recipient's Inbox copy. notmuch stores +them as ONE message with two filenames. The carve-out's query matches via the +Sent filename and strips `inbox` from the message object, which is the same +object the recipient's Inbox copy belongs to. + +Measured: one message, two paths, one in the sender account's sent folder and +one in the recipient account's `Inbox/cur`. + +The assumption is not merely incomplete, it is false in this case: the message +was genuinely sent AND genuinely received. There is no single right answer for +"was `inbox` ever true of this message", because it was true of one file and +false of another. + +**Approach.** Not settled, and the choice matters more than the code: + +1. **Strip only when EVERY file is in a sent folder.** Closest to the existing + intent, and it makes the predicate match the docstring's claim. A + self-addressed message keeps `inbox`, which is right: it did arrive. +2. **Strip only when the message has exactly one file.** Simpler to express, + but it silently stops protecting any sent message that happens to be + duplicated for an unrelated reason. +3. **Leave it.** Self-addressed mail is rare outside testing. The cost is that + it is invisible when it happens, and it looks exactly like the sync defect + item 104 turned out to be, which is how this was found. + +Option 1 is the one that makes the code true to what it already says it does. + +**Constraints.** + +- **The hook is this repo's**, `assets/hooks/post-new`, which the live + `database.hook_dir` symlinks to. It has its own suites beside it; run + `./test_post_new.py` and `./test_mailrules.py` from `assets/hooks/`. +- The hook **tags real mail unattended, every ten minutes, on the user's live + index.** A predicate that is wrong in the other direction would strip `inbox` + from arriving mail, which is the failure mode PROTECTED_REMOVALS exists to + prevent. Test against a throwaway database first. +- `notmuch tag` matching zero messages SUCCEEDS, so a log line saying the + carve-out ran is not evidence it matched anything. The count added for item + 164 is what distinguishes them; use it. +- Do not fix this by narrowing the query to exclude the recipient account. The + bug is in the per-file predicate, not in which folders are configured. + +**Fixed 2026-08-25, option 1**, the one the entry named: strip only when +every file is in a sent folder. + +`sent_only()` in `assets/hooks/post-new` filters the matches and the tag is +then applied per id. **It is a loop because no query can express it**, and both +plausible query forms were measured against a real two-file message before the +loop was written: `not path:"Inbox/**"` does NOT exclude the message, and +`notmuch count --output=files` on a path query reports every file of every +matching message rather than the files that matched. Both read as if they +worked and are wrong for one reason, that a notmuch term is a predicate over a +MESSAGE while the distinction here is between its FILES. + +The root comes from `database.mail_root`, not `database.path`, since this index +is split and no message file sits under the index directory. The mutation +putting `database.path` back passes every pre-existing test, because the +ordinary fixture keeps the index inside the mail root and both keys return the +same string; `setup_accounts(split_index=True)` is what catches it, and is the +Python counterpart to `NotmuchFixture::splitIndex()`. + +Two mutations fail: `all` to `any` loses `inbox` on the self-addressed message, +`mail_root` to `path` silently stops stripping anything. + +**Verified read-only against the live index**, tagging nothing: of 807 messages +matching a sent path, 780 are still stripped and 27 are spared, every one of +them two files with one in another account's Inbox. No arrival is affected. + +**Size: S.** Done. + +## 112. Toggle unread on a whole thread cannot reach "all unread" on a partly-read thread + +**Observed (user, 2026-08-17):** clicking a thread root and asking to mark the +whole thread unread does not do it. On a seven-message thread with two unread +replies, the result is that every message is toggled unread **except those +two**, which are left as they were. The user asks for an explicit "mark whole +thread read/unread" rather than a toggle. + +**Cause (verified in code):** the action exists, and its direction is the +defect. `toggle_unread_thread` (`src/mainwindow.cpp:931`, `Ctrl+Alt+U`) chooses +between adding and removing by asking +`everySelectedRowHasTag("unread", TagScope::Thread)`, which reads +`ThreadListModel::threadFor(index).tags`. That is notmuch's **union over the +thread** (`CLAUDE.md`, item 110), so a thread containing even one unread message +answers "unread" and the action picks *Mark thread read*. There is no input a +user can give that reaches *Mark thread unread* on a mixed thread: the only +threads that take that branch are the ones already entirely read, and the only +threads reporting "not unread" are the ones the user does not need the action +for. + +The write itself is absolute and correct. `tagSelected` with `TagScope::Thread` +adds or removes `unread` across every message, so the two unread replies in the +report are not skipped by the write. They are the reason the write ran in the +opposite direction from the one the user wanted. + +**A union is not a state, and a toggle needs a state.** This is the same class +as item 110 and the third time the union has produced a defect. Items 105 and 88 +fixed *which object* a toggle resolved; this one is about a thread having no +single answer to give. `everySelectedRowHasTag` is a two-valued predicate over a +three-valued reality: all read, all unread, or mixed. The mixed case is the one +that has no correct toggle direction, and picking either one silently is what +ships as "the action does the wrong thing". + +**Approach.** The user has already named it: stop toggling at thread scope. + +- Split `toggle_unread_thread` into two explicit actions, **Mark thread read** + and **Mark thread unread**, each with a fixed direction. Both appear in the + "Whole thread" submenu, where an entry always carries text, so a fixed label + is honest in a way a toggle's cannot be. +- The message-scoped `toggle_unread` stays a toggle. One message has a real + two-valued state, so the trap does not exist there. Do not "unify" the two: + the asymmetry is the point. + +**Constraints.** + +- **Adding an action is four places**, all enforced by tests that fail + confusingly: `KeyMap::knownActions()`, `defaultBindings()`, the icon table, + and the no-duplicate-icons exception list. See `CLAUDE.md`. Splitting one + action into two means one new entry in each, and the pair shares the twin's + icon under the existing named exemption for thread actions. +- **`Ctrl+Alt+U` is taken by the action being split**, and the whole-thread + bindings are already one modifier out from their twins because `Ctrl+Shift+U` + was claimed. Two directions need two sequences; if a second chord cannot be + found that is not worse than the menu, bind one and leave the other to the + submenu rather than inventing a three-modifier chord nobody will press. +- **This interacts with items 98 and 99**, which is the reason to decide all + three together. 99 asks for a dynamic label on the message-scoped toggle, + which is the opposite move: keep the toggle, make the label tell the truth. + A thread cannot do that, because on a mixed thread there is no true label to + show. Deciding 99 first will produce the wrong answer here by analogy. +- The undo entry must name the direction that ran (`Mark thread unread`), not + the action. `tagSelected` already takes the text, so this comes free from + splitting. +- **The test needs a MIXED thread**, which is the whole defect: a thread whose + messages are all in one state answers identically whichever way the direction + is computed, so a fixture built from a uniformly-unread thread passes against + the bug. Same trap as item 88's opposite-states requirement, recorded in + `CLAUDE.md`. + +**Built 2026-08-25 to the USER'S NOTE, not to the approach above**, which had +this half right and was shipped that way first. The approach proposed splitting +the thread toggle and explicitly said to leave the message-scoped one alone, +deciding 99 separately. The user's note is ONE design across both, and the +entry's own constraint said so ("this interacts with items 98 and 99, which is +the reason to decide all three together") without following it. The half-built +version was handed over, corrected by the user, and rebuilt. + +Four parts, all of them the note's: + +- The thread toggle splits into `mark_thread_read` and `mark_thread_unread`, + both absolute. **Neither carries a default chord**, at the user's choice: + since item 132 a shortcut is a chosen subset, and `Ctrl+Alt+U` meant + whichever direction the union happened to pick, which is what made it wrong. + It is now unbound. +- The message-scoped `toggle_unread` STAYS a toggle, because one message has a + real two-valued state, and gains a label naming the direction it will go. +- On a selection with no single state that entry is **hidden**, chosen over + disabled by the user. There is no honest label for a mixed selection, and + the thread submenu is the route the note points at. +- The label follows a WRITE as well as a selection change, keyed on the + model's `dataChanged` rather than on the six call sites that apply an + optimistic update, so a new one cannot forget. Without it, marking the + current row read left the entry offering to do it again. + +`selectionTagPresence()` is the three-valued predicate this needed; +`everySelectedRowHasTag()` now delegates to it and keeps its two-valued +answer, which is all a DIRECTION needs. A label needs the third value, and +asking a two-valued predicate a three-valued question is what this item was. + +**Three mutations fail:** restoring the union predicate reports "wrong +direction on a mixed thread: Mark thread read", which is the user's original +symptom; showing the action on a mixed selection; and dropping the +`dataChanged` refresh. The suite is 37 of 38, the one failure being item 136 on +an unrelated path, and the four new strings are translated with `lrelease` +reporting 0 unfinished. + +**Closes 99 and 147 with it**, which were the same note recorded twice. + +**Size: S.** Done, at roughly twice the entry's scope because the entry's scope +was wrong. + +## 118. No way to empty the trash from inside the app + +**Observed (user, 2026-08-17):** raised while reviewing item 103's spec, as +something that had been forgotten rather than newly noticed: "we could add +'Empty Trash' to the backlog as a future item. I forgot it existed, but I don't +want to squeeze it in this spec." + +**Blocked on 103**, which creates the trash folder this would empty. Until that +ships there is nothing to empty: Delete writes a tag and moves no file, so no +account has a populated trash folder except through another client. + +**Deliberately excluded from 103's spec**, at the user's request and recorded in +its "Out of scope" section. Worth keeping separate for a reason beyond scope +control: emptying the trash is the first action in this application that would +destroy mail with no undo. Every mutation so far is a tag or, after 103, a move, +and both are reversible. A purge is not. + +**Approach, unspecified.** The shape depends on decisions not yet made, and the +spec for 103 answers none of them: + +- **Local or remote.** Deleting the files locally and letting `Expunge Both` + carry it to the server is one thing; asking the provider to empty its own + trash is another, and mbsync offers no verb for the latter. The first is + probably what "Empty Trash" should mean here. +- **Whether the no-confirmation rule survives it.** It does not, on the face of + it. `CLAUDE.md` grants undo in place of confirmation dialogs, and this is the + action where undo cannot exist. That makes it the second item, after 103, that + re-examines the rule rather than assuming it, and unlike 103 it will probably + have to break it. +- **Per-account or all-accounts**, which should follow whatever the Trash filter + does once 103 ships rather than being decided independently. + +**Built 2026-08-25**, unblocked by 103. The three questions the entry left open +were put to the user and answered: + +- **Local, and let the sync carry it.** The files go, and a channel with + `Expunge Both` propagates that to the server. mbsync offers no verb for + asking a provider to empty its own trash, so the alternative was to delete + locally and not care, which brings the mail back on the next sync and reads + as the action having silently failed. +- **It confirms**, naming the count and the account, defaulting to Cancel, with + no default shortcut. CLAUDE.md now records this as the ONE exception to the + no-confirmation rule, in the same paragraph that states the rule, so the next + reader meets both together. +- **Scoped to the account selector**, like every other account-aware surface, + which is what the entry asked for. + +`NotmuchWorker::purgeMessages()` is a separate entry point from +`moveMessages()` rather than a flag on it, because the two look alike and only +one can be undone. It takes named ids only, never a folder sweep, so the blast +radius is what the dialog enumerated and the user confirmed. It deletes EVERY +file of a message: notmuch deduplicates by Message-ID, and leaving one behind +would leave the message alive in the folder the user emptied, which is the same +one-message-many-files property item 166 turned on. + +`resolveQueryMessages()` is a four-line wrapper over the existing private +`resolveQuery()`, so enumerating what is about to be destroyed needed no new +walk. The count in the dialog comes from the DATABASE rather than the model, +which holds whatever the current view is showing and is usually not the trash. + +**A defect surfaced while writing the tests**, and it is the one worth +remembering: the first version counted a message whose file was already gone as +destroyed, so the number reported for an irreversible action overstated it. An +absent file is correctly not an ERROR, since the index can name a path a sync +has removed; the mistake was treating "not an error" as "destroyed". The +mutation that restores it now fails. + +**A second defect was found by the user's own hand test**: the mail was +destroyed correctly and the LIST went on showing it until they re-ran the query +themselves. A purge is the one mutation with no optimistic update available, +because it removes rows rather than changing them, so `messagesPurged` re-runs +the current query. Nothing was connected to that signal at all, which is the +kind of gap a green suite is happy to keep. + +Verified against the live index after the user emptied one real account's +trash: zero files on disk, zero in the index. + +**Item 168 was filed from the same hand test**, on Delete being offered on mail +already in the trash. + +**Size: S.** Done. + +## 168. Delete is offered on mail already in the trash, and does nothing + +**Observed (user, 2026-08-25, while hand-testing item 118):** "I noticed I can +hit delete via context menu on a message already in the trash. Seems like a +bug, unless that action doesn't do for one message what Empty trash does for +the whole view." + +It does not, and the guess in the second half is worth recording as the reason +this matters: the user's mental model was that Delete on already-trashed mail +might PURGE it. It does not, and nothing about the menu says so. + +**Cause (verified in code, 2026-08-25.)** `moveMessages()` compares the file's +directory against the destination and takes an early-return branch when they +match (`notmuchworker.cpp`, the "already where it was asked to go" branch, +added when a fresh Maildir name made a path comparison useless). That branch +appends the id to `moved` and records an origin, so the message is reported as +having moved when nothing happened. The UI counts an unsynced change for it. + +Nothing is destroyed and nothing is corrupted; the cost is a menu entry that +lies about having done something, and a pending-changes count that overstates +what a sync has to carry. + +**The mirror of the same defect is already shipped beside it.** `restore` is +added unconditionally to both the Message menu (`mainwindow.cpp:1956`) and the +thread context menu (`mainwindow.cpp:2119`), so it is offered on mail that was +never deleted, where it has as little meaning as Delete has in the trash. + +**Approach.** The user chose to hide each action where it has no meaning, +which is the principle item 112 established for the unread entry: an action +with no honest meaning for the selection is absent rather than present and +inert. + +- Delete is hidden when every selected row is already in a trash folder. +- Restore is hidden when no selected row is. +- The test for both needs a MIXED selection as well as uniform ones, for the + reason item 112 records: a selection whose rows agree answers identically + whichever way the predicate is computed. + +**Constraints.** + +- **The question is about the PATH, not the tag.** A message trashed by + another client carries no `deleted` tag at all, which is why item 103 made + the trash view path-based. Asking `tags.contains("deleted")` here would + offer Delete on exactly the mail the user is most likely to be looking at + in a trash view. +- **`selectionTagPresence()` is the wrong instrument** for the same reason, + though it is the right shape. A path predicate needs the row's path, which + `MessageNode` carries. +- Deciding this does not require deciding item 118's relationship to it: a + purge stays an explicit whole-view action, and hiding Delete does not make + Delete a purge. + +**A second request, from the same tangent (user, 2026-08-25):** "messages moved +to the trash should be automatically marked `-unread`." Deleting is a decision +about the message, so leaving it bold and unread in the trash is noise; the +count of unread mail should not include what the user threw away. + +It is one line where Delete already composes its tag change, and it carries a +constraint worth stating rather than discovering. `maildir.synchronize_flags` +is true, so removing `unread` REWRITES the Maildir filename and reaches the +server on the next mbsync. That is acceptable here and is a deliberate +exception: it is the same mechanism the `post-new` hook refuses to touch on +arriving mail, for the good reason that the hook acts unattended on mail the +user has not seen. A Delete is an explicit gesture on a message in front of +them, which is the difference. + +Undo must put it back. `TagChange::inverted()` already does, provided the +removal travels as part of the SAME change rather than as a second write, so +one undo returns both the folder and the tag. + +**Built 2026-08-25**, both halves, to the user's own choice of "hide each +where it has no meaning". + +`everySelectedRowIsInATrashFolder()` asks each row about its own file, a reply +row's message and a thread row's displayed message, the same rule +`everySelectedRowHasTag()` follows. `refreshTrashActions()` runs beside +`refreshUnreadAction()` on both the selection change and the model's +`dataChanged`, so the entries follow a write as well as a selection. + +The `unread` removal travels inside the SAME `sendMove()` call rather than as a +second write, which is what makes one undo return the folder and the tag +together. + +**A mutation survived the first round and is worth recording**: comparing the +prefix WITHOUT its trailing separator passed every test, because no fixture had +a folder whose name starts with the trash folder's. `acct/trash-old` is a +different folder, and under that mutation Delete silently disappeared from mail +that had never been trashed, which is the quiet half of the same mistake. The +fixture carries that row now and the mutation fails. + +All three properties are mutation-checked: the separator, Restore's visibility, +and the `unread` removal. The suite is 37 of 38, the failure being item 136 on +an unrelated path, and no new user-facing strings were added. + +**Size: S** for the visibility half, XS for the `unread` half. Done. + + +## 68. A forwarded subject gets no `passed` tag + +**Observed (user, from the notes):** "passed tag should appear when subject is +`Fwd:` and `Fw:`." Refined in session on 2026-08-11: the user had noticed +`passed` appearing on messages whose subject carried `Fwd:` and not on `Fw:`, +and asked to expand the rule to both. + +**Cause:** there is no rule to expand. `passed` is the Maildir `P` flag in the +message filename, translated into a tag by notmuch because +`maildir.synchronize_flags=true`. The flag is written by whichever client +forwarded the message, or by the server over IMAP; nothing reads a subject line +anywhere in the chain. qtmaildir only ever colours the tag +(`src/tagcolors.cpp:36-37`) and the database's `post-new` hook does not mention +it either. + +**Measured against the real database (2026-08-11):** + +| Query | Count | +|---|---| +| `tag:passed` | 6 | +| `tag:passed and subject:"Fwd:"` | 1 | +| `tag:passed and subject:"Fw:"` | 0 | +| `subject:"Fwd:" and not tag:passed` | 194 | +| `subject:"Fw:" and not tag:passed` | 28 | + +Six tagged messages in the whole database, and every one of them carries `P` in +its filename flags. The single overlap with `Fwd:` is a message that was +forwarded and whose subject was already a forward, not evidence of a rule: 194 +`Fwd:` subjects carry no tag at all. The correlation the observation rests on +does not exist. + +**Approach and the decision it needs first.** Two different features, and the +measurements above decide how far apart they are. + +*Display only.* The card shows a forwarded mark when the subject matches. Touches +no mail, changes no flag, reversible by deleting the rule. XS. + +*Write the tag.* qtmaildir sets `P` from a subject heuristic. With +`maildir.synchronize_flags=true` that flag is a filename change that mbsync +carries out to the server, on 222 existing messages, on a guess about a string. +Not cleanly undoable, and it asserts a meaning for a flag this application did +not define. Recommended against; recorded so the choice is deliberate rather than +forgotten. + +**Constraints:** localised clients use their own prefixes, and `Fwd:` can appear +inside a subject rather than at its head, so whatever matches must be anchored. +If the tag is ever written, it must not be re-applied on every sync in a way that +produces pending edits the user never made, item 28 is the record of a count +going wrong. The display-only route avoids that entirely, since it derives the +mark at paint time and stores nothing. + +**Size: S** as written, XS if it is display only. Most of it is the decision, not +the code. + +**Status:** left open deliberately on 2026-08-11. The cause is settled and the +options are costed; the user has not chosen, and no code was written. + +**Built 2026-08-26, and the observation was wrong in a way worth recording.** +The note asked for one thing (expand a subject rule to `Fw:`) and the +measurement above had already shown there was no subject rule and no +correlation to expand. Taken literally the item was unbuildable; taken as what +the user actually wanted ("I want to know visually if someone has forwarded a +message to me") it split into three, and the user chose all three. + +**1. `replied` on a reply, `passed` on a forward.** The gap the item was +really sitting on, and it was never reported. Measured 2026-08-26 against the +developer's own index: 317 `replied` and 6 `passed`, spread over five +accounts, every one of them written by another client or the server. Nothing +in qtmaildir has ever written either flag. `ComposeWindow` emits +`sourceMessageAnswered` after a SUCCESSFUL send; `MainWindow` routes it +through `sendMessageTagChange`, message-scoped, off the undo stack for the +reason `markCurrentThreadRead` gives (the flag records that the mail went, and +the send cannot be undone, so an undo that retracted only the flag would leave +the two disagreeing). + +**Two traps here, one of which was caught only by reading.** `inReplyTo` is +deliberately EMPTY on a forward (carrying In-Reply-To would file the forward +under the thread it left, in the recipient's client), so keying the emit on it +made the `passed` half dead code that compiled and never fired. `ComposeContext` +carries `sourceMessageId` instead, set for all three kinds. And a resumed +`Kind::Draft` is excluded: its kind records how the FILE was opened, not what +the user is doing, so a draft that began as a reply cannot be told from one +that began as a new message. The cost is a missing flag on a reply finished in +two sittings, which is the safe direction, since `maildir.synchronize_flags` is +on and a wrong flag reaches the server. + +**2. A received-forward mark, display only.** `Marks::Mark::ReceivedForward`, +a seventh SVG, drawn from `ThreadListModel::IsReceivedForwardRole` in BOTH the +thread and the message branch per CLAUDE.md's rule. It is a different mark +from `passed` on purpose: `P` means "I forwarded this", which is a different +fact about a different person, and setting it from a subject guess would +assert something false on 222 existing messages and propagate it to the +server. Derived at paint time, stores nothing. + +**3. `[general] forward_prefixes`.** `subjectIsForwarded()` lives beside +`forwardSubject()` and shares its prefix table, so "do not double the prefix" +and "this is a forward" cannot drift apart. The config key EXTENDS that table +rather than replacing it, so adding a locale does not lose the measured +English/German/Iberian/French spellings. A `Re:` chain is stripped first +(bounded to 8, since the subject is input from a stranger and this runs per row +per repaint), so `Re: Fwd: x` is recognised. + +**A mutation survived the first round and corrected a claim in the code.** The +word-validation guard on a configured prefix was commented, and tested, as +protecting against an invalid pattern from an unescaped `(`. Measured with a +standalone probe: `QRegularExpression::escape` already makes punctuation inert +rather than invalid, so that test passed against the guard being removed. What +the guard actually buys is narrower and real: a configured `-` would match +`-: x` and a digit would match `2: x`. The comment and the test now assert +that instead. + +**Not built, and left as the item's own recommendation:** writing `P` from a +subject heuristic. Rejected on the same grounds the entry gave before the +work started. + + +## 119. The unsynced-changes count cannot be opened to see what it counts + +**Observed (user, from the notes):** "the bottom left statusbar message needs to +be clickable and show what 'N unsynced changes' are in a modal window". + +**Cause (verified in the code).** `m_pendingLabel` is a plain `QLabel` added to +the status bar with `addPermanentWidget` (`src/mainwindow.cpp:502-505`). A +`QLabel` has no clicked signal and none is installed, so there is nothing to +click and no route to a list. It carries a tooltip and nothing else. + +**The count is a SUM OVER FOUR SOURCES, and that is what makes this bigger than +it looks.** `pendingEditCount()` returns +`m_pendingTagEdits.size() + m_unnettablePendingEdits + held + heldMoves`. +Three of those can name what they hold: `m_pendingTagEdits` is a +`QHash<QString, bool>` keyed by message id, `m_heldEdits` and `m_heldMoves` are +queues of edits waiting for a sync to end. **`m_unnettablePendingEdits` is a +bare `int`** (`src/mainwindow.h:1248`), deliberately so: it counts confirmed +changes that carry no message ids and therefore cannot be netted against +anything. + +So a dialog built from what is currently kept would list three of the four +groups and then have to account for a remainder it cannot describe. Showing "and +3 more" is worse than the tooltip, because the user opened the window +specifically to find out what those were. + +**Approach.** Two halves, and the second is the real work. + +- The clickable half is small: a label that emits on click (an event filter, or + a flat `QToolButton` styled as a label), plus a dialog listing what the three + describable groups hold. The message pane already resolves an id to a subject. +- The complete half needs `m_unnettablePendingEdits` to become something that + can name its entries. Its comment says why it is an int: understating the + indicator is the direction that costs the user work, so it counts what it + cannot identify rather than dropping it. Making it describable means finding + out what those changes actually are and whether they can carry an id. + +**Constraints.** + +- **The count is deliberately conservative and must stay so.** Item 28 and item + 54 both landed on this indicator being wrong in the direction that made the + user think their work was safe. A dialog that lists fewer changes than the + count claims is the same failure in a new place: reconcile the two, or state + the remainder honestly rather than hiding it. +- **An external `notmuch` run can clear pending changes without this count + noticing**, which the tooltip already admits. A dialog makes that staleness + much more visible, since a listed change may no longer exist. Worth deciding + whether the dialog re-verifies against the database before showing. +- Read-only. This is an information window, not a place to retry or discard a + change; either would be a new mutation path with its own undo question. + +**Size: S** for the clickable half over the three describable groups. **Unknown** +for the fourth, and the item is not complete without it. + +**Closed 2026-08-26.** The blocker above was investigated first and did not +survive: `m_unnettablePendingEdits` counted confirmed changes carrying no +message ids, and `NotmuchWorker::applyTags()` (the only emitter of +`tagsApplied`) returns early on an empty id list, which is that exact +condition. `applyTagsToThreads()` resolves through a query and errors out on +an empty result, so it cannot hand `applyTags()` an empty list either. + +**Measured rather than read**, twice, because reading is what produced the +wrong answer the first time: a `qFatal` in the branch fired in 4 of 70 +`test_mainwindow` cases, all four building a `TagChange` by hand and invoking +the slot directly with no worker, and a `Q_ASSERT` before the worker's own +emit never fired across the whole suite. The counter was deleted and the +guard it shadowed is pinned where it lives, by +`applyTagsWithNoIdsDoesNothing()` in `test_notmuchworker`. + +Built in four commits: the snapshot, the subject resolve, the dialog and the +click, then a sizing fix after a hand test. + +Three decisions the user made, each of which shapes the code: + +- **Scope follows the ACTION, not the storage.** A thread action shows one + thread row with the count of messages it covered; a message action shows + its message. The three queues already encoded this, so nothing is expanded + and nothing is escalated: `HeldEdit` is thread-scoped because a `*_thread` + action made it, and everything else carries message ids. +- **A snapshot, frozen.** Taken at the click and never refreshed under the + user, who asked for exactly this: "if I keep the popup open for 20 minutes, + I don't want the popup to keep updating the info it's showing me." +- **Subjects resolved, stale rows kept.** An id the index no longer holds + still gets a row saying its subject is unknown, because the count the user + clicked has to equal the list they are shown. + +The re-verify question this item worried about mostly dissolved: item 54 +already clears the count when an external sync carries the edits, so a change +applied by cron does not survive to be clicked on. + +`resolvePendingSubjects()` answers POSITIONALLY, one subject per input row, +because one id can legitimately appear on several rows and a combined query +returns a set. `PendingChangeRow::startsMessage` is carried rather than +inferred from a non-empty subject, so an unresolved id still opens a run of +its own instead of folding its actions under the message above it. + +Read-only, per the constraint above. The dialog's height is sized to its +content; that is a hand test, since the offscreen platform returns an +identical frame either way. + |