feat(ui): add missing vim mode motions (X, ~, r, f/F/t/T, df/dt and friends)#21932
feat(ui): add missing vim mode motions (X, ~, r, f/F/t/T, df/dt and friends)#21932jacob314 merged 2 commits intogoogle-gemini:mainfrom
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Vim emulation by integrating a suite of essential normal mode commands. These additions provide users with more robust text manipulation capabilities, bringing the editor closer to a full Vim experience. The changes involve extending the text buffer's action types and logic, updating the Vim hook to process these new commands, and ensuring comprehensive test coverage for reliability. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
gemini-cli
bot
added
the
status/need-issue
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds several useful vim motions, along with comprehensive tests. The implementation is solid, but there are a couple of areas in packages/cli/src/ui/hooks/vim.ts with significant code duplication that could be refactored for better maintainability. I've provided suggestions to simplify the logic for handling pending find operations and for repeating find commands.
aanari
changed the title
aanari
force-pushed
the
feat/vim-tier1
branch
7 times, most recently
from
72c3bfe to
b1d09f1
Compare
|
Generated by `/review-frontend` running with Gemini 3.1 Pro, reviewed by Jacob. I have reviewed the frontend implementation for PR #21932 (`feat/vim-tier1`), focusing on the addition of the new Vim motions: `X`, `~`, `r`, `f`/`F`/`t`/`T`, and their operator combinations (`df`, `ct`, etc.). Review Summary
Current StatusI've fixed the bug locally and ran the `preflight` equivalence checks on the UI package (`npm run build`, `npm run lint`, and `npm run test` on the affected files). All 5,900+ workspace tests are passing. I also cleaned up an obsolete UI snapshot while running the tests. The workspace contains the uncommitted fixes for you to review and push. |
| @@ -35,6 +35,9 @@ const CMD_TYPES = { | |||
| CHANGE_BIG_WORD_BACKWARD: 'cB', | |||
| CHANGE_BIG_WORD_END: 'cE', | |||
| DELETE_CHAR: 'x', | |||
There was a problem hiding this comment.
not your fault but did notice that the behavior of DEETE_CHAR doesn't seem quite aligned with vim. could be nice to fix that as a follow up now that the DELETE_CHAR_BEFORE implementation is solid.
There was a problem hiding this comment.
Acked, will fix in follow-up.
packages/cli/src/ui/hooks/vim.ts
Outdated
| const { op, operator, count: findCount } = state.pendingFindOp; | ||
| dispatch({ type: 'SET_PENDING_FIND_OP', pendingFindOp: undefined }); | ||
| dispatch({ type: 'CLEAR_COUNT' }); | ||
| if (targetChar && targetChar.length === 1) { |
There was a problem hiding this comment.
only issue. this shouldn't be targetChar.length. you should
call
toCodePoints(targetChar) and check whether that is 1. Otherwise you will get confused about emojis.
There was a problem hiding this comment.
Great catch - updated!
jacob314
left a comment
jacob314
left a comment
There was a problem hiding this comment.
Approved with one edge case to fix.
String.length counts UTF-16 code units, not codepoints, so it returns 2 for emoji and other chars above U+FFFF. Use toCodePoints() to correctly accept exactly one logical character as the target.
|
Hi there! Thank you for your contribution to Gemini CLI. To improve our contribution process and better track changes, we now require all pull requests to be associated with an existing issue, as announced in our recent discussion and as detailed in our CONTRIBUTING.md. This pull request is being closed because it is not currently linked to an issue. Once you have updated the description of this PR to link an issue (e.g., by adding How to link an issue: Thank you for your understanding and for being a part of our community! |
gemini-cli
bot
added
area/core
In NORMAL mode the cursor can never rest past the last character of a line. Several delete commands left the cursor one position past the new line end after shrinking the line: x (vim_delete_char) dw / dW (vim_delete_word_forward / vim_delete_big_word_forward) de / dE (vim_delete_word_end / vim_delete_big_word_end) D (vim_delete_to_end_of_line) df / dt (vim_delete_to_char_forward) Added a clampNormalCursor() helper in vim-buffer-actions.ts and applied it to every delete action that stays in NORMAL mode. Change actions (cw, C, cf, ...) are intentionally excluded since they immediately enter INSERT mode where the cursor is allowed past the last character. Flagged by @jacob314 in review of google-gemini#21932.
In NORMAL mode the cursor can never rest past the last character of a line. Several delete commands left the cursor one position past the new line end after shrinking the line: x (vim_delete_char) dw / dW (vim_delete_word_forward / vim_delete_big_word_forward) de / dE (vim_delete_word_end / vim_delete_big_word_end) D (vim_delete_to_end_of_line) df / dt (vim_delete_to_char_forward) Added a clampNormalCursor() helper in vim-buffer-actions.ts and applied it to every delete action that stays in NORMAL mode. Change actions (cw, C, cf, ...) are intentionally excluded since they immediately enter INSERT mode where the cursor is allowed past the last character. Flagged by @jacob314 in review of google-gemini#21932.
In NORMAL mode the cursor can never rest past the last character of a line. Several delete commands left the cursor one position past the new line end after shrinking the line: x (vim_delete_char) dw / dW (vim_delete_word_forward / vim_delete_big_word_forward) de / dE (vim_delete_word_end / vim_delete_big_word_end) D (vim_delete_to_end_of_line) df / dt (vim_delete_to_char_forward) Added a clampNormalCursor() helper in vim-buffer-actions.ts and applied it to every delete action that stays in NORMAL mode. Change actions (cw, C, cf, ...) are intentionally excluded since they immediately enter INSERT mode where the cursor is allowed past the last character. Flagged by @jacob314 in review of google-gemini#21932.
2 participants
A handful of normal mode commands that were missing:
X— delete char before cursor~— toggle caser{char}— replace char under cursorf/F/t/T— find/till char on current line;/,— repeat last find forward/backwarddf/dt/dF/dT— delete to found char (operator + find)cf/ct/cF/cT— change to found char (delete + enter insert)Added tests for all new paths.
A step towards resolving #21970