diff --git a/frontend/taskdeck-web/src/composables/useReviewProposals.ts b/frontend/taskdeck-web/src/composables/useReviewProposals.ts index 644f6e0eb..490f2725d 100644 --- a/frontend/taskdeck-web/src/composables/useReviewProposals.ts +++ b/frontend/taskdeck-web/src/composables/useReviewProposals.ts @@ -617,6 +617,65 @@ export function useReviewProposals() { */ const queueAnnouncementKey = computed(() => awaitingProposalIds.value.join('\n')) + /** + * The board scope of a queue read, as one comparable value. Lower-cased + * because the scope is the BOARD, not the casing the query string happened to + * carry — the same rule `matchesActiveBoardFilter` applies — and an empty + * filter is the unscoped queue, exactly as `boardId: activeBoardFilter.value + * || undefined` sends it. + */ + function queueScopeOf(boardId: string | null | undefined): string | null { + return boardId ? boardId.toLowerCase() : null + } + + /** + * The scope a queue read has actually LANDED for, or `undefined` while no read + * has landed at all. + */ + const landedQueueScope = ref(undefined) + + /** + * Whether a queue read has landed for the board scope currently on screen + * (#2599 item 1). This is what the two skins' announcement gates need, and + * `!proposalsLoading` was the wrong approximation of it. + * + * An explicit `loadProposals` raises `proposalsLoading` WITHOUT clearing + * `proposals`, so gating on it unmounted the announcement node for the length + * of every reload and remounted it with the identical sentence: the live + * region wrote count -> '' -> count, and a node addition is exactly what + * `aria-live` speaks. The reviewer heard the same figure read back after the + * header Refresh and after filing away a settled proposal — reads they asked + * for, about a queue that had not moved. + * + * The states this DOES withhold, all of them cases where the rendered count + * is not a count of the queue on screen: + * - before the first read lands (the #2593 skeleton gate — the count is 0 + * because nothing has been read, not because nothing awaits review); + * - after a board-filter change, until the new scope's read lands: the rows + * still rendered belong to the previous board; + * - for a scope no read has landed for, including one whose only read + * failed: the entry load, or the first read after a filter change. + * + * What it deliberately does NOT withhold is the count after a LATER read + * failed. `landedQueueScope` is written only where a read replaced the queue + * and cleared only by `recordQueueAccessRevoked`; the catch arm leaves it + * alone. So an entry load that landed for board A followed by a header + * Refresh that 500s keeps the count announceable — correctly, because the + * rows on screen are still that landed answer. Re-withholding there would put + * back the count -> '' -> count flicker this signal exists to remove, on a + * read that changed nothing. A failing refresh has its own reports: the + * degraded and refused disclosures (#2214). + * + * A same-scope reload keeps it settled, because the queue it is about is + * still the one being counted. `queueAccessRevoked` keeps its own separate + * gate: a revocation is a different fact with a different remedy. + */ + const queueScopeLoaded = computed( + () => + landedQueueScope.value !== undefined && + landedQueueScope.value === queueScopeOf(activeBoardFilter.value), + ) + function isProposalDismissable(proposal: ApiProposal): boolean { const status = normalizeProposalStatus(proposal.status) return ( @@ -808,6 +867,10 @@ export function useReviewProposals() { limit: 200, boardId: activeBoardFilter.value || undefined, } + // The scope this read is ASKING about, snapshotted before the await. A + // late answer describes the board it queried, never whichever board is on + // screen when it lands (#2599 item 1). + const requestedScope = queueScopeOf(filters.boardId) // The second argument is forwarded ONLY when a caller supplied options, // so every existing call site keeps its exact single-argument shape. const loadedProposals = options @@ -821,6 +884,10 @@ export function useReviewProposals() { // authority behind its back, and proves nothing about queue freshness. if (signal?.aborted) return 'aborted' proposals.value = loadedProposals + // A read has now landed for that scope, so the count the surfaces render + // is a real count of the queue on screen and may be announced (#2599 + // item 1). + landedQueueScope.value = requestedScope // An explicit successful load is as trustworthy as a successful poll and // clears any older degraded indication without changing load semantics. // It goes through the same accounting as a successful poll so both exits @@ -967,6 +1034,11 @@ export function useReviewProposals() { function recordQueueAccessRevoked() { queueAccessRevoked.value = true proposals.value = [] + // What is rendered is no longer any read's answer, so no read has landed + // for this scope any more (#2599 item 1). The revoked panel has its own + // gate on both surfaces; this only stops the count coming back as speakable + // the moment the panel clears for some other reason. + landedQueueScope.value = undefined suspendQueueRefreshForPermission() } @@ -1317,6 +1389,11 @@ export function useReviewProposals() { } } proposals.value = next + // `isCurrentRead` has already refused any answer whose scope moved, so + // this read landed for the board on screen (#2599 item 1). The poll is a + // landing site in its own right: after a failed entry load, it is what + // makes the count speakable again without the reviewer reloading. + landedQueueScope.value = queueScopeOf(requestedBoardId) recordQueueRefreshSuccess({ source: 'poll', recoveryAlreadyRaised: listRecoveryRaised }) // The queue moved under a reviewer who did not ask for it. Surfaces use // this to notice that the row they were rendering has just been dropped @@ -1594,6 +1671,7 @@ export function useReviewProposals() { summaryCards, awaitingProposalIds, queueAnnouncementKey, + queueScopeLoaded, dismissableProposalIds, matchesActiveBoardFilter, isProposalExpired, diff --git a/frontend/taskdeck-web/src/tests/composables/useReviewProposals.spec.ts b/frontend/taskdeck-web/src/tests/composables/useReviewProposals.spec.ts index ec18d2598..ba013ef80 100644 --- a/frontend/taskdeck-web/src/tests/composables/useReviewProposals.spec.ts +++ b/frontend/taskdeck-web/src/tests/composables/useReviewProposals.spec.ts @@ -1209,6 +1209,124 @@ describe('useReviewProposals', () => { }) }) + // #2599 item 1. Both skins gated the announcement on "no read is in flight", + // so every EXPLICIT reload unmounted the sentence and remounted it: the live + // region wrote count -> '' -> count and the restore was spoken even when the + // queue had not moved (the header Refresh, and filing away a settled proposal + // that leaves the pending-review set identical, both reach it). The gate + // actually needs "a read has landed for the board scope on screen", which is + // what this signal is: a reload of the SAME scope keeps it settled, a scope + // change unsettles it until the new scope's read lands, and a failed read + // never settles it at all. + describe('queue scope load signal (#2599 item 1)', () => { + it('settles on the first landed read and stays settled across a same-scope reload', async () => { + mockRoute.query = { boardId: 'board-a' } + const rp = useReviewProposals() + // Nothing has been read yet, so the count is 0 for that reason rather + // than because nothing awaits review. + expect(rp.queueScopeLoaded.value).toBe(false) + + mockAutomationApi.getProposals.mockResolvedValueOnce([ + makeProposal({ id: 'p-a', boardId: 'board-a' }), + ]) + await rp.loadProposals() + expect(rp.queueScopeLoaded.value).toBe(true) + + // An explicit reload raises `proposalsLoading` WITHOUT clearing + // `proposals`, so the rendered count is still the last landed read's and + // still true of the board on screen. + let releaseReload!: (value: unknown[]) => void + mockAutomationApi.getProposals.mockReturnValueOnce( + new Promise((resolve) => { + releaseReload = resolve as (value: unknown[]) => void + }), + ) + const reload = rp.loadProposals() + expect(rp.proposalsLoading.value).toBe(true) + expect(rp.queueScopeLoaded.value).toBe(true) + + releaseReload([makeProposal({ id: 'p-a', boardId: 'board-a' })]) + await reload + expect(rp.proposalsLoading.value).toBe(false) + expect(rp.queueScopeLoaded.value).toBe(true) + }) + + it('unsettles the moment the board scope changes and settles again when that scope lands', async () => { + mockRoute.query = { boardId: 'board-a' } + const rp = useReviewProposals() + mockAutomationApi.getProposals.mockResolvedValueOnce([ + makeProposal({ id: 'p-a', boardId: 'board-a' }), + ]) + await rp.loadProposals() + expect(rp.queueScopeLoaded.value).toBe(true) + + // The rendered queue is still board-a's, so under board-b it counts + // nothing that is on screen -- the one case where withholding is right. + mockRoute.query = { boardId: 'board-b' } + expect(rp.queueScopeLoaded.value).toBe(false) + // The composable's own scope watcher is what issues the reload below. + expect(watcherForCurrentSourceValue('board-b')).toBeDefined() + + mockAutomationApi.getProposals.mockResolvedValueOnce([ + makeProposal({ id: 'p-b', boardId: 'board-b' }), + ]) + await rp.loadProposals() + expect(rp.queueScopeLoaded.value).toBe(true) + + // Case is the query string's, not the scope's: the same board asked for + // twice is one scope, and `matchesActiveBoardFilter` compares it the same + // way. + mockRoute.query = { boardId: 'BOARD-B' } + expect(rp.queueScopeLoaded.value).toBe(true) + }) + + it('stays unsettled after a failed entry load and settles on the next successful read', async () => { + const rp = useReviewProposals() + mockAutomationApi.getProposals.mockRejectedValueOnce(new Error('network down')) + await rp.loadProposals() + + // The read is over, so the loading term is false -- and a gate built on + // it would announce "0 proposals awaiting review." for a queue nobody has + // read. + expect(rp.proposalsLoading.value).toBe(false) + expect(rp.queueScopeLoaded.value).toBe(false) + + mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'p-a' })]) + await rp.loadProposals() + expect(rp.queueScopeLoaded.value).toBe(true) + }) + + it('settles from a background poll, so a failed entry load recovers without an explicit reload', async () => { + const rp = useReviewProposals() + mockAutomationApi.getProposals.mockRejectedValueOnce(new Error('network down')) + await rp.loadProposals() + expect(rp.queueScopeLoaded.value).toBe(false) + + mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'p-a' })]) + await rp.refreshProposals() + expect(rp.queueScopeLoaded.value).toBe(true) + }) + + it('unsettles when a refusal clears the queue and settles again when access returns', async () => { + const rp = useReviewProposals() + mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'p-a' })]) + await rp.loadProposals() + expect(rp.queueScopeLoaded.value).toBe(true) + + // `recordQueueAccessRevoked` empties the queue: what is rendered is no + // longer any read's answer. + mockAutomationApi.getProposals.mockRejectedValueOnce({ response: { status: 403 } }) + await rp.loadProposals() + expect(rp.queueAccessRevoked.value).toBe(true) + expect(rp.queueScopeLoaded.value).toBe(false) + + mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'p-a' })]) + await rp.loadProposals() + expect(rp.queueAccessRevoked.value).toBe(false) + expect(rp.queueScopeLoaded.value).toBe(true) + }) + }) + describe('navigation helpers', () => { it('openInbox pushes inbox path with board filter', () => { mockRoute.query = { boardId: 'board-x' } diff --git a/frontend/taskdeck-web/src/tests/views/ReviewView.spec.ts b/frontend/taskdeck-web/src/tests/views/ReviewView.spec.ts index d7ea0203f..94d363102 100644 --- a/frontend/taskdeck-web/src/tests/views/ReviewView.spec.ts +++ b/frontend/taskdeck-web/src/tests/views/ReviewView.spec.ts @@ -1947,6 +1947,135 @@ describe('ReviewView', () => { } }) + it('keeps the same announcement node across an explicit reload that did not change the queue (#2599 item 1)', async () => { + // The gate was `!proposalsLoading`, and an explicit `loadProposals` raises + // that flag WITHOUT clearing `proposals`: the node unmounted and remounted + // with the same sentence, so the live region wrote count -> '' -> count and + // the restore was spoken for a queue that had not moved. Reached by the + // header Refresh below, and by filing away a settled proposal. + mocks.getProposals.mockResolvedValue([buildProposal({ id: 'proposal-first' })]) + const { wrapper } = await mountAt('/workspace/review') + + const region = wrapper.get('[data-testid="review-queue-live"]').element + const before = wrapper.get('[data-testid="review-queue-announcement"]') + expect(before.text()).toContain('1 proposal awaiting review') + const beforeEl = before.element + + const pending = createDeferred() + mocks.getProposals.mockReturnValue(pending.promise) + const refresh = wrapper.findAll('button').find((node) => node.text() === 'Refresh Review')! + await refresh.trigger('click') + await wrapper.vm.$nextTick() + + // Mid-read: the skeleton is up, and the count beside it is still the last + // landed read's count of this same board. + expect(wrapper.find('.td-review__skeleton').exists()).toBe(true) + expect(wrapper.get('[data-testid="review-queue-announcement"]').element).toBe(beforeEl) + + pending.resolve([buildProposal({ id: 'proposal-first' })]) + await flushPromises() + await wrapper.vm.$nextTick() + + // Byte-identical answer: the same node, so nothing was added to the region + // and nothing is spoken. A CHANGED reload still re-keys (the item-4 test + // above owns that half). + expect(wrapper.get('[data-testid="review-queue-announcement"]').element).toBe(beforeEl) + expect(wrapper.get('[data-testid="review-queue-live"]').element).toBe(region) + }) + + it('re-keys the announcement once when an explicit reload did change the queue (#2599 item 1)', async () => { + // The other half of the same gate: silence for a reload that changed + // nothing must not become silence for one that did. #2710's key still + // carries it, and now it carries it without the node being unmounted and + // rebuilt around the reload. + mocks.getProposals.mockResolvedValue([buildProposal({ id: 'proposal-first' })]) + const { wrapper } = await mountAt('/workspace/review') + + const region = wrapper.get('[data-testid="review-queue-live"]').element + const beforeEl = wrapper.get('[data-testid="review-queue-announcement"]').element + + // Same count, different proposal: the sentence is byte-identical, so only + // the key can carry the change. + mocks.getProposals.mockResolvedValue([buildProposal({ id: 'proposal-second' })]) + const refresh = wrapper.findAll('button').find((node) => node.text() === 'Refresh Review')! + await refresh.trigger('click') + await flushPromises() + await wrapper.vm.$nextTick() + + const after = wrapper.get('[data-testid="review-queue-announcement"]') + expect(after.text()).toContain('1 proposal awaiting review') + expect(after.element).not.toBe(beforeEl) + expect(wrapper.get('[data-testid="review-queue-live"]').element).toBe(region) + }) + + it('withholds the announcement across a board-filter change until the new scope lands (#2599 item 1)', async () => { + // The one reload where the rendered count genuinely stops being real: the + // queue on screen belongs to the previous board until the new scope's read + // replaces it. + mocks.getProposals.mockResolvedValue([ + buildProposal({ id: 'proposal-a', boardId: 'board-a' }), + ]) + const { wrapper, router } = await mountAt('/workspace/review?boardId=board-a') + expect(wrapper.get('[data-testid="review-queue-announcement"]').text()).toContain( + '1 proposal awaiting review', + ) + + const pending = createDeferred() + mocks.getProposals.mockReturnValue(pending.promise) + await router.push('/workspace/review?boardId=board-b') + await flushPromises() + await wrapper.vm.$nextTick() + + expect(wrapper.find('[data-testid="review-queue-announcement"]').exists()).toBe(false) + expect(wrapper.get('[data-testid="review-queue-live"]').text()).toBe('') + + pending.resolve([ + buildProposal({ id: 'proposal-b1', boardId: 'board-b' }), + buildProposal({ id: 'proposal-b2', boardId: 'board-b' }), + ]) + await flushPromises() + await wrapper.vm.$nextTick() + + expect(wrapper.get('[data-testid="review-queue-announcement"]').text()).toContain( + '2 proposals awaiting review', + ) + }) + + it('moves focus to the queue after leaving an unavailable pin (#2599 item 2)', async () => { + // The panel the return control lives in is removed by the click, so focus + // fell to : no announcement, and the next keystroke acts on nothing. + mocks.getProposals.mockResolvedValue([buildProposal({ id: 'proposal-live' })]) + mocks.getProposal.mockRejectedValue({ response: { status: 404 } }) + + const { wrapper } = await mountAt('/workspace/review#proposal-proposal-gone') + const back = wrapper.get('[data-testid="review-unavailable-return"]') + expect(wrapper.find('.td-review__list').exists()).toBe(false) + + await back.trigger('click') + await flushPromises() + await wrapper.vm.$nextTick() + + // The queue list is this skin's focusable queue: it carries the + // "Proposals awaiting review" label and the Arrow cursor that starts on the + // first row. + const list = wrapper.get('.td-review__list').element + expect(document.activeElement).toBe(list) + }) + + it('moves focus to the empty state after leaving an unavailable pin with nothing left to review (#2599 item 2)', async () => { + mocks.getProposals.mockResolvedValue([]) + mocks.getProposal.mockRejectedValue({ response: { status: 404 } }) + + const { wrapper } = await mountAt('/workspace/review#proposal-proposal-gone') + await wrapper.get('[data-testid="review-unavailable-return"]').trigger('click') + await flushPromises() + await wrapper.vm.$nextTick() + + const empty = wrapper.get('.td-review-empty').element + expect(empty.getAttribute('tabindex')).toBe('-1') + expect(document.activeElement).toBe(empty) + }) + it('renders the pinned proposal, not the unavailable panel, after moving from a dead pin to a live one (#2214)', async () => { // What this pins: navigating from a refused pin X to a resolvable pin Y // shows Y's card and no panel. diff --git a/frontend/taskdeck-web/src/tests/views/paper/review/PaperReviewView.spec.ts b/frontend/taskdeck-web/src/tests/views/paper/review/PaperReviewView.spec.ts index 13388d71f..52209af72 100644 --- a/frontend/taskdeck-web/src/tests/views/paper/review/PaperReviewView.spec.ts +++ b/frontend/taskdeck-web/src/tests/views/paper/review/PaperReviewView.spec.ts @@ -3847,6 +3847,157 @@ describe('PaperReviewView', () => { } }) + it('keeps the same announcement node across an explicit reload that did not change the queue (#2599 item 1)', async () => { + // The rail's gate was the parent's `loading` flag, which an explicit + // `loadProposals` raises without clearing the queue: the node unmounted and + // came back with the same sentence, and a node addition is exactly what a + // live region speaks. Filing away a settled proposal is the path #2599 item + // 1 names -- it reloads the queue and leaves the pending-review set + // identical, so the reviewer heard the same count read back for nothing. + const wrapper = await mountView([ + makeProposal({ id: 'pending-1', status: 'PendingReview', summary: 'Still pending' }), + makeProposal({ + id: 'settled-1', + status: 'Expired', + expiresAt: new Date(Date.now() - 60_000).toISOString(), + summary: 'Settled already', + }), + makeProposal({ + id: 'settled-2', + status: 'Expired', + expiresAt: new Date(Date.now() - 60_000).toISOString(), + summary: 'Settled too', + }), + ]) + try { + const region = wrapper.get('[data-testid="paper-review-queue-live"]').element + const before = wrapper.get('[data-testid="paper-review-queue-announcement"]') + expect(before.text()).toContain('1 proposal awaiting review') + const beforeEl = before.element + + let releaseReload!: (value: Proposal[]) => void + const reload = new Promise((resolve) => { + releaseReload = resolve + }) + // Fewer dismissed than asked for, which is the branch that re-reads the + // queue authoritatively instead of patching it locally -- an explicit + // `loadProposals` that leaves the pending-review set untouched. + mocks.dismissProposals.mockResolvedValueOnce({ dismissed: 1 }) + mocks.getProposals.mockReturnValue(reload) + + await wrapper.get('[data-testid="queue-file-away-all"]').trigger('click') + await flushPromises() + + // The reload really is in flight: without this the identity assertions + // below would pass on a surface that never reloaded at all. + expect(mocks.getProposals).toHaveBeenCalledTimes(2) + // Mid-read: the pending-review count on screen is still this board's. + expect(wrapper.get('[data-testid="paper-review-queue-announcement"]').element).toBe(beforeEl) + + releaseReload([ + makeProposal({ id: 'pending-1', status: 'PendingReview', summary: 'Still pending' }), + ]) + await flushPromises() + await wrapper.vm.$nextTick() + + expect(wrapper.get('[data-testid="paper-review-queue-announcement"]').element).toBe(beforeEl) + expect(wrapper.get('[data-testid="paper-review-queue-live"]').element).toBe(region) + } finally { + wrapper.unmount() + } + }) + + it('withholds the rail announcement across a board-filter change until the new scope lands (#2599 item 1)', async () => { + // The one reload where the count really does stop being real: the rail is + // still rendering the previous board's queue. + const wrapper = await mountView( + [makeProposal({ id: 'board-a-1', status: 'PendingReview', boardId: 'board-a' })], + '/workspace/review?boardId=board-a', + ) + try { + expect(wrapper.get('[data-testid="paper-review-queue-announcement"]').text()).toContain( + '1 proposal awaiting review', + ) + + let releaseScopeRead!: (value: Proposal[]) => void + mocks.getProposals.mockReturnValue( + new Promise((resolve) => { + releaseScopeRead = resolve + }), + ) + await routerOf(wrapper).replace('/workspace/review?boardId=board-b') + await flushPromises() + await wrapper.vm.$nextTick() + + expect(wrapper.find('[data-testid="paper-review-queue-announcement"]').exists()).toBe(false) + expect(wrapper.get('[data-testid="paper-review-queue-live"]').text()).toBe('') + + releaseScopeRead([ + makeProposal({ id: 'board-b-1', status: 'PendingReview', boardId: 'board-b' }), + makeProposal({ id: 'board-b-2', status: 'PendingReview', boardId: 'board-b' }), + ]) + await flushPromises() + await wrapper.vm.$nextTick() + + expect(wrapper.get('[data-testid="paper-review-queue-announcement"]').text()).toContain( + '2 proposals awaiting review', + ) + } finally { + wrapper.unmount() + } + }) + + it('moves focus to the first queue row after leaving an unavailable pin (#2599 item 2)', async () => { + // The unavailable panel is the whole decision column, so the click that + // dismisses it removes the element focus is in and focus falls to . + // Paper's settled-elsewhere notice already hands focus on (#2215); this is + // the same handoff one branch over, and the same target Legacy uses. + mocks.getProposal.mockRejectedValueOnce({ response: { status: 404 } }) + const wrapper = await mountView( + [makeProposal({ id: 'proposal-first', summary: 'First proposal' })], + '/workspace/review#proposal-PROPOSAL-MISSING', + [], + [], + { attachTo: true }, + ) + try { + expect(wrapper.get('[data-testid="paper-review-empty"]').text()).toContain( + 'This proposal is unavailable.', + ) + + await wrapper.get('[data-testid="paper-review-unavailable-return"]').trigger('click') + await flushPromises() + await wrapper.vm.$nextTick() + + const firstRow = wrapper.get('.paper-review-rail__queue-row button').element + expect(document.activeElement).toBe(firstRow) + } finally { + wrapper.unmount() + } + }) + + it('moves focus to the empty state after leaving an unavailable pin with nothing left to review (#2599 item 2)', async () => { + mocks.getProposal.mockRejectedValueOnce({ response: { status: 404 } }) + const wrapper = await mountView( + [], + '/workspace/review#proposal-PROPOSAL-MISSING', + [], + [], + { attachTo: true }, + ) + try { + await wrapper.get('[data-testid="paper-review-unavailable-return"]').trigger('click') + await flushPromises() + await wrapper.vm.$nextTick() + + const empty = wrapper.get('[data-testid="paper-review-empty"]').element + expect(empty.getAttribute('tabindex')).toBe('-1') + expect(document.activeElement).toBe(empty) + } finally { + wrapper.unmount() + } + }) + it('says the queue is no longer available when a poll is refused with 403 (#2194)', async () => { vi.useFakeTimers({ toFake: ['setInterval', 'clearInterval', 'Date'] }) try { diff --git a/frontend/taskdeck-web/src/tests/views/paper/review/ReviewQueueRail.spec.ts b/frontend/taskdeck-web/src/tests/views/paper/review/ReviewQueueRail.spec.ts index 7732b32b3..4527c5ecf 100644 --- a/frontend/taskdeck-web/src/tests/views/paper/review/ReviewQueueRail.spec.ts +++ b/frontend/taskdeck-web/src/tests/views/paper/review/ReviewQueueRail.spec.ts @@ -31,18 +31,28 @@ function mountRail(props?: Partial<{ cadence: number[] scopeLabel: string scopeClearLabel: string - loading: boolean + queueScopeLoaded: boolean queueUnavailable: boolean awaitingCount: number announcementKey: string + attachTo: boolean + /** + * The RETIRED pre-fix input (#2599 item 1). It is no longer a prop, so the + * rail receives it as an inert fallthrough attribute; only the regression pin + * below passes it, and only so that test fails on the pre-fix rail, where + * this was the announcement gate's first term. + */ + loading: boolean }>) { return mount(ReviewQueueRail, { + ...(props?.attachTo ? { attachTo: document.body } : {}), props: { items: props?.items ?? [makeItem()], activeId: props?.activeId ?? null, awaitingCount: props?.awaitingCount ?? 3, staleCount: 2, ...(props?.announcementKey !== undefined ? { announcementKey: props.announcementKey } : {}), + ...(props?.queueScopeLoaded !== undefined ? { queueScopeLoaded: props.queueScopeLoaded } : {}), ...(props?.loading !== undefined ? { loading: props.loading } : {}), ...(props?.queueUnavailable !== undefined ? { queueUnavailable: props.queueUnavailable } : {}), dismissableCount: props?.dismissableCount ?? 0, @@ -304,19 +314,20 @@ describe('ReviewQueueRail apply-approved action (#1307)', () => { }) describe('ReviewQueueRail queue announcement (#2214)', () => { - it('announces the awaiting count once the queue is loaded', () => { - const wrapper = mountRail({ awaitingCount: 3 }) + it('announces the awaiting count once a read has landed for the current scope', () => { + const wrapper = mountRail({ awaitingCount: 3, queueScopeLoaded: true }) const live = wrapper.get('[data-testid="paper-review-queue-live"]') expect(live.attributes('role')).toBe('status') expect(live.text()).toContain('3 proposals awaiting review') }) - it('announces nothing while the queue is still loading', () => { - // A loading rail carries awaitingCount 0 because nothing has been read yet, - // so an ungated region reads "0 proposals awaiting review." and then the - // real count. Only the content is withheld: the region stays mounted so a - // later change lands in a live region that was already present. - const wrapper = mountRail({ loading: true, awaitingCount: 0 }) + it('announces nothing before a read has landed for the current scope', () => { + // Before the first read the rail carries awaitingCount 0 because nothing + // has been read yet, so an ungated region reads "0 proposals awaiting + // review." and then the real count. Only the content is withheld: the + // region stays mounted so a later change lands in a live region that was + // already present. + const wrapper = mountRail({ queueScopeLoaded: false, awaitingCount: 0 }) const live = wrapper.get('[data-testid="paper-review-queue-live"]') expect(live.attributes('role')).toBe('status') expect(live.text()).toBe('') @@ -324,21 +335,81 @@ describe('ReviewQueueRail queue announcement (#2214)', () => { it('announces nothing once queue access is revoked', () => { // The revoked state clears the queue, so awaitingCount drops to 0 for a - // reason that is not "nothing is awaiting review". Same defect as loading, - // one branch over. - const wrapper = mountRail({ queueUnavailable: true, awaitingCount: 0, items: [] }) + // reason that is not "nothing is awaiting review". Its own gate, not the + // scope signal, is what must withhold it -- hence the landed read here. + const wrapper = mountRail({ + queueScopeLoaded: true, + queueUnavailable: true, + awaitingCount: 0, + items: [], + }) const live = wrapper.get('[data-testid="paper-review-queue-live"]') expect(live.attributes('role')).toBe('status') expect(live.text()).toBe('') }) - it('keeps announcing when no loading flag is supplied', () => { - // The prop is optional so the parent view needs no change to keep today's - // behaviour; an omitted flag must never silence the announcement. + it('withholds the announcement when no scope flag is supplied', () => { + // The prop is optional and defaults to withholding: a parent that does not + // say a read has landed cannot have its count spoken, because 0 from a + // never-read queue is the #2593 defect (#2599 item 1). const wrapper = mountRail({ awaitingCount: 2 }) - expect(wrapper.get('[data-testid="paper-review-queue-live"]').text()).toContain( - '2 proposals awaiting review', + expect(wrapper.get('[data-testid="paper-review-queue-live"]').text()).toBe('') + expect(wrapper.find('[data-testid="paper-review-queue-announcement"]').exists()).toBe(false) + }) + + it('keeps the announcement node across a reload of the same scope (#2599 item 1)', async () => { + // The rail's old gate was the parent's `loading` flag, so an explicit + // reload -- which raises it without clearing the queue -- unmounted this + // node and remounted it with the same sentence. A live region speaks a node + // addition, so the reviewer heard the same count read back for a queue that + // had not moved. + // + // A reload is invisible to the rail now, which is the fix and also why this + // case has nothing of its own to drive: re-setting identical props does not + // even re-render. So the pin is the retired input itself -- `loading: true` + // is what the parent sent mid-reload before, and it withheld the + // announcement. Here it is an inert fallthrough attribute and only the + // scope signal decides, which is why this test is RED on the pre-fix rail + // and green here. The end-to-end evidence that a real reload stays silent + // is at view level: ReviewView.spec's and PaperReviewView.spec's + // "keeps the same announcement node across an explicit reload" cases. + const wrapper = mountRail({ + awaitingCount: 2, + queueScopeLoaded: true, + announcementKey: 'p-a\np-b', + loading: true, + }) + const announced = wrapper.get('[data-testid="paper-review-queue-announcement"]') + expect(announced.text()).toContain('2 proposals awaiting review') + const before = announced.element + + // A genuine re-render for an unrelated reason must not rebuild the keyed + // node either: the identity key is what re-announces, never the render. + await wrapper.setProps({ staleCount: 3, busy: true }) + + expect(wrapper.get('[data-testid="paper-review-queue-announcement"]').element).toBe(before) + }) + + it('withholds the announcement when the scope changes under it, without remounting the region', async () => { + // A board-filter change is the one reload where the rendered count really + // does stop being a count of what is on screen, so it is withheld until the + // new scope's read lands. + const wrapper = mountRail({ + awaitingCount: 2, + queueScopeLoaded: true, + announcementKey: 'p-a\np-b', + }) + const region = wrapper.get('[data-testid="paper-review-queue-live"]').element + + await wrapper.setProps({ queueScopeLoaded: false }) + expect(wrapper.find('[data-testid="paper-review-queue-announcement"]').exists()).toBe(false) + expect(wrapper.get('[data-testid="paper-review-queue-live"]').text()).toBe('') + + await wrapper.setProps({ queueScopeLoaded: true, awaitingCount: 1, announcementKey: 'p-c' }) + expect(wrapper.get('[data-testid="paper-review-queue-announcement"]').text()).toContain( + '1 proposal awaiting review', ) + expect(wrapper.get('[data-testid="paper-review-queue-live"]').element).toBe(region) }) it('replaces the announcement node when the queue identity changes under an unchanged count (#2214 item 4)', async () => { @@ -346,7 +417,11 @@ describe('ReviewQueueRail queue announcement (#2214)', () => { // not the awaiting set the count is about, and a rail-local derivation is // exactly how the two skins drift (#1124 / ADR-0038). The key comes from the // shared composable so Legacy and Paper re-announce on the same evidence. - const wrapper = mountRail({ awaitingCount: 2, announcementKey: 'p-a\np-b' }) + const wrapper = mountRail({ + awaitingCount: 2, + queueScopeLoaded: true, + announcementKey: 'p-a\np-b', + }) const region = wrapper.get('[data-testid="paper-review-queue-live"]').element const announced = wrapper.get('[data-testid="paper-review-queue-announcement"]') expect(announced.text()).toContain('2 proposals awaiting review') @@ -369,10 +444,45 @@ describe('ReviewQueueRail queue announcement (#2214)', () => { // The identity moves for a reason that is not "the awaiting queue changed" // when the queue is withdrawn: `recordQueueAccessRevoked` empties it. The // #2593 gate still wins over the re-announcement. - const wrapper = mountRail({ awaitingCount: 2, announcementKey: 'p-a\np-b' }) + const wrapper = mountRail({ + awaitingCount: 2, + queueScopeLoaded: true, + announcementKey: 'p-a\np-b', + }) await wrapper.setProps({ queueUnavailable: true, awaitingCount: 0, announcementKey: '' }) const live = wrapper.get('[data-testid="paper-review-queue-live"]') expect(live.text()).toBe('') expect(wrapper.find('[data-testid="paper-review-queue-announcement"]').exists()).toBe(false) }) }) + +describe('ReviewQueueRail focus handoff (#2599 item 2)', () => { + it('focuses the first queue row on request and reports that it did', () => { + // The queue's own rows are the target the unavailable-pin panel hands focus + // to. The rail owns them, so it owns the handoff: a parent reaching into + // this subtree would be the drift seam (#1124 / ADR-0038). + const wrapper = mountRail({ + attachTo: true, + items: [makeItem({ id: 'p-1', serial: '#0001' }), makeItem({ id: 'p-2', serial: '#0002' })], + }) + try { + expect(wrapper.vm.focusFirstQueueRow()).toBe(true) + const first = wrapper.get('.paper-review-rail__queue-row button').element + expect(document.activeElement).toBe(first) + expect(first.getAttribute('data-serial')).toBe('#0001') + } finally { + wrapper.unmount() + } + }) + + it('reports that it did not when the queue has no rows', () => { + // The caller needs the honest answer: an empty queue has no row to hold + // focus, and the panel's own empty state must take it instead. + const wrapper = mountRail({ attachTo: true, items: [] }) + try { + expect(wrapper.vm.focusFirstQueueRow()).toBe(false) + } finally { + wrapper.unmount() + } + }) +}) diff --git a/frontend/taskdeck-web/src/views/LegacyReviewView.vue b/frontend/taskdeck-web/src/views/LegacyReviewView.vue index 916c8c210..1f3255a49 100644 --- a/frontend/taskdeck-web/src/views/LegacyReviewView.vue +++ b/frontend/taskdeck-web/src/views/LegacyReviewView.vue @@ -1,5 +1,6 @@