From 98914f2c184539c9be9cac7c3ae8def6820868b8 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 20 Aug 2026 09:47:11 +0200 Subject: fix(worker): read the mail root, not the index directory notmuch can be configured with `mail_root` and `path` as separate keys, which puts the Xapian index outside the Maildir. Under that layout notmuch_database_get_path() returns the INDEX directory, and the worker treated it as the mail root at four sites. The consequences are not symmetric. Message paths resolved to `../..` escapes that match no account prefix, which is a display defect. But moveMessages() composes its destination from the same root, so Delete would have written into the Xapian tree: outside the Maildir, invisible to mbsync, and gone from every other client. That is the stranded-mail failure of item 103 with a new cause. notmuch_config_get(NOTMUCH_CONFIG_MAIL_ROOT) is correct under both layouts, so no conditional is needed. Verified against the live database: with only `path` set it returns the same string as get_path(), making this a no-op for the current configuration. The fixture gains an opt-in splitIndex(). That is load-bearing rather than convenience: in the ordinary layout the index lives inside the mail root and both accessors return the same string, so a test written against it passes whichever one the code uses. All three new tests fail against the old accessor, confirmed by mutation. Also records the finding as backlog item 124, and corrects item 121's timings, which had been copied from item 74 rather than measured. A cold run seven minutes after boot, with the index verifiably unread, gives 2008 ms to the first rows and 38618 ms to a complete list, against the 642 ms and 5714 ms recorded there. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01YDq53rMd3AQp7QmcZzpuBM --- .../plans/2026-08-03-post-0.1.0-usability.md | 235 ++++++++++++++++++++- 1 file changed, 233 insertions(+), 2 deletions(-) (limited to 'docs') 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 be52da9..4039753 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 @@ -19,8 +19,10 @@ came from one such pass on 2026-08-04 and included two defects that had gone unrecorded here for a while. Items 39 to 45 came from the 2026-08-05 pass, which found one more defect (41, a message body silently dropped by the MIME walk) and one item that cannot be planned at all until the user says where the thing it -manages lives (44). Compare the two at the start of a session; the procedure is -in `CLAUDE.md`. +manages lives (44). Items 121 to 123 came from the 2026-08-20 pass, which found +that item 74 had closed only half of what its note asked for, and that the +README had gone stale enough to document a mandatory config key by omitting it. +Compare the two at the start of a session; the procedure is in `CLAUDE.md`. Numbering is stable. New items append with the next free number and never renumber, so a note referring to "item 7" keeps meaning the same thing. An item @@ -185,6 +187,12 @@ taking that too literally. | 118 | No way to empty the trash from inside the app | workflow | S | open, 2026-08-17. **Blocked on 103**, which creates the trash in the first place. Deliberately left out of 103's spec at the user's request rather than squeezed in | | 119 | The unsynced-changes count cannot be opened to see what it counts | information | S | open, 2026-08-19, from the notes. One of the four things it sums carries no message ids at all, so a list cannot be complete without a change to how the count is kept | +| 121 | The thread list shows nothing while a query is running | feedback | S | open, 2026-08-20, from the notes. Follows item 74, which fixed the status-bar half and left the list itself blank | +| 122 | The README documents a version of the app that no longer exists | documentation | M | open, 2026-08-20, from the notes. Delete-to-trash is entirely undocumented, including a config key a user must now set | +| 123 | Sending mail is not designed | v2 | ? | open, 2026-08-20, from the notes. Brainstorm only, explicitly `#plan-only`; the user places most open UX behind it | + +| 124 | The worker reads the index directory as the mail root | defect | S | open, 2026-08-20. Blocks moving the index to an SSD. Under a split `mail_root`/`path` config, Delete would move mail INSIDE the index directory, where mbsync cannot see it | + Sizes are rough: XS under an hour, S a sitting, M a session. --- @@ -805,6 +813,229 @@ specifically to find out what those were. for the fourth, and the item is not complete without it. +## 121. The thread list shows nothing while a query is running + +**Observed (user, from the notes):** "can we show a spinner in the left panel +while 'Searching' is going? Especially at first run, the loading wait is several +seconds, and the status bar starts updating 'Searching N threads' after the +first have already appeared. Before that the program seems broken." + +**This is the half of item 74 that was never built**, and the note is precise +about which half. Item 74 closed on 2026-08-15 having fixed the status bar, +which used to set "Searching..." once and hold it for the whole walk. The count +the note describes is that fix working as designed: it is written from +`m_model->rowCount()` in `onThreadsReady`, so by construction it cannot report +anything before the first batch has landed. + +**Cause (verified in the code).** `MainWindow::runQuery` clears the model and +sets the status text (`src/mainwindow.cpp:2414`), and nothing else in the view +changes. The thread list is then an empty `QTreeView` until `appendBatch` runs +on the first batch, so **a query in progress and a query that matched nothing +render identically**. There is no busy state on the view at all. + +**The gap is measured, and item 74's numbers understate it badly.** Re-measured +on 2026-08-20 against the user's real inbox, seven minutes after boot, with the +index verifiably unread (0.0% of 1037 MB resident). Item 74's figures came from +`posix_fadvise(POSIX_FADV_DONTNEED)` eviction, which does not reproduce a real +cold boot on this hardware: + +| phase | item 74, 2026-08-11 | measured cold, 2026-08-20 | warm | +|---|---|---|---| +| `search_threads` returns | 411 ms | **673 ms** | 2 ms | +| first batch of 200 rows | 642 ms | **2008 ms** | 12 ms | +| walk complete | 5714 ms | **38618 ms** | 154 ms | +| threads | 4444 | 4628 | 4628 | + +So the list is blank for **two seconds**, and keeps growing for **thirty-eight**, +on 4% more mail. The user's note said "several seconds" and the note was right. + +**The cause is the storage, not the code.** `/data` is `/dev/sda1`, a 7200rpm +platter (`rotational: 1`); warm, the identical walk is 154 ms, a 250x +difference. Item 124 is the prerequisite for moving the index to the NVMe SSD +already in the machine, which would make this gap ~12 ms and reduce this item to +a nicety. + +**Approach.** A busy state on the left pane between `runQuery` and the first +`onThreadsReady`, cleared by whichever of the first batch or `queryFinished` +arrives first. The empty-result case must be distinguishable from it: when +`queryFinished` reports zero, the pane should say so rather than returning to a +blank list, which is the same ambiguity one step later. + +The likely shape is an overlay or a placeholder row rather than a literal +spinner widget, but that is a design question for the user, not a decision to +take here. A spinner also has to be animated by the UI thread, which is free +here since the work is on the worker, but that is worth stating because it is +the usual reason a spinner does not spin. + +**Constraints.** + +- **A background refresh must stay silent.** `onThreadsReady` returns early on + the refresh branch and `onQueryFinished` does the same, deliberately, so a + sync-driven refresh does not flicker the status bar. A busy indicator that + ignored that guard would make every cron sync flash the list. That silence is + already a test, and it should cover this too. +- **Item 74's decision not to address the cold cost was taken on wrong + numbers** and is worth revisiting, though not here. It judged a 5.7 s wait not + worth prefaulting 1.1 GB; the real figure is 38.6 s. The answer is not + prefaulting either way: it is item 124 plus moving the index off the platter. + This item makes the remaining wait legible, nothing more. +- Nothing about the query timing may change. + +**Size: S.** + +## 122. The README documents a version of the app that no longer exists + +**Observed (user, from the notes):** "documentation needs updating, EG the +README.md reports various things not up-to-date anymore." + +**Cause (verified).** `README.md` was last touched on 2026-08-15 by b405e32, +which moved the SlackBuild out to the `my-slackbuilds` repo. Everything released +since then is absent from it. Releases 0.19.0 through 0.26.1 all landed after +that commit. + +**Measured, by grepping both documents for the same terms:** + +| term | README | CHANGELOG | +|---|---|---| +| `trash` | 0 | 14 | +| `restore` | 0 | 7 | +| `Select all` | 0 | 3 | +| `deleted-from` | 0 | 0 | + +**One of these is worse than stale documentation.** Item 103 made a per-account +`trash` key MANDATORY: an account without one produces a config warning, and +Delete cannot work. The README is the only place a user reads about configuring +an account, and it does not mention the key at all. So the documented config +produces a warning against the current binary, and the feature that needs it is +undocumented. The `deleted-from:` tag is likewise invisible, and a user +who sees it on a message has nowhere to look it up. + +**Approach.** An audit against the changelog rather than a rewrite: walk the +sections from 0.19.0 forward and check each user-visible change for a README +home. The config section and the keyboard-shortcut table are the two most +likely to have drifted, since both enumerate things that have been added to. + +**Constraints.** + +- **The changelog is the evidence, not memory.** Every entry since b405e32 is + written down; work from it. +- **`### Upgrading` sections are the priority.** They exist precisely because a + user's config or habits had to change, and those are the paragraphs whose + absence from the README costs the user a broken setup rather than a moment of + confusion. +- The "Development Approach" section at the bottom is required by the user's + global preference and must survive any edit. +- No personal details, per the same preference: account names in examples stay + generic. + +**Size: M.** The audit is most of it; the writing is small once the list exists. + +## 123. Sending mail is not designed + +**Observed (user, from the notes):** "we should start brainstorming sending +emails. Most of the open issues are UX. #v2 #plan-only #new-branch". + +**Not a defect and not implementation work.** The note tags it `#plan-only`, and +this repo's own scope statement agrees: `CLAUDE.md` records that v1 is +read-and-organize only and that compose and send are v2. Recorded here so the +backlog stops being silent about the largest thing the user has written down. + +**What it blocks.** Item 72 (khard/khal) is explicitly placed after send by the +user's own note, and cannot be specified before it. The completion machinery +that would serve recipient completion already exists as `QueryCompleter`, and +the `QLineEdit::setCompleter` trap in `CLAUDE.md` applies directly to any +multi-recipient field, so there is prior art to reuse rather than a blank page. + +**What it does NOT change.** The architecture note that this application does +**no network protocol work at all** is load-bearing: fetching is `mbsync` via +`assets/mailsync.sh`, and sending should be an external script on the same +model, not an SMTP client written here. A design that puts a socket in this +process is out of scope regardless of how the UI turns out. + +**Approach.** Brainstorm first, on its own branch, producing a spec under +`docs/superpowers/specs/` before any code. The open questions are all UX, as the +note says: where a composer lives, how a draft is stored so `notmuch` can see +it, what reply and forward do to the thread the user is looking at, and how a +queued message reaches the sending script. + +**Constraints.** + +- **Ask the user before designing.** They have not said what they pictured, and + this is the item where guessing costs the most. +- Drafts are already visible to the app: item 67 counts them in the placeholder + pane, so a draft folder is configured and indexed. +- `#new-branch` is the user's own tag on it. + +**Size: `?`** until the brainstorm has happened. It is the largest open item by +some distance. + +## 124. The worker reads the index directory as the mail root + +**Observed (measured, 2026-08-20):** not reported from use. Found while +measuring item 121's cold-start cost, which established that the 1.1 GB notmuch +index sits on a 7200rpm platter (`/dev/sda1`, `rotational: 1`) while an NVMe SSD +sits idle in the same machine. Moving the index to the SSD is a notmuch +configuration change and needs no code, but qtmaildir does not survive it. + +**Cause (verified against notmuch 0.39 and a throwaway database).** notmuch +supports splitting the index from the mail with two keys: + +```ini +[database] +mail_root=/data/Mail +path=/home/you/.local/share/notmuch +``` + +Under that layout `notmuch_database_get_path()` returns the **index** +directory, not the mail root. Measured on a split test database: + +| accessor | legacy (`path` only) | split | +|---|---|---| +| `notmuch_database_get_path()` | mail root | **index dir** | +| `notmuch_config_get(NOTMUCH_CONFIG_MAIL_ROOT)` | mail root | mail root | + +`notmuchworker.cpp` calls `get_path()` at six sites and treats every one as the +mail root: lines 297, 755, 900, 901, 1062, and the `relativeFilePath()` calls +at 364, 380 and 918 that consume them. + +**One of those six moves mail, and it is the dangerous one.** +`moveMessages` composes its destination as +`root + "/" + destFolder + "/cur"` (`src/notmuchworker.cpp:755`). Under a split +config `root` is the index directory, so Delete would move the message into +`/Trash/cur`: outside the Maildir, invisible to mbsync, and gone from +every other client. That is item 103's stranded-mail failure with a new cause, +and this repo has already shipped that class of bug once. + +The rest degrade rather than destroy. `relativeFilePath()` against the wrong +root yields `../../../data/Mail/account/cur/...` instead of `account/cur/...`, +so no path matches an account prefix and every row resolves to no account, +which is exactly what the comment at line 294 already warns about. + +**Approach.** Replace `notmuch_database_get_path()` with +`notmuch_config_get(m_db, NOTMUCH_CONFIG_MAIL_ROOT)` at the six sites. + +**No conditional is needed, and that is the point.** `MAIL_ROOT` returns the +mail root under BOTH layouts, verified above: under a legacy `path`-only config +it equals `get_path()`, so the change is a no-op against the current +configuration and correct against the split one. A fallback to `get_path()` when +`MAIL_ROOT` is NULL is the tempting belt-and-braces addition and should be +resisted unless a NULL is actually observed, since it reintroduces the wrong +answer on the path where it matters. + +**Constraints.** + +- **`moveMessages` is the site to test hardest.** A wrong root there reaches the + mail server, per the "Delete MOVES the file" note in `CLAUDE.md`. +- The test fixture must build a database whose index is NOT inside the mail + root, or it cannot tell the two accessors apart: under the ordinary fixture + layout both return the same string and a mutation stays green. +- `notmuch_config_get` returns a string owned by notmuch (`notmuch.h:2585`); + do not free it. +- Nothing about the legacy layout may change. The migration is the user's to + perform, separately, once this ships. + +**Size: S.** Six call sites and a fixture that can tell them apart. + ## Deferred, unsized, or split out Items noted while triaging but not part of the original list. Same numbering -- cgit v1.2.3