Skip to content

fix(inbox): repair a dropped scope-replacement load instead of latching it - #2713

Merged
Chris0Jeky merged 4 commits into
mainfrom
issue-2591/scope-latch
Sep 5, 2026
Merged

fix(inbox): repair a dropped scope-replacement load instead of latching it#2713
Chris0Jeky merged 4 commits into
mainfrom
issue-2591/scope-latch

Conversation

@Chris0Jeky

Copy link
Copy Markdown
Owner

Summary

A Paper Inbox scope-replacement load that the store dropped left isScopeReplacement raised
with nothing coming. PaperTriageTable then fell through every branch — the count span is hidden
by !scopeReplacement, the Loading block needs loadingList, the empty state needs !hasItems,
and the list renders display: none — so the user saw an empty table body under a count-free
eyebrow 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.ts

  • The observable for "an applied load for this scope landed" is the applied boolean from
    captureStore.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 is
    waiting on. Why not a store observable: the store keeps latestListLoadRequestId private and
    publishes only items, loadingList and listError; items also moves on writes that prove
    nothing 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.
  • Re-issue rule. A load that resolves not applied while a replacement is latched for the
    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 a
    plain 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 loadingList is true, so the table shows its Loading state,
    not a hidden body.
  • Honest terminal state. If the repair is dropped too, it does not re-issue again. The flag
    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 fetchItems caller (captureStore.batchTriage's post-POST refresh) resolves the same
    currentListQuery thunk 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 this
    scope's rows, not the retained previous scope's.
  • Extracted currentScopeKey() (the scope key was built inline twice). Failure behaviour is
    untouched: a throw skips the whole branch, so the flag stays sticky across a failed load.

src/tests/views/paper/inbox/PaperInboxScopeTruth.spec.tscollateral, test-only, 2 lines.
That mock predates #2584: it was typed Promise<void> and resolved undefined, which the
orchestrator 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 and
the same fidelity fix #2584 applied to useInboxOrchestrator.spec.ts. This is the only file touched
outside 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:

  1. 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 is
    owed and the flag falls when the second load applies, with rows exposed.
  2. re-issues the load once when a dropped replacement leaves nothing in flight — exactly two
    reads, the second at the current board scope.
  3. stops after one re-issue when the repair load is dropped too — exactly two reads, flag down,
    rows visible.
  4. 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.
  5. gives each scope replacement its own single repair read — replaces the old
    clears the scope-replacement state on the next applied response, whose shape (one drop, then a
    separately 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):

  • (2) AssertionError: expected "vi.fn()" to be called 2 times, but got 1 times at
    useInboxOrchestrator.spec.ts:1021 (expect(mockCaptureStore.fetchItems).toHaveBeenCalledTimes(2))
  • (3) AssertionError: expected "vi.fn()" to be called 2 times, but got 1 times at
    useInboxOrchestrator.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)
  • widened to the rest of the seam: the three above plus
    src/tests/views/paper/inbox/PaperInboxScopeTruth.spec.ts,
    src/tests/views/paper/inbox/PaperTriageTable.degraded.spec.ts,
    src/tests/views/InboxView.spec.tsTest Files 6 passed (6), Tests 319 passed (319)
  • npm run typecheck → clean; npx eslint on the three changed files → clean;
    git diff --check → clean

Not verified

  • No E2E was run and none is owed: no data-testid changed, no template changed, and the seam is a
    response-ordering race a browser run cannot schedule deterministically.
  • The rendered outcome is argued from PaperTriageTable.vue's branch conditions (read, not
    changed), not from a mounted assertion of the post-repair table; the component specs that do
    mount it stay green.
  • Backend, the wider frontend suite, and the production reachability path (a Legacy batch action
    plus a Paper-theme toggle mid-batch) were not exercised.
  • src/tests/views/InboxView.spec.ts still mocks fetchItems as Promise<void>; it is green
    either way, so it was left alone rather than expanding this claim.

Boundaries

