fix(review): decision-locus guard on the execute and reject receipt recorders - #2692
Conversation
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
…ecorders 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
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Review record (alpha product-trust lane, review-and-ship round 1 at head Reviewer: one fresh-context independent reviewer subagent (read-only; Codex credits exhausted, SC-9), given the worktree at the head and the merge-base diff. Verdict: SHIP, no CRITICAL, HIGH or MEDIUM. Lenses that found nothing: the operand order is right ( Findings, triaged once:
Not verified by the reviewer (no shell): the worker's run of |
Summary
onConfirmExecuteandonConfirmRejectinPaperReviewView.vuerecorded their decision receipt unconditionally after the awaited confirm call. The queue rail stays interactive across that round trip, andrecordDecisionReceiptalso re-pinsexplicitActiveId, so a reviewer who selected proposal B while the execute or reject request for proposal A was still in flight was pulled back to A the moment the response landed.Both recorders now compare
explicitActiveId.value ?? activeProposal.value?.idagainst the decided proposal withproposalIdsEqualbefore recording, which is the same decision-locus guardonApplycarries from #2069 andonDefercarries from PR #2629. This is the last of the three sibling recorders named in the issue.The confirm call, the collected reject reason, the normalized status checks and the deep-link handling are unchanged in both functions.
Closes #2128 (Refs #2069, #2073, PR #2629)
Changes
4083bdc7ftest(review): six red-first specs inPaperReviewView.spec.ts, placed next to theonDeferpair they mirror. One trio per sibling: no receipt for a proposal left during the await, a receipt when the locus still matches, and the suppressed-receipt-with-the-hash-still-present combination that PR fix: guard deferred review receipts by decision locus #2629's review recorded as unobserved. Adds two small typed helpers,routerOfandhashOf, using the same structural-cast idiom the existing deep-link specs use for$route.7fbcb8eecfix(review): the locus guard in both recorders, inonApply's early-return shape since nothing follows the recorder in either function. 14 added lines, no deletions, no other change in the view.Test plan
Verified, all from
frontend/taskdeck-web:npx vitest --run --maxWorkers=2 src/tests/views/paper/review/PaperReviewView.spec.ts -t "applied receipt|rejected receipt|deep-link hash still names"gaveTest Files 1 failed (1),Tests 4 failed | 2 passed | 177 skipped (183).Test Files 1 passed (1),Tests 6 passed | 177 skipped (183).npx vitest --run --maxWorkers=2 src/tests/views/paper/review/PaperReviewView.spec.tsgivesTest Files 1 passed (1),Tests 183 passed (183). That is the 177 that existed on the base commit plus the 6 new ones.npm run typecheckpasses with no output.npx eslint src/views/paper/PaperReviewView.vue src/tests/views/paper/review/PaperReviewView.spec.tsexits 0 with no findings.git diff --checkexits 0.Which assertions failed on the unmodified view, per sibling:
Execute (
onConfirmExecute)expect(wrapper.get('[data-testid="paper-review-main"]').text()).toContain('Second proposal'). The main pane had re-anchored to the abandoned proposal and rendered its applied record instead ("APPLIED . READ-ONLY ... First proposal").expect(wrapper.find('[data-testid="paper-review-decision-receipt"]').exists()).toBe(false), receivedtrue.Reject (
onConfirmReject)toContain('Second proposal')assertion, with the pane showing the rejected record for the abandoned proposal.exists()).toBe(false), receivedtrue.What each sibling does with the
#proposal-<id>hash today, since PR #2629's review asked for it: neither one touches it.clearProposalDeepLinkis called only fromonFileAway,onFileAwayBulk,onDefer,returnToReviewandonQueueFilterChange. The two hash specs assert that the route hash is still#proposal-aaa-1after the response lands and that thevisibleProposalscarve-out still anchors the surface on the linked row, so the unchanged behaviour is now observed rather than assumed. Those specs hold the hash on the left proposal with a routerbeforeEachthat aborts the pending navigation, which reproduces the lagging queue-click navigation that is the stated reason the locus expression reads the explicit selection first and the hash only as a fallback. Without that ordering the guard would compare against the stale hash target and the receipt would still surface, so the two hash specs also pin the operand order.NOT verified:
npm run build, and Playwright were not run. This change touches two functions in one view and its own spec file, and CI covers the rest.Boundaries and risks
frontend/taskdeck-web/src/views/paper/PaperReviewView.vue(14 added lines, no deletions, confined to the two functions) andfrontend/taskdeck-web/src/tests/views/paper/review/PaperReviewView.spec.ts. No composable, locale, router or component change, so nothing outside the Paper review view can be affected.LegacyReviewView.vuewires its dialogs'@confirmstraight toconfirmExecuteProposalandconfirmRejectProposalfromuseReviewActions.tswith no local wrapper, and it never callsrecordDecisionReceipt. The decision receipt is a Paper-skin surface only, rendered byviews/paper/review/ReviewMain.vue. There is no unguarded Legacy sibling to follow up on.onApplyandonDeferonmain: if the active row leaves the filtered list mid-flight without arming the poll notice, the receipt can still re-anchor the surface. This change neither introduces nor alters that path.