(vim): Implement multi-register system and grammar parser #380
+2,822
−287
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit introduces a new multi-register system to store and retrieve content for named registers (a-z, 0-9, ", +, *, _, /). It also initializes default registers (unnamed, yank, delete, black hole, search pattern, system clipboard, selection clipboard). The legacy register is still updated for backward compatibility. Additionally, this commit implements a grammar-based parser system for Vim commands. This provides a more robust and extensible way to parse Vim commands, and supports future features. The grammar parser implementation includes: - A new Abstract Syntax Tree (AST) for representing commands. - A streaming incremental parser. - A compatibility layer that enables incremental migration between the old and new parser systems. - An executor to execute AST commands. This commit also introduces new types for working with the grammar parser.
mehmetozguldev
added
type: improvement
* Migrate components folder into respective feature domains * Add LLM rules file and create symlinks * Update documentation folder structure * Migrate types folder into vertical slice * Migrate models folder into types folders
This commit introduces a new multi-register system to store and retrieve content for named registers (a-z, 0-9, ", +, *, _, /). It also initializes default registers (unnamed, yank, delete, black hole, search pattern, system clipboard, selection clipboard). The legacy register is still updated for backward compatibility. Additionally, this commit implements a grammar-based parser system for Vim commands. This provides a more robust and extensible way to parse Vim commands, and supports future features. The grammar parser implementation includes: - A new Abstract Syntax Tree (AST) for representing commands. - A streaming incremental parser. - A compatibility layer that enables incremental migration between the old and new parser systems. - An executor to execute AST commands. This commit also introduces new types for working with the grammar parser.
WalkthroughThis PR adds a full grammar-based Vim subsystem: Trie token registry, AST types, streaming parser, normalization and motion-kind utilities, and an AST executor with a compatibility feature flag. It also expands vim state with multi-registers, last-repeat storage, paste API changes, persistent yank/delete registers, and VISUAL BLOCK support. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as User Input
participant Keyboard as use-vim-keyboard
participant Flag as Feature Flag
participant Parser as Grammar Parser
participant Executor as AST Executor
participant Store as Vim Store
participant Legacy as Legacy Handler
UI->>Keyboard: Key press
Keyboard->>Flag: isNewParserEnabled()?
alt New parser enabled
Keyboard->>Parser: parse(keys)
Parser-->>Keyboard: ParseResult (complete|incomplete|invalid|needsChar)
alt complete
Keyboard->>Executor: executeAST(command)
Executor->>Store: update registers, cursor, selection, lastRepeatable
Executor-->>Keyboard: success
Keyboard->>UI: render updates
else incomplete/needsChar
Keyboard->>UI: await more keys
else invalid
Keyboard->>UI: error/reset
end
else Legacy path
Keyboard->>Legacy: parse/execute legacy command
Legacy->>Store: update state
Legacy->>UI: render updates
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/vim/stores/vim-editing.ts (1)
184-203: "P" with characterwise registers now no-ops
pasteAbovereturns immediately wheneverpasteType !== "line", so a very common flow likeyw(characterwise yank) followed byPnow does nothing at all. In Vim,Pmust paste characterwise content before the cursor; with this guard in place the executor callspasteAbovebut it exits before touching the buffer, leaving the editor unchanged. Please branch the logic instead: keep the current linewise path, and add a characterwise path that inserts before the cursor (sharing the cursor/offset bookkeeping withpaste).
🧹 Nitpick comments (1)
src/features/vim/hooks/use-vim-keyboard.ts (1)
480-556: Drop verbose console logging from applyMotion.Lines 480-556: the
console.loginstrumentation fires on every visual motion and will spam the console in normal usage. Please remove it or wrap it behind an explicit debug flag so production builds stay clean.- console.log("=== applyMotion START ==="); - console.log("Motion keys:", motionKeys); - - const vimStoreBefore = useVimStore.getState(); - console.log("Before motion - visualSelection:", vimStoreBefore.visualSelection); - console.log("Before motion - cursor:", useEditorStateStore.getState().cursorPosition); - let success = false; if (isNewParserEnabled()) { const result = parseGrammar(motionKeys); if (result.status === "complete") { success = executeAST(result.command); } } else { success = executeVimCommand(motionKeys); } if (!success) { console.warn("Failed to execute visual motion:", motionKeys.join("")); return false; } const newPosition = useEditorStateStore.getState().cursorPosition; const lines = useEditorViewStore.getState().lines; const vimStore = useVimStore.getState(); - - console.log("After motion - cursor:", newPosition); - - - // Get fresh visual selection state const currentVisualSelection = vimStore.visualSelection; const currentVisualMode = vimStore.visualMode; - - console.log("After motion - visualSelection from store:", currentVisualSelection); if (currentVisualSelection.start) { // Line mode: always select full lines if (currentVisualMode === "line") { const newStart = { line: currentVisualSelection.start.line, column: 0 }; const newEnd = { line: newPosition.line, column: lines[newPosition.line].length }; - console.log("Setting line visual selection:", newStart, "to", newEnd); vimStore.actions.setVisualSelection(newStart, newEnd); } else { // Char/block mode: select from start to cursor const newEnd = { line: newPosition.line, column: newPosition.column }; - console.log( - "Setting char/block visual selection:", - currentVisualSelection.start, - "to", - newEnd, - ); vimStore.actions.setVisualSelection(currentVisualSelection.start, newEnd); } } // Update textarea selection const textarea = document.querySelector(".editor-textarea") as HTMLTextAreaElement; if (textarea && currentVisualSelection.start) { const startOffset = calculateOffsetFromPosition( currentVisualSelection.start.line, currentVisualSelection.start.column, lines, ); const endOffset = newPosition.offset; - - console.log("Textarea offsets - start:", startOffset, "end:", endOffset); - console.log( - "Setting textarea selection:", - Math.min(startOffset, endOffset), - "to", - Math.max(startOffset, endOffset), - ); - textarea.selectionStart = Math.min(startOffset, endOffset); textarea.selectionEnd = Math.max(startOffset, endOffset); // Don't dispatch select event - it causes cursor position to be overridden // textarea.dispatchEvent(new Event("select")); } - - console.log("=== applyMotion END ===\n");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/features/vim/components/vim-status-indicator.tsx(2 hunks)src/features/vim/core/core/grammar/ast.ts(1 hunks)src/features/vim/core/core/grammar/compat.ts(1 hunks)src/features/vim/core/core/grammar/executor.ts(1 hunks)src/features/vim/core/core/grammar/motion-kind.ts(1 hunks)src/features/vim/core/core/grammar/normalize.ts(1 hunks)src/features/vim/core/core/grammar/parser.ts(1 hunks)src/features/vim/core/core/grammar/tokens.ts(1 hunks)src/features/vim/core/core/grammar/trie.ts(1 hunks)src/features/vim/core/index.ts(3 hunks)src/features/vim/core/operators/delete-operator.ts(5 hunks)src/features/vim/core/operators/yank-operator.ts(3 hunks)src/features/vim/hooks/use-vim-keyboard.ts(12 hunks)src/features/vim/stores/vim-editing.ts(7 hunks)src/features/vim/stores/vim-store.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/features/vim/core/core/grammar/normalize.ts (1)
src/features/vim/core/core/grammar/ast.ts (1)
Command(29-60)
src/features/vim/core/core/grammar/executor.ts (2)
src/features/vim/core/core/grammar/ast.ts (2)
Command(29-60)Motion(128-134)src/features/vim/core/core/grammar/normalize.ts (4)
normalize(22-109)effectiveCount(125-144)getRegisterName(152-176)isRepeatable(184-220)
src/features/vim/core/core/grammar/parser.ts (2)
src/features/vim/core/core/grammar/ast.ts (5)
RegisterRef(22-24)Target(112-123)ParseResult(211-211)Command(29-60)Motion(128-134)src/features/vim/core/index.ts (5)
actions(161-161)operators(166-166)forcedKinds(163-163)isTextObjectKey(164-164)motions(165-165)
src/features/vim/hooks/use-vim-keyboard.ts (3)
src/features/vim/stores/vim-store.ts (1)
useVimStore(310-310)src/features/vim/core/index.ts (2)
isNewParserEnabled(144-144)executeVimCommand(113-113)src/features/vim/core/core/grammar/executor.ts (1)
executeAST(33-75)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: check format and cargo check
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/features/vim/stores/vim-store.ts (2)
42-42: Consider stronger typing for the AST command.The
anytype loses compile-time safety. If the grammar parser defines aCommandorASTNodetype, use that instead to catch misuse at build time.For example:
- lastRepeatableCommand: any | null; // Store the last Command AST for repeat (.) functionality + lastRepeatableCommand: Command | null; // Store the last Command AST for repeat (.) functionality
283-283: Simplify the legacy register sync condition.The
!namecheck is unnecessary sincenameis typed asstring(not nullable). Simplify to justname === '"'.- if (name === '"' || !name) { + if (name === '"') {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/features/vim/core/core/grammar/compat.ts(1 hunks)src/features/vim/core/core/grammar/executor.ts(1 hunks)src/features/vim/core/core/grammar/tokens.ts(1 hunks)src/features/vim/core/operators/delete-operator.ts(5 hunks)src/features/vim/core/operators/yank-operator.ts(3 hunks)src/features/vim/hooks/use-vim-keyboard.ts(12 hunks)src/features/vim/stores/vim-editing.ts(6 hunks)src/features/vim/stores/vim-store.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/features/vim/core/operators/yank-operator.ts
- src/features/vim/core/core/grammar/tokens.ts
🧰 Additional context used
🧬 Code graph analysis (5)
src/features/vim/core/operators/delete-operator.ts (2)
src/features/vim/stores/vim-store.ts (1)
useVimStore(316-316)src/features/editor/extensions/api.ts (1)
setCursorPosition(126-154)
src/features/vim/stores/vim-editing.ts (2)
src/features/editor/extensions/api.ts (4)
getCursorPosition(122-124)getLines(205-207)setCursorPosition(126-154)getContent(41-43)src/features/editor/utils/position.ts (1)
calculateOffsetFromPosition(37-55)
src/features/vim/core/core/grammar/executor.ts (6)
src/features/vim/core/core/grammar/ast.ts (2)
Command(29-60)Motion(128-134)src/features/vim/core/core/grammar/normalize.ts (4)
normalize(22-109)effectiveCount(125-144)getRegisterName(152-176)isRepeatable(184-220)src/features/vim/stores/vim-editing.ts (1)
createVimEditing(34-485)src/features/editor/utils/position.ts (1)
calculateOffsetFromPosition(37-55)src/features/editor/stores/view-store.ts (1)
useEditorViewStore(128-156)src/features/editor/stores/buffer-store.ts (1)
useBufferStore(144-648)
src/features/vim/core/core/grammar/compat.ts (1)
src/features/vim/core/core/grammar/executor.ts (1)
executeAST(33-75)
src/features/vim/hooks/use-vim-keyboard.ts (7)
src/features/vim/stores/vim-store.ts (1)
useVimStore(316-316)src/features/vim/core/core/grammar/compat.ts (1)
isNewParserEnabled(35-37)src/features/vim/core/core/grammar/executor.ts (1)
executeAST(33-75)src/features/vim/core/index.ts (1)
executeVimCommand(113-113)src/features/editor/extensions/api.ts (3)
getCursorPosition(122-124)getLines(205-207)setCursorPosition(126-154)src/features/editor/utils/position.ts (1)
calculateOffsetFromPosition(37-55)src/features/editor/stores/view-store.ts (1)
useEditorViewStore(128-156)
🔇 Additional comments (6)
src/features/vim/stores/vim-store.ts (6)
9-15: LGTM!The
RegisterContentinterface cleanly models Vim register content with appropriate type constraints.
62-75: LGTM!The multi-register initialization correctly sets up standard Vim registers with sensible defaults.
192-196: LGTM!The updated signature correctly supports VISUAL BLOCK mode.
290-293: LGTM!The fallback to an empty register is a safe default for non-existent registers.
301-304: LGTM!Defaulting to the unnamed register matches Vim's behavior when no register is explicitly selected.
262-288: All callers properly pass the correct source option — verified.The implementation correctly addresses the past review feedback. All yank operations pass
source: "yank"and all delete operations passsource: "delete". This ensures the yank register (0) is only updated for explicit yanks, preventing delete operations from polluting it.
Comment on lines
+214
to
+227
| // Move cursor to last character of pasted content | ||
| const newOffset = currentPos.offset + pasteContent.length - 1; | ||
| const newLines = newContent.split("\n"); | ||
| let line = 0; | ||
| let offset = 0; | ||
|
|
||
| while (offset + newLines[line].length + 1 <= newOffset && line < newLines.length - 1) { | ||
| offset += newLines[line].length + 1; | ||
| line++; | ||
| } | ||
|
|
||
| const column = newOffset - offset; | ||
| const newPosition = { line, column, offset: newOffset }; | ||
| setCursorPosition(newPosition); |
There was a problem hiding this comment.
Keep the P cursor on the first inserted character
For charwise P, Vim leaves the caret on the first character of the inserted text. The new offset calculation adds pasteContent.length - 1, so multi-character inserts now park the cursor on the last character instead. That breaks parity with real Vim and makes follow-up motions land in the wrong place. Please keep the offset at the original cursor position so P behaves correctly.
- const newOffset = currentPos.offset + pasteContent.length - 1;
+ const newOffset = currentPos.offset;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Move cursor to last character of pasted content | |
| const newOffset = currentPos.offset + pasteContent.length - 1; | |
| const newLines = newContent.split("\n"); | |
| let line = 0; | |
| let offset = 0; | |
| while (offset + newLines[line].length + 1 <= newOffset && line < newLines.length - 1) { | |
| offset += newLines[line].length + 1; | |
| line++; | |
| } | |
| const column = newOffset - offset; | |
| const newPosition = { line, column, offset: newOffset }; | |
| setCursorPosition(newPosition); | |
| // Move cursor to last character of pasted content | |
| const newOffset = currentPos.offset; | |
| const newLines = newContent.split("\n"); | |
| let line = 0; | |
| let offset = 0; | |
| while (offset + newLines[line].length + 1 <= newOffset && line < newLines.length - 1) { | |
| offset += newLines[line].length + 1; | |
| line++; | |
| } | |
| const column = newOffset - offset; | |
| const newPosition = { line, column, offset: newOffset }; | |
| setCursorPosition(newPosition); |
🤖 Prompt for AI Agents
In src/features/vim/stores/vim-editing.ts around lines 214-227, the code moves
the caret to currentPos.offset + pasteContent.length - 1 which leaves the cursor
on the last pasted character; for charwise P Vim expects the caret on the first
inserted character. Replace the newOffset calculation to use the original
insertion start (e.g., newOffset = currentPos.offset) so the subsequent
line/column computation and setCursorPosition keep the caret on the first
inserted character.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit introduces a new multi-register system to store and retrieve content for named registers (a-z, 0-9, ", +, *, _, /). It also initializes default registers (unnamed, yank, delete, black hole, search pattern, system clipboard, selection clipboard). The legacy register is still updated for backward compatibility.
Additionally, this commit implements a grammar-based parser system for Vim commands. This provides a more robust and extensible way to parse Vim commands, and supports future features.
The grammar parser implementation includes:
This commit also introduces new types for working with the grammar parser.
Summary by CodeRabbit
New Features
Improvements