Skip to content
199 changes: 162 additions & 37 deletions frontend/taskdeck-web/src/composables/useReviewProposals.ts

Large diffs are not rendered by default.

10 changes: 10 additions & 0 deletions frontend/taskdeck-web/src/locales/en/review.ts
Original file line number Diff line number Diff line change
Expand Up @@ -96,13 +96,23 @@ export default {
// try. It shares the visible slot with `degraded.body` and takes precedence
// over it, and it is also the text of a mounted sr-only live region in each
// skin, so keep it speakable as one sentence run.
//
// `refused.recovered` is that failure's own recovery sentence, and it is
// deliberately NARROWER than `degraded.recovered` (#2638 item 2). The refusal
// is retracted the moment the LIST read answers, on a tick whose composite
// read can still fail at the deep-link leg and leave the rendered rows
// untouched, so this sentence must say only that the server is accepting the
// refresh again. Translators: do NOT add a clause about the proposals being
// current or up to date — that is `degraded.recovered`'s job, and saying it
// here overclaimed for up to two poll intervals before this key existed.
queue: {
degraded: {
body: 'This review queue may be out of date. Showing the last available proposals while Taskdeck retries.',
recovered: 'This review queue is up to date again. Showing current proposals.',
},
refused: {
body: 'This review queue has stopped updating. The server is refusing the refresh rather than failing temporarily, so these are the last proposals it confirmed. Reload the page, or check the board filter in the address bar.',
recovered: 'The server is accepting refreshes for this review queue again.',
},
},

Expand Down
1 change: 1 addition & 0 deletions frontend/taskdeck-web/src/locales/es/review.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ export default {
},
refused: {
body: 'Esta cola de revisión ha dejado de actualizarse. El servidor está rechazando la actualización en lugar de fallar temporalmente, así que estas son las últimas propuestas que confirmó. Recarga la página o revisa el filtro de tablero en la barra de direcciones.',
recovered: 'El servidor vuelve a aceptar las actualizaciones de esta cola de revisión.',
},
},

Expand Down
1 change: 1 addition & 0 deletions frontend/taskdeck-web/src/locales/it/review.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ export default {
},
refused: {
body: 'Questa coda di revisione ha smesso di aggiornarsi. Il server sta rifiutando la richiesta di aggiornamento invece di fallire temporaneamente, quindi queste sono le ultime proposte che ha confermato. Ricarica la pagina oppure controlla il filtro bacheca nella barra degli indirizzi.',
recovered: 'Il server accetta di nuovo gli aggiornamenti di questa coda di revisione.',
},
},

Expand Down
182 changes: 182 additions & 0 deletions frontend/taskdeck-web/src/tests/composables/useReviewProposals.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2409,6 +2409,126 @@ describe('useReviewProposals', () => {
rp.stopQueueRefresh()
})

it('is not retired by an explicit load inside the poll interval (#2638 item 2)', async () => {
vi.useFakeTimers()
mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'current' })])
const rp = useReviewProposals()
await rp.loadProposals()
rp.startQueueRefresh()

await pollTransientFailures(REVIEW_QUEUE_CONSECUTIVE_FAILURE_THRESHOLD)
mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'recovered' })])
await vi.advanceTimersByTimeAsync(REVIEW_QUEUE_REFRESH_MS)
expect(rp.queueRefreshRecovered.value).toBe(true)

// The reviewer's already-clicked Approve completes a few hundred
// milliseconds after the recovering poll and reloads the queue. Every
// explicit reload took the same success path, so it emptied the region
// before a polite live region had any chance to speak the sentence --
// the #2638 defect. The load itself is unchanged: the fresh queue lands.
mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'after-decision' })])
await rp.loadProposals()
expect(rp.proposals.value.map((p: any) => p.id)).toEqual(['after-decision'])
expect(rp.queueRefreshRecovered.value).toBe(true)
expect(rp.queueRefreshRecoveredKind.value).toBe('degraded')

// Nor does a second one age it: explicit loads never retire, however many
// of them land inside the interval.
mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'and-again' })])
await rp.loadProposals()
expect(rp.queueRefreshRecovered.value).toBe(true)

// The next BACKGROUND success is what retires it, exactly as #2630
// intended -- about one poll interval of life.
mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'still-fine' })])
await vi.advanceTimersByTimeAsync(REVIEW_QUEUE_REFRESH_MS)
expect(rp.queueRefreshRecovered.value).toBe(false)
expect(rp.queueRefreshRecoveredKind.value).toBe(null)
rp.stopQueueRefresh()
})

it('is not retired by an explicit load that follows a FAILED background tick (#2638 item 2)', async () => {
vi.useFakeTimers()
mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'current' })])
const rp = useReviewProposals()
await rp.loadProposals()
rp.startQueueRefresh()

