aboutsummaryrefslogtreecommitdiffstats
path: root/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md
diff options
context:
space:
mode:
authorDanilo M. <danix@danix.xyz>2026-08-28 19:17:06 +0200
committerDanilo M. <danix@danix.xyz>2026-08-28 19:17:06 +0200
commitdb38ca136c45f34f5fa2d342ee180f068073be37 (patch)
treee5ec786f47989bb594fca896802849b365056ddc /docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md
parentae74237ca11640ceb68887b2c6d09ecf1befb342 (diff)
downloadqtmaildir-db38ca136c45f34f5fa2d342ee180f068073be37.tar.gz
qtmaildir-db38ca136c45f34f5fa2d342ee180f068073be37.zip
docs: close items 170, 176 and 177
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.md159
1 files changed, 159 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 81705a2..f062106 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
@@ -9090,3 +9090,162 @@ forwards correctly with the strip box both checked and unchecked. The
recipient sees the original's formatting either way; with the box checked the
remote images are gone and the layout survives without them.
+## 170. A row that stops matching the view only leaves it on the Delete path
+
+**Observed (user, from the notes):** "should we refactor the list UI to be
+responsive so changes are applied immediately instead of waiting for a view
+change to repaint?"
+
+**Cause (verified in the code, 2026-08-26).** Two different properties were
+being called "responsive", and only one of them was built.
+
+The optimistic **repaint** is universal. `ThreadListModel::applyTagChange()`
+covers a thread-scoped write, `applyMessageTagChange()` a message-scoped one
+(items 105 to 111), and `revertPendingTagChange()` undoes either if the write
+is rejected. A chip, a bold row and a dimmed row all move the moment the user
+acts.
+
+The optimistic **membership** is not. `ThreadListModel::removeThreadsWithoutTag()`
+has exactly ONE caller, in `trashMessages()`, added last session because Delete
+strips `inbox` and a deleted message sat in the Inbox view across restarts. The
+ordinary tag path never calls it: neither `sendMessageTagChange()` nor
+`sendThreadTagChange()` asks whether the row still belongs in the view.
+
+So in the Unread view, marking a message read repaints the row and leaves it in
+a list defined by `tag:unread`, which it no longer matches. Un-flagging in the
+Flagged view is the same, and so is removing `inbox` by hand from the Inbox
+view. It corrects itself at the next query or sync, which is exactly the "waits
+for a view change" the note describes.
+
+**Approach.** Not a refactor. `viewFilterTag()` already resolves the view's own
+tag from the query, and `removeThreadsWithoutTag()` already does the removal.
+The gap is that the guard sits in `trashMessages()` rather than at the funnel
+every tag write passes. Move it, or call it from both send paths.
+
+**Constraints.**
+
+- The guard's existing reasoning is what makes this safe and must be kept: only
+ a plain `tag:<x>` view has a membership one tag decides. A path query (Trash,
+ Sent, Drafts) is unaffected by a tag going away, and a hand-typed query cannot
+ be reasoned about. Both are left alone. Without that, marking read in an `id:`
+ view would empty the list.
+- A row leaving is not revertible by `revertPendingTagChange()`, which repaints
+ rather than reinserts. A REJECTED write would leave the row gone until the
+ next query. The move path already carries that exposure; check whether it is
+ acceptable at the tag path's much higher frequency, or make the removal wait
+ for confirmation there.
+- The inverse case is deliberately out of scope: a row that starts matching
+ cannot be inserted optimistically, since the model has no summary for a
+ thread the query never returned.
+- Undo goes back through the same funnel, so a removal must not make an undone
+ mark-read invisible in the view it was undone in.
+
+**Resolved 2026-08-28**, on `thread-row-identity`, released unreleased.
+
+`MainWindow::syncViewMembership()` is the guard. It was moved off the move
+path, where `trashMessages()` was its only caller, and is now called from all
+three funnels: the message write, the thread write and the move. The existing
+reasoning under Constraints is kept unchanged, so only a plain `tag:<x>` view
+is acted on; a path query and a hand-typed query are still left alone.
+
+Three things the section did not anticipate:
+
+- **The inverse case is built after all.** A write that puts the view's tag
+ BACK refreshes the list. The model still cannot insert a row for a thread the
+ query never returned, which is why it is a refresh rather than an insertion,
+ but leaving it out made an undone mark-read invisible in the view it was
+ undone in, which is worse than the query cost.
+- **A row is never evicted while it is current**, and an AUTOMATIC write defers
+ its eviction until the selection moves. The automatic mark-read fires two
+ seconds after selection, so evicting on it takes the row out from under the
+ user mid-read. A write the user asked for evicts at once.
+- **A second defect was found while testing it.**
+ `ThreadListModel::applyTagChange()` never updated the ROOT message's own
+ tags, which a thread row's card drew in preference to the summary since item
+ 110, so an archived thread both kept drawing `inbox` and was judged to still
+ match. Item 177 has since removed that per-message tier entirely, so the
+ card and the judgement now read the same union.
+
+One lag is documented rather than fixed, in the spec and in CLAUDE.md: reading
+the LAST unread message of a long conversation does not evict it immediately,
+because `applyMessageTagChange` deliberately leaves a long thread's summary
+alone and judging on a stale union would be wrong in both directions. The row
+leaves at the next query or sync.
+
+## 176. Undoing a thread-scoped action applies its inverse to messages it never changed
+
+**Observed (user, 2026-08-28):** found while hand-testing item 170. A
+thread-scoped `Mark thread read`, then Ctrl+Z, left almost the whole
+conversation unread rather than restoring the two messages that had been.
+
+**Cause (measured, not read).** `ThreadTagCommand::undo()`
+(`src/mainwindow.h`) sends `sendThreadTagChange(m_threadIds, m_remove, m_add,
+...)`: the tags are inverted and the SCOPE is not. `applyTags()` is a blind
+add/remove over whatever ids the thread resolves to, so the inverse of
+"remove `unread` from 44 messages" is "add `unread` to 44 messages",
+regardless of which of them carried it.
+
+Measured on the live index: thread of 44 messages, 2 unread. Mark thread read
+resolved 44 and removed the tag; the undo resolved 44 and added it, leaving
+43 unread. The 42 that were read before the user touched anything were
+rewritten. `maildir.synchronize_flags` is on, so the Maildir filenames were
+rewritten too and the next sync would have carried it to the server.
+
+**Approach (not decided).** The command has to record what the write actually
+CHANGED, not what it asked for. `applyTags()` is the only place that knows:
+it holds each message open and can report the ids whose tags actually moved.
+That is a worker change (`tagsApplied` carrying the effective set) plus a
+command that stores it.
+
+**Constraints.**
+- The undo stack is this application's substitute for confirmation dialogs
+ (CLAUDE.md), so an undo that damages state is worse than the dialog it
+ replaces.
+- `MessageTagCommand` has the same shape. It is harmless on a single message,
+ where asked and changed agree, and has the same defect on a multi-row
+ selection.
+- The five `*_thread` actions are the ones that resolve to a large id set, so
+ they carry almost all of the exposure.
+- A test needs a thread whose messages DISAGREE about the tag. Two messages in
+ the same state answer identically whichever way the code resolves them,
+ which is the trap CLAUDE.md already records for item 87.
+
+**Resolved 2026-08-28**, on `thread-row-identity`, unreleased.
+
+The Approach was right about where the knowledge lives.
+`NotmuchWorker::applyTags()` now reads each message's tags before writing and
+reports back only the ids whose tags actually MOVED. A `TagCommand` base
+carries that effective set, and both `ThreadTagCommand` and
+`MessageTagCommand` derive from it, so the multi-row message case named under
+Constraints is fixed by the same change rather than left behind.
+
+Two details that are decisions rather than implementation:
+
+- **`tagsApplied` does not fire when the effective list is empty.** Emitting an
+ empty change would push an undo entry whose inverse adds a tag no message
+ ever carried, which is this same defect one step later.
+- **`sendThreadTagChange` takes an `onlyMessageIds` parameter** so it keeps the
+ THREAD as its repaint scope while restricting the WRITE to that set. The card
+ that changed on screen and the messages that changed on disk are genuinely
+ different sets, and collapsing them would either under-repaint or over-write.
+
+**The spec's own order of work was wrong about this item**, and the correction
+is worth keeping because the wrong version reads as sound. It said item 177
+would make a thread action's undo "honestly what the user asked for", leaving
+only the multi-row message case exposed. An undo is not the inverse of a SCOPE,
+it is the inverse of an EFFECT: the user asking to mark a whole conversation
+read does not make it honest to mark the whole conversation unread afterwards,
+because most of it was already read before they touched anything. Item 177
+changed nothing about the exposure except to make the largest case reachable by
+the ordinary gesture rather than by a submenu entry. Corrected in
+`specs/2026-08-28-thread-row-identity-design.md` rather than deleted from it.
+
+**Evidence.** Measured on the user's live index before the fix: a conversation
+of 44 messages holding 2 unread was marked read, then undone, and came back
+with 43 unread. The 42 that had been read before the user touched anything were
+rewritten. `maildir.synchronize_flags` is on, so the Maildir filenames were
+rewritten too and the next sync would have carried it to the mail server. The
+mail was repaired by hand the same day. The regression test builds a thread
+whose messages DISAGREE about the tag, since two messages in the same state
+answer identically whichever way the code resolves them, which is the trap item
+87 already records.