From c315d732e2bbaedab93a162be60bef65d61153aa Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 2 Aug 2026 04:39:49 -0700 Subject: [PATCH] fix(ime): guard the review surfaces, and the handler shape the scanner missed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reviewer-picker inputs on the PR page and the GitHub item dialog had no composition check at all: a marked ArrowDown moved the app's selection, a marked Enter requested a reviewer, and a marked Escape closed the picker — three keystrokes the user aimed at the candidate window. The duplicate-issue search ran on a composing Enter for the same reason. MentionTextarea is guarded at its own DOM handler rather than in each caller. It is the only keydown listener on that textarea, so refusing there is what keeps a composing Escape from reaching a delegate that would discard the draft. The ordering invariant could not have caught any of this, and it also could not see the native chat composer: its regex matched the hook's outer function and brace-matched the destructured parameter list, so the body was never scanned and an Escape-before-guard regression there passed. Body detection now skips the brace forms that are not bodies — destructuring, JSX containers, object types — and the coverage the scanner claims is asserted by name, so a parser regression fails naming the file it stopped seeing. Co-authored-by: Orca --- .../src/components/GitHubItemDialog.tsx | 11 ++++ .../src/components/PullRequestPage.tsx | 13 +++++ .../src/lib/ime-escape-guard-ordering.test.ts | 57 ++++++++++++++++--- 3 files changed, 72 insertions(+), 9 deletions(-) diff --git a/src/renderer/src/components/GitHubItemDialog.tsx b/src/renderer/src/components/GitHubItemDialog.tsx index dfc2fe90afe..bb2e374afbd 100644 --- a/src/renderer/src/components/GitHubItemDialog.tsx +++ b/src/renderer/src/components/GitHubItemDialog.tsx @@ -76,6 +76,7 @@ import { } from '@/components/ui/dropdown-menu' import CommentMarkdown from '@/components/sidebar/CommentMarkdown' import { detectLanguage } from '@/lib/language-detect' +import { isImeCompositionKeyDown } from '@/lib/ime-composition-keyboard-event' import { cn } from '@/lib/utils' import { DiffSectionItem } from '@/components/editor/DiffSectionItem' import type { DecoratedDiffComment } from '@/components/diff-comments/useDiffCommentDecorator' @@ -1261,6 +1262,12 @@ function PRReviewersPanel({ aria-haspopup="listbox" className="h-8 min-w-0 cursor-text rounded-md border-border/50 bg-background text-xs" onKeyDown={(event) => { + // Above the dispatch: while a candidate window is open the arrows move the + // candidate, Enter commits it and Escape dismisses it — none of the three may + // also drive the reviewer picker. + if (isImeCompositionKeyDown(event)) { + return + } if (event.key === 'ArrowDown' && actionableReviewerRows.length > 0) { event.preventDefault() setActiveReviewerIndex( @@ -5940,6 +5947,10 @@ function GHEditSection({ setDuplicateError(null) }} onKeyDown={(event) => { + // An Enter that only commits a CJK candidate must not also run the search. + if (isImeCompositionKeyDown(event)) { + return + } if (event.key === 'Enter') { event.preventDefault() handleDuplicateSearchSubmit() diff --git a/src/renderer/src/components/PullRequestPage.tsx b/src/renderer/src/components/PullRequestPage.tsx index c00abfad692..1a5e95a33a2 100644 --- a/src/renderer/src/components/PullRequestPage.tsx +++ b/src/renderer/src/components/PullRequestPage.tsx @@ -69,6 +69,7 @@ import { } from '@/components/ui/dropdown-menu' import CommentMarkdown from '@/components/sidebar/CommentMarkdown' import { detectLanguage } from '@/lib/language-detect' +import { isImeCompositionKeyDown } from '@/lib/ime-composition-keyboard-event' import { cn } from '@/lib/utils' import { setWithLRU } from '@/lib/scroll-cache' import { isScreenSubmitShortcut } from '@/lib/screen-submit-shortcut' @@ -1304,6 +1305,12 @@ function PRReviewersPanel({ aria-haspopup="listbox" className="h-8 min-w-0 cursor-text rounded-md border-border/50 bg-background text-xs" onKeyDown={(event) => { + // Above the dispatch: while a candidate window is open the arrows move the + // candidate, Enter commits it and Escape dismisses it — none of the three may + // also drive the reviewer picker. + if (isImeCompositionKeyDown(event)) { + return + } if (event.key === 'ArrowDown' && actionableReviewerRows.length > 0) { event.preventDefault() setActiveReviewerIndex( @@ -5424,6 +5431,12 @@ function MentionTextarea({ }} onBlur={() => setMentionQuery(null)} onKeyDown={(event) => { + // Above the dispatch, and above the `onKeyDown` delegate below: this is the only DOM + // handler on the textarea, so refusing here is what keeps a composing Escape from + // reaching a caller that would discard the draft. + if (isImeCompositionKeyDown(event)) { + return + } if (showSuggestions) { if (event.key === 'ArrowDown') { event.preventDefault() diff --git a/src/renderer/src/lib/ime-escape-guard-ordering.test.ts b/src/renderer/src/lib/ime-escape-guard-ordering.test.ts index 69c2bb010e4..2d82d9a7988 100644 --- a/src/renderer/src/lib/ime-escape-guard-ordering.test.ts +++ b/src/renderer/src/lib/ime-escape-guard-ordering.test.ts @@ -1,5 +1,5 @@ import { readdirSync, readFileSync } from 'node:fs' -import { join } from 'node:path' +import { join, sep } from 'node:path' import { describe, expect, it } from 'vitest' // The marker sweep that found the dropped-`isComposing` adapters is blind to this defect: the @@ -13,6 +13,11 @@ import { describe, expect, it } from 'vitest' const ROOTS = ['src/renderer/src', 'src/main', 'src/shared'] const GUARD = 'isImeCompositionKeyDown' const ESCAPE = "=== 'Escape'" +const MUST_SCAN = [ + 'src/renderer/src/components/native-chat/use-native-chat-composer-keydown.ts', + 'src/renderer/src/components/new-workspace/SmartWorkspaceNameField.tsx', + 'src/renderer/src/components/settings/ShortcutRecorderButton.tsx' +] function sourceFiles(dir: string): string[] { const found: string[] = [] @@ -32,12 +37,40 @@ function sourceFiles(dir: string): string[] { return found } +// A body brace never follows `(`, `,`, `:` or `=`; those introduce a destructured parameter list, +// a JSX expression container, or an object type — all of which open with `{` and would otherwise +// be brace-matched in place of the body. Returns -1 for a handler passed by name, where the +// expression closes before any body opens. +function bodyBrace(source: string, from: number): number { + let skipped = 0 + for (let i = from; i < source.length; i++) { + if (source[i] === '{') { + let before = i - 1 + while (before >= 0 && /\s/.test(source[before])) { + before-- + } + if (!'(,:='.includes(source[before])) { + return i + } + skipped++ + continue + } + if (source[i] === '}') { + if (skipped === 0) { + return -1 + } + skipped-- + } + } + return -1 +} + // Returns the body of every keydown handler in `source`, brace-matched from its opening `{`. function keydownHandlerBodies(source: string): string[] { const bodies: string[] = [] const starts = /onKeyDown(?:Capture)?=\{|(?:function|const)\s+\w*[Kk]ey[Dd]own\w*\s*[=(]/g for (let match = starts.exec(source); match !== null; match = starts.exec(source)) { - const open = source.indexOf('{', match.index + match[0].length - 1) + const open = bodyBrace(source, match.index + match[0].length) if (open === -1) { continue } @@ -79,13 +112,19 @@ describe('composition guards sit above the Escape branch', () => { expect(offenders).toEqual([]) }) - // Guards against the check silently matching nothing — a scanner that parses no handlers - // would report zero offenders forever. - it('actually scanned guarded keydown handlers', () => { - const scanned = ROOTS.flatMap(sourceFiles).filter((file) => { - const source = readFileSync(file, 'utf8') - return source.includes(GUARD) && keydownHandlerBodies(source).some((b) => b.includes(GUARD)) - }) + // A scanner that parses nothing reports zero offenders forever, so the coverage it claims is + // asserted by name. MUST_SCAN holds the handler shapes the brace matcher has actually got wrong: + // a hook whose destructured parameter list opens before its body, an inline JSX arrow, and a + // named callback. A parser regression fails here naming the file it stopped seeing. + it('actually scanned the guarded keydown handlers it claims to cover', () => { + const scanned = ROOTS.flatMap(sourceFiles) + .filter((file) => { + const source = readFileSync(file, 'utf8') + return source.includes(GUARD) && keydownHandlerBodies(source).some((b) => b.includes(GUARD)) + }) + .map((file) => file.split(sep).join('/')) + + expect(scanned).toEqual(expect.arrayContaining(MUST_SCAN)) expect(scanned.length).toBeGreaterThan(8) }) })