From 1062eedaff00a46918f379bd70d210ce95861c93 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Sat, 15 Aug 2026 15:54:03 +0200 Subject: fix(sync): re-arm the automatic sync when it skips a concurrent run runAutoSync() returned without rescheduling when a sync was already in flight. The comment defending it argued the edits were not lost, because they reached the mail store at edit time and the running sync was "very likely" to carry them. Very likely is not always: an edit made after mbsync has already passed that account's mailbox is not carried by it, the timer had fired, nothing re-armed it, and the pending count sat non-zero until a manual sync or the next cron run. Skipping is unchanged and still required by item 71: the cron job holds the same lock and mbsync fails on a second concurrent run. What changes is that the skip schedules another attempt. scheduleAutoSync() re-checks the delay, the sync command and the pending count on the way in, so this cannot arm a sync for nothing, and against a long external sync it re-arms once per debounce interval, which is a timer rather than a sync. The test fires the timer by hand and asserts it is active again afterwards, at the configured interval rather than a shorter one, with the pending indicator still showing. It fails against the old skip path. Item 89's other half is dropped rather than built. The list churn it described is a tag-defined view working as intended: a thread that loses `unread` leaves the Unread view, and the user resolved it by living in the Inbox view instead. Three designs were drafted before asking and none is worth building. Co-Authored-By: Claude Opus 5 --- .../2026-08-03-post-0.1.0-usability-closed.md | 80 ++++++++++++++++++++++ .../plans/2026-08-03-post-0.1.0-usability.md | 61 +---------------- 2 files changed, 81 insertions(+), 60 deletions(-) (limited to 'docs') diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md index d5f58dd..393279e 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 @@ -5338,3 +5338,83 @@ how the first version of it was green. It asserts the ids are cleared, because that is the fix's contract, and then that the pane leaves the placeholder, which is what the user sees. Mutation check: reverting the fix fails it with "runQuery blanked the pane but still names a current thread". + +## 89. A sync moves the list under the user's hands, and the auto-sync skips rather than retries + +**Observed (user, notes):** "the auto sync after a delay needs to be reviewed, +its behavior is not exactly right." Then, more broadly: "the sync in general is +worth rethinking, as it is now is not polished and shows too many moving parts. +messages disappearing from views, lists changing while the user is +interacting." And the workaround the user already found: "setting Inbox as the +default view mitigates the problem as messages are not removed from the list +after being read for 2s." + +**Two separate faults sit under one complaint**, and only the first is small. + +**Cause, the concrete half.** `MainWindow::runAutoSync()` +(`src/mainwindow.cpp:3213`) returns without rescheduling when a sync is already +in flight: + +```cpp +if (m_externalSyncBusy || (m_sync && m_sync->isRunning())) + return; +``` + +The comment beside it argues the edits are not lost, because they reached the +mail store at edit time and the running sync is "very likely" to carry them. +Very likely is not always: an edit made after the running mbsync has already +passed that account's mailbox is not carried, the timer has fired and is not +re-armed, and nothing arms it again until the next edit. The pending count then +sits non-zero until a manual sync or the cron job. That is exactly "not exactly +right", and it is a missing `m_autoSyncTimer->start(delay)` on the skip path +rather than a redesign. `scheduleAutoSync()` (`:3184`) already re-checks +everything on the way in, so restarting the timer there is safe by its own +argument. + +**Cause, the larger half.** Nothing to do with the auto-sync: it is what a +refresh does to the list. The refresh after a sync replaces the result set, and +a query like `tag:unread` no longer matches a thread the user has just read, so +rows vanish from under the pointer. Item 35 built the refresh to keep the user's +place, and it does, but keeping the selection is not the same as keeping the +row: a thread that has left the result set has nowhere to be kept. The user's +own mitigation, using an `tag:inbox` view where reading does not change +membership, is the real diagnosis. + +**Approach.** Ship the timer restart on its own, as an XS fix with a test that +arms the timer while a sync is running and asserts it is still active. Then +treat the list-churn half as a design question and put it to the user before +building: the plausible answers (defer a refresh while the pointer is over the +list, keep a read thread visible until the next explicit query, refresh only +rows rather than the result set) differ enough in feel that guessing wastes the +work. + +**Constraints.** + +- The skip itself must stay. Item 71 requires it and mbsync fails on a second + concurrent run; this item restarts the timer, it does not queue a sync. +- A restart must not turn into a spin against a long external sync. The delay is + the debounce interval, and `SyncMonitor` polls `/proc/locks`, so an + `m_externalSyncBusy` that never clears would re-arm indefinitely at that + interval. Cheap, but say so in the test. +- Do not restore the pre-0.16.0 behaviour by making the delay negative for the + user. `auto_sync_delay_ms` is theirs to set. + +**Outcome, 2026-08-15.** Split, and only half was built. + +**The timer half shipped.** `runAutoSync()` now calls `scheduleAutoSync()` on +the skip path instead of returning. The skip itself is unchanged, as item 71 +requires. `scheduleAutoSync()` re-checks the delay, the sync command and the +pending count on the way in, so it cannot arm a sync for nothing, and against a +long external sync it re-arms once per debounce interval, which is a timer and +not a sync. + +**The list-churn half is dropped rather than deferred, at the user's own +reading of it:** "this part I think I solved by using inbox as main view and +checking unread from time to time. It's more of my mental model that was +causing the issue. Unread is supposed to be volatile because once you read a +mail it no longer belongs there." A view defined by a tag stops showing a +thread when the thread loses that tag, and that is the view working. Three +designs were drafted before asking (keep non-matching rows until an explicit +query, defer the refresh while the pointer is over the list, append-only +refresh) and none is worth building against a complaint that resolved itself. +Reopen only if the churn is reported somewhere it is NOT the view doing its job. 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 c376f6e..14a242a 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 @@ -153,7 +153,7 @@ taking that too literally. | 84 | A config problem blocks `test_mainwindow` on a modal nobody can dismiss | testing | S | **done** 2026-08-14, unreleased. `showWarnings()` split: the status label stays in the constructor, `main.cpp` raises the modal after `show()` | | 85 | Nothing on screen can be searched for by right-clicking it | workflow | M | **done** 2026-08-14, unreleased; see `specs/2026-08-14-search-from-message-design.md`. Split from 78; rebuilt the details dialog as rows | | 86 | A right-click search can replace or narrow, but never exclude | workflow | S | **done** 2026-08-14, unreleased; see `specs/2026-08-14-exclude-from-search-design.md`. Follows 85. The `extend` bool became a `SearchMode` enum across four signatures | -| 89 | A sync moves the list under the user's hands, and the auto-sync skips rather than retries | workflow | M | open; from the 2026-08-15 notes pass. Two faults under one complaint, and the larger half is a design question | +| 89 | A sync moves the list under the user's hands, and the auto-sync skips rather than retries | workflow | XS | **done** 2026-08-15, unreleased. The timer half only: a skipped auto-sync re-arms instead of giving up. The list-churn half is **dropped**, not built: the user resolved it as a mental-model question, an Unread view is SUPPOSED to be volatile | | 90 | A saved-query button clears the account selection | workflow | S | **folded into 93** 2026-08-15. Not fixed in place: the button that misbehaves stops being a saved query at all. See `specs/2026-08-15-builtin-filters-design.md` | | 91 | Double-clicking a thread could open it in its own window | workflow | ? | open, unspecified; the user marked it "(?) UX not sure" | | 92 | Nothing distinguishes a tag written by a rule from one the user applied | information | ? | open, unspecified; the user asked it as a question, and the answer decides whether it is a display item or a format change across two repos | @@ -447,65 +447,6 @@ in CLAUDE.md. **Size: S**, down from M now that item 85 has built the menus and item 81 the seeded dialog. -## 89. A sync moves the list under the user's hands, and the auto-sync skips rather than retries - -**Observed (user, notes):** "the auto sync after a delay needs to be reviewed, -its behavior is not exactly right." Then, more broadly: "the sync in general is -worth rethinking, as it is now is not polished and shows too many moving parts. -messages disappearing from views, lists changing while the user is -interacting." And the workaround the user already found: "setting Inbox as the -default view mitigates the problem as messages are not removed from the list -after being read for 2s." - -**Two separate faults sit under one complaint**, and only the first is small. - -**Cause, the concrete half.** `MainWindow::runAutoSync()` -(`src/mainwindow.cpp:3213`) returns without rescheduling when a sync is already -in flight: - -```cpp -if (m_externalSyncBusy || (m_sync && m_sync->isRunning())) - return; -``` - -The comment beside it argues the edits are not lost, because they reached the -mail store at edit time and the running sync is "very likely" to carry them. -Very likely is not always: an edit made after the running mbsync has already -passed that account's mailbox is not carried, the timer has fired and is not -re-armed, and nothing arms it again until the next edit. The pending count then -sits non-zero until a manual sync or the cron job. That is exactly "not exactly -right", and it is a missing `m_autoSyncTimer->start(delay)` on the skip path -rather than a redesign. `scheduleAutoSync()` (`:3184`) already re-checks -everything on the way in, so restarting the timer there is safe by its own -argument. - -**Cause, the larger half.** Nothing to do with the auto-sync: it is what a -refresh does to the list. The refresh after a sync replaces the result set, and -a query like `tag:unread` no longer matches a thread the user has just read, so -rows vanish from under the pointer. Item 35 built the refresh to keep the user's -place, and it does, but keeping the selection is not the same as keeping the -row: a thread that has left the result set has nowhere to be kept. The user's -own mitigation, using an `tag:inbox` view where reading does not change -membership, is the real diagnosis. - -**Approach.** Ship the timer restart on its own, as an XS fix with a test that -arms the timer while a sync is running and asserts it is still active. Then -treat the list-churn half as a design question and put it to the user before -building: the plausible answers (defer a refresh while the pointer is over the -list, keep a read thread visible until the next explicit query, refresh only -rows rather than the result set) differ enough in feel that guessing wastes the -work. - -**Constraints.** - -- The skip itself must stay. Item 71 requires it and mbsync fails on a second - concurrent run; this item restarts the timer, it does not queue a sync. -- A restart must not turn into a spin against a long external sync. The delay is - the debounce interval, and `SyncMonitor` polls `/proc/locks`, so an - `m_externalSyncBusy` that never clears would re-arm indefinitely at that - interval. Cheap, but say so in the test. -- Do not restore the pre-0.16.0 behaviour by making the delay negative for the - user. `auto_sync_delay_ms` is theirs to set. ## 94. `pinned` has nothing left to decide once the buttons are built-in -- cgit v1.2.3