await pollTransientFailures(REVIEW_QUEUE_CONSECUTIVE_FAILURE_THRESHOLD)
mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'recovered' })])
await vi.advanceTimersByTimeAsync(REVIEW_QUEUE_REFRESH_MS)
expect(rp.queueRefreshRecovered.value).toBe(true)

// A later background read alone is not the rule: that read has to SUCCEED
// and be the one recording the success. This tick fails (below the
// threshold, so nothing is disclosed), and the explicit load that follows
// is still an explicit load.
mockAutomationApi.getProposals.mockRejectedValueOnce({ response: { status: 500 } })
await vi.advanceTimersByTimeAsync(REVIEW_QUEUE_REFRESH_MS)
mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'explicit' })])
await rp.loadProposals()
expect(rp.queueRefreshStale.value).toBe(false)
expect(rp.queueRefreshRecovered.value).toBe(true)
rp.stopQueueRefresh()
})

it('gives an EXPLICIT-load recovery a full interval before a poll can retire it (#2638 round 2)', async () => {
vi.useFakeTimers()
mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'current' })])
const rp = useReviewProposals()
await rp.loadProposals()
rp.startQueueRefresh()

await pollTransientFailures(REVIEW_QUEUE_CONSECUTIVE_FAILURE_THRESHOLD)
expect(rp.queueRefreshStale.value).toBe(true)

// The post-decision reload is the read that ends the degraded state here,
// and it lands BETWEEN ticks -- 14.9 s into a 15 s cycle in the worst
// case. Stamping that raise with the ordinal already on the counter names
// a read that has finished, so the tick 100 ms later would retire the
// sentence: the same defect this rule exists to close, with the roles
// swapped (round-2 review finding).
mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'explicit' })])
await rp.loadProposals()
expect(rp.queueRefreshStale.value).toBe(false)
expect(rp.queueRefreshRecovered.value).toBe(true)
expect(rp.queueRefreshRecoveredKind.value).toBe('degraded')

// The next poll success is the one the sentence lives THROUGH.
mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'first-poll' })])
await vi.advanceTimersByTimeAsync(REVIEW_QUEUE_REFRESH_MS)
expect(rp.queueRefreshRecovered.value).toBe(true)

// The one after retires it, so it is bounded exactly as a poll-raised
// sentence is -- at least one full interval, never the session.
mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'second-poll' })])
await vi.advanceTimersByTimeAsync(REVIEW_QUEUE_REFRESH_MS)
expect(rp.queueRefreshRecovered.value).toBe(false)
expect(rp.queueRefreshRecoveredKind.value).toBe(null)
rp.stopQueueRefresh()
})

it('still retires an explicit-load recovery immediately at a degraded onset (#2638 round 2)', async () => {
vi.useFakeTimers()
mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'current' })])
const rp = useReviewProposals()
await rp.loadProposals()
rp.startQueueRefresh()

await pollTransientFailures(REVIEW_QUEUE_CONSECUTIVE_FAILURE_THRESHOLD)
mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'explicit' })])
await rp.loadProposals()
expect(rp.queueRefreshRecovered.value).toBe(true)

// The extra interval of life is about the sentence being OLD. An onset
// makes it FALSE, and that retirement stays immediate for either stamp,
// or the next real recovery would be silent.
await pollTransientFailures(REVIEW_QUEUE_CONSECUTIVE_FAILURE_THRESHOLD)
expect(rp.queueRefreshStale.value).toBe(true)
expect(rp.queueRefreshRecovered.value).toBe(false)
rp.stopQueueRefresh()
})

it('clears at the next degraded onset so a second recovery announces again', async () => {
vi.useFakeTimers()
mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'current' })])
Expand Down Expand Up @@ -2558,6 +2678,24 @@ describe('useReviewProposals', () => {
rp.stopQueueRefresh()
})

it('announces the retraction with the refusal sentence, not the queue sentence (#2638 item 2)', async () => {
const rp = await startedWithCurrentQueue()

await pollListFailures(REVIEW_QUEUE_CONSECUTIVE_FAILURE_THRESHOLD, { response: { status: 400 } })
expect(rp.queueRefreshRefused.value).toBe(true)

mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'recovered' })])
await vi.advanceTimersByTimeAsync(REVIEW_QUEUE_REFRESH_MS)

// The retraction is raised by the LIST leg, and the composite success
// that follows it in the same read must not swap the sentence for the
// queue one: this signal's job is to retract the refusal claim, and the
// surfaces say only that refreshes are being accepted again.
expect(rp.queueRefreshRecovered.value).toBe(true)
expect(rp.queueRefreshRecoveredKind.value).toBe('refused')
rp.stopQueueRefresh()
})