captureStore.ts is untouched — the #2584 applied/not-applied contract stands as shipped, and the
fix needed nothing from the store. No PaperTriageTable.vue prop change was needed. No
LegacyInboxView, capture API, Inbox draft seam (#2698), docs or OUTSTANDING_TASKS.md edits.

Worktree .worktrees/codex-2591-scope-latch is clean; --ignored lists only
frontend/taskdeck-web/node_modules/ (an npm ci install, nothing to preserve).

Refs #2591, #2501, PR #2584

…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
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@Chris0Jeky

Copy link
Copy Markdown
Owner Author

Checkpoint (alpha lane session ending 2026-09-05 ~21:50Z): ready for review at e3adaab26; a fresh-context read-only review was in flight when the session ended and its record was not posted, so the next session owes one fresh-context review (the lens list: the #2501 invariant on the second-drop clear, the repair re-issue's loop guards, the user-visible states after a second drop, openItemFromHash, the mock fidelity edit, spec non-vacuity). CI in progress at e3adaab26. Merge gate: that review resolved, CI green, three-minute age.

…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.
@Chris0Jeky

Copy link
Copy Markdown
Owner Author

Review record (alpha product-trust lane; one fresh-context read-only reviewer subagent since Codex credits are exhausted, SC-9; round 1 at head e3adaab26; the reviewer's worktree copy was torn down mid-read by the session wind-down, so its consumer facts were read from main at the same merge-base, which this diff does not touch). This supersedes the checkpoint above: the review is done; round 2 is owed, not a review.

Verdict at round 1: SHIP; no merge blocker (every traced path preserves the #2501 rule or degrades into a state the table narrates; the re-issue is bounded at one read per latch). Triage for the next session's round 2, one commit:

  • MEDIUM, fix-now as a comment: useInboxOrchestrator.ts ~497-508 states as a guarantee that on the second drop "the rows that won are this scope's rows"; that conflates issued with applied. The superseding read is still in flight when the repair resolves false, so items holds the last APPLIED list, which can be the previous scope's rows if nothing applied since the scope change. Reachable only with two non-orchestrator fetchItems reads bracketing one replacement, and today batchTriage's reconciliation read is the only such caller and only Legacy has a batch surface while only Paper renders the flag, so it is a latent contract, not a live defect. Reword to what holds (the clear is safe while the orchestrator is the only Paper-side caller; a Paper batch surface must clear only when a read has applied since the latch was raised), or clear only on an applied read.
  • LOW, take in the same round if cheap (it is logic, so a scoped verification pass would then be owed): the repair is issued after await openItemFromHash(), so when the superseding read already applied, the table shows the count-free empty body for the duration of that await (a real GET on a board-scoped hash); moving the re-issue above that await closes the window. Strictly better than the permanent pre-PR state either way.
  • LOW, tracked on #2591: the inbox-load perf measure is clobbered by the repair's own start() and reports only the repair leg; openItemFromHash() runs twice for one repaired load (a second peekDetail on the board-scoped path); the repair can fire after a concurrent load already cleared the latch, and after unmount (one bounded read, consistent with the composable's pre-existing behaviour); the "changed scope" spec is decided by the request-id guard alone, so the scope-key half and a plain-load drop under a latch for another scope are untested, and no spec renders the table for either terminal state; the red-first count is three specs, not two.
  • Informational: no STATUS line; the lane's next block records the PR.

Clean lenses corroborated by the reviewer: currentScopeKey() byte-faithful to both inlined keys; the request-id guard compares the id captured before the await; the latch and the flag are written in lockstep at three sites; the repair never mints a latch, so recursion is bounded; a scope change during the repair stays coherent; a second drop with an empty list still shows the loading branch; the failure path and the Retry affordance are unchanged; batchTriage's read resolves the current query at issue time; the batch poll cannot cause a drop; the removed spec's coverage is re-pinned; the mock fidelity edit matches the real fetchItems signature.

Unverified by the reviewer: the spec files at the head (read from the diff only), the reported green runs, and whether PaperInboxScopeTruth.spec.ts still proves what it did. Round count: 1 of 2. Merge gate: the round-2 commit, CI green at its head plus the age (and one scoped verification pass only if the re-issue is moved).

@Chris0Jeky

Copy link
Copy Markdown
Owner Author

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 issue-2591/scope-latch), then CI green at the new head plus the three-minute age, then gh pr merge 2713 --merge --match-head-commit <head>.

@Chris0Jeky

Copy link
Copy Markdown
Owner Author

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:

  • MEDIUM, fixed: src/tests/views/InboxView.spec.ts (lines 80 and 238) kept the stale fetchItems mock (Promise, resolved undefined) that the PR corrected in PaperInboxScopeTruth.spec.ts; that spec mounts the real Legacy view and orchestrator, so under the new code every mount read a dropped response and issued the repair read, with the second chain able to leak past waitForUi into later tests. The mock now resolves true. Local run after the fix: InboxView.spec, useInboxOrchestrator.spec and PaperInboxScopeTruth.spec 151/151.
  • LOW, tracked on [Frontend][Inbox] A dropped scope-replacement response latches isScopeReplacement with no retry affordance (from PR #2584 review) #2591: the new catch comment says the table's error surface carries a Retry, which holds only while listError is set; a batch poll success clears listError without touching isScopeReplacement, so the hidden-rows, no-count, no-Retry state is still reachable through the failure path. Pre-existing on main.
  • LOW, tracked on [Frontend][Inbox] A dropped scope-replacement response latches isScopeReplacement with no retry affordance (from PR #2584 review) #2591: the repair is issued only after await openItemFromHash(), so the broken state stays on screen for a detail round-trip, a transient hash failure fires a duplicate peekDetail GET, and a rejection from openItemFromHash() would skip the repair.

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.
@Chris0Jeky
Chris0Jeky merged commit 9b864ea into main Sep 5, 2026
35 checks passed
@github-project-automation github-project-automation Bot moved this from Pending to Done in Taskdeck Execution Sep 5, 2026
@Chris0Jeky
Chris0Jeky deleted the issue-2591/scope-latch branch September 6, 2026 02:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant