diff --git a/docs/editor-find-layout-aware-shortcut.md b/docs/editor-find-layout-aware-shortcut.md index fc2eae8642c..9f75390c176 100644 --- a/docs/editor-find-layout-aware-shortcut.md +++ b/docs/editor-find-layout-aware-shortcut.md @@ -1,34 +1,35 @@ -# Layout-aware find in the source editor +# 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. -The source editor currently installs Orca's layout-aware save shortcut through `editor-shortcuts.ts` (`src/renderer/src/components/editor/editor-shortcuts.ts:18`), but find is left entirely to Monaco's internal keycode dispatch (`src/renderer/src/components/editor/MonacoEditor.tsx:393`). 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`). +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 the Monaco source editor does 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. +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 source-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. In `MonacoEditor`, install that handler on Monaco's container and run Monaco's existing `actions.find` action. Dispose it from the editor's existing teardown callback. +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 Monaco container. +- 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. @@ -37,14 +38,14 @@ Monaco's built-in find keybinding follows the physical/virtual keycode delivered - 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 editor must not retain the listener. +- 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 layout-aware matching 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. +- 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. @@ -64,12 +65,12 @@ No new UI. The existing Monaco find widget must appear in its current position a 1. Add failing unit coverage for the layout-aware find installer contract. 2. Implement the installer in `editor-shortcuts.ts`. -3. Wire it to Monaco's existing find action and lifecycle cleanup. +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 source-editor shortcut installer, one Monaco hookup, and regression coverage; no new find implementation or global shortcut interception. +- 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. @@ -82,7 +83,7 @@ No new UI. The existing Monaco find widget must appear in its current position a - 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 source editor, doing a constant-time keybinding comparison only on keydown. It is removed with Monaco disposal; no polling, scans, IPC, storage, or render-loop work. +- 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. diff --git a/src/renderer/src/components/editor/DiffSectionItem.tsx b/src/renderer/src/components/editor/DiffSectionItem.tsx index 71fe21cf983..ffb1aa36b9b 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 7f58688754f..9aae498c1c1 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 d813e3ff90b..fc19ca04175 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 da83a73d7b9..d99af690af5 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 { installEditorFindShortcut, installEditorSaveShortcut } from './editor-shortcuts' +import { installEditorSaveShortcut, installMonacoEditorFindShortcut } from './editor-shortcuts' import { Plus } from 'lucide-react' import { getMonacoMarkdownSelectionAnnotationTarget, @@ -395,9 +395,7 @@ export default function MonacoEditor({ const value = editorInstance.getValue() propsRef.current.onSave(value) }) - const cleanupFindShortcut = installEditorFindShortcut(editorDomNode, () => { - void editorInstance.getAction('actions.find')?.run() - }) + const cleanupFindShortcut = installMonacoEditorFindShortcut(editorInstance) const searchInFilesAction = editorInstance.addAction({ id: 'orca.searchInFiles', label: translate('auto.components.editor.MonacoEditor.fd68ae03b3', 'Search in Files'), diff --git a/src/renderer/src/components/editor/editor-shortcuts.test.ts b/src/renderer/src/components/editor/editor-shortcuts.test.ts index 2fd5cb8e8d0..daacea7f647 100644 --- a/src/renderer/src/components/editor/editor-shortcuts.test.ts +++ b/src/renderer/src/components/editor/editor-shortcuts.test.ts @@ -17,7 +17,7 @@ vi.mock('@/store', () => ({ } })) -import { installEditorFindShortcut } from './editor-shortcuts' +import { installEditorFindShortcut, installMonacoEditorFindShortcut } from './editor-shortcuts' type ShortcutFixture = { container: HTMLDivElement @@ -176,4 +176,23 @@ describe('installEditorFindShortcut', () => { 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 2bc2a6cb515..86a7e721226 100644 --- a/src/renderer/src/components/editor/editor-shortcuts.ts +++ b/src/renderer/src/components/editor/editor-shortcuts.ts @@ -45,3 +45,14 @@ export function installEditorFindShortcut(target: HTMLElement, onFind: () => voi 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() + }) +}