Repository navigation
Conversation
|
Important Review skippedWe couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the
Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough
🚥 Pre-merge checks | ✅ 4 | ❌ 1
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/renderer/src/components/settings/EditorKeybindingsSetting.test.tsx (2)
17-51: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify test setup with
@testing-library/react.While the manual
createRootand DOM container management works well, you can eliminate this boilerplate and manualactwrapping by using@testing-library/react, which is the officially recommended testing approach for modern React. It also provides accessible queries that replace the customradio()helper.🛠️ Proposed refactor
-let root: Root | null = null -let container: HTMLDivElement | null = null - -afterEach(() => { - if (root) { - act(() => root?.unmount()) - } - container?.remove() - root = null - container = null -}) - -function renderSetting( - editorKeybindings: GlobalSettings['editorKeybindings'], - updateSettings = vi.fn() -) { - container = document.createElement('div') - document.body.appendChild(container) - root = createRoot(container) - act(() => { - root?.render( - <EditorKeybindingsSetting - settings={{ ...getDefaultSettings(join('test', 'home')), editorKeybindings }} - updateSettings={updateSettings} - /> - ) - }) - return { container, updateSettings } -} - -function radio(container: HTMLDivElement, label: string): HTMLButtonElement | undefined { - return [...container.querySelectorAll<HTMLButtonElement>('[role="radio"]')].find( - (button) => button.textContent === label - ) -}(You can then update the tests to use
render()andscreen.getByRole('radio', { name: 'Vim' })from@testing-library/react)
17-51: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider simplifying the test setup with
@testing-library/react.While the manual
createRoot,actwrapping, and DOM management correctly conform to React 19 API usage, this boilerplate can be significantly reduced by adopting@testing-library/react. It is the officially recommended approach for modern React testing and provides accessible queries that would replace the customradio()helper.For example, replacing the custom render and query logic with RTL would look like this:
import { render, screen, fireEvent } from '`@testing-library/react`' // Inside tests: render( <EditorKeybindingsSetting settings={{ ...getDefaultSettings(join('test', 'home')), editorKeybindings: 'default' }} updateSettings={updateSettings} /> ) fireEvent.click(screen.getByRole('radio', { name: 'Vim' }))
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 807fd82f-dbbb-4cde-9ccf-e51f2157617a
📥 Commits
Reviewing files that changed from the base of the PR and between 827cd49 and fd0e823c1e41e2cbeb641ed2f479a4f59b631906.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
package.jsonsrc/renderer/src/components/editor/MonacoEditor.tsxsrc/renderer/src/components/editor/monaco-vim-mode.tssrc/renderer/src/components/settings/EditorKeybindingsSetting.test.tsxsrc/renderer/src/components/settings/EditorKeybindingsSetting.tsxsrc/renderer/src/components/settings/GeneralEditorSettingsSection.tsxsrc/renderer/src/i18n/locales/en.jsonsrc/renderer/src/i18n/locales/es.jsonsrc/renderer/src/i18n/locales/ja.jsonsrc/renderer/src/i18n/locales/ko.jsonsrc/renderer/src/i18n/locales/zh.jsonsrc/shared/constants.test.tssrc/shared/constants.tssrc/shared/types.ts
fd0e823 to
8bf5d05
Compare
|
Thanks for the review! Addressed in the latest push (
|
|
+1 |
8bf5d05 to
691f4a3
Compare
|
+1 |
aa19359 to
6f16e24
Compare
6f16e24 to
bace026
Compare
|
Friendly bump 🙂 Just rebased this onto the latest Would really appreciate a review whenever a maintainer has a moment — happy to rebase again or make any changes. Thanks for the great project! 🙏 |
|
Have it released? |
|
+1 A feature I'm really looking forward to ❤️ |
|
So looking forward to this feature could be added to Orca. Would be more effective for me to manipulate the file. |
|
+1 would appreciate having this feature since I always use vim mode in editors. |
bace026 to
9c0c3f3
Compare
|
@nwparker — apologies for the direct tag; I've noticed you've been merging a lot of community PRs, so I'm hoping you might have a moment for this one 🙏 There's been steady demand for it — several 👍 and "really looking forward to this" comments here and on #9699, plus a detailed implementation spec in the issue that this PR already matches. It's a small, opt-in, off-by-default setting (Default / Vim), so it's low-risk for existing users. I've kept it rebased on the latest No rush at all, and I'm happy to make any changes you'd like. Thanks for the great project! |
9c0c3f3 to
6d9ae49
Compare
|
@brennanb2025 — I see you're the assigned reviewer here, so hoping it's OK to gently flag this 🙏 Just a friendly check-in since it's been a while. It's rebased on the latest No rush at all, and I'm happy to make any changes you'd like. Thanks for taking a look! |
|
+1 looking forward to this feature |
6d9ae49 to
9aae241
Compare
9aae241 to
f0d9210
Compare
f0d9210 to
849c640
Compare
|
CI fix update:
|
|
+1 - I would highly appreciate this feature! It is really one of the few things I am missing right now |
|
+1 🙏 |
|
+1 |
1 similar comment
|
+1 |
|
PLEASE -- this is the one thing standing in the way of Orca being perfect for my workflow. Everything is amazing and then I go to manually edit or review code and it feels like my fingers have been tied together. |
|
+1 |
1 similar comment
|
+1 |
535edb1 to
6612843
Compare
|
Pushed a follow-up that expands and documents how far the Vim preset actually handles. The first pass wired the basics; this pins down the real coverage, all verified end-to-end against the live editor. Added
Corrected the earlier limitation notes now that there is e2e evidence: registers, visual mode, macros, and |
|
+1 |
6612843 to
1354bc1
Compare
|
Rebased onto latest Conflicts resolved across:
Local checks pass: No functional changes from the last review pass — same Vim preset, cheatsheet, and |
Add an `editorKeybindings: 'default' | 'vim'` setting (default 'default') that enables monaco-vim modal editing in the built-in file editor, with a status bar and :w/:wq/:x wired to save. The toggle lives in Settings → Editor next to Word Wrap as a Default/Vim segmented control. monaco-vim loads lazily and is scoped to the editable, full-height editor (not read-only diffs or auto-height preview editors); the status bar is a real flex-column row below the editor rather than an overlay. The adapter disposes idempotently on toggle or unmount, and :w routes to the focused editor. It shares Orca's locally-bundled monaco instance via loader.config. monaco-vim is a keymap emulator, so LSP-backed :gd, :%s live preview, and macro replay @q are unsupported — documented as known limitations. Closes stablyai#9699.
…tsheet Build on the initial Vim keybindings preset with the app-integration and discoverability pieces the first pass left out: - Wire :q/:q! (close tab) and make :wq/:x save-and-close, routed through the focused editor's Orca save/close like :w already was. - Map editor-display :set options onto Monaco -- wrap/nowrap, number/nonumber, tabstop/shiftwidth/expandtab -- with :set paste/nopaste and unknown options as graceful no-ops (Monaco has no terminal paste-mangling to disable). - Add a command cheatsheet: a hint button in the Vim status bar opens a popover listing supported motions/operators/registers/visual/macro/Ex commands plus the documented limitations. - Correct the preset's limitation notes: registers, visual mode, macros, and :%s substitution are verified working (save/sync proven clean); the real gaps are gd, :%s live-preview, Backspace replay inside a macro, and monaco-vim's extra trailing blank line on linewise paste of a newline-terminated buffer.
Drive the real monaco-vim adapter in a headless Electron run: mode switching with i/Esc/o/dd, :w save-to-disk, yank/paste + :%s substitution + visual-mode delete, macro record/replay, and the :set line-number toggle + :q close. Also exercises the status-bar cheatsheet popover.
1354bc1 to
f7c1827
Compare

ELI5
The built-in file editor can use Vim keybindings via a setting. Default stays normal; turn on Vim for modal editing with a small mode status bar, without leaving Orca for an external editor.
What Changed
editorKeybindings: 'default' | 'vim'(defaults to'default', so existing users are unaffected).'vim'enables modal editing viamonaco-vim, with a status bar (mode + pending keystrokes), a command cheatsheet, and Ex commands wired to Orca::w(save),:q/:q!(close),:wq/:x(save & close), and:setfor editor options.Implementation notes
await import) and attaches to the same locally-bundledmonaco-editorinstance Orca configures vialoader.config({ monaco })— no CDN/instance mismatch, and the default keymap never loads the adapter.:wroutes to the focused editor).Why
Vim users can make quick manual edits with familiar modal keybindings while orchestrating agents — instead of dropping to an external editor.
Linked Issue
Closes #9699.
Visual Proof
Settings → Editor → Editor Keybindings (Default / Vim):

