diff --git a/frontend/taskdeck-web/src/composables/useInboxOrchestrator.ts b/frontend/taskdeck-web/src/composables/useInboxOrchestrator.ts index 503522ce4..68c21440c 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,61 @@ 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. 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 { + 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 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() + 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) }) 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]) { 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'