feat(tui): add native composer keymap modes (standard/vim)#12497
feat(tui): add native composer keymap modes (standard/vim)#12497gh-xj wants to merge 2 commits intoopenai:mainfrom
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
There was a problem hiding this comment.
Pull request overview
This PR adds native Vim-style modal editing support to the TUI chat composer, allowing users to choose between standard (Emacs-style) and Vim keymaps. The feature is opt-in via config (tui.keymap = "vim") or runtime slash command (/keymap vim|standard), with standard mode as the default to maintain backward compatibility.
Changes:
- Added
TuiKeymapconfig enum withstandard(default) andvimvariants - Implemented vim modal editing in
TextAreawith normal/insert/replace-char modes supporting core motions (hjkl, w/b/e, 0/^/$), operators (d/c/y), and undo - Added
/keymapslash command for runtime switching between modes - Updated UI to show vim mode indicator in footer and change prompt glyph in normal mode
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| codex-rs/core/src/config/types.rs | Added TuiKeymap enum and keymap field to Tui config struct |
| codex-rs/core/src/config/mod.rs | Integrated tui_keymap into Config struct with tests for serialization and defaults |
| codex-rs/core/config.schema.json | Added TuiKeymap JSON schema definition and keymap field to Tui properties |
| codex-rs/tui/src/slash_command.rs | Registered Keymap slash command with metadata and tests |
| codex-rs/tui/src/chatwidget.rs | Implemented /keymap command handler and sync_vim_keymap_enabled method |
| codex-rs/tui/src/chatwidget/tests.rs | Added tests for /keymap command switching and error handling |
| codex-rs/tui/src/bottom_pane/mod.rs | Added set_vim_enabled method to forward vim mode changes to composer |
| codex-rs/tui/src/bottom_pane/chat_composer.rs | Integrated vim mode with paste-burst logic and added UI mode indicators |
| codex-rs/tui/src/bottom_pane/textarea.rs | Implemented core vim modal editing logic with modes, operators, motions, and undo stack |
| docs/tui-chat-composer.md | Documented vim mode behavior and /keymap command usage |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let next = self.next_atomic_boundary(self.cursor_pos); | ||
| self.replace_range(self.cursor_pos..next, &c.to_string()); | ||
| // Keep cursor on the replaced character. | ||
| self.set_cursor(self.cursor_pos.saturating_sub(c.len_utf8())); |
There was a problem hiding this comment.
The cursor positioning logic after character replacement may be incorrect. The replace_range method (line 249-250) already moves the cursor to start + inserted_len, which would be at the position after the replaced character. Then line 621 subtracts c.len_utf8() from that position. This double adjustment could lead to incorrect cursor placement, especially with multi-byte UTF-8 characters.
For standard Vim r behavior, the cursor should remain on the replaced character (not move). Consider setting the cursor to the original position before the replacement instead of adjusting after.
| let next = self.next_atomic_boundary(self.cursor_pos); | |
| self.replace_range(self.cursor_pos..next, &c.to_string()); | |
| // Keep cursor on the replaced character. | |
| self.set_cursor(self.cursor_pos.saturating_sub(c.len_utf8())); | |
| let original_cursor = self.cursor_pos; | |
| let next = self.next_atomic_boundary(self.cursor_pos); | |
| self.replace_range(self.cursor_pos..next, &c.to_string()); | |
| // Keep cursor on the replaced character (original position). | |
| self.set_cursor(original_cursor); |
There was a problem hiding this comment.
Addressed in a7c1c25. I now capture original_cursor before replace_range and restore that exact position after replacement, so r keeps the cursor on the replaced character without relying on byte-length subtraction.
| if matches!(event.code, KeyCode::Esc) { | ||
| self.vim_mode = VimMode::Normal; | ||
| self.vim_operator = None; | ||
| self.preferred_col = None; | ||
| return; |
There was a problem hiding this comment.
When exiting insert mode with Escape, standard Vim moves the cursor one position to the left (unless at the start of a line). This implementation doesn't adjust the cursor position when transitioning from insert to normal mode. Consider adding cursor adjustment to match standard Vim behavior: move left by one grapheme cluster unless already at the beginning of the line.
There was a problem hiding this comment.
Addressed in a7c1c25. Esc from insert mode now moves left by one atomic boundary when possible before returning to NORMAL mode, matching expected Vim behavior more closely.
| code: KeyCode::Char('u'), | ||
| modifiers: KeyModifiers::NONE, | ||
| .. | ||
| } => { | ||
| self.undo(); | ||
| } | ||
| KeyEvent { | ||
| code: KeyCode::Esc, .. | ||
| } => { | ||
| self.vim_operator = None; | ||
| } | ||
| _ => {} | ||
| } | ||
| } |
There was a problem hiding this comment.
The undo functionality (triggered by 'u' in normal mode) lacks test coverage. Consider adding tests for: basic undo after insertion, undo after deletion, undo after replacement, and undo stack limit (MAX_UNDO_ENTRIES). Also add test coverage for the 'r' (replace character) command.
There was a problem hiding this comment.
Addressed in a7c1c25. Added tests for replace and undo behavior: vim_replace_char_keeps_cursor_on_replaced_char, vim_u_undoes_replace, vim_u_undoes_delete_word, and undo-cap coverage via vim_undo_stack_is_capped.
| // Render vim mode indicator at the far left of the footer. | ||
| if let Some(label) = self.textarea.vim_mode_label() { | ||
| let vim_line = Line::from(format!("-- {label} --")).dim(); | ||
| let area = inset_footer_hint_area(hint_rect); | ||
| vim_line.render(area, buf); | ||
| } |
There was a problem hiding this comment.
The vim mode indicator (lines 3695-3700) is rendered after other footer content using the same area, which will cause it to overwrite any previously rendered left-aligned footer content (flash messages, footer hints, status line, etc.). This could hide important information from the user. Consider either: 1) rendering the vim indicator first so other content can overwrite it if needed, 2) allocating a dedicated space for the vim indicator, or 3) only rendering the vim indicator when other footer content is not present.
There was a problem hiding this comment.
Addressed in a7c1c25. I limited vim indicator rendering to the default footer path only, so it no longer overlays flash/status/hint-override content (the higher-priority footer states).
|
We've updated our contribution guidelines to indicate that we're no longer accepting unsolicited code contributions. All code contributions are by invitation only. To read more about why we've taken this step, please refer to this announcement. |
|
Opened a dedicated tracking issue for this feature request so discussion can proceed in policy-aligned form: This separates it from #2387 (external-editor If code PRs are not actionable under current policy, please treat #12497 as implementation reference only and continue product/UX discussion on #12508. |
What
standard), with opt-in Vim keymap.[tui] keymap = "standard" | "vim"(default:standard)/keymap vim/keymap standardWhy
standardas default avoids regressions for existing users and preserves backward compatibility.Scope
How
TuiKeymapconfig enum and wire it through runtime config:standard(default)vim/keymapcommand parsing with explicit usage errors.TextArea:NORMAL,INSERT,REPLACE-CHARh/j/k/l,w/b/e,0/^/$,d/c/y,x,p,u, etc.)yy/ddwithpHuman + AI Calibration Process
standarddefault,vimopt-in) before coding.yy+p/keymaparg handlingReviewer Notes
tui.keymap = "vim"is set or/keymap vimis used./keymapusage errors are explicit (Usage: /keymap standard|vim).