diff --git a/frontend/taskdeck-web/src/composables/useInboxOrchestrator.ts b/frontend/taskdeck-web/src/composables/useInboxOrchestrator.ts index 880396d0a..503522ce4 100644 --- a/frontend/taskdeck-web/src/composables/useInboxOrchestrator.ts +++ b/frontend/taskdeck-web/src/composables/useInboxOrchestrator.ts @@ -1,6 +1,7 @@ import { computed, nextTick, onMounted, onUnmounted, ref, watch } from 'vue' import { useRoute, useRouter } from 'vue-router' import { useCaptureStore } from '../store/captureStore' +import type { DetailCacheOutcome } from '../store/captureStore' import { boardsApi } from '../api/boardsApi' import { isTriageTerminalStatus } from '../types/capture' import type { CaptureItem, CaptureItemSummary, CaptureListQuery } from '../types/capture' @@ -298,6 +299,12 @@ export function useInboxOrchestrator(options: { // only the list write is suppressed. Paper loads through `peekDetail` and is // unaffected either way. const syncSummary = cacheSummary && !isArchivedHistory.value + // Both halves of the selection. `selectedItemId` drives `aria-selected` and + // the detail panel; `activeItemIndex` drives the active row and + // `aria-activedescendant`. Restoring one without the other would leave the + // list pointing at a row the panel is not showing. + const previousSelectedItemId = selectedItemId.value + const previousActiveItemIndex = activeItemIndex.value primeSelection(itemId, preferredIndex) hashLoadFailedItemId.value = null try { @@ -305,7 +312,47 @@ export function useInboxOrchestrator(options: { captureStore.cacheDetail(preloadedDetail, syncSummary) return true } - await captureStore.fetchDetail(itemId, { syncSummary }) + // `onCacheOutcome` is the store reporting what it did with THIS read + // (#2640). `fetchDetail` resolves with the body it fetched on three paths + // that write nothing, so resolution alone is not evidence the panel has a + // detail to render: returning `true` regardless left `selectedItemId` on + // an id `detailById` holds nothing for, which `InboxDetailPanel` renders + // as "Unable to load capture detail." for a read that succeeded. A store + // that reports nothing is read as `cached`, the behaviour before #2640. + // A holder, not a bare `let`: TypeScript's control-flow analysis does not + // track an assignment made inside the callback and would narrow a plain + // variable to its initializer for every compare below. + const detailRead: { outcome: DetailCacheOutcome } = { outcome: 'cached' } + const report = (reported: DetailCacheOutcome) => { detailRead.outcome = reported } + await captureStore.fetchDetail(itemId, { syncSummary, onCacheOutcome: report }) + + // A `generation` drop is a LIVE-session case, not a post-logout one: a + // first open observes generation 0 for an uncached item, and a batch + // triage that includes it moves that generation while the read is in + // flight. `refreshTerminalDetails` skips such an item — it has a list row + // and no cached detail — so unlike a detail-path write, the batch write + // has cached no body of its own and there is nothing here to render. Not + // re-reading turned the click into a silent no-op and stripped a live + // deep link. Re-read ONCE, against the generation that caused the drop; + // a second collision in that window is left to the user's next click + // rather than looped over. + if (detailRead.outcome === 'generation' && !captureStore.detailById[itemId]) { + detailRead.outcome = 'cached' + await captureStore.fetchDetail(itemId, { syncSummary, onCacheOutcome: report }) + } + + // `superseded` means a newer read for the same id is already the + // authority, so the selection stands. Only the logout case is terminal: + // there is no newer read coming and no body to show. It is not a failure + // either, so restore the selection the user had — both halves of it — + // rather than clearing it, and report the read as not opened. + if (detailRead.outcome === 'epoch' && !captureStore.detailById[itemId]) { + if (selectedItemId.value === itemId) { + selectedItemId.value = previousSelectedItemId + activeItemIndex.value = previousActiveItemIndex + } + return false + } return true } catch { if (selectedItemId.value === itemId) { diff --git a/frontend/taskdeck-web/src/store/captureStore.ts b/frontend/taskdeck-web/src/store/captureStore.ts index 3d0e6c45e..ca0382857 100644 --- a/frontend/taskdeck-web/src/store/captureStore.ts +++ b/frontend/taskdeck-web/src/store/captureStore.ts @@ -24,6 +24,13 @@ function toSummary(item: CaptureItem): CaptureItemSummary { } } +/** + * What `fetchDetail` did with a response it resolved with (#2640). `cached` is + * the only value that means `detailById` holds this item's detail; the other + * three name which guard rejected the response. See `onCacheOutcome`. + */ +export type DetailCacheOutcome = 'cached' | 'superseded' | 'generation' | 'epoch' + type DetailLoadOptions = { forceRefresh?: boolean recordError?: boolean @@ -41,6 +48,45 @@ type DetailLoadOptions = { * quiet reads pass `false` and leave the flag to foreground loads. */ trackLoading?: boolean + /** + * REPORT what this call did with its response (#2640) — the detail-read + * counterpart of `fetchItems`'s applied boolean. + * + * `fetchDetail` has three paths that resolve with the body they fetched and + * write nothing, and resolution alone therefore never meant "the store now + * holds this detail". Callers that assumed it did acted on a response the + * store had dropped: `useInboxOrchestrator.selectItemById` left + * `selectedItemId` pointing at an id with no detail, which the Legacy panel + * renders as "Unable to load capture detail." for a read that succeeded, and + * `refreshTerminalDetails` announced a reconciliation that never reached + * `detailById`. + * + * The REASON is reported, not just the fact, because the three drops call for + * different handling and only one of them is post-logout-only: + * + * - `cached` — the response is in `detailById` now. Also covers the two paths + * that resolve from state already present, the non-forced cached early + * return and the demo branch: the question a caller asks is whether the + * store holds this detail, not whether this call did the writing. + * - `superseded` — the caller's own `shouldCache` said no, so a newer read + * for the same id is already the authority. + * - `generation` — a successful write for this id landed mid-read. Reachable + * in a LIVE session, not only after a logout: a first open observes + * generation 0 for an uncached item and a batch triage that includes it + * moves that generation. The write has cached its own newer body only when + * it went through the detail path; a batch write has not, so a caller that + * needs a body must re-read rather than treat this as final. + * - `epoch` — `resetForLogout` moved the session epoch under this read. + * + * A read that THROWS reports nothing: failure and a drop stay + * distinguishable. + * + * A callback rather than a return value because `fetchDetail` resolves with + * the `CaptureItem` itself and a view annotates that type + * (`views/paper/inbox/PaperTriageRowEdit.vue`), so widening the return would + * change a surface outside this seam. + */ + onCacheOutcome?: (outcome: DetailCacheOutcome) => void } export const BATCH_TRIAGE_POLL_INTERVAL_MS = 3_000 @@ -119,9 +165,12 @@ export const useCaptureStore = defineStore('capture', () => { let latestListWriteGeneration = 0 const latestDetailWriteGenerationById = new Map() const latestSummaryGenerationById = new Map() - // Bumped by `resetForLogout`. Every detail read captures it when the request - // is issued, so a read that crosses a logout is dropped outright instead of - // being compared against generations the reset has already discarded. + // Bumped by `resetForLogout`. Every read that is compared against the + // generations above captures it when its request is issued — the detail reads + // through `fetchDetail`, the single-item triage poll, and the background list + // snapshot in `pollBatchTriageCompletion` (#2640) — so a response that + // crosses a logout is dropped outright instead of being compared against + // generations the reset has already discarded. let sessionEpoch = 0 const actionBusyItemId = ref(null) const listError = ref(null) @@ -180,6 +229,11 @@ export const useCaptureStore = defineStore('capture', () => { * authority for membership and order — the list still owns scope and the * newest-first cap — and only a row whose own summary is newer than this * read keeps its local value. + * + * This compare is only meaningful WITHIN one session. `resetForLogout` clears + * `latestSummaryGenerationById`, which puts the read below back at 0 for + * every row, so the caller drops a snapshot the reset crossed before calling + * here at all (#2640) rather than letting the cleared map invert the guard. */ function applyBackgroundListSnapshot( loadedItems: CaptureItemSummary[], @@ -260,9 +314,11 @@ export const useCaptureStore = defineStore('capture', () => { requestOptions, shouldCache = () => true, trackLoading = true, + onCacheOutcome, } = options if (!forceRefresh && detailById.value[itemId]) { + onCacheOutcome?.('cached') return detailById.value[itemId] } @@ -271,6 +327,7 @@ export const useCaptureStore = defineStore('capture', () => { if (summary) { const detail = { ...summary, rawText: summary.textExcerpt, retryCount: 0, provenance: null } cacheDetail(detail, syncSummary) + onCacheOutcome?.('cached') return detail } } @@ -287,12 +344,22 @@ export const useCaptureStore = defineStore('capture', () => { const detail = requestOptions ? await captureApi.getItem(itemId, requestOptions) : await captureApi.getItem(itemId) - if (!shouldCache()) return detail - // Before any generation compare: the reset discards the generations this - // read observed, so after a logout the compare is no longer meaningful. - if (observedSessionEpoch !== sessionEpoch) return detail - if (observedDetailWriteGeneration !== detailWriteGeneration(itemId)) return detail - cacheDetail(detail, syncSummary) + // The three drop paths, in the order they have always been checked and + // now named for the caller (#2640). The epoch comes before any generation + // compare: the reset discards the generations this read observed, so + // after a logout the compare is no longer meaningful. + let outcome: DetailCacheOutcome = 'cached' + if (!shouldCache()) { + outcome = 'superseded' + } else if (observedSessionEpoch !== sessionEpoch) { + outcome = 'epoch' + } else if (observedDetailWriteGeneration !== detailWriteGeneration(itemId)) { + outcome = 'generation' + } + if (outcome === 'cached') { + cacheDetail(detail, syncSummary) + } + onCacheOutcome?.(outcome) return detail } catch (e: unknown) { const message = getErrorDisplay(e, 'Failed to load inbox item').message @@ -311,6 +378,9 @@ export const useCaptureStore = defineStore('capture', () => { } async function peekDetail(itemId: string, options: DetailLoadOptions = {}) { + // Shares `DetailLoadOptions` and therefore ACCEPTS `onCacheOutcome`, but + // never calls it: this read writes neither cache, so it has no cache + // outcome to report (#2640). const { forceRefresh = false, recordError = true, @@ -617,6 +687,13 @@ export const useCaptureStore = defineStore('capture', () => { await Promise.all( stale.map(async (id) => { try { + // `onRefreshed` is the caller's record that this id was RECONCILED, + // and for a tracked item with no summary row it is the whole of the + // batch poll's completion evidence. `fetchDetail` resolves on its + // drop paths too, so firing on resolution alone claimed a + // reconciliation that never reached `detailById` and let the poll + // complete on pre-batch state (#2640). + let cached = false await fetchDetail(id, { forceRefresh: true, showToast: false, @@ -647,8 +724,9 @@ export const useCaptureStore = defineStore('capture', () => { trackLoading: false, requestOptions: options.requestOptions, shouldCache: isCurrent, + onCacheOutcome: (outcome) => { cached = outcome === 'cached' }, }) - if (isCurrent()) options.onRefreshed?.(id) + if (cached && isCurrent()) options.onRefreshed?.(id) } catch { // A later poll tick retries transient detail failures. } @@ -782,13 +860,22 @@ export const useCaptureStore = defineStore('capture', () => { const observedListLoadRequestId = latestListLoadRequestId const observedListWriteGeneration = latestListWriteGeneration const observedSummaryGeneration = nextCaptureGeneration + const observedSessionEpoch = sessionEpoch const controller = new AbortController() activeRequest = controller const requestOptions = { signal: controller.signal, skipRetry: true } + // The epoch is checked first, for the same reason the detail path checks + // it first (#2640). `applyBackgroundListSnapshot` keeps a locally newer + // row only while `latestSummaryGenerationById` outranks this read's + // observed generation, and the reset CLEARS that map: the read comes back + // 0, so an older in-flight snapshot would outrank a row a newer read had + // moved and regress it. Dropping a snapshot the reset crossed leaves both + // halves of the guard with one post-logout contract. const isCurrent = () => !stopped && !controller.signal.aborted && activeRequest === controller && + observedSessionEpoch === sessionEpoch && observedListLoadRequestId === latestListLoadRequestId && observedListWriteGeneration === latestListWriteGeneration @@ -967,10 +1054,26 @@ export const useCaptureStore = defineStore('capture', () => { * starts with no recorded write for its id observes generation 0; if a write * then lands (generation N) and the map is cleared, the read's compare * becomes 0 !== 0, which is false, and the PRE-write body would be cached - * over the newer one and its status pushed back into the list. So the epoch - * moves here, after the clear: every detail read captures it when it issues - * its request and drops its response when it moved, before any generation - * compare. The shared clock itself stays monotonic. + * over the newer one and its status pushed back into the list. Clearing + * `latestSummaryGenerationById` inverts the summary half the same way, which + * is why the background list snapshot is guarded too (#2640). So the epoch + * moves here, after the clear: every read that is compared against these + * generations captures it when it issues its request and drops its response + * when it moved, before any generation compare. The shared clock itself stays + * monotonic. + * + * SCOPE, stated exactly: the reads that capture the epoch are `fetchDetail`, + * the single-item triage poll, and the batch poll's list snapshot. NOT + * `peekDetail` — it is compared against no generation because it writes + * neither cache. The `detailById` write on that path is the caller's: + * `useInboxOrchestrator.openBoardScopedHashItem` hands the body to + * `selectItemById` as `preloadedDetail` with `cacheSummary: false`, which + * reaches `cacheDetail` guarded only by the route-hash re-check. Capturing + * the epoch inside `peekDetail` would not cover that write, since the write + * is not `peekDetail`'s to drop; the guard would have to live in the + * composable, a per-mount surface the logout's route change tears down + * anyway. What the `cacheSummary: false` buys is that nothing from that path + * can reach `items`, so no row of a previous session's list survives it. */ function resetForLogout() { latestDetailWriteGenerationById.clear() diff --git a/frontend/taskdeck-web/src/tests/composables/useInboxOrchestrator.spec.ts b/frontend/taskdeck-web/src/tests/composables/useInboxOrchestrator.spec.ts index 2b8484ed1..78e6b330b 100644 --- a/frontend/taskdeck-web/src/tests/composables/useInboxOrchestrator.spec.ts +++ b/frontend/taskdeck-web/src/tests/composables/useInboxOrchestrator.spec.ts @@ -70,6 +70,7 @@ vi.mock('../../utils/navigation', () => ({ })) import { useInboxOrchestrator } from '../../composables/useInboxOrchestrator' +import type { DetailCacheOutcome } from '../../store/captureStore' function createOrchestrator() { mountedCallback = null @@ -169,6 +170,11 @@ describe('useInboxOrchestrator', () => { ], }) mockCaptureStore.pollBatchTriageCompletion.mockReset().mockReturnValue(vi.fn()) + // `vi.clearAllMocks()` clears recorded calls but keeps implementations, + // including UNCONSUMED `mockImplementationOnce` entries. A case that queues + // a re-read the code under test does not take would hand that queued + // implementation to the next test. + mockCaptureStore.fetchDetail.mockReset() // The store reports whether it APPLIED the response (#2501). The default is // the ordinary case: the response was the latest and was written. mockCaptureStore.fetchItems.mockReset().mockResolvedValue(true) @@ -442,6 +448,7 @@ describe('useInboxOrchestrator', () => { await orch.openItemFromList(summaryRow('archived-capture', 'archived-board'), 0) expect(mockCaptureStore.fetchDetail).toHaveBeenCalledWith('archived-capture', { syncSummary: false, + onCacheOutcome: expect.any(Function), }) // Refresh Detail is a READ affordance and stays available in history mode, @@ -465,6 +472,7 @@ describe('useInboxOrchestrator', () => { await orch.openItemFromList(summaryRow('live-capture', 'live-board'), 0) expect(mockCaptureStore.fetchDetail).toHaveBeenCalledWith('live-capture', { syncSummary: true, + onCacheOutcome: expect.any(Function), }) mockCaptureStore.fetchDetail.mockClear() @@ -686,7 +694,10 @@ describe('useInboxOrchestrator', () => { const event = { key: 'Enter', preventDefault: vi.fn() } as unknown as KeyboardEvent await orch.handleKeydown(event) expect(event.preventDefault).toHaveBeenCalled() - expect(mockCaptureStore.fetchDetail).toHaveBeenCalledWith('abc', { syncSummary: true }) + expect(mockCaptureStore.fetchDetail).toHaveBeenCalledWith('abc', { + syncSummary: true, + onCacheOutcome: expect.any(Function), + }) }) }) @@ -695,7 +706,10 @@ describe('useInboxOrchestrator', () => { mockRoute.hash = '#capture-deep-id' const orch = createOrchestrator() await orch.loadInbox() - expect(mockCaptureStore.fetchDetail).toHaveBeenCalledWith('deep-id', { syncSummary: true }) + expect(mockCaptureStore.fetchDetail).toHaveBeenCalledWith('deep-id', { + syncSummary: true, + onCacheOutcome: expect.any(Function), + }) }) it('loadInbox does not fetch detail when hash is absent', async () => { @@ -1026,4 +1040,170 @@ describe('useInboxOrchestrator', () => { expect(mockCaptureStore.fetchDetail).not.toHaveBeenCalled() }) }) + + /** + * A DROPPED read is the store resolving with the body it fetched while + * writing nothing into its caches (#2640). It is neither a failure nor a + * cached detail, and `fetchDetail` used to report it to nobody, so + * `selectItemById` returned `true` for it and left `selectedItemId` pointing + * at an id `detailById` holds nothing for — the state the Legacy detail panel + * renders as "Unable to load capture detail." for a read that succeeded. + * + * `onCacheOutcome` is the store's report. `mockImplementationOnce` on + * purpose: the suite's reset is `vi.clearAllMocks()`, which keeps + * implementations, so a persistent one here would follow the tests below. + */ + describe('dropped detail reads', () => { + const keptDetail = { id: 'kept', rawText: 'kept body', boardId: null, status: 'New' } + + it('holds the previous selection and active row when a logout dropped the read', async () => { + mockCaptureStore.items = [{ id: 'kept' }, { id: 'dropped' }] + mockCaptureStore.detailById = { kept: keptDetail } + const orch = createOrchestrator() + + mockCaptureStore.fetchDetail.mockImplementationOnce(async ( + _itemId: string, + options?: { onCacheOutcome?: (outcome: DetailCacheOutcome) => void }, + ) => { options?.onCacheOutcome?.('cached') }) + await orch.openItemFromList(summaryRow('kept', null), 0) + expect(orch.selectedItemId.value).toBe('kept') + + // The store fetched a body and cached none of it, and nothing else has + // put a detail for this id in `detailById` either. + mockCaptureStore.fetchDetail.mockImplementationOnce(async ( + _itemId: string, + options?: { onCacheOutcome?: (outcome: DetailCacheOutcome) => void }, + ) => { options?.onCacheOutcome?.('epoch') }) + await orch.openItemFromList(summaryRow('dropped', null), 1) + + expect(orch.selectedItemId.value).toBe('kept') + expect(orch.selectedItem.value).toEqual(keptDetail) + // `activeItemIndex` drives the active row and `aria-activedescendant` + // while `selectedItemId` drives `aria-selected` and the panel. Restoring + // one without the other leaves the list pointing at a row the panel is + // not showing, so both come back. + expect(orch.activeItemIndex.value).toBe(0) + }) + + it('keeps the selection when a dropped read left a newer detail cached', async () => { + mockCaptureStore.items = [{ id: 'superseded' }] + mockCaptureStore.detailById = {} + const orch = createOrchestrator() + + // The other pre-existing drop path: a successful write superseded this + // read's generation. That write cached its OWN newer body, so the panel + // has a detail to render and the selection is honest. + mockCaptureStore.fetchDetail.mockImplementationOnce(async ( + itemId: string, + options?: { onCacheOutcome?: (outcome: DetailCacheOutcome) => void }, + ) => { + mockCaptureStore.detailById[itemId] = { + id: itemId, rawText: 'newer body', boardId: null, status: 'ProposalCreated', + } + options?.onCacheOutcome?.('generation') + }) + await orch.openItemFromList(summaryRow('superseded', null), 0) + + expect(orch.selectedItemId.value).toBe('superseded') + expect(orch.selectedItem.value).toEqual({ + id: 'superseded', rawText: 'newer body', boardId: null, status: 'ProposalCreated', + }) + }) + + /** + * The write-generation drop is NOT post-logout-only. A first open observes + * generation 0 for an uncached item; a batch triage that includes it moves + * that generation through `recordCaptureWrite`, and + * `refreshTerminalDetails` skips the item because it has a list row and no + * cached detail. So the read is dropped with `detailById` still empty and + * NOTHING has cached a body for the id — restoring the selection there + * turns a live click into a silent no-op and strips a live deep link. + */ + const reReadDetail = { + id: 'crossed', rawText: 'reconciled body', boardId: null, status: 'ProposalCreated', + } + + function mockBatchCrossedThenCachingReRead() { + mockCaptureStore.fetchDetail.mockImplementationOnce(async ( + _itemId: string, + options?: { onCacheOutcome?: (outcome: DetailCacheOutcome) => void }, + ) => { options?.onCacheOutcome?.('generation') }) + mockCaptureStore.fetchDetail.mockImplementationOnce(async ( + itemId: string, + options?: { onCacheOutcome?: (outcome: DetailCacheOutcome) => void }, + ) => { + mockCaptureStore.detailById[itemId] = { ...reReadDetail, id: itemId } + options?.onCacheOutcome?.('cached') + }) + } + + it('re-reads once and keeps the deep-link hash when a batch write crossed the read', async () => { + mockRoute.hash = '#capture-crossed' + mockCaptureStore.items = [{ id: 'crossed' }] + mockCaptureStore.detailById = {} + const orch = createOrchestrator() + mockBatchCrossedThenCachingReRead() + + await orch.loadInbox() + + expect(mockCaptureStore.fetchDetail).toHaveBeenCalledTimes(2) + expect(orch.selectedItemId.value).toBe('crossed') + expect(orch.selectedItem.value).toEqual(reReadDetail) + // The hash still names a capture the user can see, so nothing clears it. + expect(mockRouter.replace).not.toHaveBeenCalled() + }) + + it('re-reads once so a click still opens an item a batch write crossed', async () => { + mockCaptureStore.items = [{ id: 'crossed' }] + mockCaptureStore.detailById = {} + const orch = createOrchestrator() + mockBatchCrossedThenCachingReRead() + + await orch.openItemFromList(summaryRow('crossed', null), 0) + + expect(mockCaptureStore.fetchDetail).toHaveBeenCalledTimes(2) + expect(orch.selectedItemId.value).toBe('crossed') + expect(orch.activeItemIndex.value).toBe(0) + expect(orch.selectedItem.value).toEqual(reReadDetail) + }) + + it('does not re-read when the crossing write already cached a body', async () => { + mockCaptureStore.items = [{ id: 'crossed' }] + mockCaptureStore.detailById = {} + const orch = createOrchestrator() + + mockCaptureStore.fetchDetail.mockImplementationOnce(async ( + itemId: string, + options?: { onCacheOutcome?: (outcome: DetailCacheOutcome) => void }, + ) => { + mockCaptureStore.detailById[itemId] = { ...reReadDetail, id: itemId } + options?.onCacheOutcome?.('generation') + }) + + await orch.openItemFromList(summaryRow('crossed', null), 0) + + expect(mockCaptureStore.fetchDetail).toHaveBeenCalledTimes(1) + expect(orch.selectedItemId.value).toBe('crossed') + }) + + it('still treats a rejected detail read as a failure', async () => { + mockCaptureStore.items = [{ id: 'kept' }, { id: 'broken' }] + mockCaptureStore.detailById = { kept: keptDetail } + const orch = createOrchestrator() + + mockCaptureStore.fetchDetail.mockImplementationOnce(async ( + _itemId: string, + options?: { onCacheOutcome?: (outcome: DetailCacheOutcome) => void }, + ) => { options?.onCacheOutcome?.('cached') }) + await orch.openItemFromList(summaryRow('kept', null), 0) + + mockCaptureStore.fetchDetail.mockRejectedValueOnce(new Error('detail unavailable')) + await orch.openItemFromList(summaryRow('broken', null), 1) + + // A failure clears the selection outright, as before. It does not restore + // the previous one: the user asked for this row and the store reported it + // unreadable, so the panel's error surface is the honest one. + expect(orch.selectedItemId.value).toBeNull() + }) + }) }) diff --git a/frontend/taskdeck-web/src/tests/store/captureStore.spec.ts b/frontend/taskdeck-web/src/tests/store/captureStore.spec.ts index 4a2e1936d..2c5f071bb 100644 --- a/frontend/taskdeck-web/src/tests/store/captureStore.spec.ts +++ b/frontend/taskdeck-web/src/tests/store/captureStore.spec.ts @@ -2557,8 +2557,19 @@ describe('captureStore', () => { summaryRow('c-2', 'ProposalCreated'), ] as never) const pendingDetails = new Map void>() - vi.mocked(captureApi.getItem).mockImplementation(((itemId: string) => - new Promise((resolve) => { pendingDetails.set(itemId, resolve) })) as never) + // Bounded to this test's three reads (#2640). A PERSISTENT + // `mockImplementation` returning never-resolving promises outlives the + // test: the suite's only reset is `vi.clearAllMocks()`, which clears + // recorded calls but keeps implementations, so any later test that + // triggers an unconfigured detail read would hang on a promise this test + // installed instead of receiving `undefined`. A fourth read here fails on + // an absent `pendingDetails` entry, which is a visible failure. + const pendingDetail = ((itemId: string) => + new Promise((resolve) => { pendingDetails.set(itemId, resolve) })) as never + vi.mocked(captureApi.getItem) + .mockImplementationOnce(pendingDetail) + .mockImplementationOnce(pendingDetail) + .mockImplementationOnce(pendingDetail) // Two stale ids, so the reconciliation runs two reads in parallel. const batch = store.batchTriage(['c-1', 'c-2'], 'triage') @@ -2592,6 +2603,22 @@ describe('captureStore', () => { expect(store.loadingDetail).toBe(false) }) + // Ordering-coupled on purpose (#2640). It sits directly after the test that + // installs never-resolving detail promises. `vi.clearAllMocks()` clears + // recorded calls but keeps implementations, so whatever that test installed + // is what the tests below inherit. A leaked `mockResolvedValue` from some + // earlier test is harmless — it settles — so the property that matters is + // not "unconfigured" but "settles": a leaked never-resolving implementation + // hangs whichever later test triggers an unconfigured detail read. A + // microtask always wins against a macrotask, so this race is deterministic. + it('leaves no never-resolving detail mock behind for the tests below', async () => { + const outcome = await Promise.race([ + Promise.resolve(vi.mocked(captureApi.getItem)('c-unconfigured')).then(() => 'settles'), + new Promise((resolve) => { setTimeout(() => resolve('hangs'), 0) }), + ]) + expect(outcome).toBe('settles') + }) + it('still raises the detail loading flag for a foreground detail load', async () => { const store = useCaptureStore() let resolveDetail!: (value: unknown) => void @@ -2759,8 +2786,75 @@ describe('captureStore', () => { }, ) + /** + * `refreshedDetailIds` is the poll's record of ids it RECONCILED, and for a + * tracked item with no summary row it is the whole of `isComplete`'s and + * `isObservedTerminal`'s evidence. `onRefreshed` used to fire on + * `fetchDetail` RESOLVING, which a dropped read also does, so the poll + * could complete and move the badge on pre-batch detail state (#2640). + * + * Isolated on the write-generation drop path on purpose: with no summary + * row, `keepItem` records its write without moving the list write + * generation, so the tick's own `isCurrent()` stays true and the only + * thing rejecting the read is `fetchDetail`'s own compare. + */ + it('does not claim a reconciliation for a detail read the store dropped', async () => { + vi.useFakeTimers() + try { + const store = useCaptureStore() + // Beyond the newest-first list cap, so the detail is the only surface + // this poll can complete on. It holds pre-batch state: the Failed + // outcome the capture was re-triaged for. + store.items = [] + store.detailById['c-1'] = detailFor('c-1', 'Failed') + vi.mocked(captureApi.listItems).mockResolvedValue([] as never) + + let resolveReconcile!: (value: unknown) => void + vi.mocked(captureApi.getItem).mockReturnValueOnce( + new Promise((resolve) => { resolveReconcile = resolve }) as never, + ) + + const stop = store.pollBatchTriageCompletion(['c-1'], { limit: 200 }) + await vi.advanceTimersByTimeAsync(3_000) + expect(captureApi.getItem).toHaveBeenCalledTimes(1) + + // The user keeps the capture while that reconciliation read is in + // flight. Keep leaves the triage status alone. + vi.mocked(captureApi.keepItem).mockResolvedValue(detailFor('c-1', 'Failed')) + await store.keepItem('c-1') + const countRefreshesAfterWrite = workspaceMocks.refreshWorkloadCounts.mock.calls.length + + resolveReconcile(detailFor('c-1', 'ProposalCreated')) + await vi.advanceTimersByTimeAsync(0) + + // The response never entered the caches, so the reconciled body is not + // what the store holds. + expect(store.detailById['c-1']?.status).toBe('Failed') + // Nothing was reconciled, so the poll must neither complete nor move + // the badge on the strength of a reconciliation that did not happen. + expect(workspaceMocks.refreshWorkloadCounts) + .toHaveBeenCalledTimes(countRefreshesAfterWrite) + await vi.advanceTimersByTimeAsync(3_000) + expect(vi.mocked(captureApi.listItems).mock.calls.length).toBeGreaterThan(1) + stop() + } finally { + vi.useRealTimers() + } + }) + describe('resetForLogout', () => { - it('clears the per-item summary generation guard', async () => { + /** + * Both halves of the #2301 guard now carry ONE post-logout contract: a + * response issued before the reset is dropped, a response issued after it + * caches normally (#2640). + * + * This case used to assert the opposite for the summary half. Clearing + * `latestSummaryGenerationById` puts `applyBackgroundListSnapshot`'s read + * back at 0, so an older in-flight snapshot outranked a row a newer read + * had moved and pushed it back to Triaging — the same inversion the epoch + * was added to stop on the detail half. + */ + it('drops a background list snapshot issued before the logout', async () => { vi.useFakeTimers() try { const store = useCaptureStore() @@ -2787,7 +2881,32 @@ describe('captureStore', () => { await Promise.resolve() await Promise.resolve() - // The entry is gone, so nothing stale-high is left to pin the row. + // The snapshot was issued in the epoch the reset ended, so it is not + // applied at all and the row it would have regressed stands. + expect(store.items.find((item) => item.id === 'c-1')?.status).toBe('Failed') + stop() + } finally { + vi.useRealTimers() + } + }) + + it('applies a background list snapshot issued after the logout', async () => { + vi.useFakeTimers() + try { + const store = useCaptureStore() + store.items = [summaryRow('c-1', 'Failed')] + store.resetForLogout() + + vi.mocked(captureApi.listItems).mockResolvedValue([ + summaryRow('c-1', 'Triaging'), + ] as never) + const stop = store.pollBatchTriageCompletion(['c-1'], { limit: 200 }) + await vi.advanceTimersByTimeAsync(3_000) + + // The epoch drops what a reset CROSSED, not everything after it. A + // request issued in the current epoch is current, and the snapshot is + // still the authority for membership, order and status. + expect(captureApi.listItems).toHaveBeenCalledTimes(1) expect(store.items.find((item) => item.id === 'c-1')?.status).toBe('Triaging') stop() } finally { diff --git a/frontend/taskdeck-web/src/tests/views/InboxView.spec.ts b/frontend/taskdeck-web/src/tests/views/InboxView.spec.ts index ae9fc421a..ec6ee4ebb 100644 --- a/frontend/taskdeck-web/src/tests/views/InboxView.spec.ts +++ b/frontend/taskdeck-web/src/tests/views/InboxView.spec.ts @@ -409,7 +409,10 @@ describe('InboxView', () => { await waitForUi() expect(mockCaptureStore.fetchItems).toHaveBeenCalledWith({ limit: 200 }) - expect(mockCaptureStore.fetchDetail).toHaveBeenCalledWith('capture-2', { syncSummary: true }) + expect(mockCaptureStore.fetchDetail).toHaveBeenCalledWith('capture-2', { + syncSummary: true, + onCacheOutcome: expect.any(Function), + }) expect(wrapper.text()).toContain('Full text for capture-2') }) @@ -684,7 +687,10 @@ describe('InboxView', () => { const wrapper = mount(InboxView) await waitForUi() - expect(mockCaptureStore.fetchDetail).toHaveBeenCalledWith('missing-capture', { syncSummary: true }) + expect(mockCaptureStore.fetchDetail).toHaveBeenCalledWith('missing-capture', { + syncSummary: true, + onCacheOutcome: expect.any(Function), + }) expect(wrapper.text()).toContain('Select an item to inspect the captured text') expect(routerMocks.replace).toHaveBeenCalledWith({ name: 'workspace-inbox', @@ -712,7 +718,10 @@ describe('InboxView', () => { await firstRow.trigger('click') await waitForUi() - expect(mockCaptureStore.fetchDetail).toHaveBeenCalledWith('capture-1', { syncSummary: true }) + expect(mockCaptureStore.fetchDetail).toHaveBeenCalledWith('capture-1', { + syncSummary: true, + onCacheOutcome: expect.any(Function), + }) expect(wrapper.text()).toContain('Full text for capture-1') }) @@ -725,7 +734,10 @@ describe('InboxView', () => { await listbox.trigger('keydown', { key: 'Enter' }) await waitForUi() - expect(mockCaptureStore.fetchDetail).toHaveBeenCalledWith('capture-2', { syncSummary: true }) + expect(mockCaptureStore.fetchDetail).toHaveBeenCalledWith('capture-2', { + syncSummary: true, + onCacheOutcome: expect.any(Function), + }) expect(wrapper.text()).toContain('Full text for capture-2') }) @@ -767,7 +779,10 @@ describe('InboxView', () => { await listbox.trigger('keydown', { key: 'Enter' }) await waitForUi() - expect(mockCaptureStore.fetchDetail).toHaveBeenCalledWith('capture-2', { syncSummary: true }) + expect(mockCaptureStore.fetchDetail).toHaveBeenCalledWith('capture-2', { + syncSummary: true, + onCacheOutcome: expect.any(Function), + }) expect(wrapper.text()).toContain('Full text for capture-2') })