[codex] Support more Vim normal commands#25158
Conversation
jinghanx88
force-pushed
the
jinghanx/vim-mode-jump-top-bottom
branch
from
753a90f to
ce5e592
Compare
jinghanx88
changed the title
jinghanx88
force-pushed
the
jinghanx/vim-mode-jump-top-bottom
branch
4 times, most recently
from
32f66d6 to
2f6b50d
Compare
jinghanx88
force-pushed
the
jinghanx/vim-mode-jump-top-bottom
branch
2 times, most recently
from
159f9c3 to
3231e87
Compare
jinghanx88
force-pushed
the
jinghanx/vim-mode-jump-top-bottom
branch
from
3231e87 to
0dce6d6
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0dce6d6b30
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if self.vim_normal_keymap.move_line_end.is_pressed(event) { | ||
| self.set_cursor(self.vim_line_end_cursor()); | ||
| if self.vim_normal_keymap.repeat_last_edit.is_pressed(event) { | ||
| self.repeat_last_vim_edit(); |
There was a problem hiding this comment.
With the new . binding, a normal-mode D/delete_to_line_end edit is not repeatable because that handler deletes text but never updates vim_last_edit; pressing . after D therefore does nothing or replays an older edit, while the equivalent d$ path does record DeleteMotion(LineEnd). Please record this edit when the delete_to_line_end branch mutates text so repeat behaves consistently for the built-in Vim aliases.
Useful? React with 👍 / 👎.
| plain(KeyCode::Char('V')) | ||
| ], | ||
| undo: default_bindings![plain(KeyCode::Char('u'))], | ||
| redo: default_bindings![ctrl(KeyCode::Char('r'))], |
There was a problem hiding this comment.
This default redo binding is shadowed in the real composer flow: ChatComposer::handle_key_event checks history_search_previous_keys and opens reverse history search before dispatching keys to handle_key_event_without_popup/textarea.input, so pressing Ctrl-R in Vim normal mode never reaches TextArea::redo_last_undo. Either choose an unshadowed default or special-case Vim normal mode before the history-search shortcut so redo is usable outside direct textarea tests.
Useful? React with 👍 / 👎.
| (keymap.vim_normal.undo.as_ref(), vim_normal.undo.as_slice()), | ||
| (keymap.vim_normal.redo.as_ref(), vim_normal.redo.as_slice()), |
There was a problem hiding this comment.
When adding new defaults like undo = u, this preservation list decides which new defaults to drop for existing custom configs, but it omits the pre-existing substitute_char action. A user who already configured tui.keymap.vim_normal.substitute_char = ["u"] now gets a substitute_char/undo conflict instead of having the new undo default pruned, unlike the covered move-left/move-right compatibility cases.
Useful? React with 👍 / 👎.
| VimMode::Visual => self.handle_vim_visual(event), | ||
| VimMode::VisualLine => self.handle_vim_visual_line(event), |
There was a problem hiding this comment.
Once these visual modes are added, Up/Down arrows in visual selection can still be intercepted as composer history navigation because ChatComposer::handle_key_event_without_popup only uses the Vim-normal branch for is_vim_normal_mode(); in visual mode it falls through to the editor Up/Down history check, so on an empty draft or a recalled entry at a boundary the arrow navigates history instead of reaching handle_vim_visual/handle_vim_visual_line to extend the selection. Treat visual modes as modal for that history gate, or route their keys to the textarea before history navigation.
Useful? React with 👍 / 👎.
1 participant
Summary
gg/G,dG,yG,dgg, andcG/cgg.c{motion}for normal motions such ascw,c$, andcc, entering insert mode after deleting the target range.vand linewise visual mode withV, includingy,d/x, andc/sactions on the selected range.., multi-level undo withu, and redo withCtrl-R./keymap, and render visual/visual-line indicators without panicking.Test plan
just write-config-schemajust test -p codex-configjust test -p codex-tui ...coverage for Vim change motions, visual line mode, repeat, undo/redo, andgg/Goperator targets.just test -p codex-tui keymap_setup::testsafter accepting expected keymap action-count snapshots.cargo insta pending-snapshots --manifest-path tui/Cargo.tomlreports no pending snapshots.just test -p codex-tuipasses 2772 tests and still fails only the existing Guardian tests:app::tests::update_feature_flags_disabling_guardian_clears_manual_review_policy_without_history,app::tests::update_feature_flags_disabling_guardian_clears_review_policy_and_restores_default.just fix -p codex-tuigit diff --check