fix(inbox): refresh the list in the caller's scope after a Legacy batch action - #2614
Conversation
batchTriage takes an optional list query and forwards it to the post-batch fetchItems read. Callers that pass nothing keep the unscoped read, so no other call site changes behaviour. The read still ignores the applied boolean and keeps its 401/403 rethrow: a superseded post-batch read is not a failed write.
batchAction hands the store the Inbox's current list query, so a Legacy batch ignore, cancel or triage on a board-scoped Inbox refreshes with boardId and keeps the board's rows. Ignore and cancel start no poll, so before this they kept the unscoped rows until the next scoped load. The scoped query moves into one currentListQuery helper that the explicit load, the batch refresh and the bounded triage poll all share. Archived history is not part of it: batchAction already returns early there. The InboxView spec's two exact-argument batch assertions are updated for the new argument; the view itself is unchanged.
|
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 caca1ea). 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. Confirmed clean: batchTriage was the only unscoped list read in the store (the single-item actions reconcile through fetchDetail); the fetchItems applied-boolean contract from #2584 and the 401/403 rethrow are untouched; a board-scoped list read cannot 403 where an unscoped one would not (CaptureController.List declares 200/400/401 and treats boardId as a filter); the bounded poll's query is built after the await and is now consistent with the post-batch read; archived history is unaffected (batchAction returns early; loadInboxInternal's query is byte-identical to the inline object it replaced, so PaperInboxScopeTruth still describes reality); Paper has no batchAction caller; no store-to-composables import; the two InboxView.spec.ts assertions became stricter. Triage, all taken in one round-2 commit (small, same seam; the MEDIUM is a truthfulness defect in exactly the path this PR changes):
Merge gate: ci-required green at the round-2 head, aged three minutes, then merge commit. The round-2 diff is re-proved with the store, orchestrator and InboxView specs, typecheck, ESLint and diff check; no further review round is owed unless the fixes change the risk boundary. |
Review round 2 on PR #2614. MEDIUM: batchAction evaluated the scope before the POST went out, so a board change while the batch was in flight sent the post-batch read at the old board. Because fetchItems supersedes by request id, that late stale read won the list over the new board's own load, writing board A's rows under board B's label with no poll to repair it after ignore or cancel. batchTriage's third parameter now accepts a thunk as well as a value and calls it immediately before fetchItems; batchAction passes currentListQuery itself. The default stays unscoped. LOW: the scoped-forwarding store spec now asserts the scoped rows reach items, not just that the read was scoped; the orchestrator ordering comment no longer claims more than a bare mock can prove; the helper docstring gives the real reason archived history carries no key (CaptureListQuery has none and archived history is always board-scoped) and flags that adding one must revisit the helper.
Summary
captureStore.batchTriagere-read the Inbox list with no query after the batch POST, while the caller'sInbox may be board-scoped. A Legacy batch action on a board-scoped Inbox therefore replaced the visible
rows with the unscoped list. For
triagethe bounded poll starts with the correct scope and its firsttick (3 s) restores the scoped snapshot, so the wrong rows show for at most one tick. For
ignoreandcancelno poll starts, so the unscoped rows persisted until the next scoped load.batchTriagenow takes the caller's list scope and forwards it to the post-batchfetchItemsread, anduseInboxOrchestrator.batchActionsupplies it. The signature is:The scope is resolved WHEN THE READ IS ISSUED, not when the batch starts. The store calls the thunk
immediately before
fetchItems, andbatchActionpassescurrentListQueryitself rather than itsresult. A scope captured at entry would be wrong whenever the user moves to another board while the POST
is in flight: the reconciliation read would go out at the old board, and because
fetchItemssupersedesby request id, that later stale read would win the list over the new board's own load and write board A's
rows under board B's label, with no poll to repair it after
ignoreorcancel. The union type keepsthe plain-value form working, and callers that pass nothing keep the unscoped read, so no other call site
changes behaviour.
The scoped query is extracted into one
currentListQuery()helper that the explicit load, the batchrefresh and the triage poll all share, so the three cannot drift apart.
Archived history is not threaded through.
CaptureListQueryhas nohistorykey, and archived historyis always board-scoped, so
boardIdalready selects the archived board's captures.Closes #2570
Refs #2305, PR #2567.
Changes
fix(inbox): let batchTriage refresh the list in the caller's scope(08f2c7f) -batchTriagegainsan optional third
CaptureListQueryargument and forwards it tofetchItems. Two store specs coverforwarding a scoped query and the unscoped default.
fix(inbox): scope the post-batch refresh to the Inbox's active board(454dc2b) -batchActionpasses the Inbox's scope, and a new
currentListQuery()helper replaces the duplicated inline queryliterals in
loadInboxInternaland the triage poll. Four orchestrator specs cover board-scoped ignore,cancel and triage plus the unscoped case. The two exact-argument batch assertions in
InboxView.spec.tsare updated for the new argument;
InboxView.vueitself is unchanged, since its batch path goesthrough the orchestrator.
style(inbox): separate the list-query helper from the following computed(caca1ea) - one blank line.fix(inbox): resolve the post-batch refresh scope when the read is issued(e4ec5cb) - review round 2.The third parameter accepts a thunk as well as a value and is called immediately before
fetchItems;batchActionpassescurrentListQueryitself. One new orchestrator spec covers a board change duringthe POST, and one new store spec covers the store resolving the thunk late (the orchestrator spec mocks
the store wholesale, so it cannot exercise the store's own branch). Three review LOWs in the same
commit: the scoped-forwarding store spec now asserts the scoped rows reach
itemsrather than onlythat the read was scoped; the orchestrator ordering comment no longer claims more than a bare mock can
prove; the helper docstring gives the real reason archived history carries no key and flags that adding
one must revisit the helper. Five exact-argument assertions became
expect.any(Function)plus anassertion on what the thunk returns.
Test plan
Red first, twice.
The three board-scoped orchestrator specs and the unscoped one were written before any source change and
failed on the base (
495da2568):Each failure was the missing third argument, for example:
The round-2 spec was written before the thunk change and failed on the then-current head (
caca1ea84),showing the defect itself, a scope captured at batch start surviving the move to board-9:
Verified at head
e4ec5cb4c, infrontend/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- 3 files, 220 passed.npx vitest --run --maxWorkers=2 src/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.ts- 61 files, 1148 passed.npm run typecheck- clean.npx eslint src/store/captureStore.ts src/composables/useInboxOrchestrator.ts src/tests/store/captureStore.spec.ts src/tests/composables/useInboxOrchestrator.spec.ts src/tests/views/InboxView.spec.ts- clean.git diff --check- clean.Earlier, at head
caca1ea84: every spec that referencesbatchTriage(8 files, 155 passed) andnpm run build(built in 4.34s; theINEFFECTIVE_DYNAMIC_IMPORTwarning fortoastStore.tsispre-existing and unrelated). The build was not re-run after round 2, which changed no build-affecting
surface beyond the two source files that typecheck covers.
NOT verified: no Playwright run, so no browser-level proof that the board-scoped rows survive a batch
ignore against a live stack. No backend tests (no backend change). The full frontend vitest suite was not
run; only the sets above.
Boundaries and risks
appliedboolean thatfetchItemsreturns (fix(inbox): clear a scope replacement only on an applied list response #2584). A post-batch readsuperseded by a newer list load is not a failed write, so nothing reclassifies the batch. The
try/catchcontract around the read, including the 401/403 rethrow and the comment that an exhaustedfollow-up read must not reclassify a successful batch, is unchanged.
refreshTerminalDetails,pollBatchTriageCompletion, the authoritative POST resulthandling, or any other store method.
batchAction.historykey toCaptureListQuerylater must revisitcurrentListQuery(): the helper is board-id-and-limit only, andloadInboxInternalcalls it inarchived mode too, so the omission is not covered by
batchAction's early return on archived history.The docstring on the helper says so.
loadingDetail, count-refresh coalescing, mapgrowth, aborted control, comment wording) are deliberately not in this PR. They touch the same two
files and are a separate PR after this one merges, one writer per path.
.worktrees/codex-2570-batch-triage-scopeholds onlyfrontend/taskdeck-web/dist/andfrontend/taskdeck-web/node_modules/as ignored files. Both are rebuildable, nothing was copied out.