From f2e7b462a932c654f8c9810387a73c12bd4e198f Mon Sep 17 00:00:00 2001 From: Jinghan Xu Date: Fri, 29 May 2026 14:01:27 -0700 Subject: [PATCH] [codex] Fix Vim normal mode editing (#25022) ## Summary - add Vim normal-mode `s` support to substitute the character under the cursor and enter insert mode - fix Vim normal-mode `o` so opening below the final line moves the cursor onto the new blank line - update keymap config/schema and keymap picker snapshots for the new action ## Validation - `just fmt` - `just write-config-schema` - `just test -p codex-config` - focused `just test -p codex-tui` coverage for the Vim `s` and `o` behavior, keymap conflict handling, and keymap picker snapshots - `cargo insta pending-snapshots --manifest-path tui/Cargo.toml` - `git diff --check` ## Notes A full `just test -p codex-tui` run still has two unrelated Guardian feature-flag failures in this checkout: - `app::tests::update_feature_flags_disabling_guardian_clears_review_policy_and_restores_default` - `app::tests::update_feature_flags_disabling_guardian_clears_manual_review_policy_without_history` --- codex-rs/config/src/tui_keymap.rs | 2 + codex-rs/core/config.schema.json | 10 ++++ codex-rs/tui/src/bottom_pane/textarea.rs | 57 ++++++++++++++++++- codex-rs/tui/src/keymap.rs | 43 ++++++++++++++ codex-rs/tui/src/keymap_setup/actions.rs | 3 + ...ap_setup__tests__keymap_picker_custom.snap | 2 +- ...ests__keymap_picker_fast_mode_enabled.snap | 2 +- ...p__tests__keymap_picker_first_actions.snap | 4 +- ...ap_setup__tests__keymap_picker_narrow.snap | 2 +- ...ymap_setup__tests__keymap_picker_wide.snap | 2 +- 10 files changed, 119 insertions(+), 8 deletions(-) diff --git a/codex-rs/config/src/tui_keymap.rs b/codex-rs/config/src/tui_keymap.rs index e063cac0e..99cb3e50f 100644 --- a/codex-rs/config/src/tui_keymap.rs +++ b/codex-rs/config/src/tui_keymap.rs @@ -223,6 +223,8 @@ pub struct TuiVimNormalKeymap { pub move_line_end: Option, /// Delete character under cursor (`x`). pub delete_char: Option, + /// Delete character under cursor and enter insert mode (`s`). + pub substitute_char: Option, /// Delete from cursor to end of line (`D`). pub delete_to_line_end: Option, /// Change from cursor to end of line and enter insert mode (`C`). diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index b7bdff813..2d7fd818c 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -2858,6 +2858,7 @@ "start_change_operator": null, "start_delete_operator": null, "start_yank_operator": null, + "substitute_char": null, "yank_line": null }, "vim_operator": { @@ -3548,6 +3549,7 @@ "start_change_operator": null, "start_delete_operator": null, "start_yank_operator": null, + "substitute_char": null, "yank_line": null } }, @@ -3975,6 +3977,14 @@ ], "description": "Begin yank operator; next key selects motion (`y`)." }, + "substitute_char": { + "allOf": [ + { + "$ref": "#/definitions/KeybindingsSpec" + } + ], + "description": "Delete character under cursor and enter insert mode (`s`)." + }, "yank_line": { "allOf": [ { diff --git a/codex-rs/tui/src/bottom_pane/textarea.rs b/codex-rs/tui/src/bottom_pane/textarea.rs index 39da5752c..f67c3f878 100644 --- a/codex-rs/tui/src/bottom_pane/textarea.rs +++ b/codex-rs/tui/src/bottom_pane/textarea.rs @@ -677,9 +677,10 @@ impl TextArea { } if self.vim_normal_keymap.open_line_below.is_pressed(event) { let eol = self.end_of_current_line(); - let insert_at = if eol < self.text.len() { eol + 1 } else { eol }; + let old_len = self.text.len(); + let insert_at = if eol < old_len { eol + 1 } else { eol }; self.insert_str_at(insert_at, "\n"); - let cursor = if eol < self.text.len() { + let cursor = if eol < old_len { insert_at } else { insert_at + 1 @@ -735,6 +736,13 @@ impl TextArea { self.delete_forward_kill(/*n*/ 1); return; } + if self.vim_normal_keymap.substitute_char.is_pressed(event) { + if self.cursor_pos < self.end_of_current_line() { + self.delete_forward_kill(/*n*/ 1); + } + self.vim_mode = VimMode::Insert; + return; + } if self.vim_normal_keymap.delete_to_line_end.is_pressed(event) { self.vim_kill_to_end_of_line(); return; @@ -2360,6 +2368,38 @@ mod tests { assert_eq!(t.cursor(), 6); } + #[test] + fn vim_s_substitutes_current_character_and_enters_insert_mode() { + let mut t = ta_with("abc"); + t.set_cursor(/*pos*/ 1); + t.set_vim_enabled(/*enabled*/ true); + + t.input(KeyEvent::new(KeyCode::Char('s'), KeyModifiers::NONE)); + + assert_eq!(t.text(), "ac"); + assert_eq!(t.cursor(), 1); + assert_eq!(t.vim_mode_label(), Some("Insert")); + + t.input(KeyEvent::new(KeyCode::Char('X'), KeyModifiers::NONE)); + + assert_eq!(t.text(), "aXc"); + assert_eq!(t.cursor(), 2); + assert_eq!(t.vim_mode_label(), Some("Insert")); + } + + #[test] + fn vim_s_on_empty_line_enters_insert_without_deleting_newline() { + let mut t = ta_with("before\n\nnext"); + t.set_cursor(/*pos*/ "before\n".len()); + t.set_vim_enabled(/*enabled*/ true); + + t.input(KeyEvent::new(KeyCode::Char('s'), KeyModifiers::NONE)); + + assert_eq!(t.text(), "before\n\nnext"); + assert_eq!(t.cursor(), "before\n".len()); + assert_eq!(t.vim_mode_label(), Some("Insert")); + } + #[test] fn vim_d_at_line_end_does_not_remove_newline() { let mut t = ta_with("hello\nworld"); @@ -2414,6 +2454,19 @@ mod tests { assert_eq!(t.cursor(), "one\n".len()); } + #[test] + fn vim_o_opens_line_below_final_line_and_moves_to_new_line() { + let mut t = ta_with("one"); + t.set_cursor(/*pos*/ 1); + t.set_vim_enabled(/*enabled*/ true); + + t.input(KeyEvent::new(KeyCode::Char('o'), KeyModifiers::NONE)); + + assert_eq!(t.text(), "one\n"); + assert_eq!(t.vim_mode_label(), Some("Insert")); + assert_eq!(t.cursor(), "one\n".len()); + } + #[test] fn vim_delete_word() { let mut t = ta_with("hello world"); diff --git a/codex-rs/tui/src/keymap.rs b/codex-rs/tui/src/keymap.rs index 7bb554a25..9a656d846 100644 --- a/codex-rs/tui/src/keymap.rs +++ b/codex-rs/tui/src/keymap.rs @@ -158,6 +158,7 @@ pub(crate) struct VimNormalKeymap { pub(crate) move_line_start: Vec, pub(crate) move_line_end: Vec, pub(crate) delete_char: Vec, + pub(crate) substitute_char: Vec, pub(crate) delete_to_line_end: Vec, pub(crate) change_to_line_end: Vec, pub(crate) yank_line: Vec, @@ -494,6 +495,7 @@ impl RuntimeKeymap { move_line_start: resolve_local!(keymap, defaults, vim_normal, move_line_start), move_line_end: resolve_local!(keymap, defaults, vim_normal, move_line_end), delete_char: resolve_local!(keymap, defaults, vim_normal, delete_char), + substitute_char: resolve_local!(keymap, defaults, vim_normal, substitute_char), delete_to_line_end: resolve_local!(keymap, defaults, vim_normal, delete_to_line_end), change_to_line_end: resolve_local!(keymap, defaults, vim_normal, change_to_line_end), yank_line: resolve_local!(keymap, defaults, vim_normal, yank_line), @@ -579,6 +581,10 @@ impl RuntimeKeymap { keymap.vim_normal.delete_char.as_ref(), vim_normal.delete_char.as_slice(), ), + ( + keymap.vim_normal.change_to_line_end.as_ref(), + vim_normal.change_to_line_end.as_slice(), + ), ( keymap.vim_normal.delete_to_line_end.as_ref(), vim_normal.delete_to_line_end.as_slice(), @@ -599,6 +605,10 @@ impl RuntimeKeymap { keymap.vim_normal.start_yank_operator.as_ref(), vim_normal.start_yank_operator.as_slice(), ), + ( + keymap.vim_normal.start_change_operator.as_ref(), + vim_normal.start_change_operator.as_slice(), + ), ( keymap.vim_normal.cancel_operator.as_ref(), vim_normal.cancel_operator.as_slice(), @@ -610,6 +620,11 @@ impl RuntimeKeymap { .start_change_operator .retain(|binding| !configured_vim_normal_bindings_to_preserve.contains(binding)); } + if keymap.vim_normal.substitute_char.is_none() { + vim_normal + .substitute_char + .retain(|binding| !configured_vim_normal_bindings_to_preserve.contains(binding)); + } let mut vim_operator = VimOperatorKeymap { delete_line: resolve_local!(keymap, defaults, vim_operator, delete_line), @@ -990,6 +1005,7 @@ impl RuntimeKeymap { shift(KeyCode::Char('$')) ], delete_char: default_bindings![plain(KeyCode::Char('x'))], + substitute_char: default_bindings![plain(KeyCode::Char('s'))], delete_to_line_end: default_bindings![ shift(KeyCode::Char('d')), plain(KeyCode::Char('D')) @@ -1434,6 +1450,10 @@ impl RuntimeKeymap { ), ("move_line_end", self.vim_normal.move_line_end.as_slice()), ("delete_char", self.vim_normal.delete_char.as_slice()), + ( + "substitute_char", + self.vim_normal.substitute_char.as_slice(), + ), ( "delete_to_line_end", self.vim_normal.delete_to_line_end.as_slice(), @@ -2311,6 +2331,29 @@ mod tests { expect_conflict(&keymap, "move_left", "start_change_operator"); } + #[test] + fn configured_legacy_vim_normal_bindings_prune_new_substitute_default() { + let mut keymap = TuiKeymap::default(); + keymap.vim_normal.move_left = Some(one("s")); + + let runtime = RuntimeKeymap::from_config(&keymap).expect("config should parse"); + + assert_eq!( + runtime.vim_normal.move_left, + vec![key_hint::plain(KeyCode::Char('s'))] + ); + assert_eq!(runtime.vim_normal.substitute_char, Vec::new()); + } + + #[test] + fn explicit_new_vim_normal_substitute_binding_still_conflicts_with_legacy_binding() { + let mut keymap = TuiKeymap::default(); + keymap.vim_normal.move_left = Some(one("s")); + keymap.vim_normal.substitute_char = Some(one("s")); + + expect_conflict(&keymap, "move_left", "substitute_char"); + } + #[test] fn configured_legacy_vim_operator_bindings_prune_new_text_object_defaults() { let mut keymap = TuiKeymap::default(); diff --git a/codex-rs/tui/src/keymap_setup/actions.rs b/codex-rs/tui/src/keymap_setup/actions.rs index 87d65b225..b355cda69 100644 --- a/codex-rs/tui/src/keymap_setup/actions.rs +++ b/codex-rs/tui/src/keymap_setup/actions.rs @@ -135,6 +135,7 @@ pub(super) const KEYMAP_ACTIONS: &[KeymapActionDescriptor] = &[ action("vim_normal", "Vim normal", "move_line_start", "Move to the start of the line."), action("vim_normal", "Vim normal", "move_line_end", "Move to the end of the line."), action("vim_normal", "Vim normal", "delete_char", "Delete the character under the cursor."), + action("vim_normal", "Vim normal", "substitute_char", "Delete the character under the cursor and enter insert mode."), action("vim_normal", "Vim normal", "delete_to_line_end", "Delete from cursor to end of line."), action("vim_normal", "Vim normal", "change_to_line_end", "Change from cursor to end of line and enter insert mode."), action("vim_normal", "Vim normal", "yank_line", "Yank the entire line."), @@ -277,6 +278,7 @@ pub(super) fn binding_slot<'a>( ("vim_normal", "move_line_start") => Some(&mut keymap.vim_normal.move_line_start), ("vim_normal", "move_line_end") => Some(&mut keymap.vim_normal.move_line_end), ("vim_normal", "delete_char") => Some(&mut keymap.vim_normal.delete_char), + ("vim_normal", "substitute_char") => Some(&mut keymap.vim_normal.substitute_char), ("vim_normal", "delete_to_line_end") => Some(&mut keymap.vim_normal.delete_to_line_end), ("vim_normal", "change_to_line_end") => Some(&mut keymap.vim_normal.change_to_line_end), ("vim_normal", "yank_line") => Some(&mut keymap.vim_normal.yank_line), @@ -401,6 +403,7 @@ pub(super) fn bindings_for_action<'a>( ("vim_normal", "move_line_start") => Some(runtime_keymap.vim_normal.move_line_start.as_slice()), ("vim_normal", "move_line_end") => Some(runtime_keymap.vim_normal.move_line_end.as_slice()), ("vim_normal", "delete_char") => Some(runtime_keymap.vim_normal.delete_char.as_slice()), + ("vim_normal", "substitute_char") => Some(runtime_keymap.vim_normal.substitute_char.as_slice()), ("vim_normal", "delete_to_line_end") => Some(runtime_keymap.vim_normal.delete_to_line_end.as_slice()), ("vim_normal", "change_to_line_end") => Some(runtime_keymap.vim_normal.change_to_line_end.as_slice()), ("vim_normal", "yank_line") => Some(runtime_keymap.vim_normal.yank_line.as_slice()), diff --git a/codex-rs/tui/src/snapshots/codex_tui__keymap_setup__tests__keymap_picker_custom.snap b/codex-rs/tui/src/snapshots/codex_tui__keymap_setup__tests__keymap_picker_custom.snap index 6c0d008aa..7969d461e 100644 --- a/codex-rs/tui/src/snapshots/codex_tui__keymap_setup__tests__keymap_picker_custom.snap +++ b/codex-rs/tui/src/snapshots/codex_tui__keymap_setup__tests__keymap_picker_custom.snap @@ -5,7 +5,7 @@ expression: "render_picker(params, 120)" Keymap All configurable shortcuts. - 107 actions, 1 customized, 2 unbound. + 108 actions, 1 customized, 2 unbound. [All] Common Customized (1) Unbound (2) App Composer Editor Vim Navigation Approval Debug diff --git a/codex-rs/tui/src/snapshots/codex_tui__keymap_setup__tests__keymap_picker_fast_mode_enabled.snap b/codex-rs/tui/src/snapshots/codex_tui__keymap_setup__tests__keymap_picker_fast_mode_enabled.snap index b7384d1c4..343815833 100644 --- a/codex-rs/tui/src/snapshots/codex_tui__keymap_setup__tests__keymap_picker_fast_mode_enabled.snap +++ b/codex-rs/tui/src/snapshots/codex_tui__keymap_setup__tests__keymap_picker_fast_mode_enabled.snap @@ -5,7 +5,7 @@ expression: "render_picker(params, 120)" Keymap All configurable shortcuts. - 108 actions, 0 customized, 3 unbound. + 109 actions, 0 customized, 3 unbound. [All] Common Customized (0) Unbound (3) App Composer Editor Vim Navigation Approval Debug diff --git a/codex-rs/tui/src/snapshots/codex_tui__keymap_setup__tests__keymap_picker_first_actions.snap b/codex-rs/tui/src/snapshots/codex_tui__keymap_setup__tests__keymap_picker_first_actions.snap index 53ccba7e3..4bb6c4dbf 100644 --- a/codex-rs/tui/src/snapshots/codex_tui__keymap_setup__tests__keymap_picker_first_actions.snap +++ b/codex-rs/tui/src/snapshots/codex_tui__keymap_setup__tests__keymap_picker_first_actions.snap @@ -2,14 +2,14 @@ source: tui/src/keymap_setup.rs expression: snapshot --- -tab: All (107 selectable) +tab: All (108 selectable) tab: Common (20 selectable) tab: Customized (0) (0 selectable) tab: Unbound (2) (2 selectable) tab: App (10 selectable) tab: Composer (5 selectable) tab: Editor (17 selectable) -tab: Vim (47 selectable) +tab: Vim (48 selectable) tab: Navigation (20 selectable) tab: Approval (8 selectable) tab: Debug (1 selectable) diff --git a/codex-rs/tui/src/snapshots/codex_tui__keymap_setup__tests__keymap_picker_narrow.snap b/codex-rs/tui/src/snapshots/codex_tui__keymap_setup__tests__keymap_picker_narrow.snap index 3b9ebb7a6..a4a65e332 100644 --- a/codex-rs/tui/src/snapshots/codex_tui__keymap_setup__tests__keymap_picker_narrow.snap +++ b/codex-rs/tui/src/snapshots/codex_tui__keymap_setup__tests__keymap_picker_narrow.snap @@ -5,7 +5,7 @@ expression: "render_picker(params, 78)" Keymap All configurable shortcuts. - 107 actions, 0 customized, 2 unbound. + 108 actions, 0 customized, 2 unbound. [All] Common Customized (0) Unbound (2) App Composer Editor Vim Navigation Approval Debug diff --git a/codex-rs/tui/src/snapshots/codex_tui__keymap_setup__tests__keymap_picker_wide.snap b/codex-rs/tui/src/snapshots/codex_tui__keymap_setup__tests__keymap_picker_wide.snap index c5efeb30e..1138ebbf1 100644 --- a/codex-rs/tui/src/snapshots/codex_tui__keymap_setup__tests__keymap_picker_wide.snap +++ b/codex-rs/tui/src/snapshots/codex_tui__keymap_setup__tests__keymap_picker_wide.snap @@ -5,7 +5,7 @@ expression: "render_picker(params, 120)" Keymap All configurable shortcuts. - 107 actions, 0 customized, 2 unbound. + 108 actions, 0 customized, 2 unbound. [All] Common Customized (0) Unbound (2) App Composer Editor Vim Navigation Approval Debug