From 4083bdc7fc26725f711b053f4447e0dd22458dc3 Mon Sep 17 00:00:00 2001 From: Chris0Jeky Date: Sat, 5 Sep 2026 15:02:56 +0100 Subject: [PATCH 1/2] test(review): red-first locus specs for the execute and reject recorders Six specs mirroring the onDefer pair from PR #2629, one trio per sibling: no receipt for a proposal left during the await, a receipt when the locus still matches, and the suppressed-receipt-while-the-deep-link-hash-is-still-present combination that PR #2629's review recorded as unobserved. Four of the six fail against the unguarded view. Both "does not record" specs fail on the main pane, which re-anchors to the abandoned proposal, and both "suppresses ... deep-link hash" specs fail on the receipt, which renders for it. The two locus-matching specs pass already and stand as the over-suppression complements, exactly as the defer matching spec does. The hash pair holds the route on the proposal the reviewer left with a router guard, reproducing the lagging navigation that is the stated reason the locus reads the explicit selection first and the hash only as a fallback. Refs #2128 --- .../paper/review/PaperReviewView.spec.ts | 219 ++++++++++++++++++ 1 file changed, 219 insertions(+) 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 b8e48e1a3..9a0278d67 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 @@ -272,6 +272,25 @@ async function confirmApplyDialog() { await flushPromises() } +/** + * `mountView` keeps its router private, so the live instance is reached through + * the mounted component. Structural casts, matching the `$route` idiom the + * deep-link specs above already use: the spec tree is type-checked without the + * router's own generics (tsconfig.vitest.json). + */ +function routerOf(wrapper: { vm: unknown }) { + return (wrapper.vm as { + $router: { + beforeEach: (guard: (to: { hash: string }) => false | undefined) => () => void + replace: (path: string) => Promise + } + }).$router +} + +function hashOf(wrapper: { vm: unknown }): string { + return (wrapper.vm as { $route: { hash: string } }).$route.hash +} + describe('PaperReviewView', () => { // Unmount every mounted wrapper after each test. PaperReviewView attaches a // window keydown listener (review keymap) and a 60s clock interval; without @@ -1717,6 +1736,206 @@ describe('PaperReviewView', () => { .toBe('deferred') }) + // #2128: the two confirm-gated recorders are the siblings of the approve + // (#2069) and defer (PR #2629) guards. Same locus expression, same hazard — + // the queue stays interactive across the execute/reject round trip, so the + // continuation must not pull the reviewer back to the proposal they left. + it('does not record an applied receipt for a proposal left during the execute confirmation', async () => { + let resolveExecute!: (proposal: Proposal) => void + mocks.executeProposal.mockImplementationOnce( + () => new Promise((resolve) => { resolveExecute = resolve }), + ) + const wrapper = await mountView([ + makeProposal({ id: 'aaa-1', status: 'Approved', summary: 'First proposal' }), + makeProposal({ id: 'bbb-1', summary: 'Second proposal' }), + ]) + + await wrapper.find('[data-serial="#AAA-"]').trigger('click') + await flushPromises() + await wrapper.find('[data-testid="decision-apply"]').trigger('click') + await confirmApplyDialog() + + // The queue stays interactive while the execute request is in flight. + await wrapper.find('[data-serial="#BBB-"]').trigger('click') + await flushPromises() + // Keep the current explicit selection while removing the route hash; this + // makes a stale receipt visible if the continuation re-anchors itself. + await wrapper.vm.$router.replace('/workspace/review') + await flushPromises() + + resolveExecute(makeProposal({ + id: 'aaa-1', + status: 'Applied', + summary: 'First proposal', + appliedAt: new Date().toISOString(), + })) + await flushPromises() + + expect(mocks.executeProposal).toHaveBeenCalledWith('aaa-1', expect.any(String)) + expect(wrapper.get('[data-testid="paper-review-main"]').text()).toContain('Second proposal') + expect(wrapper.find('[data-testid="paper-review-decision-receipt"]').exists()).toBe(false) + }) + + it('records an applied receipt when the current decision locus still matches', async () => { + let resolveExecute!: (proposal: Proposal) => void + mocks.executeProposal.mockImplementationOnce( + () => new Promise((resolve) => { resolveExecute = resolve }), + ) + const wrapper = await mountView([ + makeProposal({ id: 'matching-execute', status: 'Approved' }), + ]) + + await wrapper.find('[data-testid="decision-apply"]').trigger('click') + await confirmApplyDialog() + resolveExecute(makeProposal({ + id: 'matching-execute', + status: 'Applied', + appliedAt: new Date().toISOString(), + })) + await flushPromises() + + expect(wrapper.get('[data-testid="paper-review-decision-receipt"]').attributes('data-decision')) + .toBe('applied') + }) + + it('suppresses the applied receipt while the deep-link hash still names the proposal the reviewer left', async () => { + let resolveExecute!: (proposal: Proposal) => void + mocks.executeProposal.mockImplementationOnce( + () => new Promise((resolve) => { resolveExecute = resolve }), + ) + const wrapper = await mountView( + [ + makeProposal({ id: 'aaa-1', status: 'Approved', summary: 'First proposal' }), + makeProposal({ id: 'bbb-1', summary: 'Second proposal' }), + ], + '/workspace/review#proposal-aaa-1', + ) + const router = routerOf(wrapper) + + await wrapper.find('[data-testid="decision-apply"]').trigger('click') + await confirmApplyDialog() + + // Router navigation can lag behind a queue click — the reason the locus + // reads the explicit selection FIRST and the hash only as a fallback. Hold + // the hash on the proposal the reviewer left so the two disagree at the + // moment the response lands. + const releaseHash = router.beforeEach((to) => (to.hash === '#proposal-bbb-1' ? false : undefined)) + await wrapper.find('[data-serial="#BBB-"]').trigger('click') + await flushPromises() + releaseHash() + + resolveExecute(makeProposal({ + id: 'aaa-1', + status: 'Applied', + summary: 'First proposal', + appliedAt: new Date().toISOString(), + })) + await flushPromises() + + // Neither confirm recorder touches the deep link (only onDefer/onFileAway + // do), so the hash is exactly where the queue click left it and the + // visibleProposals carve-out still anchors the surface on the linked row. + expect(hashOf(wrapper)).toBe('#proposal-aaa-1') + expect(wrapper.find('[data-testid="paper-review-decision-receipt"]').exists()).toBe(false) + expect(wrapper.get('[data-testid="paper-review-main"]').text()).toContain('First proposal') + + // Dropping the hash reveals the locus underneath: still the reviewer's own + // selection, not the proposal a stale receipt would have re-pinned. + await router.replace('/workspace/review') + await flushPromises() + + expect(wrapper.get('[data-testid="paper-review-main"]').text()).toContain('Second proposal') + expect(wrapper.find('[data-testid="paper-review-decision-receipt"]').exists()).toBe(false) + }) + + it('does not record a rejected receipt for a proposal left during the reject confirmation', async () => { + let resolveReject!: (proposal: Proposal) => void + mocks.rejectProposal.mockImplementationOnce( + () => new Promise((resolve) => { resolveReject = resolve }), + ) + const wrapper = await mountView([ + makeProposal({ id: 'aaa-1', summary: 'First proposal' }), + makeProposal({ id: 'bbb-1', summary: 'Second proposal' }), + ]) + + await wrapper.find('[data-serial="#AAA-"]').trigger('click') + await flushPromises() + await wrapper.find('[data-testid="decision-reject"]').trigger('click') + await flushPromises() + await acceptRejectDialog() + + // The queue stays interactive while the reject request is in flight. + await wrapper.find('[data-serial="#BBB-"]').trigger('click') + await flushPromises() + // Keep the current explicit selection while removing the route hash; this + // makes a stale receipt visible if the continuation re-anchors itself. + await wrapper.vm.$router.replace('/workspace/review') + await flushPromises() + + resolveReject(makeProposal({ id: 'aaa-1', status: 'Rejected', summary: 'First proposal' })) + await flushPromises() + + expect(mocks.rejectProposal).toHaveBeenCalledWith('aaa-1', null) + expect(wrapper.get('[data-testid="paper-review-main"]').text()).toContain('Second proposal') + expect(wrapper.find('[data-testid="paper-review-decision-receipt"]').exists()).toBe(false) + }) + + it('records a rejected receipt when the current decision locus still matches', async () => { + let resolveReject!: (proposal: Proposal) => void + mocks.rejectProposal.mockImplementationOnce( + () => new Promise((resolve) => { resolveReject = resolve }), + ) + const wrapper = await mountView([makeProposal({ id: 'matching-reject' })]) + + await wrapper.find('[data-testid="decision-reject"]').trigger('click') + await flushPromises() + await acceptRejectDialog('no longer needed') + resolveReject(makeProposal({ id: 'matching-reject', status: 'Rejected' })) + await flushPromises() + + expect(mocks.rejectProposal).toHaveBeenCalledWith('matching-reject', 'no longer needed') + expect(wrapper.get('[data-testid="paper-review-decision-receipt"]').attributes('data-decision')) + .toBe('rejected') + }) + + it('suppresses the rejected receipt while the deep-link hash still names the proposal the reviewer left', async () => { + let resolveReject!: (proposal: Proposal) => void + mocks.rejectProposal.mockImplementationOnce( + () => new Promise((resolve) => { resolveReject = resolve }), + ) + const wrapper = await mountView( + [ + makeProposal({ id: 'aaa-1', summary: 'First proposal' }), + makeProposal({ id: 'bbb-1', summary: 'Second proposal' }), + ], + '/workspace/review#proposal-aaa-1', + ) + const router = routerOf(wrapper) + + await wrapper.find('[data-testid="decision-reject"]').trigger('click') + await flushPromises() + await acceptRejectDialog() + + // Same lagging-navigation window as the execute sibling above. + const releaseHash = router.beforeEach((to) => (to.hash === '#proposal-bbb-1' ? false : undefined)) + await wrapper.find('[data-serial="#BBB-"]').trigger('click') + await flushPromises() + releaseHash() + + resolveReject(makeProposal({ id: 'aaa-1', status: 'Rejected', summary: 'First proposal' })) + await flushPromises() + + expect(hashOf(wrapper)).toBe('#proposal-aaa-1') + expect(wrapper.find('[data-testid="paper-review-decision-receipt"]').exists()).toBe(false) + expect(wrapper.get('[data-testid="paper-review-main"]').text()).toContain('First proposal') + + await router.replace('/workspace/review') + await flushPromises() + + expect(wrapper.get('[data-testid="paper-review-main"]').text()).toContain('Second proposal') + expect(wrapper.find('[data-testid="paper-review-decision-receipt"]').exists()).toBe(false) + }) + it('removes a snoozed proposal from the visible queue after defer resolves', async () => { const deferred = makeProposal({ id: 'snooze-me', From 7fbcb8eec242bec0723bf750c6aee2d7380d1ad6 Mon Sep 17 00:00:00 2001 From: Chris0Jeky Date: Sat, 5 Sep 2026 15:03:01 +0100 Subject: [PATCH 2/2] fix(review): decision-locus guard on the execute and reject receipt recorders onConfirmExecute and onConfirmReject recorded their receipt unconditionally after the awaited confirm. The queue stays interactive across that round trip, and recordDecisionReceipt also re-pins explicitActiveId, so a reviewer who selected proposal B while the execute or reject for proposal A was in flight was pulled back to A the moment the response landed. Both now compare explicitActiveId ?? activeProposal.id against the decided proposal with proposalIdsEqual before recording, the same guard onApply (#2069) and onDefer (PR #2629) already carry, in onApply's early-return shape since nothing follows the recorder in either function. The confirm call, the collected reject reason, the normalized status checks and the deep-link handling are unchanged. Neither function touched the route hash before this change and neither does now. Refs #2128 --- .../src/views/paper/PaperReviewView.vue | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/frontend/taskdeck-web/src/views/paper/PaperReviewView.vue b/frontend/taskdeck-web/src/views/paper/PaperReviewView.vue index 00865b7e2..e97b55208 100644 --- a/frontend/taskdeck-web/src/views/paper/PaperReviewView.vue +++ b/frontend/taskdeck-web/src/views/paper/PaperReviewView.vue @@ -2121,6 +2121,14 @@ async function onConfirmExecute() { ? proposals.value.find((proposal) => proposalIdsEqual(proposal.id, proposalId)) : undefined if (applied && normalizeProposalStatus(applied.status) === 'Applied') { + // The queue stays interactive across the execute round trip, so the + // reviewer can be on another proposal by the time it lands. Only surface a + // receipt when they are still at the decision locus this execute started + // from. Prefer the explicit selection over the route hash, because router + // navigation can lag behind a queue click: a late response must never pull + // them back to proposal A after they have selected proposal B. + const currentDecisionLocusId = explicitActiveId.value ?? activeProposal.value?.id + if (!proposalIdsEqual(currentDecisionLocusId, applied.id)) return recordDecisionReceipt(applied.id, 'applied') } } @@ -2132,6 +2140,12 @@ async function onConfirmReject(reason: string) { ? proposals.value.find((proposal) => proposalIdsEqual(proposal.id, proposalId)) : undefined if (rejected && normalizeProposalStatus(rejected.status) === 'Rejected') { + // Reject shares that interactive-queue window with execute above, and with + // approve (#2069) and defer (PR #2629): the same locus check decides only + // whether the receipt is surfaced. The collected reason and the status + // check are untouched — the rejection itself already stands. + const currentDecisionLocusId = explicitActiveId.value ?? activeProposal.value?.id + if (!proposalIdsEqual(currentDecisionLocusId, rejected.id)) return recordDecisionReceipt(rejected.id, 'rejected') } }