fix(inbox): pin the foreground detail state, coalesce count refreshes, clear the generation maps at logout - #2631
Conversation
The comment above the #2304 spec said the open detail was unrelated while the test reconciles the same id. The assertion is right; the framing was not.
… triage batchTriage is a foreground caller of refreshTerminalDetails and no spec pinned that its reconciliation leaves loadingDetail alone. loadingDetail is one store-wide boolean, so raising it for the batch would blank the panel of an open capture outside the selection and would be cleared by the first parallel read to settle. batchBusy is the foreground state for a batch.
The poll already refreshes the workload count at most once per tick, but no spec covered a single tick observing several new terminal outcomes at once.
… clear Superseded and 401/403 were proven not to clear a standing foreground listError. A poll stopped by the orchestrator, or aborted by the 60 s deadline, while its read is in flight was not.
Both generation guards are keyed by capture id and grow for the lifetime of the store with no eviction. resetForLogout clears them where the workspace store's own logout reset already runs. The shared clock stays monotonic, so a detail read still in flight from the previous session is dropped rather than cached.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Review record (alpha product-trust lane, review-and-ship round 1 at head 12afa8f). Reviewer: one fresh-context independent reviewer subagent (read-only), input = merge-base..head diff plus the worktree at the head. Verdict: SHIP, no CRITICAL or HIGH; two MEDIUMs and two LOWs taken in round 2. Confirmed clean: the session watcher fires the reset only on the authenticated-to-false transition (isAuthenticated is a computed over the token and a refresh never produces an intermediate false; the immediate run on first paint is inert); a reset while a request is in flight cannot leak one user's rows to another because the Inbox mounts through the scope-replacement seam that hides retained rows until the new response is applied (#2501) and the detail cache is keyed by GUID; the bounded poll's guards and the #2614 scoped refresh are byte-identical; the two item-4 controls are genuinely mid-flight (the read is issued before the stop or deadline and its response is dropped by isCurrent()); the item-2 spec exercises the per-tick loop (three terminal ids in one snapshot, one count refresh, a second tick gives the second); the reactive mockSession cannot leak state across AppShell specs; no import cycle. Triage:
Merge gate: ci-required green at the round-2 head, aged three minutes, one verification pass scoped to the fix diff (the epoch guard is new logic), then merge commit. |
|
CI attribution at the round-1 head 12afa8f: the Windows Frontend Unit leg failed in the dev-up launcher suite (AssertionError ifError got unwanted exception: spawnSync C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe ETIMEDOUT), the #2378 / #2161 launcher-timeout cohort on the platform lane, not this PR (captureStore and AppShell only; no spec of this PR appears in the failure). No re-run of this head: round 2 replaces it, and the merge gate is ci-required at the round-2 head. |
…n epoch Clearing the generation maps alone inverted the detail write guard: a read that started with no recorded write observed generation 0, and after a write landed and the map was cleared the compare became 0 !== 0, so the pre-write body was cached over the newer one. A session epoch moves in resetForLogout after the clear, and fetchDetail and the single-item triage poll capture it when they issue their request and drop the response when it moved, before any generation compare. The docstring now says the maps are evicted at logout rather than bounded, in-session growth is unchanged, the batchBusy justification names the Legacy skin, and the foreground batch spec runs two stale ids against a concurrent genuine detail load.
|
Round-2 verification record (scoped to the fix diff 12afa8f..6af0a34; read-only pass). Verdict: SHIP for the fix diff, no CRITICAL or HIGH. The session epoch is safe by construction: at both changed sites it can only narrow a cache branch (an extra early return before the generation compare; a compound condition on the poll's cache branch), so it cannot open a new write path into detailById or items. Confirmed: fetchDetail captures the epoch synchronously before the request is issued and every detail-path caller (refreshTerminalDetails, ignoreItem, cancelItem, triageItem, selectItemById, refreshSelectedDetail, the Paper row editor) goes through that one check; the single-item poll captures and re-checks; peekDetail never writes through the guarded path; the epoch is store-scoped, incremented only by resetForLogout, whose only caller is the shell's session watcher; a dropped read is not displayed through the return value (the Legacy panel renders from detailById); a surviving poll after logout is stopped by the Inbox unmount or selection watcher and its next read captures the new epoch; both new specs genuinely construct the write-then-logout crossing and assert on detailById and items; the two-id fixture proves the parallel-settle claim; the skin-qualified comment claims match the bindings. One MEDIUM and four LOWs tracked as #2640 rather than a third round (the two-round ceiling is reached and none is a merge blocker; all are post-logout-only edges): the list snapshot path's stale-high guard reads 0 after the map clear, so an older in-flight background list snapshot can win in the sub-millisecond window between the reset and the Inbox unmount (the next mount replaces items through the scope-replacement seam); onRefreshed fires for epoch-dropped reads; fetchDetail's drops are unreported to callers; the docstring's "every detail read" overstates peekDetail; a persistent never-resolving mockImplementation in the spec can hang a future test. Merge gate unchanged: ci-required green at 6af0a34, aged, merge commit. |
Summary
Closes the six non-blocking residuals the fresh-context review of PR #2567 left behind. One is a real
change: the capture store's two per-item generation maps are now cleared at logout, and a session epoch
invalidates any detail read that crosses that logout. The rest are pins and a comment fix over behaviour
that is already correct, which is what the issue asked for.
Two characterization findings shaped the diff.
Item 2 needs no code.
refreshCountsForNewTerminalOutcomes()already walks every tracked id, marks eachnew terminal outcome, and calls
notifyTriageCountChanged()once behind anobservedNewOutcomeflag.That is also true at PR #2567's head
744f0cff0, so the issue's "up to ~20 GET /workspace/home calls"is about twenty ticks each observing one outcome, not per-id amplification inside a tick. The per-tick
coalescing already exists and the load characteristic is inherent to the per-tick design, which #2303
asked for on purpose. This PR only adds the missing coverage for several outcomes landing in one tick.
Item 1 takes the second branch the issue offered: keep
trackLoading: falsefor the reconciliation andpin it, rather than raising
loadingDetailfor the foregroundbatchTriagecaller.loadingDetailisone store-wide boolean. Raising it for the batch would blank the panel and disable Refresh Detail on an
open capture that is not in the batch selection, which is #2304's defect moved into the foreground, and
with
Promise.allover N stale ids the first read to settle would clear the flag while the others arestill in flight, including under a genuine foreground detail load.
batchBusystays true for the wholebatchTriagebody and the Legacy skin'sInboxListPanel.vuerenders "Processing..." from it, so theforeground feedback is not lost. Nothing under
views/paper/binds either flag, so the Paper inbox isunaffected either way. The store comment now says all of this at the call site.
Item 5 needs no code. The "same scope" contract for the
listErrorclear stays caller-side: the storeclears on any accepted snapshot for the query the poll was constructed with, and only the orchestrator's
cancelBatchTriagePolling()on board and archived-history changes, plus unmount, prevents a cross-scopeclear. That is documented in
useInboxOrchestrator.spec.tsand is unchanged here.Closes #2571 (Refs #2301, #2303, #2304, #2305, PR #2567)
Changes
One commit per item, plus one review-round commit.
test(inbox): correct the batch reconciliation comment framing(item 6). The comment above the [Frontend][Inbox] Background batch reconciliation obscures an unrelated open detail #2304spec said the open detail was unrelated while the test reconciles the same id. The assertion was right,
the framing was not.
test(inbox): pin the quiet detail reconciliation for foreground batch triage(item 1). Extends thetrackLoading: falsecomment inrefreshTerminalDetailsto state why the foreground caller is quiettoo, and adds a spec that
loadingDetailstays false whilebatchTriage's reconciliation reads are inflight, with
batchBusytrue for the same window.test(inbox): pin the per-tick coalescing of the batch count refresh(item 2). A tick observing threenew terminal outcomes issues exactly one count refresh, and a second tick with its own new outcome
issues the second.
test(inbox): add the aborted negative control for the poll list error clear(item 4). Two specs: apoll stopped by the caller mid-flight, and a poll whose read the 60 s deadline aborts mid-flight.
Neither clears a standing foreground
listError, matching the superseded and 401/403 controls.fix(inbox): bound the capture store's per-item generation maps at logout(item 3). AddsresetForLogout()tocaptureStore, clearinglatestDetailWriteGenerationByIdandlatestSummaryGenerationByIdand nothing else, and calls it next toworkspace.resetForLogout()inAppShell.vue's session watcher.fix(inbox): invalidate in-flight detail reads at logout with a session epoch(review round 2). Seebelow.
Review round 2
Clearing the maps alone inverted the detail write guard, which the review caught. A read that starts with
no recorded write for its id observes generation 0. If a write then lands for that id (generation N) and
the map is cleared, the read's compare becomes
0 !== 0, which is false, so the pre-write body would becached and its status pushed back into the list through
upsertSummary. Before this PR the same comparewas
0 !== Nand dropped it.The fix is invalidation, not only clearing, shaped after
workspaceStore.clearHomeSummary()and itsworkloadRequestVersion.let sessionEpoch = 0sits with the generation clock.resetForLogout()stillclears both maps for memory, then increments the epoch. Every detail read that can be in flight across a
logout captures
sessionEpochwhen it issues its request and drops its response when the value moved,before any generation compare:
fetchDetail(which is also the pathrefreshTerminalDetailsand so bothpoll reconciliations take) and
pollTriageCompletion's own read. The shared generation clock staysmonotonic. The
resetForLogoutdocstring now states that the epoch is what makes an in-flight read safe.The docstring also no longer claims the maps are "bounded", and neither does the PR title. Clearing at
logout is eviction and it is the only eviction point; within a session both maps still grow by one entry
per distinct capture id touched, exactly as before.
Two smaller review points: the
batchBusyjustification in the store comment now names the Legacy skinrather than implying every skin, and notes that the pre-existing
trackLoadingdocstring describes thatsame Legacy panel. The item 1 spec now runs two stale ids against a concurrent genuine
fetchDetail, sothe parallel-settle half of the rationale is load-bearing: the first reconciliation leg to settle must not
clear a flag the foreground load owns.
Test plan
Verified, all from
frontend/taskdeck-web:npx vitest --run --maxWorkers=2 src/tests/store/captureStore.spec.ts src/tests/composables/useInboxOrchestrator.spec.ts src/tests/views/InboxView.spec.ts src/tests/components/AppShell.spec.ts src/tests/components/AppShell.paperVariant.spec.tsgives 5 files passed, 279 tests passed at the round 2 head.src/tests/components/paper/PaperShortcutsOverlay.spec.ts src/tests/guards/shortcutLedgerTruth.spec.ts src/tests/router/workspaceRouteStability.spec.ts src/tests/store/workspaceStore.spec.tsgave 9 files passed, 348 tests passed, andsrc/tests/store src/tests/composables/useInboxOrchestrator.spec.ts src/tests/views/InboxView.spec.ts src/tests/views/InboxView.paperMode.spec.ts src/tests/views/paper/inbox src/tests/resilience src/tests/property/storeResilience.spec.tsgave 61 files passed, 1171 tests passed.npm run typecheckclean,npm run buildclean,npx eslintover the changed files clean,git diff --checkclean, at both heads.Red first, recorded per item:
captureStore.tsstashed back to the round 1 head, the two new specs fail:drops a detail read that a write and a logout crossedanddrops a single-item triage poll read that a write and a logout crossed, both withAssertionError: expected { id: 'c-1', userId: 'u1', ...(11) } to be undefined. That object is the pre-write body being cached, which is the inversion itself.store.resetForLogout is not a functionfor both store specs, andexpected "vi.fn()" to be called once, but got 0 timesfor the AppShell wiring spec.correct. The issue calls them a coverage gap, a load characteristic and a missing negative control, not
defects. Each was proved load-bearing with a temporary mutation of the store instead, each reverted
with
git checkout --and the spec file re-run green afterwards:trackLoading: trueinrefreshTerminalDetailsfails the new spec withexpected true to be falseat theloadingDetailassertion, and fails the existing poll-tick spectoo.
notifyTriageCountChanged()inside the loop inrefreshCountsForNewTerminalOutcomesfails the new spec at the first count assertion.isCurrent()to its generation comparisons fails both new specs withexpected null to be 'Failed to load inbox items'.Not verified: Playwright E2E was not run, so no browser-level proof of the detail panel state during a
batch triage. No backend tests were run and no backend code is touched. The full frontend vitest suite
was not run, only the sets above.
Boundaries and risks
The bounded poll's semantics, the batch POST handling, the scoped reconciliation read from PR #2614 and
every view except the two
AppShell.vuehunks are unchanged. No documentation is touched: nothing inshipped reality, sequencing or decisions moved.
The epoch changes only what happens to a detail response that arrives after a logout: it is dropped
instead of written into
detailByIdanditems. Two specs pin it. Nothing else observes the epoch, andwithin a session it never moves.
Clearing
latestSummaryGenerationByIdhas one visible consequence, pinned by the firstresetForLogoutspec: a background list snapshot that was already in flight at logout now applies in full, because no
per-row summary generation is left to hold a row back. That is the snapshot being authoritative, and the
list is not cleared at logout today either, so nothing is lost that was previously kept. Clearing
latestDetailWriteGenerationByIdhas no other observable effect now that the epoch guards the reads; itis a memory measure and the docstring says so.
mockSessioninAppShell.spec.tsis nowreactiveso the session watcher can be driven from a test. Itis referenced in only two places in that file and every existing spec there still passes.
Worktree:
.worktrees/codex-2571-poll-truth-residuals. Its only gitignored contents arefrontend/taskdeck-web/node_modules/andfrontend/taskdeck-web/dist/, both regenerable build outputwith nothing to preserve, so removal loses no work.