[dolho]

Summary

onEscapeKeydown handled the voice call in a branch above shouldCancelOnEscape that read none of its preconditions — so an overlay claiming Escape in the capture phase with preventDefault() closed and ended the call on one keystroke. preventDefault() cannot help where nothing reads it.

Why not the one-liner

The issue offers if (event.defaultPrevented) return at the top. That is smaller and worse twice over:

• it fixes the reported symptom and leaves the next Escape owner free to re-decide the preconditions the same way;
• it keeps the decision inside an SFC vitest cannot mount (environment: 'node', no @vue/test-utils) — which is exactly how the original shipped able to ignore defaultPrevented and isComposing at once.

So both branches now start from one private ownsEscape(event) in utils/turnCancel.js — Escape, not composing, not already claimed — and shouldEndCallOnEscape(event, { callActive }) sits beside shouldCancelOnEscape as a sibling rule. Ordering is unchanged: the call is still asked first, because while a call is up the composer and the picker are inert and there is no turn to cancel.

A second instance nobody reported

The old branch ignored isComposing, so abandoning an IME candidate with Escape ended the call. Same class, one field over; closed by the same change.

Tests

portalVoiceMode.spec.js pinned the literal old condition and its ordering. The ordering is the property ent#534 actually cares about and is untouched, so that assertion is re-pointed at the new spelling rather than dropped. turnCancel.spec.js gains the rule's own arms plus a source guard that the branch dispatches through the rule — a predicate the branch bypasses passes its own tests.

Frontend suite: 2254 passed (101 files).

Docs

chat-turn-cancellation.md recorded this as a known residual; it now records the fix, why a guard clause was rejected, and the IME instance.

Fixes #2598

🤖 Generated with Claude Code

https://claude.ai/code/session_01CdxGmvuKuqaWJZ6pKUgaUJ

[dolho]

/review — PR #2601 (self-review)

Branch: fix/2598-escape-defaultprevented → dev · Files: 5 (+163/−17)
Scope: CLEAN — the issue's AC is one behaviour; the diff changes that behaviour, one stale comment it names, and the tests around both.

Critical

None.

Findings from this pass — fixed here

[F1] Both #2598 source guards read raw source, including comments (Confidence: 9/10)
src/frontend/tests/unit/turnCancel.spec.js

This file's read is a bare readFileSync — no stripComments, unlike its siblings portalAgentPageUx.spec.js and portalLoadingTreatment.spec.js. Fine for every guard that predates this PR; not fine for the two it adds, and they fail in opposite directions:

• the positive guard (toContain('shouldEndCallOnEscape(event, { callActive: voiceCallActive.value })')) would pass on a comment alone if the call were deleted — vacuous, on the one assertion that proves the branch dispatches through the rule rather than around it;
• the negative guard asserts the old condition is gone, over a region whose comments now quote that condition verbatim to explain the fix — one explanatory sentence away from failing spuriously and telling a reviewer the fix regressed.

Both now read a stripComments view. The comment guard keeps reading raw on purpose: the comment is the thing under test there.

Mutation-checked rather than assumed — restoring the old inline condition turns the dispatch guard red (1 failed | 47 passed), reverting the mutation turns it green (48 passed). A guard that cannot fail is not a guard, and this one demonstrably can.

Informational

[I1] The fix is larger than the issue's suggestion, and that is a judgement call worth stating (Confidence: n/a — design note)

The issue offers if (event.defaultPrevented) return at the top of the handler. I extracted ownsEscape and added a sibling rule instead. Reasons, so the trade is visible rather than assumed:

• the one-liner fixes the reported symptom and leaves the next Escape owner free to re-decide the preconditions the same way — this is already the second owner to do it;
• it keeps the decision inside an SFC vitest cannot mount, which is how the original shipped able to ignore defaultPrevented and isComposing at once.

Cost: one more exported name, and shouldCancelOnEscape now delegates its first three lines. A reviewer who prefers the one-liner has a fair case; I would rather argue it here than have it inferred from a diff.

[I2] A second instance of the class, not in the issue (Confidence: 9/10)

