fix(ime): guard the review surfaces, and the handler shape the scanner missed

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 <help@stably.ai>
This commit is contained in:
Neil
2026-08-02 04:39:49 -07:00
co-authored by Orca
parent c2e59f6aff
commit c315d732e2
3 changed files with 72 additions and 9 deletions
@@ -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()
@@ -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()
@@ -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)
})
})