diff --git a/frontend/taskdeck-web/src/tests/views/ReviewView.spec.ts b/frontend/taskdeck-web/src/tests/views/ReviewView.spec.ts index 6065a9f55..655e09d82 100644 --- a/frontend/taskdeck-web/src/tests/views/ReviewView.spec.ts +++ b/frontend/taskdeck-web/src/tests/views/ReviewView.spec.ts @@ -1441,4 +1441,156 @@ describe('ReviewView', () => { expect(invalid.text()).not.toContain('no operations') expect(mocks.errorToast).not.toHaveBeenCalled() }) + + // --- #2214: the hash-pinned target's own unavailable state ---------------- + + it('names a hash-pinned proposal that is unavailable instead of the generic empty queue (#2214)', async () => { + // The queue read succeeds and simply does not contain the pinned proposal; + // the proposal-level read then refuses it. That is an identity failure of + // the link the reviewer followed, not an empty queue. + mocks.getProposals.mockResolvedValue([]) + mocks.getProposal.mockRejectedValue({ response: { status: 404 } }) + + const { wrapper } = await mountAt('/workspace/review#proposal-proposal-gone') + + const unavailable = wrapper.find('[data-testid="review-unavailable-target"]') + expect(unavailable.exists()).toBe(true) + expect(unavailable.attributes('role')).toBe('status') + expect(unavailable.text()).toContain('This proposal is unavailable.') + expect(unavailable.text()).toContain('proposal-gone') + // The ordinary empty queue must not stand in for a refused deep link. + expect(wrapper.find('.td-review-empty').exists()).toBe(false) + }) + + it('returns to the unpinned queue from the unavailable state (#2214)', async () => { + mocks.getProposals.mockResolvedValue([]) + mocks.getProposal.mockRejectedValue({ response: { status: 404 } }) + + const { wrapper, router } = await mountAt('/workspace/review#proposal-proposal-gone') + + const back = wrapper.get('[data-testid="review-unavailable-return"]') + await back.trigger('click') + await flushPromises() + await wrapper.vm.$nextTick() + + expect(router.currentRoute.value.hash).toBe('') + expect(wrapper.find('[data-testid="review-unavailable-target"]').exists()).toBe(false) + }) + + it('recovers to the pinned proposal when the target resolves again (#2214)', async () => { + vi.useFakeTimers({ toFake: ['setInterval', 'clearInterval', 'Date'] }) + try { + mocks.getProposals.mockResolvedValue([]) + mocks.getProposal.mockRejectedValue({ response: { status: 404 } }) + + const { wrapper } = await mountAt('/workspace/review#proposal-proposal-flaky') + expect(wrapper.find('[data-testid="review-unavailable-target"]').exists()).toBe(true) + + // The target is readable again and the next background read carries it, + // so the queue read alone settles the pin. + const restored = buildProposal({ id: 'proposal-flaky' }) + mocks.getProposals.mockResolvedValue([restored]) + mocks.getProposal.mockResolvedValue(restored) + + vi.advanceTimersByTime(REVIEW_QUEUE_REFRESH_MS) + await flushPromises() + await wrapper.vm.$nextTick() + + expect(wrapper.find('[data-testid="review-unavailable-target"]').exists()).toBe(false) + expect(wrapper.find('#proposal-proposal-flaky').exists()).toBe(true) + } finally { + vi.useRealTimers() + } + }) + + it('announces nothing from the queue live region while the queue is loading (#2214)', async () => { + // The live region sits above the skeleton, so an ungated one reads "0 + // proposals awaiting review." under the loading state and then the real + // count — the first of which was never true. + const pending = createDeferred() + mocks.getProposals.mockReturnValue(pending.promise) + + const { wrapper } = await mountAt('/workspace/review') + + expect(wrapper.find('.td-review__skeleton').exists()).toBe(true) + const live = wrapper.get('[data-testid="review-queue-live"]') + // The region itself stays mounted so a later count change is announced in + // an already-present live region; only its content is withheld. + expect(live.attributes('role')).toBe('status') + expect(live.text()).toBe('') + + pending.resolve([buildProposal({ id: 'proposal-loaded' })]) + await flushPromises() + await wrapper.vm.$nextTick() + + expect(wrapper.get('[data-testid="review-queue-live"]').text()).toContain( + '1 proposal awaiting review', + ) + }) + + it('announces nothing from the queue live region once queue access is revoked (#2214)', async () => { + // A current-scope 403 sets queueAccessRevoked AND clears the queue, so the + // announcement changes from a real count to 0 — a change, therefore spoken — + // while the panel beside it says the queue is gone and has stopped updating. + vi.useFakeTimers({ toFake: ['setInterval', 'clearInterval', 'Date'] }) + try { + mocks.getProposals.mockResolvedValue([buildProposal({ id: 'proposal-visible' })]) + const { wrapper } = await mountAt('/workspace/review') + expect(wrapper.get('[data-testid="review-queue-live"]').text()).toContain( + '1 proposal awaiting review', + ) + + mocks.getProposals.mockRejectedValue({ response: { status: 403 } }) + vi.advanceTimersByTime(REVIEW_QUEUE_REFRESH_MS) + await flushPromises() + await wrapper.vm.$nextTick() + + expect(wrapper.find('[data-testid="review-access-revoked"]').exists()).toBe(true) + const live = wrapper.get('[data-testid="review-queue-live"]') + expect(live.attributes('role')).toBe('status') + expect(live.text()).toBe('') + } 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. + // + // What it does NOT pin, measured: the `renderedProposals.length === 0` half + // of the panel's own condition. Deleting that half leaves this test green. + // The window where the recorded id still names X while the hash names a + // renderable Y exists only while `proposalsLoading` is true, because + // `openProposalFromHash` early-returns there — and the loading branch + // precedes the panel in the same v-if chain, so the skeleton renders + // instead. When the read settles, `proposalsLoading = false` and the + // clearing of the recorded id happen in one synchronous block, so no + // intermediate state is ever rendered. The length half is therefore + // defence-in-depth against that ordering changing, not a live guard. + const y = buildProposal({ id: 'proposal-y' }) + mocks.getProposals.mockResolvedValue([y]) + mocks.getProposal.mockRejectedValue({ response: { status: 404 } }) + + const { wrapper, router } = await mountAt('/workspace/review#proposal-proposal-x') + expect(wrapper.find('[data-testid="review-unavailable-target"]').exists()).toBe(true) + + 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() + + await router.push('/workspace/review#proposal-proposal-y') + await flushPromises() + await wrapper.vm.$nextTick() + + pending.resolve([y]) + await flushPromises() + await wrapper.vm.$nextTick() + + // The panel must never stand in front of a proposal that renders. + expect(wrapper.find('#proposal-proposal-y').exists()).toBe(true) + expect(wrapper.find('[data-testid="review-unavailable-target"]').exists()).toBe(false) + }) }) 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 a7bcbe4bc..727b12011 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 @@ -3030,6 +3030,66 @@ describe('PaperReviewView', () => { wrapper.unmount() }) + it('speaks the awaiting count only while it is a real count (#2214)', async () => { + // The rail's count is 0 both before the first read lands and after a 403 + // clears the queue, and neither 0 means "nothing is awaiting review". The + // second is the worse of the two: it is a CHANGE from a real count, so an + // ungated region speaks it beside a panel saying the queue is gone. + vi.useFakeTimers({ toFake: ['setInterval', 'clearInterval', 'Date'] }) + let wrapper: ReturnType | null = null + try { + let resolveQueue!: (value: Proposal[]) => void + const pendingQueue = new Promise((resolve) => { + resolveQueue = resolve + }) + mocks.getProposals.mockReturnValue(pendingQueue) + mocks.getBoards.mockResolvedValue([]) + mocks.getColumns.mockResolvedValue([]) + + const router = createRouter({ + history: createMemoryHistory(), + routes: [ + { path: '/workspace/review', name: 'workspace-review', component: PaperReviewView }, + ], + }) + router.push('/workspace/review') + await router.isReady() + wrapper = mount(PaperReviewView, { global: { plugins: [router] } }) + await nextTick() + + // Still reading: the region is mounted (so a later change lands in a live + // region that was already there) and says nothing. + const loadingLive = wrapper.find('[data-testid="paper-review-queue-live"]') + expect(loadingLive.exists()).toBe(true) + expect(loadingLive.attributes('role')).toBe('status') + expect(loadingLive.text()).toBe('') + + const proposal = makeProposal({ id: 'announce-1', status: 'PendingReview' }) + mocks.getProposals.mockResolvedValue([proposal]) + resolveQueue([proposal]) + await flushPromises() + await wrapper.vm.$nextTick() + + expect(wrapper.find('[data-testid="paper-review-queue-live"]').text()).toContain( + '1 proposal awaiting review', + ) + + // Access is withdrawn: the queue is cleared, so the count would fall to 0. + mocks.getProposals.mockRejectedValue({ response: { status: 403 } }) + vi.advanceTimersByTime(REVIEW_QUEUE_REFRESH_MS) + await flushPromises() + await wrapper.vm.$nextTick() + + expect(wrapper.find('[data-testid="paper-review-access-revoked"]').exists()).toBe(true) + const revokedLive = wrapper.find('[data-testid="paper-review-queue-live"]') + expect(revokedLive.attributes('role')).toBe('status') + expect(revokedLive.text()).toBe('') + } 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 3f0d1a9e0..3ce690efc 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,13 +31,18 @@ function mountRail(props?: Partial<{ cadence: number[] scopeLabel: string scopeClearLabel: string + loading: boolean + queueUnavailable: boolean + awaitingCount: number }>) { return mount(ReviewQueueRail, { props: { items: props?.items ?? [makeItem()], activeId: props?.activeId ?? null, - awaitingCount: 3, + awaitingCount: props?.awaitingCount ?? 3, staleCount: 2, + ...(props?.loading !== undefined ? { loading: props.loading } : {}), + ...(props?.queueUnavailable !== undefined ? { queueUnavailable: props.queueUnavailable } : {}), dismissableCount: props?.dismissableCount ?? 0, busy: props?.busy ?? false, batchSelectedCount: props?.batchSelectedCount ?? 0, @@ -295,3 +300,42 @@ describe('ReviewQueueRail apply-approved action (#1307)', () => { expect(wrapper.find('[data-testid="queue-batch-execute"]').exists()).toBe(true) }) }) + +describe('ReviewQueueRail queue announcement (#2214)', () => { + it('announces the awaiting count once the queue is loaded', () => { + const wrapper = mountRail({ awaitingCount: 3 }) + 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 }) + const live = wrapper.get('[data-testid="paper-review-queue-live"]') + expect(live.attributes('role')).toBe('status') + expect(live.text()).toBe('') + }) + + 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: [] }) + 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. + const wrapper = mountRail({ awaitingCount: 2 }) + expect(wrapper.get('[data-testid="paper-review-queue-live"]').text()).toContain( + '2 proposals awaiting review', + ) + }) +}) diff --git a/frontend/taskdeck-web/src/views/LegacyReviewView.vue b/frontend/taskdeck-web/src/views/LegacyReviewView.vue index b813aa147..fa3adb722 100644 --- a/frontend/taskdeck-web/src/views/LegacyReviewView.vue +++ b/frontend/taskdeck-web/src/views/LegacyReviewView.vue @@ -29,6 +29,7 @@ const { summaryCards, queueAccessRevoked, queueRefreshStale, + unavailableProposalId, dismissableProposalIds, isProposalExpired, clearProposalDeepLink, @@ -113,6 +114,18 @@ const renderedProposals = computed(() => { return target ? [target] : [] }) +/** + * Leave the refused deep link (#2214). Mirror of `PaperReviewView.returnToReview` + * so the two skins cannot drift (#1124 / ADR-0038): clearing the hash is the only + * action offered, and it is taken against the id the unavailable state names — + * never against whatever the hash happens to hold by then. + */ +function returnToReview() { + const proposalId = unavailableProposalId.value + if (!proposalId) return + void clearProposalDeepLink(proposalId) +} + async function dismissProposalAndReconcileHash(proposalId: string) { await handleDismissProposal(proposalId) if (!proposals.value.some((proposal) => proposalIdsEqual(proposal.id, proposalId))) { @@ -194,9 +207,10 @@ function handleReviewKeydown(event: KeyboardEvent) { const workspace = useWorkspaceStore() -// This skin renders no translated strings (`$t` appears nowhere in this file and -// its specs install no i18n plugin), so the announcement is plain text, matching -// the existing hardcoded copy above. +// This skin's own copy is hardcoded English, so the announcement is plain text, +// matching the existing hardcoded copy above. (The refused-deep-link state +// below is the one exception: it reuses the `review.empty.unavailable.*` keys +// Paper already renders, rather than forking that wording per skin.) const awaitingCount = computed( () => summaryCards.value.find((card) => card.id === 'pending-review')?.value ?? 0, ) @@ -206,6 +220,15 @@ const awaitingAnnouncement = computed(() => : `${awaitingCount.value} proposals awaiting review.`, ) +/** + * Whether `awaitingCount` is a real count right now (#2214). Kept in step with + * `ReviewQueueRail.countIsAnnounceable`, which gates the Paper skin's identical + * region on the same two states (#1124 / ADR-0038). + */ +const countIsAnnounceable = computed( + () => !proposalsLoading.value && !queueAccessRevoked.value, +) + /** * Same badge contract as the Paper skin (#2194 acceptance 3): the shell's * `Review · N` count is a home-summary workload figure AppShell reads once at @@ -285,9 +308,16 @@ onUnmounted(() => { - +

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