The old branch ignored isComposing, so abandoning an IME candidate with Escape ended the call. Closed by the same change and pinned by its own test. Worth flagging because it is evidence for [I1]: the bug was never only defaultPrevented, it was a branch that read none of the preconditions.

Clean (verified)

• shouldCancelOnEscape is behaviourally unchanged: ownsEscape runs the same three checks in the same order that were previously inline, before the same inFlight/cancelling/overlays logic. All 41 pre-existing assertions in the file pass untouched.
• The ent#534 ordering property survives. portalVoiceMode.spec.js pinned the literal old condition and the ordering; the ordering is what ent#534 actually cares about, so I re-pointed that assertion at the new spelling rather than dropping it — the same treatment I asked for in #2591's review, applied to my own change.
• No other surface is affected: ChatPanel.vue and PublicChat.vue have no voice branch (grepped); this is Workspace-only.
• Frontend suite: 2254 passed (101 files).
• Docs: chat-turn-cancellation.md carried this as a known residual and now carries the fix, the rejected alternative, and the IME instance.

Summary

Critical: 0 · Findings fixed in-pass: 1 · Informational: 2 · Scope: clean.

Cannot self-approve; posting as a review comment. [I1] is the one that wants a second opinion.

[dolho]

/review — re-review of the fix commit (PR #2601)

Scope: the stripComments commit added since the first review. Full frontend suite 2254 passed (101 files) at the new head.

Critical

None.

Verified, not assumed

The point of the fix was that the two source guards were vacuous/fragile. A fix for a vacuous guard is worth nothing unless the guard can now actually fail, so I mutation-tested it rather than trusting the green:

restore `if (voiceCallActive.value && event.key === 'Escape')`  → 1 failed | 47 passed
revert the mutation                                            → 48 passed

The failing test is the right one — "the Workspace dispatches on the rule instead of re-deciding it in the SFC". That is the assertion the whole PR rests on, and it is now demonstrably load-bearing.

I also read helpers/stripComments.js before relying on it, because a stripper that mangles source would move the failure rather than fix it. It handles Vue SFCs properly: HTML comments by an index scan (not the non-greedy regex, which leaves residue on <!<!---->--> — documented there with the reasoning), block comments to a fixpoint, and // comments last with a [^:] guard so URLs survive. Correct for this use.

Informational

[I3] The negative guard got stricter as a side effect, and that is an improvement worth naming (Confidence: 7/10)

It changed from /if \(voiceCallActive\.value && event\.key === 'Escape'\)/ to /voiceCallActive\.value && event\.key === 'Escape'/ — the if ( prefix dropped. Against a comment-stripped view the prefix bought nothing and would have missed the same condition re-introduced as a const, a ternary, or a second && clause. Not a behaviour change to the product; noting it so the widening is a recorded choice rather than an artefact of the rewrite.

[I4] One guard still reads raw source, on purpose (Confidence: 8/10)

expect(workspace).not.toContain('takes Escape for itself in the first branch below') — the stale comment the issue asks to be removed. Stripping comments there would make it vacuous in the strictest sense: the thing under test is a comment. Called out in the code so a future pass "consistently" converting it to the stripped view knows why not to.

Clean (re-verified at the new head)

• shouldCancelOnEscape is still behaviourally unchanged: ownsEscape runs the same three checks in the same order, and all 41 pre-existing assertions pass untouched.
• The stripped view is scoped to the #2598 guards. workspace (raw) still feeds the pre-existing "all three surfaces" block, so no guard outside this PR changed meaning.
• The ent#534 ordering guard still passes against the new spelling, so the property that PR cares about is intact.
• No product code changed in this commit — it is tests only, on top of a fix whose behaviour the first review already covered.

Summary

Critical: 0 · Informational: 2 · Product code unchanged by this commit.

The substantive question from the first review — [I1], whether the sibling-rule extraction is the right size of fix versus the issue's one-liner — is unaffected by this pass and still the one I would like a second opinion on.

[vybe]

merge-train: batch validated on train/20260908-1521 (train PR #2630 green: 27 checks).