diff options
Diffstat (limited to 'docs/superpowers')
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 149 |
1 files changed, 149 insertions, 0 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 6beb35b..5d175a5 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 @@ -111,6 +111,8 @@ taking that too literally. | 51 | Clicking a subject scrolls the list sideways | presentation | XS | open | | 52 | `test_querycompleter` fails under Wayland, passes offscreen | testing | XS | **done** | | 53 | Message rows still read as a table, not as a conversation | presentation | ? | open, unspecified | +| 54 | A cron sync carries the edits but the count still says pending | correctness | S | **done** | +| 55 | In a narrow window the message pane is invisible | presentation | XS | open | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -3238,6 +3240,153 @@ to answer for the strip on thread rows, which must not change. **Do not start any of these from this description.** It says what is and why, not what was wanted. +## 54. A cron sync carries the edits but the count still says pending + +**Observed (user, 2026-08-08):** "If I apply some edits in the program, then the +sync runs from crontab, my edits should go through the sync, instead I still see +`N changes pending`." + +**Cause, verified in code.** The edits really do go out; only the count is +wrong. A tag edit reaches the notmuch index at edit time, so a `notmuch new` +fired by cron carries it to the mail store exactly as a local sync would. But +the pending count is cleared in **one place only**: the success branch of the +local sync-finished handler, `m_pendingTagEdits.clear()` at +`src/mainwindow.cpp:1719`, reached from the `MailSync` process this window +started. + +The external path never touches it. `onExternalSyncStateChanged()` +(`src/mainwindow.cpp:1852`) is driven by `SyncMonitor` watching the lock in +`/proc/locks`. On `State::Idle` it clears `m_externalSyncBusy`, calls +`flushHeldEdits()`, and shows "Background sync completed. Press Enter in the +query bar to refresh." (`src/mainwindow.cpp:1901`). It does not clear +`m_pendingTagEdits`, does not reset `m_unnettablePendingEdits`, and does not +drain `m_editedAccounts`. So the indicator keeps counting edits that have +already shipped, until the user runs a sync from the window. + +This is a **defect, not an enhancement**, and it is the same class as item 28: +the indicator exists to answer "is my work safe to quit on", and here it says no +when the answer is yes. It also feeds the exit prompt (`pendingEditCount()` at +`src/mainwindow.cpp:173`), so the user is asked to sync on quit for work that a +cron run already carried. Adjacent to item 49, which made the account set drive +which channels a sync runs: `m_editedAccounts` is stale in exactly the same way. + +**Approach.** Clear the same three pieces of state on an observed external +`Idle` that a successful local sync clears. Two things make this harder than +copying the block, and both must be answered before it is written: + +- **The monitor sees a lock, not an outcome.** A cron run that fails releases + the lock exactly as a successful one does, and the comment at + `src/mainwindow.cpp:1715` records the rule that only a *successful* sync may + clear the count. An external run's exit status is not observable from + `/proc/locks`. Either the count is cleared optimistically on any external + release, or `mailsync.sh` grows a status file the window can read. Ask before + choosing: the optimistic version can clear a count whose edits a failed cron + run did not carry. + + **Resolved 2026-08-09, and the dilemma was false: the script already writes + the outcome.** Every run ends with a + `===== RUN END: <ts> status=OK =====` or `status=FAILED mbsync=<n> + notmuch=<n>` banner in its log (`assets/mailsync.sh:115-117`), which survives + the process that wrote it. `MailSync::lastRunOutcome()` reads it, so neither + the optimistic clear nor a change to the sync script was needed. The user + chose this over both alternatives. +- **The held-edit race is already solved for the local path and must hold + here.** `flushHeldEdits()` is called on the external `Idle` branch too, and it + calls `sendThreadTagChange()`, which writes `m_editedAccounts` + **synchronously**, while the pending map is written on the worker's queued + reply. The local path handles this by snapshotting `m_editedAccounts` before + the flush (`src/mainwindow.cpp:1711`) and subtracting only the snapshot. + Anything written here has to do the same, or edits made during the cron run + are marked as carried by the sync that did not carry them. + +**Constraints.** + +- `State::Unknown` must not clear anything. It means `/proc/locks` could not be + read, so nothing was observed, and the existing code is careful to distinguish + that from `Idle`. +- The local path must keep working unchanged, including the `m_localSyncHoldsLock` + early return at `src/mainwindow.cpp:1874`, which exists so a local sync's own + lock release is not mistaken for an external one. + +**Verification.** `test_mainwindow` can drive `onExternalSyncStateChanged()` +directly, which is how the existing external-sync tests work, so this does not +need a real cron run. Assert on the count and on the exit prompt, and mutate: +the test must fail if the clear is removed. Item 49's account-set behaviour +needs its own assertion, since a count that reaches zero while +`m_editedAccounts` stays full would look correct and still sync the wrong +channels. + +**Built 2026-08-09.** `SyncOutcome` and `MailSync::lastRunOutcome()` parse the +banner; `Config::syncLog()` supplies the path, defaulting to the script's own +and overridable through a new `[sync] log` key so a test never reads the +developer's real log. `onExternalSyncStateChanged()` clears the map, the +unnettable counter and the account set on `Idle` **and** a definite `OK`, before +`flushHeldEdits()`, matching the local path's ordering. 13 tests across +`test_mailsync`, `test_config` and `test_mainwindow`; 15/15 binaries green. + +Two notes on the verification, both worth more than the passing count: + +- **A timing probe endorsed a tail read that was not happening.** The first + version of `lastRunOutcomeReadsATailOfAHugeLog` required the call under + 100 ms, and it **passed with the seek deleted**, because reading 10 MB is + fast either way. It measured nothing, exactly as CLAUDE.md's rendering-probe + entry describes. Replaced with an assertion on content: a marker reachable + only from the head of a large file must be invisible to a tail read, plus a + guard appending a marker within reach to prove the parser can still find one. + That version fails when the seek is removed. +- **Every fixture was invented, and the first batch was wrong.** They wrote the + banner as `RUN END: 2026-08-09 10:20:03`, where the script uses + `date -Iseconds` (`assets/mailsync.sh:111`) and so emits + `2026-08-09T09:20:33+02:00`. The tests passed anyway, because the parser keys + on the `===== RUN END:` prefix and the `status=` token and never looks at the + timestamp. Found only by reading the user's real log while waiting for a cron + run, not by any test. A fixture invented to match the code tests the code + against itself. `lastRunOutcomeReadsABannerTheScriptActuallyWrote` now builds + the line by running `date -Iseconds` the way the script does, and the parser + was confirmed against the real `~/.local/state/mailsync.log`, which it reads + as `Ok`. +- **The ordering claim is not covered by any test.** With no lock file present + `aSyncHoldsTheWriteLock()` is false throughout the suite, so `flushHeldEdits()` + is a no-op and moving the clear after it changes no result. The ordering is + inherited from the local path rather than independently verified; a test for + it needs a held lock, the way item 37's tests stage one. + +## 55. In a narrow window the message pane is invisible + +**Observed (user, 2026-08-08):** "when opening in a squared window, the left +pane takes the whole width (right pane almost invisible)". + +**Cause, verified in code.** The splitter is built at +`src/mainwindow.cpp:584` with `setStretchFactor(1, 2)` and **no** +`setSizes()`, and neither pane carries a minimum width. A stretch factor only +distributes space left over **after** every child's size hint is satisfied. The +thread view's hint follows its columns, which are set to fixed widths at +`src/mainwindow.cpp:557-561` and total roughly 886px (28 + 28 + 130 + 180 + +520), every one of them `Interactive` with `setStretchLastSection(false)` at +`src/mainwindow.cpp:513`. In a window narrower than that sum plus a usable +message pane there is no leftover space at all, so the stretch factor never +applies and the thread view takes essentially everything. + +This only bites on first run. Once a splitter position is saved it is restored +from `window/splitter` (`src/mainwindow.cpp:120-123`), which is why the window +behaves after the user has dragged it once. Item 1 built that persistence; it +does not supply a sane starting point. + +**Approach.** Give the message view a minimum width so the splitter cannot +collapse it, and set an initial `setSizes()` proportional to the window rather +than letting the hints decide. Sizes only apply when nothing is restored, so it +must go behind the same "no saved state" check as the geometry. + +**Constraints.** + +- Must not fight the restore path. A `setSizes()` call that runs after + `restoreState()` would throw away the user's saved position, which is item 1. +- A minimum width on the thread view as well would reintroduce the same problem + from the other side. Only the message pane needs one. +- `resize(1200, 800)` at `src/mainwindow.cpp:206` is the default only when no + geometry is saved; the reported case is a user-sized window, so widening the + default fixes nothing. + ## Deferred, unsized, or split out Items noted while triaging but not part of the original list. Same numbering |