fix(inbox): clear a scope replacement only on an applied list response - #2584
Conversation
captureStore.fetchItems returns without writing anything when its request id has been superseded, and it does so by resolving rather than throwing, in both the success path and the catch. useInboxOrchestrator inferred "applied" from resolution alone and cleared isScopeReplacement even when the store had dropped the response, which un-hid the retained old-scope rows under the new scope's label. fetchItems now returns true when it wrote the response into items and false when it dropped it as superseded. A failure that is still the latest request still throws, so failure and supersession stay distinguishable. The value is additive: call sites that ignore it are unchanged. loadInboxInternal clears isScopeReplacement only on an applied response. The request-id and scope-key checks stay: they guard a stale caller, while the new flag guards a dropped response. Two orchestrator specs resolved the deferred load with undefined, so they passed under both contracts. They now resolve true, and two new specs cover the dropped response and the next applied one. Refs #2501
Two presentation defects with the same cause: the Inbox showed nothing, or the wrong thing, about a load that was running. The eyebrow. useInboxCounts runs unconditionally over captureStore.items, so while a scope replacement was in flight the eyebrow published counts computed from the retained old-scope rows next to the chip that already named the new scope. The triage table hides those same rows for exactly that reason. The eyebrow now renders a count-free key while isScopeReplacement is true and returns to the counted form when the response is applied. The PaperScopeDisclosure chip is untouched. The table. A same-scope refresh keeps the retained rows mounted, visible and usable, so aria-busy on the section was the only sign a load was running, which is nothing for a sighted user. The header's count line now carries a refreshing note while such a load runs. Row actions stay enabled: the rows are still the right rows for this scope, and disabling them was declined. inbox.eyebrowLoading and inbox.refreshing are added to en, it and es. Neither carries an interpolation or plural forms. Refs #2501 Refs #2022
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Review gate (Codex credits exhausted, SC-9): one fresh-context adversarial reviewer on head c3258be (merge base 0886b6c). Verdict: SHIP. Confirmed: every fetchItems exit returns true only after items are written and false on every superseded or dropped path; the background snapshot path from #2567 never bumps the load request id, so it cannot make a foreground call misreport; store and orchestrator request ids stay in lockstep, so the last-issued load either applies (clears the flag) or throws (error plus Retry), across A-B mid-flight capture, rapid B-C-B switching, both scope watchers firing together and out-of-order resolution; only onMounted and the two scope watchers set the flag, so an ordinary same-scope refresh does not flicker the eyebrow; the refreshing note is double-gated and cannot coexist with the Loading block, the empty state or the error block; row actions untouched; en/it/es parity holds; the orchestrator spec is a real mutation detector. Findings by bin:
Merge order note: independent of PR #2580 (different hunks); whichever lands second gets a merge of main and a re-prove. Evidence at the reviewed head (from the implementation run): glob 466 to 474 passed, captureStore.spec.ts 76 to 77, four other fetchItems-consumer specs 51, typecheck, ESLint, build, diff check; the applied-response and eyebrow specs red first. Not verified: Playwright, the race end to end over a real network. Merge after the fix commit ages and ci-required is green at the new head. |
…pped Review of PR #2584 (MEDIUM, introduced by the PR itself). The count-free eyebrow branched on isScopeReplacement alone, but that flag is deliberately sticky across failure: loadInboxInternal swallows the throw so the retained rows stay hidden rather than being shown as the new scope. With the API unreachable on mount, or a 500 on a scope switch, the header read "Inbox - capture surface - loading captures..." permanently, directly above the table's own "Failed to load inbox items" and its Retry button. The eyebrow now makes no claim about the load at all. inbox.eyebrowLoading becomes inbox.eyebrowUncounted in en, it and es, and reads just "Inbox - capture surface". Loading, error and retry belong to the table, which already states all three; this line's only job is refusing to publish a count it cannot stand behind. Also from review: the refreshing-note spec asserted toContain('refreshing'), which a missing catalog key would satisfy by rendering the key itself. It now asserts the rendered English. And the demo branch's unreachable return false keeps its guard with a one-line note saying why it cannot currently fail. Refs #2501
|
Round 2 (head fef363c): the fix diff was verified by the coordinator reading it rather than a second reviewer pass, because it changes copy, one template branch and specs only: the count-free key is renamed inbox.eyebrowUncounted and reads "Inbox · capture surface" in en, it and es with no loading claim (the template comment records why: isScopeReplacement is sticky across failure, so a loading word driven off it would outlive the load and sit above the table's own error and Retry), a new spec mounts with the replacement flag AND listError set and asserts no digit and no loading word while the table still shows the error (red first against the round-1 sources), the refreshing-note assertion is now exact, and the demo-branch return keeps a comment stating why it cannot currently fail. closingIssuesReferences verified as [2022, 2501]; #2022's two recorded residuals were the page-level counts (this PR) and same-scope refresh failures (shipped as #2044 via PR #2484). Counts at this head (implementation run): standing glob plus catalogs.spec.ts 11 files / 475 passed (474 at round 1, 466 at the branch point), four fetchItems-consumer specs 51, typecheck, scoped ESLint, diff check. Not verified: npm run build this round, the failure state against a real failing server. Round count: 2. Merge after ci-required is green at fef363c and the head has aged; the Smart CI / Plan red on this head is the shadow lane's stacked-base planner shape (#2562) and is advisory. Whichever of #2580 and #2584 lands second gets a merge of main and a re-prove. |
|
Base moved under this PR when #2580 merged (92a8192; adjacent locale hunks, no textual conflict). Re-proved against the new base: gh pr update-branch created merge head 4648303, and locally on that head the combined Inbox set (catalogs.spec.ts, PaperInboxView.spec.ts, PaperBoardManagement.spec.ts, views/paper/inbox) passes 9 files / 268 tests with typecheck clean. Merge after ci-required is green at 4648303 and the head has aged. |
…ill open, shipped captureAria string) and tidy wording
Summary
While the Inbox replaces one route scope with another, it kept two rows of state that did not belong together. The eyebrow published counts computed from the retained old-scope rows next to a chip that already named the new scope, and the orchestrator decided the replacement was over the moment
fetchItemsresolved, even when the store had dropped that response as superseded and written nothing.The second half is the more serious one.
captureStore.fetchItemsreturns without writing anything when its request id is no longer the latest, and it does so by resolving rather than throwing, in both the success path and the catch.useInboxOrchestratorinferred "the new scope's rows have arrived" from resolution alone, so a dropped response clearedisScopeReplacementand un-hid the retained old-scope rows underneath the new scope's label. That is exactly the state the flag exists to prevent.fetchItemsnow reports whether it applied its response, and the orchestrator clears the flag only on an applied one. The eyebrow drops its counts while a replacement is in flight rather than publishing numbers about a scope the user has left.Closes #2501
Closes #2022
#2022 recorded exactly two open residuals in its 2026-08-24 comment, and both are now accounted for:
With both residuals resolved, #2022 has nothing left open, hence
Closesrather thanRefs. No doc was edited here;docs/STATUS.mdstill lists #2022 as open and will need that line updated when this lands, which is a maintainer call, not something this PR should do silently.Changes
The applied-response contract:
src/store/captureStore.ts—fetchItemsreturnsPromise<boolean>.truemeans the response was written intoitems;falsemeans it was dropped as superseded. A failure that is still the latest request still throws, so failure and supersession remain distinguishable. The change is additive: every existing call site that ignores the value keeps working, and the demo-mode branch reports the same way.src/composables/useInboxOrchestrator.ts—loadInboxInternalclearsisScopeReplacementonly when the store reports an applied response. The existing request-id and scope-key checks stay: they guard against a stale caller, while the new value guards against a dropped response.The in-flight presentation:
src/views/paper/PaperInboxView.vue— the eyebrow renders a count-free key whileisScopeReplacementis true, and the counted form otherwise. That key's final wording is in Round 2 below: it makes no claim about the load. ThePaperScopeDisclosurechip is untouched.src/views/paper/inbox/PaperTriageTable.vue— a newisBackgroundRefreshcomputed (loadingList && !scopeReplacement && !listError && hasItems) drives a refreshing note appended to the header's count line. Row actions are NOT disabled.src/locales/{en,it,es}/inbox.ts— two new keys,eyebrowUncountedandrefreshing. Neither carries an interpolation or plural forms. (eyebrowUncountedwas namedeyebrowLoadingin round 1; see Round 2 for why the verb went.)Specs:
src/tests/store/captureStore.spec.ts— the two existing superseded-response tests now assert the returned value as well, and a new test covers a superseded FAILURE, which is deliberately neither thrown nor recorded but is no longer silent: it reports that it applied nothing.src/tests/composables/useInboxOrchestrator.spec.ts— two new tests (a dropped response keeps the flag; the next applied response clears it), and three existing tests that resolved the deferred load withundefinednow resolvetrue. Those three passed under both the old and the new contract, so they were proving nothing about which one was in force.src/tests/views/paper/PaperInboxView.spec.ts— two new tests beside the existing scope-replacement pins: the eyebrow carries no digit at all during a replacement, and the counts return when it resolves.src/tests/views/paper/inbox/PaperTriageTable.spec.ts— three new tests: the note appears for a same-scope refresh over retained rows with row actions still enabled, and is absent both when idle and during a scope replacement.Test plan
Verified in
frontend/taskdeck-webat headc3258bef4:npx vitest --run --maxWorkers=2 src/tests/views/paper/PaperInboxView.spec.ts src/tests/views/paper/inbox src/tests/views/paper/PaperBoardManagement.spec.ts src/tests/composables/useInboxOrchestrator.spec.ts src/tests/store/captureStore.spec.ts src/tests/i18n/catalogs.spec.ts src/tests/views/InboxView.spec.ts— 11 files, 474 tests passed. Baseline measured on the branch point0886b6c42with the same command: 11 files, 466 tests passed. So +8 tests and no test lost.captureStore.spec.tsalone is 77, from 76 at base.captureStorereturn-value assertions and the refreshing note. A tenth surfaced immediately after the source change:marks a route-scope list load as a replacement until it succeedshad been resolvingundefined, which the new contract reads as "not applied". It was updated to resolvetrue, which is what the real store returns.npx vitest --run --maxWorkers=2 src/tests/store/captureStore.demo.spec.ts src/tests/store/captureStore.integration.spec.ts src/tests/property/storeResilience.spec.ts src/tests/resilience/degradedMode.spec.ts— 4 files, 51 tests passed. These are the otherfetchItemscallers, outside the standing glob, run because the signature changed.npm run typecheck— clean.npx eslintover all 11 changed files — clean.npm run build— succeeded.git diff --check— clean.src/tests/i18n/catalogs.spec.tsis inside the first run and is green, so ADR-0054 key, placeholder-set and plural-form-count parity hold for both new keys across en, it and es.NOT verified:
fetchItemsconsumer files.Boundaries and risks
The count-free eyebrow copy is a named assumption. Assumption: option (a), a separate count-free key, is what "count-free variant" meant, rather than suppressing the numbers inside the existing pluralized string. Reason:
inbox.eyebrowis one pluralized message chosen on{total}, so there is no way to omit the counts from it without either inventing a zero-form that lies or leaving empty interpolations. A new key with no placeholders and no pipe segments is the only shape that keeps the ADR-0054 parity guard meaningful. Reversible by deleting the key from the three catalogs and restoring the single ternary in the template. Round 1 gave that key a loading verb; Round 2 removed it, and the final English is justInbox · capture surface— the eyebrow's own prefix with the count segment dropped.The eyebrow's count-free form also shows on first mount, and that is deliberate.
onMountedcallsloadInboxForScopeReplacement, becausecaptureStore.itemscan still hold rows from a previously visited board. Those counts would be about the previous board, so the same reasoning applies and the same copy is correct there.fetchItemsreturns a boolean rather than an outcome object. An object would be more extensible, butif (await fetchItems(...))would then be silently and always true, which is a worse trap than a bare boolean at the two call sites that read it. The value is documented at the definition and namedappliedat the call site.The LOW item was implemented, in the narrow form only. The visible in-flight affordance is a note appended to the existing count line, driven by one computed. Row actions are NOT disabled during a background refresh — that was declined as a product-posture change, and the rows are still the right rows for the scope on screen. The note is intentionally not a live region:
aria-busyon the section already carries this state for assistive technology, and addingrole="status"inside a busy region risks a redundant or suppressed announcement. This is the visible half only.Risk of the contract change reaching an unintended caller.
fetchItemshas two production call sites: the orchestrator's list load and the store's own batch triage poll, which ignores the value. Nothing else insrc/calls it. The four other spec files that exercise it were run and are green.Deliberately out of scope:
PaperScopeDisclosure.vue(shared with the Review rail), the Legacy skin, the composer, every backend file, anddocs/STATUS.md/OUTSTANDING_TASKS.md/docs/IMPLEMENTATION_MASTERPLAN.md.Round 2
Head
fef363c02a7c2ccfef5b211546409d1e55c54dcf. One MEDIUM this PR had introduced, two LOWs taken, one LOW left to the reviewer.MEDIUM — the count-free eyebrow claimed a load that had already stopped. Round 1 branched the eyebrow on
isScopeReplacementalone. That flag is deliberately sticky across failure:loadInboxInternalswallows the throw so the retained rows stay hidden rather than being presented as the new scope, which is the behaviourkeeps the scope-replacement state after failure so retained rows stay hiddenpins. So with the API unreachable on mount, or a 500 on a scope switch, the header readInbox · capture surface · loading captures…permanently, sitting directly above the table's ownFailed to load inbox itemsand its Retry button. A false claim, and one this PR introduced.Fixed by dropping the verb rather than by adding a second condition.
inbox.eyebrowLoadingbecomesinbox.eyebrowUncountedin en, it and es and reads simply:Inbox · capture surfaceInbox · superficie di catturaInbox · superficie de capturaGating on
captureStore.loadingListas well would have needed a second count-free key for the not-loading case anyway, so one verb-free key covers both and cannot go stale. The division of labour is now clean: the table states loading, error and retry — it already does all three — and the eyebrow's only job is refusing to publish a count it cannot stand behind.New spec
makes no loading claim in the eyebrow when the replacement has failedmounts withisScopeReplacementtrue andlistErrorset, and asserts the eyebrow contains no digit and no "loading" of any casing, while the table still shows the error. Red first, together with the two round-1 eyebrow tests whose expectations changed: against the round-1 source those three fail, which was checked by restoringc3258bef4's source files under the round-2 specs before restoring the new ones.LOW — a spec that a missing catalog key would have satisfied.
PaperTriageTable.spec.tsassertedtoContain('refreshing'), and vue-i18n renders a missing key as the key itself,inbox.refreshing, which contains that substring. It now asserts the rendered English exactly,'· refreshing…'.LOW — the unreachable
return false. Kept, with a one-line comment, rather than removed. Nothing awaits between the request-id bump and the check, so the guard cannot currently fail; but the guard is pre-existing, it mirrors the shape of the real branch, and if the demo path ever becomes genuinely async its absence would be the bug. The comment says exactly that, so the next reader does not have to re-derive it.Not fixed, tracked by the reviewer: a dropped response latches
isScopeReplacementwith no error, no load and no retry affordance — reachable today only through Legacy'sbatchTriagerefresh.Verified at this head:
catalogs.spec.ts— 11 files, 475 tests passed, up from 474 at round 1 (the one new failure-state test). Baseline at the branch point0886b6c42was 466.fetchItemsconsumer specs — 4 files, 51 tests passed.npm run typecheck— clean.npx eslintover the seven files this round changed — clean.git diff --check— clean.catalogs.spec.tsgreen with the renamed key, so ADR-0054 parity holds across en, it and es.NOT verified this round, in addition to the original test plan: the failure state was exercised only through the mounted view with
listErrorset, not against a real failing server; and the new eyebrow copy was not checked visually at any viewport.