fix(inbox): repair a dropped scope-replacement load instead of latching it - #2713
Conversation
…ng it A scope-replacement load that the store dropped (its request id superseded, so applied is false) left isScopeReplacement raised with nothing coming: PaperTriageTable then hid the rows AND their count with no Retry, an empty body under a count-free eyebrow, until the user re-navigated. The orchestrator now records which scope a raised flag belongs to and whether its one repair read has been spent. A drop that is still the latest load for the current scope re-issues the load ONCE with the current scope query; a later applied load for that scope (the superseding one, or the repair) clears the flag. A second drop does not re-issue again: it clears the flag so the table shows the store rows and their count, its empty state, or its error surface with Retry. Resolution alone still never clears the flag, and a drop under a changed scope still clears nothing (#2501). Refs #2591, #2501, PR #2584
…ontract The mock predates #2584: it was typed Promise<void> and resolved undefined, which the orchestrator now reads as a dropped response and repairs with a second read, so the spec's one-request assertion failed. The ordinary case that spec asserts is an applied response; say so. Refs #2591, #2584
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Checkpoint (alpha lane session ending 2026-09-05 ~21:50Z): ready for review at |
…contract Review round 1 (fresh-context, coordinator carrying the alpha lane's PR): the identical stale mock the PR fixed in PaperInboxScopeTruth.spec.ts was left in InboxView.spec.ts, which mounts the real Legacy view and orchestrator; with the new code a resolved undefined reads as a dropped response and every mount would issue the repair read. The mock now resolves true. Local: InboxView.spec, useInboxOrchestrator.spec and PaperInboxScopeTruth.spec 151/151.
|
Review record (alpha product-trust lane; one fresh-context read-only reviewer subagent since Codex credits are exhausted, SC-9; round 1 at head Verdict at round 1: SHIP; no merge blocker (every traced path preserves the
Clean lenses corroborated by the reviewer: Unverified by the reviewer: the spec files at the head (read from the diff only), the reported green runs, and whether |
|
Checkpoint update: the review record above replaces the earlier "review owed" line. Next session: one round-2 commit per that record (a worker in a fresh worktree on |
|
Review gate (Codex review credits exhausted, SC-9; the alpha lane's session ended, so the coordinator carries this PR): one fresh-context read-only reviewer (Opus 5) on head e3adaab. Verdict: FIX-FIRST on one MEDIUM, fixed in the follow-up commit; test-only fix, so no second pass is owed. Checked by reading (reviewer): the repair is bounded (repairIssued is set before the recursive call, the repair is a plain load that cannot reset the budget, and only loadInboxForScopeReplacement creates a new latch, so depth is at most 2 per replacement); two rapid scope changes converge on the right rows because the repair re-reads the current scope key and query at issue time and clears the flag only on that key; a stale earlier-scope response cannot clear the flag because requestScopeKey must equal the live scope key; applied === false while the orchestrator id is still latest can only come from batchTriage's reconciliation read (fetchItems has two production call sites and the poll path never bumps the request id); latch and flag are created and cleared together and no consumer writes isScopeReplacement; three of the five new specs assert two fetchItems calls and fail on revert, the other two are regression guards; only the composable and two specs changed. Findings and disposition:
Issue disposition: slice of #2591 (the dropped-response latch); the failure-path shape above remains and is recorded on the issue. |
Review round 1 (alpha lane's fresh-context reviewer, MEDIUM comment-truth): the comment claimed the rows that won a superseding read are this scope's rows; issued is not applied, so until the superseding read applies the store still holds the last applied list. The comment now says the clear is safe only while this orchestrator is the sole Paper-side fetchItems caller and what a Paper batch surface would have to do. The catch comment's Retry claim is scoped to listError.
Summary
A Paper Inbox scope-replacement load that the store dropped left
isScopeReplacementraisedwith nothing coming.
PaperTriageTablethen fell through every branch — the count span is hiddenby
!scopeReplacement, the Loading block needsloadingList, the empty state needs!hasItems,and the list renders
display: none— so the user saw an empty table body under a count-freeeyebrow with no Retry until they re-navigated or captured something.
The #2501 rule is unchanged: clearing the flag on resolution alone is wrong (it un-hid the
retained old-scope rows under the new scope's label). What is added is an exit from the latch.
Changes
src/composables/useInboxOrchestrator.tsappliedboolean fromcaptureStore.fetchItems(the fix(inbox): clear a scope replacement only on an applied list response #2584 contract), plus a new orchestrator-owned record —scopeReplacementLatch: { scopeKey, repairIssued }— that says which scope a raised flag iswaiting on. Why not a store observable: the store keeps
latestListLoadRequestIdprivate andpublishes only
items,loadingListandlistError;itemsalso moves on writes that provenothing about the current scope (optimistic summary writes, the batch poll's reconciliation
reader), so watching it would clear the flag without the new scope's rows ever having landed —
the exact [Frontend][Inbox] Scope-replacement residuals: clear isScopeReplacement only on an applied response; eyebrow counts ignore replacement (from PR #2484 review) #2501 harm. No store change was needed and none was made.
current scope, and that is still the latest orchestrator load for that scope, re-issues the load
once with
currentListQuery()(same request-id and scope-key guards). The repair is aplain load, so it cannot refill its own budget, and the budget is per replacement — the next
scope change gets its own. If a later orchestrator load is already pending, nothing is re-issued:
that load is the later applied load, and it clears the flag when it applies. While the repair is
in flight the flag stays raised and
loadingListis true, so the table shows its Loading state,not a hidden body.
clears, so the table shows whichever of its real branches applies: the store's rows with their
count, its empty state with the clear-scope affordance, or its error surface with Retry. Never
the count-free empty body. Clearing is defensible here in a way clearing on the first drop is
not: the replacement has already had a repair read at the current scope, and the only other
production
fetchItemscaller (captureStore.batchTriage's post-POST refresh) resolves the samecurrentListQuerythunk at the moment it issues its read ([Frontend][Inbox] batchTriage refreshes the list unscoped and replaces a board-scoped Legacy inbox's rows (from PR #2567 review) #2570), so the rows that won are thisscope's rows, not the retained previous scope's.
currentScopeKey()(the scope key was built inline twice). Failure behaviour isuntouched: a throw skips the whole branch, so the flag stays sticky across a failed load.
src/tests/views/paper/inbox/PaperInboxScopeTruth.spec.ts— collateral, test-only, 2 lines.That mock predates #2584: it was typed
Promise<void>and resolvedundefined, which theorchestrator now correctly reads as a dropped response and repairs with a second read, breaking
that spec's one-request assertion. It now resolves
true, matching the shipped store contract andthe same fidelity fix #2584 applied to
useInboxOrchestrator.spec.ts. This is the only file touchedoutside the claim; no production file other than the orchestrator changed.
Test plan
Specs are red-first, deferred-promise, in
src/tests/composables/useInboxOrchestrator.spec.ts:clears the scope-replacement state when the superseding load applies after the dropped one resolves— the dropped replacement resolves after the second load was issued; no repair isowed and the flag falls when the second load applies, with rows exposed.
re-issues the load once when a dropped replacement leaves nothing in flight— exactly tworeads, the second at the current board scope.
stops after one re-issue when the repair load is dropped too— exactly two reads, flag down,rows visible.
keeps the scope-replacement state when a dropped response resolves under a changed scope—the [Frontend][Inbox] Scope-replacement residuals: clear isScopeReplacement only on an applied response; eyebrow counts ignore replacement (from PR #2484 review) #2501 pin: no repair, no clear.
gives each scope replacement its own single repair read— replaces the oldclears the scope-replacement state on the next applied response, whose shape (one drop, then aseparately invoked load that applies) is the repair read now and is pinned by (2). What was
left to pin is that an exhausted budget does not make the next scope change unrepairable.
Red-first, new specs against the unmodified source
(
npx vitest --run --maxWorkers=2 src/tests/composables/useInboxOrchestrator.spec.ts) —Test Files 1 failed (1),Tests 2 failed | 75 passed (77):AssertionError: expected "vi.fn()" to be called 2 times, but got 1 timesatuseInboxOrchestrator.spec.ts:1021(expect(mockCaptureStore.fetchItems).toHaveBeenCalledTimes(2))AssertionError: expected "vi.fn()" to be called 2 times, but got 1 timesatuseInboxOrchestrator.spec.ts:1035(same assertion; the flag assertion below it was unreached)(1) and (4) pass against the unmodified source by construction — they pin behaviour the fix must
not break, and (4) is the #2501 regression.
Green, after the fix:
npx vitest --run --maxWorkers=2 src/tests/composables/useInboxOrchestrator.spec.ts src/tests/views/paper/PaperInboxView.spec.ts src/tests/views/paper/inbox/PaperTriageTable.spec.ts→
Test Files 3 passed (3),Tests 238 passed (238)src/tests/views/paper/inbox/PaperInboxScopeTruth.spec.ts,src/tests/views/paper/inbox/PaperTriageTable.degraded.spec.ts,src/tests/views/InboxView.spec.ts→Test Files 6 passed (6),Tests 319 passed (319)npm run typecheck→ clean;npx eslinton the three changed files → clean;git diff --check→ cleanNot verified
data-testidchanged, no template changed, and the seam is aresponse-ordering race a browser run cannot schedule deterministically.
PaperTriageTable.vue's branch conditions (read, notchanged), not from a mounted assertion of the post-repair table; the component specs that do
mount it stay green.
plus a Paper-theme toggle mid-batch) were not exercised.
src/tests/views/InboxView.spec.tsstill mocksfetchItemsasPromise<void>; it is greeneither way, so it was left alone rather than expanding this claim.
Boundaries
captureStore.tsis untouched — the #2584 applied/not-applied contract stands as shipped, and thefix needed nothing from the store. No
PaperTriageTable.vueprop change was needed. NoLegacyInboxView, capture API, Inbox draft seam (#2698), docs orOUTSTANDING_TASKS.mdedits.Worktree
.worktrees/codex-2591-scope-latchis clean;--ignoredlists onlyfrontend/taskdeck-web/node_modules/(annpm ciinstall, nothing to preserve).Refs #2591, #2501, PR #2584