From 5957c46e9e652c068ff9698626bb3a6d4edbfa21 Mon Sep 17 00:00:00 2001 From: Chris0Jeky Date: Sat, 5 Sep 2026 22:46:53 +0100 Subject: [PATCH 1/3] feat(review): expose a landed-read queue scope signal `!proposalsLoading` was the wrong question for both skins' announcement gates. An explicit `loadProposals` raises that flag WITHOUT clearing `proposals`, so the gate withheld the live region's content for the length of every reload and restored it afterwards: the region wrote count -> '' -> count, and a node addition inside a live region is spoken. The reviewer heard the same figure read back after the header Refresh and after filing a settled proposal away, about a queue that had not moved. `queueScopeLoaded` asks the question the gate actually needs: has a read landed for the board scope on screen? It settles at both landing sites -- the explicit load and the background poll, each stamped with the scope it asked about rather than whichever board is current when it lands -- and is false in exactly the three cases where the rendered count is not a count of the queue on screen: before the first read, after a board-filter change until the new scope's read lands, and after a failed read (including a refusal, which clears the queue outright). Scope comparison is lower-cased: the scope is the board, not the casing the query string carried, which is how `matchesActiveBoardFilter` already reads it. The signal only lands here in this commit; the two skins still gate on loading. Refs #2599, #2214 --- .../src/composables/useReviewProposals.ts | 67 ++++++++++ .../composables/useReviewProposals.spec.ts | 118 ++++++++++++++++++ 2 files changed, 185 insertions(+) diff --git a/frontend/taskdeck-web/src/composables/useReviewProposals.ts b/frontend/taskdeck-web/src/composables/useReviewProposals.ts index 644f6e0eb..c0d7fc08b 100644 --- a/frontend/taskdeck-web/src/composables/useReviewProposals.ts +++ b/frontend/taskdeck-web/src/composables/useReviewProposals.ts @@ -617,6 +617,54 @@ 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 three 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; + * - after a failed read that never landed, including the entry load. + * + * 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 +856,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 +873,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 +1023,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 +1378,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 +1660,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' } From e618311b93deb565b96d0f3ac15888e9cf9549be Mon Sep 17 00:00:00 2001 From: Chris0Jeky Date: Sat, 5 Sep 2026 22:47:10 +0100 Subject: [PATCH 2/3] fix(review): stop the reload repeat and hand focus back from a dead pin Two residuals from PR #2593's review, fixed in both skins together so the Review queue cannot drift (#1124 / ADR-0038). Item 1, no repeat announcement on a reload that changed nothing. Both gates now read `queueScopeLoaded` instead of the loading term: `LegacyReviewView.countIsAnnounceable` directly, and `ReviewQueueRail.countIsAnnounceable` through a new `queueScopeLoaded` prop that `PaperReviewView` feeds from the same composable value. A byte-identical same-scope reload keeps the SAME announcement node, so nothing is added to the live region and nothing is spoken; a reload that changed the queue still re-keys once, as #2710 shipped it; the first read, a board-filter change and a failed read still withhold, and the revoked gate is untouched beside it. The rail's `loading` prop is removed rather than left in place: it was that gate's only consumer, and a prop documenting a gate it no longer takes part in is how the next reader is misled. The replacement is optional and defaults to WITHHOLDING -- a parent that cannot say a read has landed cannot have its count spoken, because 0 from a never-read queue is the defect #2593 removed. `PaperReviewView` is its only mount. Item 2, focus after the unavailable-pin return control. Activating it removed the element focus was in and moved focus nowhere, so it fell to ``: nothing announced, and the next keystroke acting on nothing. Both skins now move focus after `nextTick`, following the `settledElsewhereReturnRef` pattern Paper already uses one branch over. The target is the queue the panel was standing in front of, at its first row -- Paper's first rail row button (the rail owns its rows, so it owns the handoff and exposes `focusFirstQueueRow`), and Legacy's queue list section, which carries the "Proposals awaiting review" label and the Arrow cursor that starts on row one. When the queue behind the panel is empty, focus goes to the empty state that replaces it instead (`tabindex="-1"`, programmatic only -- neither element enters the tab order). Refs #2599, #2214 --- .../src/tests/views/ReviewView.spec.ts | 129 +++++++++++++++ .../paper/review/PaperReviewView.spec.ts | 151 ++++++++++++++++++ .../paper/review/ReviewQueueRail.spec.ts | 127 ++++++++++++--- .../src/views/LegacyReviewView.vue | 60 ++++++- .../src/views/paper/PaperReviewView.vue | 47 +++++- .../views/paper/review/ReviewQueueRail.vue | 71 ++++++-- 6 files changed, 539 insertions(+), 46 deletions(-) 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..4e0583efb 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,19 +31,21 @@ function mountRail(props?: Partial<{ cadence: number[] scopeLabel: string scopeClearLabel: string - loading: boolean + queueScopeLoaded: boolean queueUnavailable: boolean awaitingCount: number announcementKey: string + attachTo: 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?.loading !== undefined ? { loading: props.loading } : {}), + ...(props?.queueScopeLoaded !== undefined ? { queueScopeLoaded: props.queueScopeLoaded } : {}), ...(props?.queueUnavailable !== undefined ? { queueUnavailable: props.queueUnavailable } : {}), dismissableCount: props?.dismissableCount ?? 0, busy: props?.busy ?? false, @@ -304,19 +306,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 +327,66 @@ 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. Nothing about a same-scope reload reaches the rail now. + const wrapper = mountRail({ + awaitingCount: 2, + queueScopeLoaded: true, + announcementKey: 'p-a\np-b', + }) + const before = wrapper.get('[data-testid="paper-review-queue-announcement"]').element + + await wrapper.setProps({ queueScopeLoaded: true, announcementKey: 'p-a\np-b' }) + + 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 +394,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 +421,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..9ccc919a9 100644 --- a/frontend/taskdeck-web/src/views/LegacyReviewView.vue +++ b/frontend/taskdeck-web/src/views/LegacyReviewView.vue @@ -1,5 +1,6 @@