{
+ +
+

{{ $t('review.empty.unavailable.eyebrow') }}

+

{{ $t('review.empty.unavailable.title') }}

+

{{ $t('review.empty.unavailable.body', { id: unavailableProposalId }) }}

+ +
+ (), { dismissableCount: 0, @@ -67,6 +89,8 @@ const props = withDefaults( batchExecutableCount: 0, busy: false, authorPartitionAvailable: true, + loading: false, + queueUnavailable: false, }, ) @@ -94,6 +118,13 @@ const visible = computed(() => { } }) +/** + * Whether `awaitingCount` is a real count right now (#2214). A queue that is + * still loading and one whose access was revoked both carry 0 because nothing + * has been read, not because nothing awaits review; neither is speakable. + */ +const countIsAnnounceable = computed(() => !props.loading && !props.queueUnavailable) + /** Real 7-day cadence to render; null hides the mini-cadence bars entirely. */ const hasCadence = computed( () => Array.isArray(props.cadence) && props.cadence.length > 0, @@ -155,13 +186,18 @@ function onFilterPillClick(key: QueueFilter) { under them. The eyebrow itself cannot carry the live region: it also renders the stale count and is rewritten by filter clicks, which would make it chatter on ordinary interaction. + + The region stays MOUNTED while the count is unspeakable and only + withholds its content (#2214): a live region inserted at the same moment + its text appears is unreliably announced, so gating with `v-if` would + trade one defect for another. -->

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

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