diff --git a/frontend/taskdeck-web/src/composables/useReviewProposals.ts b/frontend/taskdeck-web/src/composables/useReviewProposals.ts index ee82e86c7..644f6e0eb 100644 --- a/frontend/taskdeck-web/src/composables/useReviewProposals.ts +++ b/frontend/taskdeck-web/src/composables/useReviewProposals.ts @@ -570,6 +570,53 @@ export function useReviewProposals() { ] }) + /** + * The ORDERED ids of the proposals both skins count as "awaiting review" + * (#2214 item 4). + * + * One source for the count AND for its identity (#1124 / ADR-0038): Legacy + * read the number off `summaryCards`' `pending-review` card and Paper + * recomputed this same predicate inline, and neither skin had any notion of + * WHICH proposals the number stood for. + * + * That is the whole defect. Both queue live regions rendered a sentence + * derived from the count alone, and a live region only speaks when its text + * changes — so a poll that removed one pending proposal and added another in + * the same response produced a byte-identical "3 proposals awaiting review.", + * mutated nothing, and announced nothing. The queue moved under the reviewer + * in silence, which is the false-negative class #2194 exists to remove. + * + * Ordered, not a set: the rail renders the queue in this order, so a reorder + * is a queue that moved. A byte-identical answer is not. + */ + const awaitingProposalIds = computed(() => + visibleProposals.value + .filter( + (proposal) => + normalizeProposalStatus(proposal.status) === 'PendingReview' && + !isProposalExpired(proposal), + ) + .map((proposal) => proposal.id), + ) + + /** + * The identity above as one primitive, for use as the `key` of the node each + * skin's queue live region wraps around its sentence. + * + * A KEY rather than part of the spoken text, deliberately. Re-keying replaces + * that node inside a live region that itself stays mounted, and a node + * addition is exactly what `aria-live`'s default + * `aria-relevant="additions text"` announces — so the same count-neutral + * replacement is spoken once, with the sentence and its count unchanged from + * what #2194 shipped. The alternative, blanking the text for a frame and + * restoring it, recreates the "inserted together with its text" shape that + * #2593 and #2630 both call unreliably announced. + * + * `\n` cannot appear in a proposal id, so distinct queues cannot collide on + * one key. + */ + const queueAnnouncementKey = computed(() => awaitingProposalIds.value.join('\n')) + function isProposalDismissable(proposal: ApiProposal): boolean { const status = normalizeProposalStatus(proposal.status) return ( @@ -802,11 +849,24 @@ export function useReviewProposals() { // removed" about a proposal that was none of those things -- the board // simply was not this reviewer's any more. // - // The toast stays: this read is one the caller asked for, and every - // action composable that calls `loadProposals` still gets its failure - // signal and its 'failed' outcome unchanged. - if (isForbiddenError(e)) recordQueueAccessRevoked() - toast.error(getErrorDisplay(e, t('review.toast.loadProposalsFailed')).message) + // + // ONE report for one fact (#2214, from PR #2694's round-2 verification). + // `recordQueueAccessRevoked` raises a DURABLE panel that is the first + // branch of both skins' empty chains and names the revocation and its + // remedy; the generic "Failed to load proposals" toast beside it named + // neither, was gone seconds later, and was contradicted by a panel that + // stayed — the same two-reports-for-one-fact shape #2694 removed on the + // pin leg. Every OTHER explicit failure keeps its toast: this read is one + // the caller asked for, and nothing else on screen reports it. + // + // The outcome contract is untouched either way: every action composable + // that calls `loadProposals` still gets its failure signal and its + // 'failed' outcome. + if (isForbiddenError(e)) { + recordQueueAccessRevoked() + } else { + toast.error(getErrorDisplay(e, t('review.toast.loadProposalsFailed')).message) + } outcome = 'failed' } finally { if (requestId === latestProposalLoadRequestId) proposalsLoading.value = false @@ -1485,7 +1545,25 @@ export function useReviewProposals() { watch( () => route.hash, - () => { openProposalFromHash().catch(() => {}) }, + () => { + // A revoked queue has ONE owner and ONE explanation — the same rule the + // explicit load already applies at its own `openProposalFromHash` call + // site. A `#proposal-` link followed while the revoked panel is up (a + // stale rail row, a bookmark, the back button) can only ask the by-id + // route about a target inside a board the server has refused wholesale, + // and its answer can only write a second, narrower and wrong account of + // that refusal into `unavailableProposalId`. + // + // Invisible today because the revoked panel is the first branch of both + // skins' empty chains, which makes it a LATENT contradiction rather than + // a visible one: the state is written, and the next surface to consume it + // reads a lifecycle claim ("applied, archived, or removed") about a + // proposal that was none of those things. A successful load clears + // `queueAccessRevoked` and re-runs the hash lookup itself, so nothing is + // lost by not asking here. + if (queueAccessRevoked.value) return + openProposalFromHash().catch(() => {}) + }, ) watch( @@ -1514,6 +1592,8 @@ export function useReviewProposals() { nowMs, visibleProposals, summaryCards, + awaitingProposalIds, + queueAnnouncementKey, 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 b2b152092..ec18d2598 100644 --- a/frontend/taskdeck-web/src/tests/composables/useReviewProposals.spec.ts +++ b/frontend/taskdeck-web/src/tests/composables/useReviewProposals.spec.ts @@ -1091,6 +1091,122 @@ describe('useReviewProposals', () => { expect(rp.queueAccessRevoked.value).toBe(false) expect(rp.unavailableProposalId.value).toBe('p-forbidden') }) + + it('reports the revocation once, without the generic load-failure toast', async () => { + // Two reports for one fact -- the shape #2694 removed on the pin leg and + // left standing here. `recordQueueAccessRevoked` raises a DURABLE panel + // that is the first branch of both skins' empty chains and names both the + // fact and the remedy; the generic "Failed to load proposals" toast beside + // it names neither and is gone seconds later, contradicted by a panel that + // stays. + mockAutomationApi.getProposals.mockRejectedValueOnce({ response: { status: 403 } }) + const rp = useReviewProposals() + + // The OUTCOME contract is untouched: every action composable that calls + // `loadProposals` still gets its failure signal. + await expect(rp.loadProposalsWithOutcome()).resolves.toBe('failed') + + expect(rp.queueAccessRevoked.value).toBe(true) + expect(mockToast.error).not.toHaveBeenCalled() + }) + + it('does not let a later hash change mark a pin unavailable under the revoked panel', async () => { + // The route-hash watcher had no `queueAccessRevoked` guard, while the + // explicit load's own `openProposalFromHash` call site has had one since + // #2694. So a `#proposal-` link followed while the revoked panel is up -- + // a stale rail row, a bookmark, the back button -- still asked the by-id + // route about a target inside a board the server had refused wholesale, + // and wrote its refusal into `unavailableProposalId` as a second, + // narrower and wrong account of the same fact. + mockAutomationApi.getProposals.mockRejectedValueOnce({ response: { status: 403 } }) + const rp = useReviewProposals() + await rp.loadProposals() + expect(rp.queueAccessRevoked.value).toBe(true) + + mockRoute.hash = '#proposal-p-inside-revoked' + mockAutomationApi.getProposal.mockRejectedValueOnce({ response: { status: 403 } }) + await watcherForCurrentSourceValue('#proposal-p-inside-revoked')[1]() + + expect(mockAutomationApi.getProposal).not.toHaveBeenCalled() + expect(rp.unavailableProposalId.value).toBeNull() + expect(rp.unavailableProposalMalformed.value).toBe(false) + }) + }) + + // #2214 item 4. Both skins derived the queue live region's sentence from the + // pending COUNT alone, so a poll that removed one pending proposal and added + // another rendered a byte-identical "3 proposals awaiting review.": no DOM + // mutation, nothing announced, the queue moved under the reviewer in silence. + // The ordered awaiting ids are the identity the announcement is keyed on. + describe('queue announcement identity (#2214 item 4)', () => { + it('changes on a count-neutral replacement and not on a byte-identical queue', () => { + const rp = useReviewProposals() + rp.proposals.value = [ + makeProposal({ id: 'p-a', createdAt: '2026-01-02T00:00:00Z' }), + makeProposal({ id: 'p-b', createdAt: '2026-01-01T00:00:00Z' }), + ] as any + expect(rp.awaitingProposalIds.value).toEqual(['p-a', 'p-b']) + const identity = rp.queueAnnouncementKey.value + + // A poll answering with the same queue in a new array is not news. + rp.proposals.value = [ + makeProposal({ id: 'p-a', createdAt: '2026-01-02T00:00:00Z' }), + makeProposal({ id: 'p-b', createdAt: '2026-01-01T00:00:00Z' }), + ] as any + expect(rp.queueAnnouncementKey.value).toBe(identity) + + // One pending proposal decided elsewhere, one created in its place: the + // count is unchanged, so the SENTENCE is byte-identical and only the + // identity can carry the change. + rp.proposals.value = [ + makeProposal({ id: 'p-a', createdAt: '2026-01-02T00:00:00Z' }), + makeProposal({ id: 'p-c', createdAt: '2026-01-01T00:00:00Z' }), + ] as any + expect(rp.awaitingProposalIds.value.length).toBe(2) + expect(rp.queueAnnouncementKey.value).not.toBe(identity) + + // Order is part of the identity: the rail renders the queue in order, so + // a reordered queue is a queue that moved. + const swapped = rp.queueAnnouncementKey.value + rp.proposals.value = [ + makeProposal({ id: 'p-c', createdAt: '2026-01-02T00:00:00Z' }), + makeProposal({ id: 'p-a', createdAt: '2026-01-01T00:00:00Z' }), + ] as any + expect(rp.queueAnnouncementKey.value).not.toBe(swapped) + }) + + it('tracks exactly the proposals the awaiting count is made of', () => { + // The count and its identity must come from ONE predicate or they drift + // (#1124 / ADR-0038): announcing on a change the number cannot show would + // speak the same sentence for no visible reason. + const rp = useReviewProposals() + rp.showCompleted.value = true + rp.proposals.value = [ + makeProposal({ id: 'p-pending', createdAt: '2026-01-03T00:00:00Z' }), + makeProposal({ id: 'p-applied', status: 'Applied', createdAt: '2026-01-02T00:00:00Z' }), + makeProposal({ + id: 'p-expired', + createdAt: '2026-01-01T00:00:00Z', + expiresAt: '2026-01-01T00:00:01Z', + }), + ] as any + rp.nowMs.value = new Date('2026-02-01T00:00:00Z').getTime() + + expect(rp.awaitingProposalIds.value).toEqual(['p-pending']) + const identity = rp.queueAnnouncementKey.value + + // A settled row changing does not move the awaiting queue. + rp.proposals.value = [ + makeProposal({ id: 'p-pending', createdAt: '2026-01-03T00:00:00Z' }), + makeProposal({ id: 'p-rejected', status: 'Rejected', createdAt: '2026-01-02T00:00:00Z' }), + makeProposal({ + id: 'p-expired', + createdAt: '2026-01-01T00:00:00Z', + expiresAt: '2026-01-01T00:00:01Z', + }), + ] as any + expect(rp.queueAnnouncementKey.value).toBe(identity) + }) }) describe('navigation helpers', () => { diff --git a/frontend/taskdeck-web/src/tests/views/ReviewView.spec.ts b/frontend/taskdeck-web/src/tests/views/ReviewView.spec.ts index fa2d087a2..d7ea0203f 100644 --- a/frontend/taskdeck-web/src/tests/views/ReviewView.spec.ts +++ b/frontend/taskdeck-web/src/tests/views/ReviewView.spec.ts @@ -1900,6 +1900,53 @@ describe('ReviewView', () => { } }) + it('re-announces the awaiting count when a poll swaps the queue without changing its size (#2214 item 4)', async () => { + // The defect: the sentence was a pure function of the pending count, so a + // poll that removed one pending proposal and added another rendered a + // byte-identical "1 proposal awaiting review." -- no DOM mutation inside the + // live region, nothing announced, the queue moved in silence. + // + // The fix keeps the sentence and its count exactly as they shipped and + // re-keys the node that carries them on the queue's ordered awaiting ids. + // Replacing that node inside a region that itself stays mounted is a node + // ADDITION, which is what `aria-live`'s default + // `aria-relevant="additions text"` announces. + vi.useFakeTimers({ toFake: ['setInterval', 'clearInterval', 'setTimeout', 'clearTimeout', 'Date'] }) + try { + mocks.getProposals.mockResolvedValue([buildProposal({ id: 'proposal-first' })]) + const { wrapper } = await mountAt('/workspace/review') + + const region = wrapper.get('[data-testid="review-queue-live"]').element + const announced = wrapper.get('[data-testid="review-queue-announcement"]') + expect(announced.text()).toContain('1 proposal awaiting review') + const beforeSwap = announced.element + + // A poll answering with the same queue is not news, and must not put a + // repeat of the same figure in a reviewer's ear every 15 seconds. + vi.advanceTimersByTime(REVIEW_QUEUE_REFRESH_MS) + await flushPromises() + await wrapper.vm.$nextTick() + expect(wrapper.get('[data-testid="review-queue-announcement"]').element).toBe(beforeSwap) + + // One pending proposal decided elsewhere, one created in its place. + mocks.getProposals.mockResolvedValue([buildProposal({ id: 'proposal-second' })]) + vi.advanceTimersByTime(REVIEW_QUEUE_REFRESH_MS) + await flushPromises() + await wrapper.vm.$nextTick() + + const afterSwap = wrapper.get('[data-testid="review-queue-announcement"]') + expect(afterSwap.text()).toContain('1 proposal awaiting review') + expect(afterSwap.element).not.toBe(beforeSwap) + // The region itself is never remounted -- a live region inserted at the + // same moment its text appears is the unreliably-announced case #2593 and + // #2630 exist to avoid. + expect(wrapper.get('[data-testid="review-queue-live"]').element).toBe(region) + expect(wrapper.get('[data-testid="review-queue-live"]').attributes('aria-live')).toBe('polite') + } finally { + vi.useRealTimers() + } + }) + 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 6b63fc1a5..13388d71f 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 @@ -3809,6 +3809,44 @@ describe('PaperReviewView', () => { } }) + it('re-announces the rail count when a poll swaps the queue without changing its size (#2214 item 4)', async () => { + // The Paper half of the item-4 repair, wired end to end: the composable's + // ordered awaiting ids reach the rail as its announcement key, so the same + // count-neutral replacement that Legacy now announces is announced here too + // (#1124 / ADR-0038 -- a one-skin fix is the drift class). + vi.useFakeTimers({ toFake: ['setInterval', 'clearInterval', 'setTimeout', 'clearTimeout', 'Date'] }) + let wrapper: ReturnType | null = null + try { + wrapper = await mountView([makeProposal({ id: 'swap-first', status: 'PendingReview' })]) + + 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('1 proposal awaiting review') + const beforeSwap = announced.element + + // The same queue again: nothing to say. + vi.advanceTimersByTime(REVIEW_QUEUE_REFRESH_MS) + await flushPromises() + await wrapper.vm.$nextTick() + expect(wrapper.get('[data-testid="paper-review-queue-announcement"]').element).toBe(beforeSwap) + + mocks.getProposals.mockResolvedValue([ + makeProposal({ id: 'swap-second', status: 'PendingReview' }), + ]) + vi.advanceTimersByTime(REVIEW_QUEUE_REFRESH_MS) + await flushPromises() + await wrapper.vm.$nextTick() + + const afterSwap = wrapper.get('[data-testid="paper-review-queue-announcement"]') + expect(afterSwap.text()).toContain('1 proposal awaiting review') + expect(afterSwap.element).not.toBe(beforeSwap) + expect(wrapper.get('[data-testid="paper-review-queue-live"]').element).toBe(region) + } finally { + wrapper?.unmount() + vi.useRealTimers() + } + }) + 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 3ce690efc..7732b32b3 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 @@ -34,6 +34,7 @@ function mountRail(props?: Partial<{ loading: boolean queueUnavailable: boolean awaitingCount: number + announcementKey: string }>) { return mount(ReviewQueueRail, { props: { @@ -41,6 +42,7 @@ function mountRail(props?: Partial<{ activeId: props?.activeId ?? null, awaitingCount: props?.awaitingCount ?? 3, staleCount: 2, + ...(props?.announcementKey !== undefined ? { announcementKey: props.announcementKey } : {}), ...(props?.loading !== undefined ? { loading: props.loading } : {}), ...(props?.queueUnavailable !== undefined ? { queueUnavailable: props.queueUnavailable } : {}), dismissableCount: props?.dismissableCount ?? 0, @@ -338,4 +340,39 @@ describe('ReviewQueueRail queue announcement (#2214)', () => { '2 proposals awaiting review', ) }) + + it('replaces the announcement node when the queue identity changes under an unchanged count (#2214 item 4)', async () => { + // The rail cannot derive this itself: `items` is the whole visible queue, + // 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 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') + const before = announced.element + + // A byte-identical queue is not news. + await wrapper.setProps({ announcementKey: 'p-a\np-b' }) + expect(wrapper.get('[data-testid="paper-review-queue-announcement"]').element).toBe(before) + + // One awaiting proposal swapped for another: same count, same sentence. + await wrapper.setProps({ announcementKey: 'p-a\np-c' }) + const after = wrapper.get('[data-testid="paper-review-queue-announcement"]') + expect(after.text()).toContain('2 proposals awaiting review') + expect(after.element).not.toBe(before) + // The region is never remounted; only the node inside it is replaced. + expect(wrapper.get('[data-testid="paper-review-queue-live"]').element).toBe(region) + }) + + it('keeps the whole announcement withheld while the count is unspeakable, key or no key', async () => { + // 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' }) + 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) + }) }) diff --git a/frontend/taskdeck-web/src/views/LegacyReviewView.vue b/frontend/taskdeck-web/src/views/LegacyReviewView.vue index 6ee586fe6..916c8c210 100644 --- a/frontend/taskdeck-web/src/views/LegacyReviewView.vue +++ b/frontend/taskdeck-web/src/views/LegacyReviewView.vue @@ -27,6 +27,8 @@ const { boardOptions, visibleProposals, summaryCards, + awaitingProposalIds, + queueAnnouncementKey, queueAccessRevoked, queueRefreshStale, queueRefreshRefused, @@ -217,9 +219,13 @@ const workspace = useWorkspaceStore() // catalog rather than being forked per skin: the refused-deep-link panel reuses // `review.empty.unavailable.*`, and the background-refresh disclosures reuse // `review.queue.degraded.*` and `review.queue.refused.*` (#2214). -const awaitingCount = computed( - () => summaryCards.value.find((card) => card.id === 'pending-review')?.value ?? 0, -) +// The count and the identity the announcement is keyed on come from ONE +// composable predicate (#2214 item 4). Reading the number off `summaryCards`' +// `pending-review` card computed the same thing a second way and left the skin +// with no notion of WHICH proposals it stood for, which is how a count-neutral +// replacement stayed silent. The rendered value is unchanged: that card counts +// exactly `visibleProposals` in `PendingReview` and not expired. +const awaitingCount = computed(() => awaitingProposalIds.value.length) const awaitingAnnouncement = computed(() => awaitingCount.value === 1 ? '1 proposal awaiting review.' @@ -321,9 +327,22 @@ onUnmounted(() => { a CHANGE, so an ungated region speaks "0 proposals awaiting review." beside a panel saying the queue is gone. The region withholds its CONTENT rather than being unmounted, because a live region inserted at - the same moment its text appears is unreliably announced. --> + the same moment its text appears is unreliably announced. + + The sentence sits in a node KEYED on the queue's ordered awaiting ids + (#2214 item 4). A live region only speaks when it changes, so a poll + that removed one pending proposal and added another rendered the same + string and said nothing. Re-keying replaces this node inside the region + that stays mounted, and a node addition is what `aria-live`'s default + `aria-relevant="additions text"` announces — same sentence, same count, + spoken once. A byte-identical queue keeps its key and stays silent. + Paper keys the identical region on the same value (#1124 / ADR-0038). -->

- {{ countIsAnnounceable ? awaitingAnnouncement : '' }} + {{ awaitingAnnouncement }}

{{ countIsAnnounceable ? $t('review.queueRail.liveAnnounce', { count: awaitingCount }, awaitingCount) : '' }}

+ >{{ $t('review.queueRail.liveAnnounce', { count: awaitingCount }, awaitingCount) }}