it('leaves the 403 authority path to its own owner', async () => {
const rp = await startedWithCurrentQueue()

Expand Down Expand Up @@ -2633,6 +2771,50 @@ describe('useReviewProposals', () => {
rp.stopQueueRefresh()
})

it('says nothing about the queue when the pin leg strands the composite read (#2638 item 2)', async () => {
// The copy defect PR #2694's round-2 verification recorded on #2214. On a
// list-success/pin-fail tick the composite read returns before
// `proposals.value = next`, so the rows on screen are exactly the ones
// that were there before -- and the shared #2630 sentence's second clause
// ("Showing current proposals") stood for up to two further poll
// intervals, because the next tick's list success returns early too and
// only the degraded onset after it retires the sentence.
vi.useFakeTimers()
mockRoute.hash = '#proposal-p-pinned'
mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'p-pinned' })])
const rp = useReviewProposals()
await rp.loadProposals()
rp.startQueueRefresh()

for (let failure = 0; failure < REVIEW_QUEUE_CONSECUTIVE_FAILURE_THRESHOLD; failure += 1) {
mockAutomationApi.getProposals.mockRejectedValueOnce({ response: { status: 404 } })
await vi.advanceTimersByTimeAsync(REVIEW_QUEUE_REFRESH_MS)
}
expect(rp.queueRefreshRefused.value).toBe(true)

// The list read answers again; only the pinned row's by-id read is down.
mockAutomationApi.getProposals.mockResolvedValueOnce([])
mockAutomationApi.getProposal.mockRejectedValueOnce({ response: { status: 500 } })
await vi.advanceTimersByTimeAsync(REVIEW_QUEUE_REFRESH_MS)

expect(rp.queueRefreshRefused.value).toBe(false)
expect(rp.queueRefreshRecovered.value).toBe(true)
// The kind is what the surfaces read to pick the sentence, and this is
// the tick that proves why the two cannot share one: the queue was NOT
// replaced.
expect(rp.queueRefreshRecoveredKind.value).toBe('refused')
expect(rp.proposals.value.map((p: any) => p.id)).toEqual(['p-pinned'])

// A LATER background success retires it, the same rule the queue sentence
// follows. This tick's list carries the pinned row, so there is no by-id
// leg and the composite read completes.
mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'p-pinned' })])
await vi.advanceTimersByTimeAsync(REVIEW_QUEUE_REFRESH_MS)
expect(rp.queueRefreshRecovered.value).toBe(false)
expect(rp.queueRefreshRecoveredKind.value).toBe(null)
rp.stopQueueRefresh()
})

