From feb15bcec261501eb49ad0de1fdc05a9992227c2 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Sat, 29 Aug 2026 10:05:22 +0200 Subject: fix: refresh the conversation dashboard after a write Item 181, from the user's notes: "the thread dashboard doesn't update live with the modifications applied to the list pane. If I mark the thread as read, the dash still reports N unread". ThreadDashboard draws a ThreadDigest, which the worker builds from the index and which reached the pane only when a conversation was selected. A tag write updated the model optimistically and repainted the card beside it, and nothing touched the digest, so the pane went on reporting the unread count, the progress bar and the Waiting-for-you list the conversation had when it was opened. Reachable from the dashboard's own Mark all read button, which is the worst version of it: the number sits directly above the button that fails to move it. refreshDashboardDigest() re-asks the worker for the digest of the conversation on display, and returns at once when the pane is showing anything else. It bumps m_digestGeneration like any other request, so the guards in onThreadDigestLoaded() discard a reply that arrives after the user has moved on. No placeholder digest, unlike the selection path: the pane already holds this conversation, and blanking it to re-fill it would flicker the whole dashboard for a change to one number. Called from onTagsApplied(), where a write is CONFIRMED, and not from the two write funnels. The first attempt put it beside the optimistic model update by analogy with every other optimistic repaint, and that analogy does not hold here: the digest is rebuilt from the index, so a refresh queued beside the write reaches the worker before the write does and answers from the state before it. The test failed identically to no fix at all. Every write rather than a chosen subset, at the user's decision: narrowing it to the writes that change what the dashboard happens to draw today is a list the dashboard can outgrow silently, and this costs a round trip only while a conversation is on screen. Re-requested rather than edited in place, because the digest is a derived summary and recomputing it here would be a second place that has to agree with the worker about what a write did. The test is worker-backed over a real two-message conversation and is driven through the mark_all_read action rather than the private funnel, which is the path the dashboard's own button takes. It asserts the pane carries the unread state before the write, so the assertion after it means something. Suite: 42 of 43, with undoMovesTheMessageBack failing as it does on master (item 136). --- CHANGELOG.md | 6 ++ .../2026-08-03-post-0.1.0-usability-closed.md | 59 ++++++++++++++++ .../plans/2026-08-03-post-0.1.0-usability.md | 1 + src/mainwindow.cpp | 25 +++++++ src/mainwindow.h | 24 +++++++ tests/test_mainwindow.cpp | 78 ++++++++++++++++++++++ 6 files changed, 193 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 34d9d2e..cf7082d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -143,6 +143,12 @@ point at which they are stable. ### Fixed +- **The conversation dashboard now follows what you do to the conversation.** + Marking a thread read left the pane reporting the unread count, the progress + bar and the "Waiting for you" list it had when you opened it, including when + the write came from the dashboard's own Mark all read button, where the + number sits directly above the button that failed to move it. Every tag + write to the conversation on display now refreshes it. - **Delete and Restore now judge a whole conversation.** They asked whether a row was in the trash by looking at one of its messages, so a conversation with some messages trashed and some not answered on whichever the query 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 55c5ee7..27b4e28 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 @@ -9315,3 +9315,62 @@ before proceeding. A summary left at the default is a MESSAGE row, so a test meaning to exercise a conversation would quietly exercise the other branch and pass for the wrong reason, which is the fixture trap the top-level document records. + +## 181. The thread dashboard does not follow a write to the conversation it shows + +**Done 2026-08-29**, unreleased, on `thread-row-identity`, from the user's +notes: "the thread dashboard doesn't update live with the modifications applied +to the list pane. If I mark the thread as read, the dash still reports N +unread". + +**Cause, verified in the code.** `ThreadDashboard` draws a `ThreadDigest`, +which the worker builds from the INDEX and which reached the pane in exactly +two places: the selection handler, and `onThreadDigestLoaded()`. A tag write +updated the model optimistically and repainted the card beside it, and nothing +touched the digest, so the pane went on reporting the unread count, the +progress bar and the "Waiting for you" list the conversation had when it was +opened. + +**Reachable from the dashboard's own button.** It carries Mark all read, +Archive and Delete in its bottom strip (item 177), so the user presses Mark all +read and the number directly above it does not move. That is the worst version +of the defect and the one the note reports. + +**Fix.** `MainWindow::refreshDashboardDigest()` re-asks the worker for the +digest of the conversation the pane is showing, and returns at once when the +pane is showing anything else, which is the ordinary case. It bumps +`m_digestGeneration` like any other request, so the three guards in +`onThreadDigestLoaded()` discard a reply that arrives after the user has moved +on. No placeholder digest, unlike the selection path: the pane already holds +this conversation, and blanking it to re-fill it would flicker the whole +dashboard for a change to one number. + +**Called from `onTagsApplied()`, where a write is CONFIRMED, and this is the +part worth keeping.** The first attempt called it from both write funnels, +beside the optimistic model update, by analogy with every other optimistic +repaint. That is wrong here and the test caught it: the digest is rebuilt from +the index, so a refresh queued beside the write reaches the worker BEFORE the +write does and answers from the state before it. The reasoning that made it +look right is the reasoning that applies to a model update, which needs no +round trip; anything read back from the index has to wait for the write to +land. + +**Every write, not a chosen subset.** The user's decision, and the cheaper rule +to keep: narrowing it to the writes that change what the dashboard happens to +draw today is a list the dashboard can outgrow silently, and this costs a round +trip only while a conversation is on screen. It sits beside the unsynced-edits +counter in the same handler for the same reason. + +**Re-requested rather than edited in place.** The digest is a derived summary, +senders and buckets and timestamps and a capped unread list, so updating it +here would be a second place that has to agree with the worker about what a +write did, which is item 180's complaint exactly. + +**Test.** `theDashboardFollowsAWriteToTheConversationItShows`, worker-backed +over a real two-message conversation, driven through the `mark_all_read` ACTION +rather than the private funnel: that is the path the dashboard's own button +takes. It asserts the pane carries the unread state BEFORE the write, so the +assertion after it means something, and mutation-checked by removing the call. +Note the failing runs take 45s and the passing one 295ms: the `QTRY` timeouts +expire when the refresh never comes, which reads as a slow test rather than a +missing feature. 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 1f5a45d..68c080c 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 @@ -254,6 +254,7 @@ taking that too literally. | 175 | The send countdown says Undo, and cannot be skipped | presentation | XS | open, 2026-08-28, from the notes. Two changes in one control: the button reads Abort, and a second button sends immediately rather than waiting the countdown out | | 179 | Undo is one level deep in practice, and there is no Redo | workflow | ? | open, 2026-08-29, from the notes. The `QUndoStack` is real and multi-level; what is missing is a `redo` action (absent from `knownActions()`, never called) and an answer to the stack being CLEARED on every new query (`mainwindow.cpp:3458`), which is what makes a deep stack behave like a shallow one. The clear has a correct reason and cannot simply be removed. Redo re-applies a write to real mail, so item 176's rule binds it too | | 180 | The repaint rules are discovered one hole at a time | maintenance | S-L | open, 2026-08-29, from the notes, and a QUESTION rather than a defect. Items 105, 107, 109, 110 and 170 are each one hole in the same surface, all found by hand. Three mechanisms (optimistic repaint, `syncViewMembership()`, revert) agree by documentation rather than by code. Cheapest answer is one invariant test, not a rewrite; the user decides which, and that decides the size | +| 181 | The thread dashboard does not follow a write to the conversation it shows | defect | XS | **done 2026-08-29**, unreleased, on `thread-row-identity`, from the notes. The dashboard draws a `ThreadDigest` built by the worker from the INDEX, which arrived only on selection, so a tag write moved the model and the card and left the pane reporting the count the conversation had when it was opened. Reachable from the dashboard's OWN Mark all read button. Re-requested from `onTagsApplied()`, where the write is confirmed: queued beside the write it races it and answers from the state before it, which is how the first fix passed review and failed the test. Section in the closed file | Sizes are rough: XS under an hour, S a sitting, M a session. diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index eb8f4c6..df7062d 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -4215,6 +4215,25 @@ void MainWindow::onThreadSelected(const QModelIndex ¤t, Q_ARG(quint64, m_generation)); } +void MainWindow::refreshDashboardDigest() +{ + // Not showing a conversation: the ordinary case, and nothing to refresh. + if (m_dashboardThreadId.isEmpty() || !m_messageView->showingDashboard()) + return; + + // No placeholder digest here, unlike the selection path: the pane is + // already showing this conversation, and blanking it to re-fill it would + // flicker the whole dashboard for a change to one number. + // + // Its own generation, bumped like any other request so a reply that + // arrives after the user has moved on is discarded by the guards in + // onThreadDigestLoaded(). + ++m_digestGeneration; + QMetaObject::invokeMethod(m_worker, "loadThreadDigest", Qt::QueuedConnection, + Q_ARG(QString, m_dashboardThreadId), + Q_ARG(quint64, m_digestGeneration)); +} + void MainWindow::onThreadDigestLoaded(const ThreadDigest &digest, quint64 generation) { @@ -4710,6 +4729,12 @@ void MainWindow::onTagsApplied(const TagChange &change) m_pendingChange = {}; m_pendingThreadIds.clear(); + // Item 181. HERE, where a write is CONFIRMED, and not where one is sent: + // the digest is rebuilt from the INDEX, so a refresh queued beside the + // write would race it and answer from the state before it. The dashboard + // covers every write for the same reason the indicator below does. + refreshDashboardDigest(); + // Item 176. The ids the worker reports are the ones whose tags really // moved, which is what the undo entry has to invert. Inverting the REQUEST // instead turned the undo of "mark 44 read" into "mark 44 unread": 2 diff --git a/src/mainwindow.h b/src/mainwindow.h index d7579f4..1f68733 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -645,6 +645,30 @@ private slots: /// has moved off, or one that arrives once the pane is showing a message, /// must not repaint anything. void onThreadDigestLoaded(const ThreadDigest &digest, quint64 generation); + + /// Re-asks the worker for the dashboard's digest, after a write that may + /// have changed what it draws. + /// + /// Item 181. The digest is built from the INDEX by the worker and arrived + /// only when a conversation was selected, so a tag write moved the model + /// and the card and left the pane reporting the unread count the + /// conversation had when it was opened. Reachable from the dashboard's own + /// Mark all read button, where the number sits directly above the button + /// that fails to move it. + /// + /// Called from BOTH write funnels, per the rule that every path a + /// thread-scoped write travels a message-scoped one travels too. It is + /// deliberately not narrowed to the writes that change what the dashboard + /// happens to draw today: that list is one the dashboard can outgrow + /// silently, and this costs a round trip only while a conversation is on + /// screen. + /// + /// Does nothing when the pane is not showing a dashboard, which is the + /// ordinary case. Re-requests rather than editing the digest in place: the + /// digest is a derived summary (senders, buckets, timestamps, the unread + /// list and its cap), and reproducing that arithmetic here would be a + /// second place that has to agree with the worker about what a write did. + void refreshDashboardDigest(); void onWorkerError(const QString &message); void onSyncFinished(bool success, int exitCode); diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index e2254f1..a8e6284 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -73,6 +73,7 @@ #include #include "tagchip.h" #include "tagstrip.h" +#include "threaddashboard.h" #include "threadlistmodel.h" #include "threadlistview.h" #include "notmuchfixture.h" @@ -268,6 +269,7 @@ private slots: void aMalformedAccountIsReportedWithoutBlockingTheConstructor(); void aWorkerBackedWindowReturnsRealThreads(); void aPurgeTakesTheRowsOutOfTheViewWithoutARefresh(); + void theDashboardFollowsAWriteToTheConversationItShows(); // Compose and send, item 123 task 12. void theMailRootComesFromTheConfigNotTheIndex(); @@ -9212,6 +9214,82 @@ void TestMainWindow::aWorkerBackedWindowReturnsRealThreads() QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 1, 15000); } +/// Item 181, from the user: "the thread dashboard doesn't update live with the +/// modifications applied to the list pane. If I mark the thread as read, the +/// dash still reports N unread". +/// +/// The dashboard draws a ThreadDigest, which the worker builds from the index +/// and which arrived only when a conversation was SELECTED. A tag write moved +/// the model and the card beside it and never touched the digest, so the pane +/// went on reporting the unread count the conversation had when it was opened. +/// +/// Reachable from the dashboard's own Mark all read button, which is the worst +/// version of it: the user presses a button and the number above it does not +/// move. +void TestMainWindow::theDashboardFollowsAWriteToTheConversationItShows() +{ + WorkerBackedWindow backed; + QVERIFY(backed.fixture().addMessage( + QStringLiteral("inbox"), QStringLiteral("conv0@example.org"), + QStringLiteral("A conversation"), QStringLiteral("alice@example.org"), + // Friday, verified with `date -d 2026-08-14 +%A`. Qt::RFC2822Date + // validates the weekday against the date. + QStringLiteral("Fri, 14 Aug 2026 10:00:00 +0200"), + QStringLiteral("Root."), true)); + QVERIFY(backed.fixture().addMessage( + QStringLiteral("inbox"), QStringLiteral("conv1@example.org"), + QStringLiteral("Re: A conversation"), QStringLiteral("bob@example.org"), + QStringLiteral("Sat, 15 Aug 2026 10:00:00 +0200"), + QStringLiteral("Reply."), true, QStringLiteral("conv0@example.org"))); + QVERIFY2(backed.build(), qPrintable(backed.error())); + + MainWindow window(backed.config()); + auto *model = window.findChild(); + QVERIFY(model); + auto *view = window.findChild(); + QVERIFY(view); + auto *messageView = window.findChild(); + QVERIFY(messageView); + ThreadDashboard *dashboard = messageView->dashboard(); + QVERIFY(dashboard); + + auto *queryEdit = + window.findChild(QStringLiteral("queryEdit")); + QVERIFY(queryEdit); + queryEdit->setText(QStringLiteral("tag:inbox")); + queryEdit->returnPressed(); + QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 1, 15000); + + const QModelIndex row = model->index(0, 0, {}); + QVERIFY2(model->isConversationRow(row), + "the fixture is not a conversation, so this test cannot see the " + "dashboard at all"); + + view->setCurrentIndex(row); + + // The digest is a worker round trip, so wait for the pane to actually + // carry the conversation's state before asserting anything about it. Not a + // fixed wait: that passes when the digest never arrives. + QTRY_VERIFY_WITH_TIMEOUT(messageView->showingDashboard(), 15000); + QTRY_VERIFY_WITH_TIMEOUT(!dashboard->showingAllCaughtUp(), 15000); + QVERIFY2(dashboard->unreadCountShown() == 2, + qPrintable(QStringLiteral("expected 2 unread listed, got %1") + .arg(dashboard->unreadCountShown()))); + + // Driven through the ACTION rather than the private funnel, which is both + // the only route from here and the better assertion: it is the path the + // dashboard's own Mark all read button takes. + auto *markAllRead = + window.findChild(QStringLiteral("mark_all_read")); + QVERIFY(markAllRead); + markAllRead->trigger(); + + // The pane must follow it. Without the refresh the digest is the one built + // when the row was selected, and this stays at 2 for ever. + QTRY_VERIFY_WITH_TIMEOUT(dashboard->showingAllCaughtUp(), 15000); + QCOMPARE(dashboard->unreadCountShown(), 0); +} + void TestMainWindow::aPurgeTakesTheRowsOutOfTheViewWithoutARefresh() { // Found by hand: the mail was destroyed correctly and the list went on -- cgit v1.2.3