From d7fca42b89bdab26c7f1e38057f123967bdffe5f Mon Sep 17 00:00:00 2001 From: Chris0Jeky Date: Sat, 5 Sep 2026 23:02:01 +0100 Subject: [PATCH 1/4] fix(inbox): repair a dropped scope-replacement load instead of latching 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 --- .../src/composables/useInboxOrchestrator.ts | 86 ++++++++++-- .../composables/useInboxOrchestrator.spec.ts | 124 ++++++++++++++++-- 2 files changed, 191 insertions(+), 19 deletions(-) diff --git a/frontend/taskdeck-web/src/composables/useInboxOrchestrator.ts b/frontend/taskdeck-web/src/composables/useInboxOrchestrator.ts index 503522ce4..c63983e63 100644 --- a/frontend/taskdeck-web/src/composables/useInboxOrchestrator.ts +++ b/frontend/taskdeck-web/src/composables/useInboxOrchestrator.ts @@ -27,6 +27,22 @@ export function useInboxOrchestrator(options: { let scopedBoardLoadGeneration = 0 let latestInboxLoadRequestId = 0 const isScopeReplacement = ref(false) + /** + * Which scope a raised `isScopeReplacement` belongs to, and whether that + * replacement has already spent its one repair load (#2591). + * + * Orchestrator bookkeeping rather than a store observable on purpose. The + * store keeps its list request id private and publishes only `items`, + * `loadingList` and `listError` — and `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 #2501 harm. The + * `applied` boolean from `fetchItems` stays the only evidence that a list + * response was written, and this record is what tells a LATER load that the + * response it is about to report on is the one an outstanding replacement + * was waiting for. + */ + let scopeReplacementLatch: { scopeKey: string; repairIssued: boolean } | null = null // Batch selection state const selectedIds = ref>(new Set()) @@ -74,6 +90,18 @@ export function useInboxOrchestrator(options: { } } + /** + * The Inbox's current scope as a comparable key. Two list loads answer the + * same question only when their keys match; a load whose key no longer + * matches the current one must not touch scope-replacement state. + */ + function currentScopeKey(): string { + return JSON.stringify({ + boardId: activeBoardId.value, + archived: isArchivedHistory.value, + }) + } + const activeBoardName = computed(() => { const boardId = activeBoardId.value return boardId && scopedBoard.value?.id === boardId ? scopedBoard.value.name : boardId ?? '' @@ -424,14 +452,15 @@ export function useInboxOrchestrator(options: { async function loadInboxInternal(scopeReplacement = false) { const requestId = ++latestInboxLoadRequestId - const requestScopeKey = JSON.stringify({ - boardId: activeBoardId.value, - archived: isArchivedHistory.value, - }) + const requestScopeKey = currentScopeKey() if (scopeReplacement) { isScopeReplacement.value = true + // A fresh replacement, and a fresh repair budget with it. The repair + // below is a PLAIN load, so it can never refill its own budget. + scopeReplacementLatch = { scopeKey: requestScopeKey, repairIssued: false } } inboxLoadPerf.start() + let repairDroppedReplacement = false try { // `applied` is the store reporting that THIS call's response was written // into `items` (#2501). `fetchItems` resolves without writing anything @@ -442,18 +471,55 @@ export function useInboxOrchestrator(options: { // scope-key checks below stay: they guard against a stale caller, while // `applied` guards against a dropped response. const applied = await captureStore.fetchItems(currentListQuery()) - const currentScopeKey = JSON.stringify({ - boardId: activeBoardId.value, - archived: isArchivedHistory.value, - }) - if (applied && requestId === latestInboxLoadRequestId && requestScopeKey === currentScopeKey) { + // Still the latest load, still answering the scope the user is looking + // at. A stale load reports on nothing. + const isLatestForThisScope = + requestId === latestInboxLoadRequestId && requestScopeKey === currentScopeKey() + if (applied && isLatestForThisScope) { isScopeReplacement.value = false + scopeReplacementLatch = null + } else if ( + !applied && + isLatestForThisScope && + scopeReplacementLatch?.scopeKey === requestScopeKey + ) { + // A dropped response under an outstanding replacement for THIS scope + // (#2591). Nothing else will clear the flag: the read that superseded + // this one is the store's own post-batch refresh, whose `applied` + // result the store discards, and no later orchestrator load is coming + // (the id check above proved that). Left alone the flag latched + // forever, and `PaperTriageTable` then hid the rows AND their count + // with no Retry — an empty body under a count-free eyebrow. + // + // So: repair the scope ONCE with the current scope's query. The flag + // stays raised while that read is in flight, so the table shows its + // loading state rather than the wrong scope's rows. + if (scopeReplacementLatch.repairIssued) { + // The repair was dropped too. Do not loop. Clear the flag instead and + // let the table state the truth it has: the store's rows with their + // count, its empty state, or its error surface with Retry. Every + // read that can supersede this one resolves the SAME + // `currentListQuery` thunk at the moment it is issued (see + // `batchAction` and `captureStore.batchTriage`), so the rows that won + // are this scope's rows, not the retained previous scope's. + isScopeReplacement.value = false + scopeReplacementLatch = null + } else { + scopeReplacementLatch.repairIssued = true + repairDroppedReplacement = true + } } } catch { - // Store handles toast + error state. + // Store handles toast + error state. The flag stays raised deliberately: + // a failed load leaves the previous scope's rows in the store, and the + // table's error surface already carries a Retry. } await openItemFromHash() inboxLoadPerf.end() + if (repairDroppedReplacement) { + // A plain load: it must not restart the replacement, only finish it. + await loadInboxInternal() + } } function loadInbox() { diff --git a/frontend/taskdeck-web/src/tests/composables/useInboxOrchestrator.spec.ts b/frontend/taskdeck-web/src/tests/composables/useInboxOrchestrator.spec.ts index 78e6b330b..3e5d19bd6 100644 --- a/frontend/taskdeck-web/src/tests/composables/useInboxOrchestrator.spec.ts +++ b/frontend/taskdeck-web/src/tests/composables/useInboxOrchestrator.spec.ts @@ -959,30 +959,136 @@ describe('useInboxOrchestrator', () => { * throwing. Resolution alone therefore does not mean the new scope's rows * arrived, and treating it that way un-hid the retained OLD-scope rows * under the NEW scope's chip. Only an applied response clears the flag. + * + * #2591 keeps that rule and closes what it left open: a dropped response + * used to leave the flag latched with nothing coming, so the table hid the + * rows AND their count with no Retry. The four cases below pin the whole + * contract — a later applied load clears it, a drop with nothing in flight + * re-issues ONCE, a second drop stops rather than looping, and a drop under + * a changed scope still clears nothing. */ - it('keeps the scope-replacement state when the store drops a superseded response', async () => { - const pendingLoad = deferred() - mockCaptureStore.fetchItems.mockReturnValueOnce(pendingLoad.promise) + it('clears the scope-replacement state when the superseding load applies after the dropped one resolves', async () => { + const droppedLoad = deferred() + const supersedingLoad = deferred() + mockCaptureStore.fetchItems + .mockReturnValueOnce(droppedLoad.promise) + .mockReturnValueOnce(supersedingLoad.promise) const orch = createOrchestrator() - const load = orch.loadInboxForScopeReplacement() + const replacement = orch.loadInboxForScopeReplacement() + const superseding = orch.loadInbox() expect(orch.isScopeReplacement.value).toBe(true) - pendingLoad.resolve(false) - await load + // The store dropped the replacement's response because the second + // request superseded it, and it resolves AFTER that second request was + // issued. + droppedLoad.resolve(false) + await replacement + // The superseding load IS the later load for this scope: no repair read + // is owed, and the flag stays set until that load reports it applied. + expect(mockCaptureStore.fetchItems).toHaveBeenCalledTimes(2) expect(orch.isScopeReplacement.value).toBe(true) + + mockCaptureStore.items = [{ id: 'a' }, { id: 'b' }] + supersedingLoad.resolve(true) + await superseding + + // Coherent: rows exposed and the flag down, so the table renders the list + // and its count instead of a hidden body under a count-free eyebrow. + expect(orch.isScopeReplacement.value).toBe(false) + expect(orch.items.value).toHaveLength(2) + expect(mockCaptureStore.fetchItems).toHaveBeenCalledTimes(2) }) - it('clears the scope-replacement state on the next applied response', async () => { - mockCaptureStore.fetchItems.mockResolvedValueOnce(false).mockResolvedValueOnce(true) + it('re-issues the load once when a dropped replacement leaves nothing in flight', async () => { + mockRoute.query = { boardId: 'board-1' } + const droppedLoad = deferred() + mockCaptureStore.fetchItems + .mockReturnValueOnce(droppedLoad.promise) + .mockResolvedValueOnce(true) const orch = createOrchestrator() + const replacement = orch.loadInboxForScopeReplacement() + expect(orch.isScopeReplacement.value).toBe(true) + + // Dropped by a read the orchestrator never issued (the store's own + // post-batch refresh), so no later orchestrator load will clear this. + mockCaptureStore.items = [{ id: 'a' }] + droppedLoad.resolve(false) + await replacement + + expect(mockCaptureStore.fetchItems).toHaveBeenCalledTimes(2) + expect(mockCaptureStore.fetchItems).toHaveBeenLastCalledWith({ limit: 200, boardId: 'board-1' }) + expect(orch.isScopeReplacement.value).toBe(false) + }) + + it('stops after one re-issue when the repair load is dropped too', async () => { + mockRoute.query = { boardId: 'board-1' } + mockCaptureStore.fetchItems.mockResolvedValue(false) + const orch = createOrchestrator() + mockCaptureStore.items = [{ id: 'a' }] + await orch.loadInboxForScopeReplacement() + + // Exactly one repair read, never a loop. + expect(mockCaptureStore.fetchItems).toHaveBeenCalledTimes(2) + // Honest terminal state: the flag comes down with the store's rows + // visible, so the table shows the list and its count (or the store's + // error surface with Retry when the read recorded one) — not a hidden + // body with no affordance. + expect(orch.isScopeReplacement.value).toBe(false) + expect(orch.items.value).toHaveLength(1) + }) + + it('keeps the scope-replacement state when a dropped response resolves under a changed scope', async () => { + mockRoute.query = { boardId: 'board-1' } + const droppedLoad = deferred() + const newScopeLoad = deferred() + mockCaptureStore.fetchItems + .mockReturnValueOnce(droppedLoad.promise) + .mockReturnValueOnce(newScopeLoad.promise) + const orch = createOrchestrator() + + const boardOne = orch.loadInboxForScopeReplacement() + mockRoute.query = { boardId: 'board-2' } + const boardTwo = orch.loadInboxForScopeReplacement() + + droppedLoad.resolve(false) + await boardOne + + // No repair read for a scope nobody is looking at, and board-1's rows + // stay hidden under board-2's label (#2501). + expect(mockCaptureStore.fetchItems).toHaveBeenCalledTimes(2) expect(orch.isScopeReplacement.value).toBe(true) - await orch.loadInbox() + newScopeLoad.resolve(true) + await boardTwo + + expect(orch.isScopeReplacement.value).toBe(false) + }) + /** + * Replaces the old 'clears the scope-replacement state on the next applied + * response' case, whose shape — one dropped response, then a separately + * invoked load that applies — is now the repair read itself and is pinned + * above. What is left to pin is that the repair budget is per replacement: + * an exhausted one must not make the NEXT scope change unrepairable. + */ + it('gives each scope replacement its own single repair read', async () => { + mockCaptureStore.fetchItems + .mockResolvedValueOnce(false) + .mockResolvedValueOnce(false) + .mockResolvedValueOnce(false) + .mockResolvedValueOnce(true) + const orch = createOrchestrator() + + await orch.loadInboxForScopeReplacement() + expect(mockCaptureStore.fetchItems).toHaveBeenCalledTimes(2) + expect(orch.isScopeReplacement.value).toBe(false) + + await orch.loadInboxForScopeReplacement() + expect(mockCaptureStore.fetchItems).toHaveBeenCalledTimes(4) expect(orch.isScopeReplacement.value).toBe(false) }) From e3adaab261738a7a5a226b2c58661c8541e6bb24 Mon Sep 17 00:00:00 2001 From: Chris0Jeky Date: Sat, 5 Sep 2026 23:02:06 +0100 Subject: [PATCH 2/4] test(inbox): align the scope-truth fetchItems mock with the applied contract The mock predates #2584: it was typed Promise 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 --- .../tests/views/paper/inbox/PaperInboxScopeTruth.spec.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/frontend/taskdeck-web/src/tests/views/paper/inbox/PaperInboxScopeTruth.spec.ts b/frontend/taskdeck-web/src/tests/views/paper/inbox/PaperInboxScopeTruth.spec.ts index e0dc4ecee..d7de6f3b5 100644 --- a/frontend/taskdeck-web/src/tests/views/paper/inbox/PaperInboxScopeTruth.spec.ts +++ b/frontend/taskdeck-web/src/tests/views/paper/inbox/PaperInboxScopeTruth.spec.ts @@ -40,7 +40,7 @@ const mockCaptureStore = reactive({ listError: null as string | null, actionBusyItemId: null as string | null, triagePollingItemId: null as string | null, - fetchItems: vi.fn<(...args: unknown[]) => Promise>(), + fetchItems: vi.fn<(...args: unknown[]) => Promise>(), fetchDetail: vi.fn(), peekDetail: vi.fn(), cacheDetail: vi.fn(), @@ -129,7 +129,12 @@ describe('Paper Inbox scope truth (#1984)', () => { mockCaptureStore.detailById = {} mockCaptureStore.loadingList = false mockCaptureStore.listError = null - mockCaptureStore.fetchItems.mockResolvedValue(undefined) + // The store REPORTS whether it applied the response (#2501/#2584); this + // mock predates that contract and resolved `undefined`, which the + // orchestrator now correctly reads as a dropped response and repairs with + // a second read (#2591). The ordinary case under test here is an applied + // one, so say so. + mockCaptureStore.fetchItems.mockResolvedValue(true) mockBoardsApi.getBoard.mockResolvedValue(scopedBoard()) mockBoardStore.fetchBoards.mockResolvedValue(undefined) mockSessionStore.userId = 'user-a' From a187a10e9fda84f3f66941412e8ebee8b3da65a9 Mon Sep 17 00:00:00 2001 From: Chris0Jeky Date: Sat, 5 Sep 2026 23:16:11 +0100 Subject: [PATCH 3/4] test(inbox): align InboxView.spec's fetchItems mock with the applied 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. --- frontend/taskdeck-web/src/tests/views/InboxView.spec.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frontend/taskdeck-web/src/tests/views/InboxView.spec.ts b/frontend/taskdeck-web/src/tests/views/InboxView.spec.ts index ec6ee4ebb..07a7d0f6f 100644 --- a/frontend/taskdeck-web/src/tests/views/InboxView.spec.ts +++ b/frontend/taskdeck-web/src/tests/views/InboxView.spec.ts @@ -77,7 +77,7 @@ const mockCaptureStore = reactive({ } | null canEditSuggestion?: boolean }, syncSummary?: boolean) => void>(), - fetchItems: vi.fn<(...args: unknown[]) => Promise>(), + fetchItems: vi.fn<(...args: unknown[]) => Promise>(), fetchDetail: vi.fn<(itemId: string, options?: { forceRefresh?: boolean recordError?: boolean @@ -235,7 +235,7 @@ describe('InboxView', () => { mockCaptureStore.listError = null mockCaptureStore.detailError = null mockCaptureStore.actionError = null - mockCaptureStore.fetchItems.mockResolvedValue(undefined) + mockCaptureStore.fetchItems.mockResolvedValue(true) mockCaptureStore.fetchDetail.mockImplementation(async (itemId: string, options) => { const forceRefresh = options?.forceRefresh ?? false if (!forceRefresh && mockCaptureStore.detailById[itemId]) { From 032fa4f68903a129c121759c5252ed0d83a9522e Mon Sep 17 00:00:00 2001 From: Chris0Jeky Date: Sat, 5 Sep 2026 23:32:04 +0100 Subject: [PATCH 4/4] docs(inbox): state the second-drop clear's real guarantee 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. --- .../src/composables/useInboxOrchestrator.ts | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/frontend/taskdeck-web/src/composables/useInboxOrchestrator.ts b/frontend/taskdeck-web/src/composables/useInboxOrchestrator.ts index c63983e63..68c21440c 100644 --- a/frontend/taskdeck-web/src/composables/useInboxOrchestrator.ts +++ b/frontend/taskdeck-web/src/composables/useInboxOrchestrator.ts @@ -497,11 +497,15 @@ export function useInboxOrchestrator(options: { if (scopeReplacementLatch.repairIssued) { // The repair was dropped too. Do not loop. Clear the flag instead and // let the table state the truth it has: the store's rows with their - // count, its empty state, or its error surface with Retry. Every - // read that can supersede this one resolves the SAME - // `currentListQuery` thunk at the moment it is issued (see - // `batchAction` and `captureStore.batchTriage`), so the rows that won - // are this scope's rows, not the retained previous scope's. + // count, its empty state, or its error surface with Retry. Clearing + // here is safe only while this orchestrator is the sole Paper-side + // `fetchItems` caller: a superseding read resolves the same + // `currentListQuery` thunk when it is issued, but issued is not + // applied, and until it applies `items` still holds the last APPLIED + // list, which can be the previous scope's rows. Today the only other + // caller is Legacy's `batchTriage` reconciliation read, and Legacy + // never renders this flag. A Paper batch surface must clear only once + // a read has applied since the latch was raised. isScopeReplacement.value = false scopeReplacementLatch = null } else { @@ -512,7 +516,9 @@ export function useInboxOrchestrator(options: { } catch { // Store handles toast + error state. The flag stays raised deliberately: // a failed load leaves the previous scope's rows in the store, and the - // table's error surface already carries a Retry. + // table's error surface carries a Retry while `listError` is set. (A + // later background poll success clears `listError` without touching + // this flag; that failure-path shape is tracked on #2591.) } await openItemFromHash() inboxLoadPerf.end()