it('does not count a pin-leg failure, whose tick read the list successfully', async () => {
vi.useFakeTimers()
mockRoute.hash = '#proposal-p-pinned'
Expand Down
7 changes: 7 additions & 0 deletions frontend/taskdeck-web/src/tests/views/ReviewView.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -504,7 +504,14 @@ describe('ReviewView', () => {

expect(wrapper.find('[data-testid="review-queue-stale"]').exists()).toBe(false)
expect(wrapper.find('[data-testid="review-queue-refused"]').text()).toBe('')
// The refusal's OWN retraction sentence (#2638 item 2). The queue
// sentence would add "Showing current proposals", which this signal does
// not prove: it is raised by the list leg, on a tick whose composite read
// can still bail before the rows are replaced.
expect(wrapper.find('[data-testid="review-queue-recovered"]').text()).toBe(
enReview.queue.refused.recovered,
)
expect(wrapper.find('[data-testid="review-queue-recovered"]').text()).not.toBe(
enReview.queue.degraded.recovered,
)
} finally {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4015,6 +4015,45 @@ describe('PaperReviewView', () => {
}
})

it('announces a refusal retraction with the refusal sentence, not the queue one (#2638)', async () => {
vi.useFakeTimers({ toFake: ['setInterval', 'clearInterval', 'setTimeout', 'clearTimeout', 'Date'] })
try {
const wrapper = await mountView([makeProposal({ id: 'retained-1' })])
const region = wrapper.get('[data-testid="paper-review-queue-recovered"]')
expect(region.text()).toBe('')

mocks.getProposals.mockRejectedValue({ response: { status: 400 } })
for (let failure = 0; failure < REVIEW_QUEUE_CONSECUTIVE_FAILURE_THRESHOLD; failure += 1) {
vi.advanceTimersByTime(REVIEW_QUEUE_REFRESH_MS)
await flushPromises()
}
await wrapper.vm.$nextTick()
expect(wrapper.get('[data-testid="paper-review-queue-refused"]').text()).toBe(
enReview.queue.refused.body,
)

mocks.getProposals.mockResolvedValue([makeProposal({ id: 'retained-1' })])
vi.advanceTimersByTime(REVIEW_QUEUE_REFRESH_MS)
await flushPromises()
await wrapper.vm.$nextTick()

// One region, two sentences, picked by the kind the composable reports.
// The refusal retraction is raised by the LIST leg on a tick that may
// never replace the rendered rows, so "Showing current proposals" would
// overclaim (#2214, PR #2694 round 2); the degraded recovery beside it
// keeps saying exactly that, and its own test asserts so.
const after = wrapper.get('[data-testid="paper-review-queue-recovered"]')
expect(after.text()).toBe(enReview.queue.refused.recovered)
expect(after.text()).not.toBe(enReview.queue.degraded.recovered)
// Still the region that was mounted before anything went wrong (#2630).
expect(after.element).toBe(region.element)
expect(wrapper.find('[data-testid="paper-review-queue-refused"]').text()).toBe('')
wrapper.unmount()
} finally {
vi.useRealTimers()
}
})

it('renders the refusal warning in the empty column too (#2214)', async () => {
vi.useFakeTimers({ toFake: ['setInterval', 'clearInterval', 'setTimeout', 'clearTimeout', 'Date'] })
try {
Expand Down
20 changes: 18 additions & 2 deletions frontend/taskdeck-web/src/views/LegacyReviewView.vue
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ const {
queueRefreshStale,
queueRefreshRefused,
queueRefreshRecovered,
queueRefreshRecoveredKind,
unavailableProposalId,
unavailableProposalMalformed,
dismissableProposalIds,
Expand Down Expand Up @@ -332,14 +333,29 @@ onUnmounted(() => {
throughout, withholding its text, because a live region inserted at the
same moment its text appears is unreliably announced (#2593). The
signal comes from the shared composable, so both skins announce the
same transition with the same sentence (ADR-0038 / #1124). -->
same transition with the same sentence (ADR-0038 / #1124).

TWO sentences through this one region (#2638 item 2), chosen by the
kind the composable reports: a 'degraded' recovery follows a completed
read and may say the rows are current, while a 'refused' one is raised
as soon as the LIST read answers — a tick that may never replace the
queue — so it says only that the server is accepting refreshes again.
Paper picks the key the same way. -->
<p
class="sr-only"
role="status"
aria-live="polite"
aria-atomic="true"
data-testid="review-queue-recovered"
>{{ queueRefreshRecovered && !queueAccessRevoked ? $t('review.queue.degraded.recovered') : '' }}</p>
>{{
queueRefreshRecovered && !queueAccessRevoked
? $t(
queueRefreshRecoveredKind === 'refused'
? 'review.queue.refused.recovered'
: 'review.queue.degraded.recovered',
)
: ''
}}</p>

<!-- The refused-refresh disclosure (#2214 item 2). Same construction and
the same reason as the two regions above: the visible warning below
Expand Down
20 changes: 19 additions & 1 deletion frontend/taskdeck-web/src/views/paper/PaperReviewView.vue
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ const {
queueRefreshStale,
queueRefreshRefused,
queueRefreshRecovered,
queueRefreshRecoveredKind,
nowMs,
visibleProposals,
dismissableProposalIds,
Expand Down Expand Up @@ -2651,6 +2652,15 @@ async function onClearBoardScope() {
cannot carry this — it is a state, and a state that is merely gone
announces nothing. `queueRefreshRecovered` is the transition.

TWO sentences through this one region, because the composable retracts two
different disclosures and they claim different things (#2638 item 2). A
'degraded' recovery follows a completed read, so it may say the rows are
current; a 'refused' one is raised as soon as the LIST read answers, on a
tick that may never replace the queue, so it says only that the server is
accepting refreshes again. One region rather than two: they are the same
job — the retraction of whichever disclosure was standing — and only one
can stand at a time.

It lives HERE, above the `v-if="activeProposal"` / `v-else` pair, and not
once inside each arm. The recovering poll assigns the queue and records
the success in one synchronous block, so a recovery that puts a proposal
Expand All @@ -2668,7 +2678,15 @@ async function onClearBoardScope() {
aria-live="polite"
aria-atomic="true"
data-testid="paper-review-queue-recovered"
>{{ queueRefreshRecovered && !queueAccessRevoked ? $t('review.queue.degraded.recovered') : '' }}</p>
>{{
queueRefreshRecovered && !queueAccessRevoked
? $t(
queueRefreshRecoveredKind === 'refused'
? 'review.queue.refused.recovered'
: 'review.queue.degraded.recovered',
)
: ''
}}</p>

<!--
The refused-refresh disclosure (#2214 item 2), built the same way and
Expand Down
Loading