Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 40 additions & 4 deletions frontend/taskdeck-web/src/composables/useReviewProposals.ts
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,10 @@ export function useReviewProposals() {
// permission failure into a fresh false negative, which is the exact class
// #2194 exists to remove.
const queueAccessRevoked = ref(false)
// Board access is not a global verdict. Keep the board whose list request
// received the 403 so a later request for another board never renders that
// earlier refusal as though it described the new scope (#2214).
const queueAccessRevokedScope = ref<string | null | undefined>(undefined)
// Raised only when a user-triggered list read is refused again while the
// revoked panel is already up. Background reads and the first refusal keep
// the durable authority panel as the only report for that fact.
Expand Down Expand Up @@ -639,6 +643,10 @@ export function useReviewProposals() {
return `${boardScope}:${isArchivedHistory.value ? 'archived' : 'live'}`
}

function queueAccessScopeOf(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.
Expand Down Expand Up @@ -873,6 +881,14 @@ export function useReviewProposals() {
if (signal?.aborted) return 'aborted'
reviewLoadPerf.start()
const requestId = ++latestProposalLoadRequestId
const requestedAccessScope = queueAccessScopeOf(activeBoardFilter.value || undefined)
// A scope change clears the previous board's authority claim before the
// new request starts, but an explicit 403 for that request is still a
// repeated refusal from the reviewer's perspective. Carry only this
// request-local fact into the 403 handler; a 500/other failure never calls
// `recordQueueAccessRevoked`, so it cannot carry the old board into view.
const hadRevokedPreviousScope =
queueAccessRevoked.value && queueAccessRevokedScope.value !== requestedAccessScope
let outcome: ProposalLoadOutcome = 'landed'

try {
Expand All @@ -885,6 +901,18 @@ export function useReviewProposals() {
// late answer describes the board it queried, never whichever board is on
// screen when it lands (#2599 item 1).
const requestedScope = queueScopeOf(filters.boardId)
if (
queueAccessRevoked.value &&
queueAccessRevokedScope.value !== requestedAccessScope
) {
// The new scope has not answered yet, so retain its ordinary loading
// and error semantics. It must not inherit another board's access
// refusal while that request is pending or if it fails transiently.
queueAccessRevoked.value = false
queueAccessRevokedRetry.value = false
queueAccessRevokedScope.value = undefined
resumeQueueRefreshAfterPermissionRecovery()
Comment thread
Chris0Jeky marked this conversation as resolved.
}
// 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
Expand Down Expand Up @@ -915,6 +943,7 @@ export function useReviewProposals() {
const accessWasRevoked = queueAccessRevoked.value
queueAccessRevoked.value = false
queueAccessRevokedRetry.value = false
queueAccessRevokedScope.value = undefined
if (accessWasRevoked) resumeQueueRefreshAfterPermissionRecovery()
} catch (e: unknown) {
if (requestId !== latestProposalLoadRequestId) return 'superseded'
Expand Down Expand Up @@ -945,7 +974,7 @@ export function useReviewProposals() {
// that calls `loadProposals` still gets its failure signal and its
// 'failed' outcome.
if (isForbiddenError(e)) {
recordQueueAccessRevoked(userInitiated)
recordQueueAccessRevoked(requestedAccessScope, userInitiated, hadRevokedPreviousScope)
} else {
toast.error(getErrorDisplay(e, t('review.toast.loadProposalsFailed')).message)
}
Expand Down Expand Up @@ -1046,10 +1075,17 @@ export function useReviewProposals() {
* three statements would be how the two legs drift into telling a reviewer
* two different stories about one revocation.
*/
function recordQueueAccessRevoked(userInitiated = false) {
function recordQueueAccessRevoked(
scope: string | null,
userInitiated = false,
hadRevokedPreviousScope = false,
) {
const accessWasAlreadyRevoked = queueAccessRevoked.value
queueAccessRevoked.value = true
if (accessWasAlreadyRevoked && userInitiated) queueAccessRevokedRetry.value = true
queueAccessRevokedScope.value = scope
if ((accessWasAlreadyRevoked || hadRevokedPreviousScope) && userInitiated) {
queueAccessRevokedRetry.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
Expand Down Expand Up @@ -1433,7 +1469,7 @@ export function useReviewProposals() {
// Board access was revoked. Stop polling rather than hammering an
// endpoint that will keep refusing, drop rows the server no longer
// authorises, and let the surface say so.
recordQueueAccessRevoked()
recordQueueAccessRevoked(queueAccessScopeOf(requestedBoardId))
return
}
// A read for a board the reviewer has already left, or one superseded by
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1082,6 +1082,25 @@ describe('useReviewProposals', () => {
expect(mockToast.error).toHaveBeenCalled()
})

it('does not carry a revoked board claim into another scope that has a transient failure', async () => {
mockRoute.query = { boardId: 'board-b' }
mockAutomationApi.getProposals.mockRejectedValueOnce({ response: { status: 403 } })
const rp = useReviewProposals()
await rp.loadProposals()
expect(rp.queueAccessRevoked.value).toBe(true)

mockRoute.query = { boardId: 'board-c' }
mockAutomationApi.getProposals.mockRejectedValueOnce({ response: { status: 500 } })
await rp.loadProposals()

// A 500 says C could not be refreshed. It cannot prove that C refused
// access, so the durable panel from B must not describe C.
expect(rp.queueAccessRevoked.value).toBe(false)
expect(rp.queueAccessRevokedRetry.value).toBe(false)
expect(rp.queueScopeLoaded.value).toBe(false)
expect(mockToast.error).toHaveBeenCalled()
})

it('keeps the pin-leg 403 as the single-proposal outcome #2593 shipped', async () => {
// A readable board with one proposal this reviewer may not open is the
// opposite case, and it must stay the unavailable pin rather than tearing
Expand Down Expand Up @@ -1135,6 +1154,26 @@ describe('useReviewProposals', () => {
expect(rp.queueAccessRevokedRetry.value).toBe(false)
})

it('keeps repeated refusal feedback when the second explicit read changes scope', async () => {
mockRoute.query = { boardId: 'board-a' }
mockAutomationApi.getProposals.mockRejectedValueOnce({ response: { status: 403 } })
const rp = useReviewProposals()
await rp.loadProposals()

expect(rp.queueAccessRevoked.value).toBe(true)
expect(rp.queueAccessRevokedRetry.value).toBe(false)

mockRoute.query = { boardId: 'board-b' }
mockAutomationApi.getProposals.mockRejectedValueOnce({ response: { status: 403 } })
await rp.loadProposals()

// Clearing board A's authority claim before the board B read must not
// erase the fact that this is the second explicit refusal.
expect(rp.queueAccessRevoked.value).toBe(true)
expect(rp.queueAccessRevokedRetry.value).toBe(true)
expect(mockToast.error).not.toHaveBeenCalled()
})

it('does not raise the retry disclosure for a non-user list read', async () => {
mockAutomationApi.getProposals.mockRejectedValueOnce({ response: { status: 403 } })
const rp = useReviewProposals()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1340,10 +1340,12 @@ describe('PaperReviewView', () => {
try {
expect(wrapper.find('[data-testid="paper-review-access-revoked-retry"]').exists()).toBe(false)

// Changing the board is a deliberate list-read attempt. The queue is
// already refused, so the second refusal needs its own durable sentence.
// Switching this board into its read-only history is a second deliberate
// list-read attempt for the SAME board. A different board must start
// with its own first-refusal state, but this retry keeps the same board
// authority scope and needs its own durable sentence.
mocks.getProposals.mockRejectedValueOnce({ response: { status: 403 } })
await routerOf(wrapper).replace('/workspace/review?boardId=another-board')
await routerOf(wrapper).replace('/workspace/review?boardId=board-revoked&history=archived')
await flushPromises()
await wrapper.vm.$nextTick()

Expand Down
Loading