Vim

INSERTmode andNORMALmode with the status bar row below the editor:No "before" shot for the editor itself: Vim mode is a new opt-in and there is no visual change while the setting stays on
'default'. The "before" for Settings → Editor is the same panel without the new control.Vim command cheatsheet (the
⌨hint button in the status bar) — the handled commands at a glance:Command coverage
The first pass wired the basics; this follow-up pins down (and documents) exactly how far the keymap goes, verified end-to-end against the live editor:
yy/p), visual mode, macros (qa…q/@a), and:%s/…/…/gsubstitution.:w,:q/:q!,:wq/:x, routed to the focused editor's save/close.:setmapped onto Monaco:wrap/nowrap,number/nonumber,tabstop/shiftwidth/expandtab.:set paste/nopasteand unknown options are graceful no-ops (Monaco has no terminal paste-mangling to disable).⌨hint opens a cheatsheet popover listing the above plus the limitations below.Testing
I manually tested these changes locally
Automated tests added/updated, or explained why not below
EditorKeybindingsSetting.test.tsx— default/vim selection + persistence.constants.test.ts— default is'default'.vim-keybindings-file-editor.spec.ts— live headless-Electron e2e:i/Esc/o/dd+:wsave-to-disk, yank/paste +:%s+ visual delete, macro record/replay,:setline-number toggle +:qclose, and the cheatsheet popover.AI Disclosure
This PR was written with the help of an AI coding agent. All code was reviewed, tested, and is maintained by me (@xxjjvxb).
Review
SettingsSegmentedControl; status bar uses standard tokens (border-border,bg-editor-surface,text-muted-foreground), light/dark safe, and occupies real layout space.Agent skill upstream boundary
Not applicable — this change touches only the Monaco editor renderer and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.
Notes
Known limitations (by design) — monaco-vim is a keymap emulator layered on Orca's viewer-style Monaco, not Neovim. These are library/editor constraints, not regressions in this PR:
gd(go to definition) doesn't work — Orca intentionally disables Monaco's language intelligence (monaco-setup.ts:noSemanticValidation), so there's no definition provider for any keymap to call.:%s/…/…/gpreview — that's Neovim'sinccommand; monaco-vim runs the substitution without incremental preview.Backspacedeletions into the recording. (Earlier notes claimed macros didn't replay at all; the e2e confirms they do — this is the actual, narrower gap.)Users who need full Neovim (LSP, inccommand) should run it in the terminal; this feature covers in-editor motions/operators/registers/visual/macros/ex basics for quick edits.
i18n — new strings localized via
localize-renderer-strings.mjs; en + ko reviewed by me. ja/zh/es are best-effort seeds and would benefit from a native-speaker pass.X handle: @xxjjvxb
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)