diff --git a/docs/editor-find-layout-aware-shortcut.md b/docs/editor-find-layout-aware-shortcut.md new file mode 100644 index 000000000..9f75390c1 --- /dev/null +++ b/docs/editor-find-layout-aware-shortcut.md @@ -0,0 +1,92 @@ +# Layout-aware find in editable Monaco editors + +## Problem + +Issue [#7953](https://github.com/stablyai/orca/issues/7953) reports that `Cmd+F` can type `f` into a TypeScript file instead of opening find on macOS. + +Orca's editable Monaco surfaces currently install its layout-aware save shortcut through `editor-shortcuts.ts` (`src/renderer/src/components/editor/editor-shortcuts.ts:18`), but leave find entirely to Monaco's internal keycode dispatch. Orca already declares `editor.find` as `Mod+F` (`src/shared/keybindings.ts:784`) and its matcher resolves logical keys before physical-code fallback (`src/shared/keybindings.ts:2041`). + +A real Electron repro sends a macOS event with logical key `f`, physical code `KeyU`, and virtual key code `U`, as produced by a non-QWERTY layout. Monaco leaves its find widget closed. The equivalent QWERTY `KeyF` event opens it. + +## Root cause + +Monaco's built-in find keybinding follows the physical/virtual keycode delivered by Chromium. Orca's shortcut system is layout-aware, but its editable Monaco integrations do not use it for find. On layouts where the key that produces `f` is not physical `KeyF`, Monaco misses the chord and Native Edit Context remains in editing mode. + +## Non-goals + +- Reimplementing Monaco's find widget, match navigation, or search state. +- Changing find behavior in markdown preview, rich markdown, PDF, browser, terminal, or file search. +- Changing find behavior in read-only diff surfaces. +- Reworking all Monaco keybindings or making Monaco defaults fully obey shortcut unbinding in this patch. +- Adding telemetry for a local keyboard action. + +## Design + +1. Add a focused editor find installer beside the existing save installer in `editor-shortcuts.ts`. It matches `editor.find` through `editorShortcutMatches`, consumes every matched event before it reaches Native Edit Context or Monaco, and invokes a supplied callback only for the initial, non-repeat keydown. Repeat events must still be prevented and propagation-stopped so Monaco's QWERTY binding cannot reopen/reset the widget. +2. Install that handler on every editable Monaco container in the source editor, editable diff views, and notebook code cells. Run that editor's existing `actions.find` action and dispose the bridge from its existing teardown callback. +3. Add unit coverage at the DOM-listener seam for logical `f` with a non-`KeyF` physical code, QWERTY/default behavior, repeat suppression, unrelated typing, and cleanup. +4. Preserve an Electron regression loop that drives both QWERTY and layout-aware raw key events against a real `.ts` editor and verifies the existing Monaco find widget becomes visible without dirtying the file. + +## Data flow + +- macOS/Linux/Windows keydown reaches the focused editable Monaco container. +- `editorShortcutMatches('editor.find', event)` resolves the active platform, user bindings, modifiers, and logical key. +- On match, Orca consumes the DOM event and calls Monaco's existing `actions.find` action. +- Monaco owns the visible find widget and focus exactly as before. + +## Edge cases + +- Auto-repeat must be consumed without invoking find again; returning early before prevention would let Monaco handle a repeated QWERTY `KeyF` event. +- Ordinary unmodified `f` typing and unrelated shortcuts must continue to Monaco unchanged. +- A removed/disposed source editor, diff pane, or notebook cell must not retain the listener. +- QWERTY `Cmd/Ctrl+F` must still open the same Monaco widget once, not twice. +- User-configured bindings accepted by Orca's `editor.find` matcher should open find; Monaco's own default bindings remain outside the scope of this patch. +- The behavior is renderer-local and does not read files or execute commands, so local, SSH, and Remote Orca files share the same path. + +## Test plan + +- Unit: `editor-shortcuts.test.ts` dispatches keyboard events through a real element and parameterizes the shared bridge across macOS (`metaKey`) and Linux/Windows (`ctrlKey`) using logical `f` with physical `KeyU`. It also asserts QWERTY/default handling, matched-repeat prevention without a second callback, unrelated typing, and disposal behavior used by every editable Monaco integration. +- Electron: open a disposable `.ts` file and drive `Cmd+F`/`Ctrl+F` using the platform modifier; assert `.find-widget.visible`, focused find input, unchanged source text, and clean editor state. +- Electron layout regression: on macOS, dispatch logical `f` with non-QWERTY physical/virtual key identity; assert the same find state and unchanged source text. +- Adjacent smoke: dismiss find, type a normal character, and verify it edits the file rather than reopening find. +- Static: run focused Vitest, `pnpm typecheck`, and `pnpm lint`. + +## UI quality bar + +No new UI. The existing Monaco find widget must appear in its current position and styling, focus its input, and leave the editor content unchanged. No overlap, clipping, duplicate widget, or focus flicker is acceptable. + +## Review screenshots + +1. QWERTY/default shortcut with the existing Monaco find widget visible in a `.ts` editor. +2. Non-QWERTY logical-`f` regression path with the same find widget visible and source text unchanged. +3. Adjacent ordinary typing state after find is dismissed, showing the source editor still accepts text normally. + +## Rollout + +1. Add failing unit coverage for the layout-aware find installer contract. +2. Implement the installer in `editor-shortcuts.ts`. +3. Wire it to each editable Monaco surface's existing find action and lifecycle cleanup. +4. Run focused tests, typecheck, lint, and the Electron QWERTY/layout/typing scenarios. + +## Lightweight Eng Review + +- Scope: Kept to one shared shortcut installer and the existing mount/teardown seams for source editors, editable diff panes, and notebook code cells; no new find implementation or global shortcut interception. +- Architecture/data flow: The renderer-local Monaco container owns the keydown. Orca's canonical matcher resolves the logical key, while Monaco continues to own widget state and rendering. No main/preload/IPC, persistence, network, SSH, or provider boundary changes. +- Failure modes covered: + - Non-QWERTY logical key differs from physical/virtual keycode. + - QWERTY double handling. + - Auto-repeat escaping to Monaco's native QWERTY handler. + - Listener surviving editor disposal. + - Ordinary typing being consumed. + - Custom `editor.find` chord accepted by Orca but not Monaco. +- Test coverage required: + - DOM-listener unit tests in `src/renderer/src/components/editor/editor-shortcuts.test.ts`, parameterized for Darwin/Meta, Linux/Ctrl, and Windows/Ctrl. + - Electron-visible QWERTY and layout-aware `.ts` scenarios. + - Adjacent ordinary typing smoke test. +- Performance/blast radius: One capture listener per mounted editable Monaco editor, doing a constant-time keybinding comparison only for events within that editor. Multiple mounted diff sections do not fan out because each listener is scoped to its own container. Listeners are removed with Monaco disposal; no polling, scans, IPC, storage, or render-loop work. +- UI quality bar: Existing Monaco find widget only; verify focus, unchanged source text, no duplicate opening, and unchanged styling against `docs/STYLEGUIDE.md`. +- Required review screenshots: + 1. Default QWERTY find-open state. + 2. Non-QWERTY logical-key find-open state. + 3. Find-dismissed ordinary typing state. +- Residual risks: Monaco's internal default bindings remain active when a user explicitly rebinds `editor.find`; fully suppressing/remapping Monaco's native keybinding table is a separate, larger change. diff --git a/src/renderer/src/components/editor/DiffSectionItem.tsx b/src/renderer/src/components/editor/DiffSectionItem.tsx index 71fe21cf9..ffb1aa36b 100644 --- a/src/renderer/src/components/editor/DiffSectionItem.tsx +++ b/src/renderer/src/components/editor/DiffSectionItem.tsx @@ -27,7 +27,7 @@ import { DiffSectionHeader } from './DiffSectionHeader' import type { DiffSection } from './diff-section-types' import type { DiffComment } from '../../../../shared/types' import { isDiffComment } from '@/lib/diff-comment-compat' -import { installEditorSaveShortcut } from './editor-shortcuts' +import { installEditorSaveShortcut, installMonacoEditorFindShortcut } from './editor-shortcuts' import { DiffSectionBody } from './DiffSectionBody' import { useDiffSectionLayoutMetrics } from './useDiffSectionLayoutMetrics' import { disposeUnattachedMonacoModelPaths } from './diff-monaco-model-disposal' @@ -343,9 +343,12 @@ export function DiffSectionItem({ } modifiedEditorsRef.current.set(index, modified) + const original = editor.getOriginalEditor() const cleanupSaveShortcut = installEditorSaveShortcut(modified.getContainerDomNode(), () => handleSectionSaveRef.current(index) ) + const cleanupOriginalFindShortcut = installMonacoEditorFindShortcut(original) + const cleanupModifiedFindShortcut = installMonacoEditorFindShortcut(modified) const modelContentSub = modified.onDidChangeModelContent(() => { const current = modified.getValue() setSections((prev) => { @@ -380,9 +383,11 @@ export function DiffSectionItem({ }) }) modified.onDidDispose(() => { - // Why: editable diff sections own both the save shortcut and model-change - // subscription for this Monaco editor instance. + // Why: editable diff sections own both panes' shortcut bridges and the + // model subscription for the lifetime of this Monaco diff instance. cleanupSaveShortcut() + cleanupOriginalFindShortcut() + cleanupModifiedFindShortcut() modelContentSub.dispose() }) } diff --git a/src/renderer/src/components/editor/DiffViewer.tsx b/src/renderer/src/components/editor/DiffViewer.tsx index 7f5868875..9aae498c1 100644 --- a/src/renderer/src/components/editor/DiffViewer.tsx +++ b/src/renderer/src/components/editor/DiffViewer.tsx @@ -16,7 +16,7 @@ import { import { applyDiffEditorLineNumberOptions } from './diff-editor-line-number-options' import type { DiffComment } from '../../../../shared/types' import { isDiffComment } from '@/lib/diff-comment-compat' -import { installEditorSaveShortcut } from './editor-shortcuts' +import { installEditorSaveShortcut, installMonacoEditorFindShortcut } from './editor-shortcuts' import { diffEditorScrollbarOptions } from './diff-editor-scrollbar-options' import { LargeDiffFallback } from './LargeDiffFallback' import { getLargeDiffRenderLimit } from './large-diff-render-limit' @@ -336,15 +336,19 @@ export default function DiffViewer({ onSaveRef.current?.(modifiedEditor.getValue()) } ) + const cleanupOriginalFindShortcut = installMonacoEditorFindShortcut(originalEditor) + const cleanupModifiedFindShortcut = installMonacoEditorFindShortcut(modifiedEditor) // Track changes const modelContentSub = modifiedEditor.onDidChangeModelContent(() => { onContentChangeRef.current?.(modifiedEditor.getValue()) }) modifiedEditor.onDidDispose(() => { - // Why: editable diff views own both the save shortcut and - // model-change subscription for this Monaco editor instance. + // Why: editable diff views own both panes' shortcut bridges and the + // model subscription for the lifetime of this Monaco diff instance. cleanupSaveShortcut() + cleanupOriginalFindShortcut() + cleanupModifiedFindShortcut() modelContentSub.dispose() }) diff --git a/src/renderer/src/components/editor/IpynbViewer.tsx b/src/renderer/src/components/editor/IpynbViewer.tsx index d813e3ff9..fc19ca041 100644 --- a/src/renderer/src/components/editor/IpynbViewer.tsx +++ b/src/renderer/src/components/editor/IpynbViewer.tsx @@ -51,7 +51,11 @@ import { Tooltip, TooltipContent, TooltipTrigger } from '@/components/ui/tooltip import { ShortcutKeyCombo } from '@/components/ShortcutKeyCombo' import { useShortcutKeyDetails, type ShortcutKeyComboDetails } from '@/hooks/useShortcutLabel' import { registerPendingEditorFlush } from './editor-pending-flush' -import { editorShortcutMatches, installEditorSaveShortcut } from './editor-shortcuts' +import { + editorShortcutMatches, + installEditorSaveShortcut, + installMonacoEditorFindShortcut +} from './editor-shortcuts' import { getIpynbCodeCellEditorHeight, getIpynbCodeCellPreviewLines } from './ipynb-code-cell-lines' import MonacoCodeExcerpt from './MonacoCodeExcerpt' import { @@ -349,13 +353,15 @@ function CodeCell({ void onSaveRequestRef.current() } ) + const cleanupFindShortcut = installMonacoEditorFindShortcut(editorInstance) const blurSub = editorInstance.onDidBlurEditorWidget(() => { onDeactivateRef.current() }) editorInstance.onDidDispose(() => { - // Why: the inline source editor owns both the save shortcut and blur - // subscription for this Monaco editor instance. + // Why: the inline source editor owns its shortcut bridges and blur + // subscription for the lifetime of this Monaco editor instance. cleanupSaveShortcut() + cleanupFindShortcut() blurSub.dispose() }) editorInstance.addCommand(monacoInstance.KeyCode.Escape, () => { diff --git a/src/renderer/src/components/editor/MonacoEditor.tsx b/src/renderer/src/components/editor/MonacoEditor.tsx index bf9018831..d99af690a 100644 --- a/src/renderer/src/components/editor/MonacoEditor.tsx +++ b/src/renderer/src/components/editor/MonacoEditor.tsx @@ -44,7 +44,7 @@ import { getDiffCommentPopoverTop } from '../diff-comments/diff-comment-popover-position' import { isLinuxUserAgent } from '../terminal-pane/pane-helpers' -import { installEditorSaveShortcut } from './editor-shortcuts' +import { installEditorSaveShortcut, installMonacoEditorFindShortcut } from './editor-shortcuts' import { Plus } from 'lucide-react' import { getMonacoMarkdownSelectionAnnotationTarget, @@ -390,13 +390,12 @@ export default function MonacoEditor({ return model.getValueInRange(selection) }) - const cleanupSaveShortcut = installEditorSaveShortcut( - editorInstance.getContainerDomNode(), - () => { - const value = editorInstance.getValue() - propsRef.current.onSave(value) - } - ) + const editorDomNode = editorInstance.getContainerDomNode() + const cleanupSaveShortcut = installEditorSaveShortcut(editorDomNode, () => { + const value = editorInstance.getValue() + propsRef.current.onSave(value) + }) + const cleanupFindShortcut = installMonacoEditorFindShortcut(editorInstance) const searchInFilesAction = editorInstance.addAction({ id: 'orca.searchInFiles', label: translate('auto.components.editor.MonacoEditor.fd68ae03b3', 'Search in Files'), @@ -442,7 +441,6 @@ export default function MonacoEditor({ } }) } - const editorDomNode = editorInstance.getContainerDomNode() editorDomNode.addEventListener('paste', onLargeTextPaste, { capture: true }) // Track cursor line for "copy path to line" feature @@ -496,6 +494,7 @@ export default function MonacoEditor({ scrollStateSub.dispose() gutterMouseDownSub.dispose() cleanupSaveShortcut() + cleanupFindShortcut() editorDomNode.removeEventListener('paste', onLargeTextPaste, { capture: true }) searchInFilesAction.dispose() autoHeightSub?.dispose() diff --git a/src/renderer/src/components/editor/editor-shortcuts.test.ts b/src/renderer/src/components/editor/editor-shortcuts.test.ts new file mode 100644 index 000000000..daacea7f6 --- /dev/null +++ b/src/renderer/src/components/editor/editor-shortcuts.test.ts @@ -0,0 +1,198 @@ +// @vitest-environment happy-dom + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const shortcutState = vi.hoisted(() => ({ + keybindings: {} as Record, + platform: 'darwin' as NodeJS.Platform +})) + +vi.mock('@/lib/shortcut-platform', () => ({ + getShortcutPlatform: () => shortcutState.platform +})) + +vi.mock('@/store', () => ({ + useAppStore: { + getState: () => ({ keybindings: shortcutState.keybindings }) + } +})) + +import { installEditorFindShortcut, installMonacoEditorFindShortcut } from './editor-shortcuts' + +type ShortcutFixture = { + container: HTMLDivElement + dispose: () => void + input: HTMLTextAreaElement + onDownstreamKeyDown: ReturnType + onFind: ReturnType +} + +function createShortcutFixture(): ShortcutFixture { + const container = document.createElement('div') + const input = document.createElement('textarea') + const onDownstreamKeyDown = vi.fn() + const onFind = vi.fn() + + container.appendChild(input) + document.body.appendChild(container) + input.addEventListener('keydown', onDownstreamKeyDown) + + return { + container, + dispose: installEditorFindShortcut(container, onFind), + input, + onDownstreamKeyDown, + onFind + } +} + +function dispatchKeyDown(target: HTMLElement, init: KeyboardEventInit): KeyboardEvent { + const event = new KeyboardEvent('keydown', { + ...init, + bubbles: true, + cancelable: true + }) + target.dispatchEvent(event) + return event +} + +beforeEach(() => { + shortcutState.keybindings = {} + shortcutState.platform = 'darwin' +}) + +afterEach(() => { + document.body.replaceChildren() +}) + +describe('installEditorFindShortcut', () => { + it.each([ + { label: 'macOS', platform: 'darwin' as const, modifier: { metaKey: true } }, + { label: 'Linux', platform: 'linux' as const, modifier: { ctrlKey: true } }, + { label: 'Windows', platform: 'win32' as const, modifier: { ctrlKey: true } } + ])('matches logical f on physical KeyU for $label', ({ platform, modifier }) => { + shortcutState.platform = platform + const fixture = createShortcutFixture() + + const event = dispatchKeyDown(fixture.input, { + key: 'f', + code: 'KeyU', + ...modifier + }) + + expect(event.defaultPrevented).toBe(true) + expect(fixture.onFind).toHaveBeenCalledTimes(1) + expect(fixture.onDownstreamKeyDown).not.toHaveBeenCalled() + fixture.dispose() + }) + + it('consumes the QWERTY shortcut before Monaco can handle it again', () => { + const fixture = createShortcutFixture() + + const event = dispatchKeyDown(fixture.input, { + key: 'f', + code: 'KeyF', + metaKey: true + }) + + expect(event.defaultPrevented).toBe(true) + expect(fixture.onFind).toHaveBeenCalledTimes(1) + expect(fixture.onDownstreamKeyDown).not.toHaveBeenCalled() + fixture.dispose() + }) + + it('consumes matched repeats without invoking find again', () => { + const fixture = createShortcutFixture() + + const initialEvent = dispatchKeyDown(fixture.input, { + key: 'f', + code: 'KeyF', + metaKey: true + }) + const repeatEvent = dispatchKeyDown(fixture.input, { + key: 'f', + code: 'KeyF', + metaKey: true, + repeat: true + }) + + expect(initialEvent.defaultPrevented).toBe(true) + expect(repeatEvent.defaultPrevented).toBe(true) + expect(fixture.onFind).toHaveBeenCalledTimes(1) + expect(fixture.onDownstreamKeyDown).not.toHaveBeenCalled() + fixture.dispose() + }) + + it.each([ + { label: 'ordinary f typing', init: { key: 'f', code: 'KeyU' } }, + { + label: 'an unrelated shortcut', + init: { key: 'g', code: 'KeyG', metaKey: true } + } + ])('leaves $label untouched', ({ init }) => { + const fixture = createShortcutFixture() + + const event = dispatchKeyDown(fixture.input, init) + + expect(event.defaultPrevented).toBe(false) + expect(fixture.onFind).not.toHaveBeenCalled() + expect(fixture.onDownstreamKeyDown).toHaveBeenCalledTimes(1) + fixture.dispose() + }) + + it('honors a custom editor.find binding', () => { + shortcutState.keybindings = { 'editor.find': ['Mod+G'] } + const fixture = createShortcutFixture() + + const defaultEvent = dispatchKeyDown(fixture.input, { + key: 'f', + code: 'KeyF', + metaKey: true + }) + const customEvent = dispatchKeyDown(fixture.input, { + key: 'g', + code: 'KeyU', + metaKey: true + }) + + expect(defaultEvent.defaultPrevented).toBe(false) + expect(customEvent.defaultPrevented).toBe(true) + expect(fixture.onFind).toHaveBeenCalledTimes(1) + expect(fixture.onDownstreamKeyDown).toHaveBeenCalledTimes(1) + fixture.dispose() + }) + + it('removes the listener when disposed', () => { + const fixture = createShortcutFixture() + fixture.dispose() + + const event = dispatchKeyDown(fixture.input, { + key: 'f', + code: 'KeyU', + metaKey: true + }) + + expect(event.defaultPrevented).toBe(false) + expect(fixture.onFind).not.toHaveBeenCalled() + expect(fixture.onDownstreamKeyDown).toHaveBeenCalledTimes(1) + }) + + it('runs Monaco existing find action through the shared bridge', () => { + const container = document.createElement('div') + const input = document.createElement('textarea') + const run = vi.fn() + const getAction = vi.fn((_id: string) => ({ run })) + container.appendChild(input) + document.body.appendChild(container) + const dispose = installMonacoEditorFindShortcut({ + getAction, + getContainerDomNode: () => container + }) + + dispatchKeyDown(input, { key: 'f', code: 'KeyU', metaKey: true }) + + expect(getAction).toHaveBeenCalledWith('actions.find') + expect(run).toHaveBeenCalledTimes(1) + dispose() + }) +}) diff --git a/src/renderer/src/components/editor/editor-shortcuts.ts b/src/renderer/src/components/editor/editor-shortcuts.ts index 66f30c493..86a7e7212 100644 --- a/src/renderer/src/components/editor/editor-shortcuts.ts +++ b/src/renderer/src/components/editor/editor-shortcuts.ts @@ -28,3 +28,31 @@ export function installEditorSaveShortcut(target: HTMLElement, onSave: () => voi target.addEventListener('keydown', handleKeyDown, true) return () => target.removeEventListener('keydown', handleKeyDown, true) } + +export function installEditorFindShortcut(target: HTMLElement, onFind: () => void): () => void { + const handleKeyDown = (event: KeyboardEvent): void => { + if (!editorShortcutMatches('editor.find', event)) { + return + } + event.preventDefault() + event.stopPropagation() + // Why: matched repeats must stay consumed so Monaco cannot reopen or reset find. + if (!event.repeat) { + onFind() + } + } + + target.addEventListener('keydown', handleKeyDown, true) + return () => target.removeEventListener('keydown', handleKeyDown, true) +} + +type MonacoFindShortcutEditor = { + getAction: (id: string) => { run: () => void | Promise } | null + getContainerDomNode: () => HTMLElement +} + +export function installMonacoEditorFindShortcut(editor: MonacoFindShortcutEditor): () => void { + return installEditorFindShortcut(editor.getContainerDomNode(), () => { + void editor.getAction('actions.find')?.run() + }) +}