[FEATURE] Add opt-in Vim Mode to the search bar#4541
Open
namefailed wants to merge 32 commits into
Open
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 10 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a full opt-in Vim editing mode to the Flow Launcher search bar. The implementation introduces ChangesVim Mode feature
Sequence DiagramsequenceDiagram
participant User
participant MainWindow
participant VimManager
participant VimEngine
participant VimMotionEngine
participant QueryTextBox
User->>MainWindow: KeyDown event
MainWindow->>VimManager: HandlePreviewKeyDown(e)
VimManager->>VimManager: Vim mode enabled? Modifier pass-through?
alt Escape in Insert mode
VimManager->>VimEngine: SwitchToNormal()
VimEngine-->>VimManager: ModeChanged(Normal)
VimManager->>QueryTextBox: show VimBlockCaret overlay
else Normal mode motion (h/l/w/b/e)
VimManager->>VimMotionEngine: compute target index
VimMotionEngine-->>VimManager: target position
VimManager->>QueryTextBox: CaretIndex = target
else Operator (d/c/y) + motion
VimManager->>VimMotionEngine: OperatorRange(caret, target, inclusivity)
VimMotionEngine-->>VimManager: (start, end)
VimManager->>QueryTextBox: delete/yank text in range
else Enter Insert mode (i/a)
VimManager->>VimEngine: SwitchToInsert()
VimEngine-->>VimManager: ModeChanged(Insert)
VimManager->>QueryTextBox: hide VimBlockCaret overlay
end
VimManager-->>MainWindow: handled = true
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Flow.Launcher.Infrastructure/UserSettings/Settings.cs`:
- Around line 62-63: The default value of the _enableVimMode private field is
initialized to true, which contradicts the opt-in contract for this feature.
Change the initialization of _enableVimMode from true to false so that Vim mode
is disabled by default and users must explicitly opt-in to enable it.
- Around line 63-74: When EnableVimMode is disabled, the UI state is not fully
restored because HandlePreviewKeyDown only hides the vim block caret, leaving
InputMethod suspended and CaretBrush transparent. Add a property changed handler
in ViewModel_PropertyChanged that detects when the EnableVimMode property
changes to false and calls ApplyModeUI(VimModes.Insert) to properly restore both
the InputMethod state and the native caret brush appearance.
In `@Flow.Launcher/SettingPages/Views/SettingsPaneGeneral.xaml`:
- Around line 87-88: The Description and Header properties in the Vim
keybindings setting contain hardcoded English text instead of using
DynamicResource bindings like adjacent settings do, which prevents localization.
Replace both the hardcoded Description string and Header string with
DynamicResource bindings that reference localized resources, then add the
corresponding string entries to the appropriate resource dictionary files to
support multiple languages.
In `@Flow.Launcher/VimMode/VimManager.cs`:
- Around line 147-153: When Vim mode is disabled (in the branch where
!_settings.EnableVimMode is true), the code currently only hides the
_vimBlockCaret visual overlay but fails to reset the _vimEngine.CurrentMode from
Normal mode back to Insert mode. This causes PreviewTextInput and IsInputBlocked
to continue blocking native typing even though the setting is disabled. After
hiding the _vimBlockCaret visibility, reset _vimEngine.CurrentMode to Insert
mode to restore normal input behavior and ensure the input blocking is properly
disabled. Apply the same fix to all locations where Vim mode is disabled
(including the location noted at lines 552-560).
- Around line 375-383: The yank command (Y) is setting _pendingCommand to "y"
but ExecuteMotion method does not handle "y" as an operator like it does for "d"
and "c" operators. Update the ExecuteMotion method to recognize "y" as a valid
operator command and implement the yank functionality to copy the selected text
to the clipboard when the motion is executed, ensuring it behaves consistently
with the delete and change operators. Also apply the same fix to the
corresponding location mentioned at lines 475-500.
- Around line 243-248: When handling unrelated vim commands like Key.J
(SelectNextItemCommand) and Key.K (SelectPrevItemCommand) in the switch
statement, you need to reset the _pendingCommand field after executing these
commands. This prevents stale operators from previous delete, change, or yank
operations from remaining armed and causing unexpected behavior when a later
motion is executed. Apply this same reset pattern to all other unrelated command
cases mentioned at lines 386-406 and 428-431 to ensure _pendingCommand is
cleared whenever a non-motion command is executed after a pending operator.
- Around line 187-199: The issue is that when character commands like r, f, F,
t, or T are initiated, pressing Escape does not clear the _awaitingCharCommand
flag, causing the next Normal-mode key press to be incorrectly consumed as the
character argument instead of being treated as a new command. Fix this by
setting _awaitingCharCommand to false in the Escape key handling block within
HandleVimKey method (and in the related Escape handling section noted in the
"Also applies to" comment) to properly cancel the pending character command
state when the user presses Escape.
In `@Flow.Launcher/VimMode/VimMotionEngine.cs`:
- Around line 113-123: The MoveEndWord method incorrectly stops at trailing
whitespace when there is no next word available. After the initial whitespace
skip and word character classification, the condition in the final while loop
allows the index to advance into whitespace areas. Fix this by adding an
additional check after the main loop to skip any remaining trailing whitespace,
ensuring the method returns the actual end of the word or stays at the current
position if only whitespace follows. Additionally, add a regression test in
VimMotionEngineTest.cs that verifies MoveEndWord correctly handles the case
where the caret is at a word end with only trailing spaces remaining (e.g.,
input "abc " at position 2 should not advance into the spaces).
In `@README.md`:
- Line 5: The markdown heading on line 5 with text "Why does this exist?" is at
heading level 3 (###) but the previous heading level is 1 (#), which violates
the MD001 rule requiring incremental heading levels. Change the heading from ###
to ## to properly increment the heading level by 1 instead of jumping from level
1 to level 3.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 61074e2b-047d-4425-b77a-608ae6b69327
📒 Files selected for processing (10)
Flow.Launcher.Infrastructure/UserSettings/Settings.csFlow.Launcher.Test/VimMotionEngineTest.csFlow.Launcher/MainWindow.xamlFlow.Launcher/MainWindow.xaml.csFlow.Launcher/SettingPages/Views/SettingsPaneGeneral.xamlFlow.Launcher/VimMode/VimEngine.csFlow.Launcher/VimMode/VimManager.csFlow.Launcher/VimMode/VimMotionEngine.csFlow.Launcher/packages.lock.jsonREADME.md
Comment on lines
+63
to
+74
| public bool EnableVimMode | ||
| { | ||
| get => _enableVimMode; | ||
| set | ||
| { | ||
| if (_enableVimMode != value) | ||
| { | ||
| _enableVimMode = value; | ||
| OnPropertyChanged(); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
vim_file="$(fd -i '^VimManager\.cs$' | head -n1)"
if [ -z "${vim_file}" ]; then
echo "VimManager.cs not found"
exit 1
fi
echo "Inspecting: ${vim_file}"
echo "---- EnableVimMode disabled branch ----"
rg -n -A20 -B8 'if \(!_settings\.EnableVimMode\)' "${vim_file}"
echo "---- Change subscriptions and UI reset hooks ----"
rg -n 'PropertyChanged|EnableVimMode|ApplyModeUI|SwitchToInsert|CaretBrush|InputMethod' "${vim_file}"Repository: Flow-Launcher/Flow.Launcher
Length of output: 2839
🏁 Script executed:
#!/bin/bash
set -euo pipefail
vim_file="$(fd -i '^VimManager\.cs$' | head -n1)"
echo "---- Full ViewModel_PropertyChanged implementation ----"
rg -n -A30 'private void ViewModel_PropertyChanged' "${vim_file}"
echo ""
echo "---- Check for explicit EnableVimMode property handling ----"
rg -n 'EnableVimMode' "${vim_file}"Repository: Flow-Launcher/Flow.Launcher
Length of output: 1380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
vim_file="$(fd -i '^VimManager\.cs$' | head -n1)"
echo "---- Full ApplyModeUI implementation ----"
rg -n -A15 'private void ApplyModeUI' "${vim_file}"Repository: Flow-Launcher/Flow.Launcher
Length of output: 824
Add PropertyChanged handler to restore Insert-mode UI when EnableVimMode is disabled.
When EnableVimMode is toggled off, the UI state is not fully restored. HandlePreviewKeyDown only hides the vim block caret, leaving InputMethod suspended and the transparent CaretBrush in place. Add a check in ViewModel_PropertyChanged for the EnableVimMode property that calls ApplyModeUI(VimModes.Insert) when disabled, which properly restores both the InputMethod state and native caret brush.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Flow.Launcher.Infrastructure/UserSettings/Settings.cs` around lines 63 - 74,
When EnableVimMode is disabled, the UI state is not fully restored because
HandlePreviewKeyDown only hides the vim block caret, leaving InputMethod
suspended and CaretBrush transparent. Add a property changed handler in
ViewModel_PropertyChanged that detects when the EnableVimMode property changes
to false and calls ApplyModeUI(VimModes.Insert) to properly restore both the
InputMethod state and the native caret brush appearance.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Flow.Launcher/VimMode/VimManager.cs`:
- Around line 442-448: The Escape key handling in the Visual and VisualLine mode
cases (in the switch statement checking VimModes.Visual and VimModes.VisualLine)
is being bypassed because the HandlePreviewKeyDown method intercepts Escape
before these case handlers can execute. To fix this, ensure that the Escape key
handling for Visual and VisualLine modes takes precedence by either moving the
Visual/VisualLine mode logic earlier in the execution flow or by restructuring
HandlePreviewKeyDown to check the current vim mode and allow Visual/VisualLine
Escape handling to proceed without being intercepted by upstream handling. This
applies to both the Visual mode case around line 442 and the VisualLine mode
case around line 573.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3c3e9bf7-de00-4387-b513-a03fb1502597
📒 Files selected for processing (8)
Flow.Launcher.Infrastructure/UserSettings/Settings.csFlow.Launcher.Test/VimEngineTest.csFlow.Launcher/MainWindow.xaml.csFlow.Launcher/SettingPages/Views/SettingsPaneGeneral.xamlFlow.Launcher/VimMode/VimEngine.csFlow.Launcher/VimMode/VimManager.csFlow.Launcher/VimMode/VimMotionEngine.csREADME.md
💤 Files with no reviewable changes (1)
- Flow.Launcher/VimMode/VimMotionEngine.cs
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- Flow.Launcher/MainWindow.xaml.cs
- Flow.Launcher/SettingPages/Views/SettingsPaneGeneral.xaml
Contributor
There was a problem hiding this comment.
4 issues found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Flow.Launcher/VimMode/VimEngine.cs">
<violation number="1" location="Flow.Launcher/VimMode/VimEngine.cs:35">
P2: Mode-switch logic is duplicated across four methods with identical bodies. Consider extracting a private `SetMode(VimModes mode)` helper so that future changes (logging, guard checks, side effects) only need to be applied in one place.</violation>
</file>
<file name="Flow.Launcher/VimMode/VimManager.cs">
<violation number="1" location="Flow.Launcher/VimMode/VimManager.cs:151">
P1: When `EnableVimMode` is toggled off at runtime, only the block caret rectangle is collapsed. The `InputMethod` (IME) remains suspended and `CaretBrush` stays transparent, leaving the native text caret invisible and IME non-functional. This path needs to also call `InputMethod.SetIsInputMethodEnabled(queryTextBox, true)` and restore the default `CaretBrush` to fully revert the UI state.</violation>
<violation number="2" location="Flow.Launcher/VimMode/VimManager.cs:429">
P2: `_pendingCommand` is not reset when entering Visual mode via `v`/`V`. If `d`, `c`, or `y` was previously pressed (setting `_pendingCommand`), transitioning to Visual mode leaves the operator armed. A later motion back in Normal mode could unexpectedly trigger the stale delete/change/yank operator. Reset `_pendingCommand` when entering Visual mode.</violation>
<violation number="3" location="Flow.Launcher/VimMode/VimManager.cs:488">
P1: `_awaitingCharCommand` is not cleared when Escape is pressed in Visual mode. After pressing `f`, `F`, `t`, `T`, or `r` in Visual mode (which sets `_awaitingCharCommand`), pressing Escape switches to Normal mode but leaves the pending char command armed. The very next Normal-mode keystroke will be consumed as the character argument for the stale command instead of being processed normally. Add `_awaitingCharCommand = "";` before the mode switch.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| { | ||
| if (!_settings.EnableVimMode) | ||
| { | ||
| if (_vimBlockCaret != null && _vimBlockCaret.Visibility == Visibility.Visible) |
Contributor
There was a problem hiding this comment.
P1: When EnableVimMode is toggled off at runtime, only the block caret rectangle is collapsed. The InputMethod (IME) remains suspended and CaretBrush stays transparent, leaving the native text caret invisible and IME non-functional. This path needs to also call InputMethod.SetIsInputMethodEnabled(queryTextBox, true) and restore the default CaretBrush to fully revert the UI state.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Flow.Launcher/VimMode/VimManager.cs, line 151:
<comment>When `EnableVimMode` is toggled off at runtime, only the block caret rectangle is collapsed. The `InputMethod` (IME) remains suspended and `CaretBrush` stays transparent, leaving the native text caret invisible and IME non-functional. This path needs to also call `InputMethod.SetIsInputMethodEnabled(queryTextBox, true)` and restore the default `CaretBrush` to fully revert the UI state.</comment>
<file context>
@@ -146,14 +148,14 @@ public bool HandlePreviewKeyDown(KeyEventArgs e)
if (!_settings.EnableVimMode)
{
- if (_vimBlockCaret.Visibility == Visibility.Visible)
+ if (_vimBlockCaret != null && _vimBlockCaret.Visibility == Visibility.Visible)
{
_vimBlockCaret.Visibility = Visibility.Collapsed;
</file context>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Flow.Launcher/VimMode/VimManager.cs (1)
456-470:⚠️ Potential issue | 🟠 Major | ⚡ Quick winVisual mode motions compute from
CaretIndexinstead of_visualCaret.The motion calculations (e.g.,
MoveLeft(_queryTextBox.CaretIndex, ...)) useCaretIndex, but afterExecuteVisualMotionupdates_visualCaret, theTextBox.CaretIndexmay not match_visualCaretdue to WPF's selection behavior. Subsequent visual motions will compute from the wrong position.Suggested fix: Use _visualCaret for visual motion calculations
case Key.H: - ExecuteVisualMotion(VimMotionEngine.MoveLeft(_queryTextBox.CaretIndex, _queryTextBox.Text.Length)); + ExecuteVisualMotion(VimMotionEngine.MoveLeft(_visualCaret, _queryTextBox.Text.Length)); return true; case Key.L: - ExecuteVisualMotion(VimMotionEngine.MoveRight(_queryTextBox.CaretIndex, _queryTextBox.Text.Length)); + ExecuteVisualMotion(VimMotionEngine.MoveRight(_visualCaret, _queryTextBox.Text.Length)); return true; case Key.W: - ExecuteVisualMotion(VimMotionEngine.MoveNextWord(_queryTextBox.Text, _queryTextBox.CaretIndex)); + ExecuteVisualMotion(VimMotionEngine.MoveNextWord(_queryTextBox.Text, _visualCaret)); return true; case Key.B: - ExecuteVisualMotion(VimMotionEngine.MovePrevWord(_queryTextBox.Text, _queryTextBox.CaretIndex)); + ExecuteVisualMotion(VimMotionEngine.MovePrevWord(_queryTextBox.Text, _visualCaret)); return true; case Key.E: - ExecuteVisualMotion(VimMotionEngine.MoveEndWord(_queryTextBox.Text, _queryTextBox.CaretIndex)); + ExecuteVisualMotion(VimMotionEngine.MoveEndWord(_queryTextBox.Text, _visualCaret)); return true;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Flow.Launcher/VimMode/VimManager.cs` around lines 456 - 470, In the visual mode motion handlers for keys H, L, W, B, and E, replace all instances of `_queryTextBox.CaretIndex` with `_visualCaret` in the motion engine method calls (MoveLeft, MoveRight, MoveNextWord, MovePrevWord, MoveEndWord). The visual motions should calculate from `_visualCaret` instead of the TextBox's CaretIndex to ensure consistency, since `_visualCaret` is updated by ExecuteVisualMotion while CaretIndex may become out of sync due to WPF selection behavior.
♻️ Duplicate comments (5)
Flow.Launcher/VimMode/VimManager.cs (5)
200-206:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPending char commands (
r/f/F/t/T) not cleared on Escape.After pressing
r,f,F,t, orT, if the user presses Escape,_awaitingCharCommandremains set. The next Normal-mode key will be consumed as the command's character argument instead of being treated as a new command.Suggested fix
else { _lastEscapeTime = DateTime.Now; _pendingCommand = ""; + _awaitingCharCommand = ""; e.Handled = true; return true; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Flow.Launcher/VimMode/VimManager.cs` around lines 200 - 206, The Escape key handler in the VimManager.cs file (in the else block around lines 200-206) clears the _pendingCommand flag but does not reset the _awaitingCharCommand flag. When a user presses one of the pending character commands (r, f, F, t, or T) and then presses Escape, the _awaitingCharCommand flag remains set, causing the next keystroke to be incorrectly interpreted as a character argument instead of a new command. Add a line to set _awaitingCharCommand to false alongside the existing _pendingCommand clearing and _lastEscapeTime assignment to properly cancel the pending character command state when Escape is pressed.
385-389:⚠️ Potential issue | 🟠 Major | ⚡ Quick winYank motions (
Y,yw,y$, etc.) only move caret without copying.
Ysets_pendingCommand = "y"and callsExecuteMotion, butExecuteMotiononly handles"d"and"c"operators (line 697). The"y"case falls through to the else branch which just moves the caret.Suggested fix
- if (_pendingCommand == "d" || _pendingCommand == "c") + if (_pendingCommand == "d" || _pendingCommand == "c" || _pendingCommand == "y") { - // Deletion + var command = _pendingCommand; int start = _queryTextBox.CaretIndex; int end = targetCaret; if (start > end) { var temp = start; start = end; end = temp; } if (end > start) { try { Clipboard.SetText(_queryTextBox.Text.Substring(start, end - start)); } catch { } - _queryTextBox.Text = _queryTextBox.Text.Remove(start, end - start); + if (command != "y") + { + _queryTextBox.Text = _queryTextBox.Text.Remove(start, end - start); + } _queryTextBox.CaretIndex = start; } - if (_pendingCommand == "c") + if (command == "c") { _vimEngine.SwitchToInsert(); } _pendingCommand = ""; }Also applies to: 695-721
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Flow.Launcher/VimMode/VimManager.cs` around lines 385 - 389, The ExecuteMotion method in VimManager.cs only handles the "d" (delete) and "c" (change) operators but does not handle the "y" (yank) operator. When _pendingCommand is set to "y" (such as when the user presses Y), the method falls through to the else branch which only moves the caret without copying text. Add a case in ExecuteMotion to handle the "y" operator by copying the text from the start position to the end position to the clipboard, similar to how the "d" and "c" operators handle their respective operations. Ensure yank operations copy the text without modifying the document or moving the cursor position beyond the yanked range.
193-207:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEscape in Visual/VisualLine modes is intercepted upstream and never reaches the mode handlers.
The Escape key handling at lines 193-207 runs for all non-Insert modes (including Visual and VisualLine) before
HandleVimKeyis called. This prevents the Visual mode Escape handler (line 449) and VisualLine Escape handler (line 580) from ever executing. Users cannot exit Visual mode with Escape.Suggested fix: Only handle double-Escape in Normal mode
- else + else if (_vimEngine.CurrentMode == VimModes.Normal) { if (e.Key == Key.Escape) { if ((DateTime.Now - _lastEscapeTime).TotalMilliseconds < 400) { _lastEscapeTime = DateTime.MinValue; // Reset + _awaitingCharCommand = ""; return false; // Let MainWindow's Window_PreviewKeyDown call Hide() } else { _lastEscapeTime = DateTime.Now; _pendingCommand = ""; + _awaitingCharCommand = ""; e.Handled = true; return true; } } + } + else + { + // Visual/VisualLine modes - let HandleVimKey process Escape }Also applies to: 449-452, 580-584
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Flow.Launcher/VimMode/VimManager.cs` around lines 193 - 207, The Escape key handling in the condition checking e.Key == Key.Escape at lines 193-207 runs for all non-Insert modes before HandleVimKey is called, preventing the Visual mode handler and VisualLine mode handler from receiving Escape events. Wrap the entire double-Escape logic (the if statement checking e.Key == Key.Escape) in an additional condition that only executes this code when the current mode is Normal mode, allowing Escape to propagate to the mode-specific handlers for Visual and VisualLine modes.
153-160:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInput remains blocked when Vim mode is disabled mid-session.
When
EnableVimModeis toggled off while in Normal mode, this branch only hides the overlay but leaves_vimEngine.CurrentModeasNormal. Consequently,PreviewTextInput(line 811) andIsInputBlocked(line 817) continue blocking native typing.Suggested fix
if (!_settings.EnableVimMode) { - if (_vimBlockCaret != null && _vimBlockCaret.Visibility == Visibility.Visible) - { - _vimBlockCaret.Visibility = Visibility.Collapsed; - } + _pendingCommand = ""; + _awaitingCharCommand = ""; + + if (_vimEngine.CurrentMode != VimModes.Insert) + { + _vimEngine.SwitchToInsert(); + } + else if (_vimBlockCaret != null && _vimBlockCaret.Visibility == Visibility.Visible) + { + _vimBlockCaret.Visibility = Visibility.Collapsed; + } + return false; }Also update
PreviewTextInputandIsInputBlockedto check_settings.EnableVimMode:- public bool IsInputBlocked => _vimEngine.CurrentMode != VimModes.Insert; + public bool IsInputBlocked => _settings.EnableVimMode && _vimEngine.CurrentMode != VimModes.Insert;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Flow.Launcher/VimMode/VimManager.cs` around lines 153 - 160, When EnableVimMode is disabled, the code hides the visual caret indicator but fails to reset the vim engine's mode, leaving PreviewTextInput and IsInputBlocked still blocking input. Fix this by resetting _vimEngine.CurrentMode to a non-blocking state (such as InsertMode) in the branch where !_settings.EnableVimMode is true, right after hiding _vimBlockCaret. Additionally, update both the PreviewTextInput method and the IsInputBlocked method to check _settings.EnableVimMode in their conditions so that input blocking is properly disabled whenever Vim mode is not enabled, independent of the current engine mode state.
249-254:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStale operator commands persist after unrelated keys.
After pressing
d,c, ory, if the user pressesj,k, or another non-motion key,_pendingCommandremains armed. A subsequent motion will unexpectedly delete/change/yank.Suggested fix: Reset _pendingCommand for non-motion commands
case Key.J: + _pendingCommand = ""; _viewModel.SelectNextItemCommand.Execute(null); return true; case Key.K: + _pendingCommand = ""; _viewModel.SelectPrevItemCommand.Execute(null); return true;And in the default case:
default: + _pendingCommand = ""; if (IsVimBlockedKey(e.Key)) return true; return false;Also applies to: 438-443
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Flow.Launcher/VimMode/VimManager.cs` around lines 249 - 254, In the VimManager.cs file, when non-motion keys like Key.J and Key.K are pressed (which execute SelectNextItemCommand and SelectPrevItemCommand), the _pendingCommand field remains armed if it was previously set by an operator command (d, c, or y). This causes unexpected behavior on the next motion. Reset the _pendingCommand field to null after handling these non-motion commands in the Key.J and Key.K case statements, and also add the same reset in the default case to ensure stale operator commands do not persist when unrelated keys are pressed.
🧹 Nitpick comments (1)
Flow.Launcher/VimMode/VimManager.cs (1)
775-807: ⚖️ Poor tradeoff
GetCharFromKeyassumes US keyboard layout.The shift+number mappings (e.g.,
Shift+2=@) are US-specific. International users with different layouts may experience incorrect character recognition for find commands (f,t, etc.) involving symbols.Consider using
KeyboardStateorKeyInterop.VirtualKeyFromKeywithToUnicodefor layout-aware conversion in a future iteration. For now, document this limitation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Flow.Launcher/VimMode/VimManager.cs` around lines 775 - 807, The GetCharFromKey method assumes a US keyboard layout by hardcoding shift+number character mappings (such as Shift+2 returning '@'). Add documentation to the method explaining this limitation and that international keyboard layouts may produce different characters than expected, which could affect vim mode find commands. Include a note that proper layout-aware conversion using KeyboardState or KeyInterop.VirtualKeyFromKey with ToUnicode should be considered for future implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Flow.Launcher/VimMode/VimManager.cs`:
- Around line 456-470: In the visual mode motion handlers for keys H, L, W, B,
and E, replace all instances of `_queryTextBox.CaretIndex` with `_visualCaret`
in the motion engine method calls (MoveLeft, MoveRight, MoveNextWord,
MovePrevWord, MoveEndWord). The visual motions should calculate from
`_visualCaret` instead of the TextBox's CaretIndex to ensure consistency, since
`_visualCaret` is updated by ExecuteVisualMotion while CaretIndex may become out
of sync due to WPF selection behavior.
---
Duplicate comments:
In `@Flow.Launcher/VimMode/VimManager.cs`:
- Around line 200-206: The Escape key handler in the VimManager.cs file (in the
else block around lines 200-206) clears the _pendingCommand flag but does not
reset the _awaitingCharCommand flag. When a user presses one of the pending
character commands (r, f, F, t, or T) and then presses Escape, the
_awaitingCharCommand flag remains set, causing the next keystroke to be
incorrectly interpreted as a character argument instead of a new command. Add a
line to set _awaitingCharCommand to false alongside the existing _pendingCommand
clearing and _lastEscapeTime assignment to properly cancel the pending character
command state when Escape is pressed.
- Around line 385-389: The ExecuteMotion method in VimManager.cs only handles
the "d" (delete) and "c" (change) operators but does not handle the "y" (yank)
operator. When _pendingCommand is set to "y" (such as when the user presses Y),
the method falls through to the else branch which only moves the caret without
copying text. Add a case in ExecuteMotion to handle the "y" operator by copying
the text from the start position to the end position to the clipboard, similar
to how the "d" and "c" operators handle their respective operations. Ensure yank
operations copy the text without modifying the document or moving the cursor
position beyond the yanked range.
- Around line 193-207: The Escape key handling in the condition checking e.Key
== Key.Escape at lines 193-207 runs for all non-Insert modes before HandleVimKey
is called, preventing the Visual mode handler and VisualLine mode handler from
receiving Escape events. Wrap the entire double-Escape logic (the if statement
checking e.Key == Key.Escape) in an additional condition that only executes this
code when the current mode is Normal mode, allowing Escape to propagate to the
mode-specific handlers for Visual and VisualLine modes.
- Around line 153-160: When EnableVimMode is disabled, the code hides the visual
caret indicator but fails to reset the vim engine's mode, leaving
PreviewTextInput and IsInputBlocked still blocking input. Fix this by resetting
_vimEngine.CurrentMode to a non-blocking state (such as InsertMode) in the
branch where !_settings.EnableVimMode is true, right after hiding
_vimBlockCaret. Additionally, update both the PreviewTextInput method and the
IsInputBlocked method to check _settings.EnableVimMode in their conditions so
that input blocking is properly disabled whenever Vim mode is not enabled,
independent of the current engine mode state.
- Around line 249-254: In the VimManager.cs file, when non-motion keys like
Key.J and Key.K are pressed (which execute SelectNextItemCommand and
SelectPrevItemCommand), the _pendingCommand field remains armed if it was
previously set by an operator command (d, c, or y). This causes unexpected
behavior on the next motion. Reset the _pendingCommand field to null after
handling these non-motion commands in the Key.J and Key.K case statements, and
also add the same reset in the default case to ensure stale operator commands do
not persist when unrelated keys are pressed.
---
Nitpick comments:
In `@Flow.Launcher/VimMode/VimManager.cs`:
- Around line 775-807: The GetCharFromKey method assumes a US keyboard layout by
hardcoding shift+number character mappings (such as Shift+2 returning '@'). Add
documentation to the method explaining this limitation and that international
keyboard layouts may produce different characters than expected, which could
affect vim mode find commands. Include a note that proper layout-aware
conversion using KeyboardState or KeyInterop.VirtualKeyFromKey with ToUnicode
should be considered for future implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d4fe4a1a-8fb6-4d1d-92f8-05a1040fae3d
📒 Files selected for processing (1)
Flow.Launcher/VimMode/VimManager.cs
namefailed
force-pushed
the
vim-mode
branch
3 times, most recently
from
15af3fe to
810f314
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
README.md (1)
5-5:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix heading level increment (MD001) — still present.
Line 5 jumps from
#(h1) to###(h3); it should be##(h2) to increment by one level. This was flagged in the previous review but remains unaddressed.💡 Proposed fix
-### Why does this exist? +## Why does this exist?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` at line 5, The heading "Why does this exist?" uses three hash marks (###, h3 level) but should use two hash marks (##, h2 level) to properly increment from the main heading (#, h1 level) above it. Change the heading prefix from ### to ## to maintain correct markdown heading hierarchy and fix the MD001 linting violation.
🧹 Nitpick comments (2)
README.md (2)
6-6: 💤 Low valueConsider removing redundant "originally" for concision.
The phrase "originally created" is redundant; "created" alone is sufficient and more direct.
✨ Proposed fix
-This fork was originally created to streamline journaling workflows. +This fork was created to streamline journaling workflows.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` at line 6, In the README.md description of the fork, the word "originally" in the phrase "This fork was originally created to streamline journaling workflows" is redundant. Remove the word "originally" from this sentence so it reads "This fork was created to streamline journaling workflows" for improved clarity and concision.
41-42: 💤 Low valueMinor: Consider "until" instead of "till" for formality.
Lines 41–42 use "till"; the more formal "until" may be preferable for documentation.
✨ Proposed fixes
-- `t{char}` : Jump till just before the next occurrence of `{char}` - - `T{char}` : Jump till just after the previous occurrence of `{char}` +- `t{char}` : Jump until just before the next occurrence of `{char}` +- `T{char}` : Jump until just after the previous occurrence of `{char}`🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 41 - 42, Replace the informal word "till" with the more formal "until" in the README.md documentation for the jump commands. Specifically, in the descriptions for t{char} and T{char} commands, change "till just before" to "until just before" and "till just after" to "until just after" to improve the formality and consistency of the documentation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@README.md`:
- Line 5: The heading "Why does this exist?" uses three hash marks (###, h3
level) but should use two hash marks (##, h2 level) to properly increment from
the main heading (#, h1 level) above it. Change the heading prefix from ### to
## to maintain correct markdown heading hierarchy and fix the MD001 linting
violation.
---
Nitpick comments:
In `@README.md`:
- Line 6: In the README.md description of the fork, the word "originally" in the
phrase "This fork was originally created to streamline journaling workflows" is
redundant. Remove the word "originally" from this sentence so it reads "This
fork was created to streamline journaling workflows" for improved clarity and
concision.
- Around line 41-42: Replace the informal word "till" with the more formal
"until" in the README.md documentation for the jump commands. Specifically, in
the descriptions for t{char} and T{char} commands, change "till just before" to
"until just before" and "till just after" to "until just after" to improve the
formality and consistency of the documentation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a76723d2-6ffe-4407-987d-d273570c3bd2
📒 Files selected for processing (11)
Flow.Launcher.Infrastructure/UserSettings/Settings.csFlow.Launcher.Test/VimEngineTest.csFlow.Launcher.Test/VimMotionEngineTest.csFlow.Launcher/MainWindow.xamlFlow.Launcher/MainWindow.xaml.csFlow.Launcher/SettingPages/Views/SettingsPaneGeneral.xamlFlow.Launcher/VimMode/VimEngine.csFlow.Launcher/VimMode/VimManager.csFlow.Launcher/VimMode/VimMotionEngine.csFlow.Launcher/packages.lock.jsonREADME.md
✅ Files skipped from review due to trivial changes (1)
- Flow.Launcher/packages.lock.json
🚧 Files skipped from review as they are similar to previous changes (6)
- Flow.Launcher/MainWindow.xaml
- Flow.Launcher/SettingPages/Views/SettingsPaneGeneral.xaml
- Flow.Launcher/VimMode/VimEngine.cs
- Flow.Launcher.Infrastructure/UserSettings/Settings.cs
- Flow.Launcher/MainWindow.xaml.cs
- Flow.Launcher/VimMode/VimMotionEngine.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
README.md (1)
5-5:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the heading hierarchy.
Line 5 still jumps from
#to###, which breaks incremental heading levels and will keep markdownlint MD001 failing.💡 Proposed fix
-### Why does this exist? +## Why does this exist?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` at line 5, The heading hierarchy in README.md is broken because the heading "Why does this exist?" uses three hash marks (###) which creates a level 3 heading, jumping directly from the level 1 heading to level 3 and skipping level 2. To fix this and satisfy markdownlint MD001, change the heading "Why does this exist?" from ### to ## to maintain proper incremental heading levels throughout the document.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Flow.Launcher/VimMode/VimManager.cs`:
- Around line 508-512: The logic in the ternary operator that sets
`_awaitingCharCommand` is inverted. Currently, when Shift is pressed
(HasFlag(ModifierKeys.Shift)), it uses the uppercase key, and when Shift is not
pressed, it converts to lowercase. This causes Shift+R to set the command to
uppercase "R", but ExecuteCharCommand only checks for lowercase "r". Change the
ternary operator to always call .ToLower() on e.Key.ToString() regardless of the
Shift modifier status, so that both r and R (Shift+R) are normalized to
lowercase "r" when stored in _awaitingCharCommand.
---
Duplicate comments:
In `@README.md`:
- Line 5: The heading hierarchy in README.md is broken because the heading "Why
does this exist?" uses three hash marks (###) which creates a level 3 heading,
jumping directly from the level 1 heading to level 3 and skipping level 2. To
fix this and satisfy markdownlint MD001, change the heading "Why does this
exist?" from ### to ## to maintain proper incremental heading levels throughout
the document.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a24f3c7e-61b8-4fa3-a051-e787de9142c1
📒 Files selected for processing (11)
Flow.Launcher.Infrastructure/UserSettings/Settings.csFlow.Launcher.Test/VimEngineTest.csFlow.Launcher.Test/VimMotionEngineTest.csFlow.Launcher/MainWindow.xamlFlow.Launcher/MainWindow.xaml.csFlow.Launcher/SettingPages/Views/SettingsPaneGeneral.xamlFlow.Launcher/VimMode/VimEngine.csFlow.Launcher/VimMode/VimManager.csFlow.Launcher/VimMode/VimMotionEngine.csFlow.Launcher/packages.lock.jsonREADME.md
✅ Files skipped from review due to trivial changes (1)
- Flow.Launcher/packages.lock.json
🚧 Files skipped from review as they are similar to previous changes (8)
- Flow.Launcher/SettingPages/Views/SettingsPaneGeneral.xaml
- Flow.Launcher/MainWindow.xaml
- Flow.Launcher.Infrastructure/UserSettings/Settings.cs
- Flow.Launcher.Test/VimMotionEngineTest.cs
- Flow.Launcher/VimMode/VimEngine.cs
- Flow.Launcher.Test/VimEngineTest.cs
- Flow.Launcher/MainWindow.xaml.cs
- Flow.Launcher/VimMode/VimMotionEngine.cs
Contributor
There was a problem hiding this comment.
5 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Flow.Launcher/VimMode/VimManager.cs">
<violation number="1" location="Flow.Launcher/VimMode/VimManager.cs:270">
P2: Count digits are only accepted when `_pendingCommand` is empty, preventing counts after an operator (e.g., `d3w` fails).</violation>
<violation number="2" location="Flow.Launcher/VimMode/VimManager.cs:913">
P2: RepeatLastChange('.') does not handle motion-based operators like 'dw' or 'c3w'</violation>
</file>
<file name="Flow.Launcher/VimMode/VimMotionEngine.cs">
<violation number="1" location="Flow.Launcher/VimMode/VimMotionEngine.cs:125">
P2: MoveEndWordBig has a boundary bug for single-character words followed by whitespace. When the caret is at the start of a one-character word and the next character is a space (e.g. text \"a \", caret=0), the method returns the index of the space instead of the character index, because `i = caret + 1` lands on the space and neither loop executes (`i < text.Length - 1` is false when `text.Length == 2`). This breaks Vim `E` motion for single-char words.</violation>
<violation number="2" location="Flow.Launcher/VimMode/VimMotionEngine.cs:182">
P1: TextObjectWord returns a zero-width selection when the caret is on whitespace instead of selecting the whitespace block, breaking expected Vim `iw`/`aw` behavior.</violation>
<violation number="3" location="Flow.Launcher/VimMode/VimMotionEngine.cs:238">
P1: TextObjectQuote only finds the first quote pair in the string, not the pair surrounding the caret, breaking multi-pair scenarios.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
added 2 commits
June 18, 2026 21:25
Contributor
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Flow.Launcher/VimMode/VimManager.cs">
<violation number="1" location="Flow.Launcher/VimMode/VimManager.cs:151">
P1: When `EnableVimMode` is toggled off at runtime, only the block caret rectangle is collapsed. The `InputMethod` (IME) remains suspended and `CaretBrush` stays transparent, leaving the native text caret invisible and IME non-functional. This path needs to also call `InputMethod.SetIsInputMethodEnabled(queryTextBox, true)` and restore the default `CaretBrush` to fully revert the UI state.</violation>
<violation number="2" location="Flow.Launcher/VimMode/VimManager.cs:397">
P2: All Clipboard.SetText catch blocks log the same misleading message 'Clipboard/Redo operation failed' regardless of the actual operation (delete, yank, change, etc.), reducing diagnostic value.</violation>
<violation number="3" location="Flow.Launcher/VimMode/VimManager.cs:429">
P2: `_pendingCommand` is not reset when entering Visual mode via `v`/`V`. If `d`, `c`, or `y` was previously pressed (setting `_pendingCommand`), transitioning to Visual mode leaves the operator armed. A later motion back in Normal mode could unexpectedly trigger the stale delete/change/yank operator. Reset `_pendingCommand` when entering Visual mode.</violation>
<violation number="4" location="Flow.Launcher/VimMode/VimManager.cs:488">
P1: `_awaitingCharCommand` is not cleared when Escape is pressed in Visual mode. After pressing `f`, `F`, `t`, `T`, or `r` in Visual mode (which sets `_awaitingCharCommand`), pressing Escape switches to Normal mode but leaves the pending char command armed. The very next Normal-mode keystroke will be consumed as the character argument for the stale command instead of being processed normally. Add `_awaitingCharCommand = "";` before the mode switch.</violation>
<violation number="5" location="Flow.Launcher/VimMode/VimManager.cs:913">
P2: RepeatLastChange('.') does not handle motion-based operators like 'dw' or 'c3w'</violation>
</file>
<file name="Flow.Launcher/VimMode/VimEngine.cs">
<violation number="1" location="Flow.Launcher/VimMode/VimEngine.cs:35">
P2: Mode-switch logic is duplicated across four methods with identical bodies. Consider extracting a private `SetMode(VimModes mode)` helper so that future changes (logging, guard checks, side effects) only need to be applied in one place.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
added 4 commits
June 18, 2026 21:57
…tespace, TextObjectQuote, shift+R, and count digits. Update README docs.
…rcle to prevent covering input text
Contributor
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Flow.Launcher/VimMode/VimManager.cs">
<violation number="1" location="Flow.Launcher/VimMode/VimManager.cs:139">
P1: Functional regression: Vim mode indicator no longer displays the mode label text. The `label` variable ('NORMAL', 'VISUAL', 'V-LINE') is computed but never assigned to any UI element. The `VimModeIndicator` Border has no child TextBlock and no Text property, so users cannot distinguish which Vim mode is active.</violation>
<violation number="2" location="Flow.Launcher/VimMode/VimManager.cs:151">
P1: When `EnableVimMode` is toggled off at runtime, only the block caret rectangle is collapsed. The `InputMethod` (IME) remains suspended and `CaretBrush` stays transparent, leaving the native text caret invisible and IME non-functional. This path needs to also call `InputMethod.SetIsInputMethodEnabled(queryTextBox, true)` and restore the default `CaretBrush` to fully revert the UI state.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| { | ||
| _vimModeIndicator.Visibility = _settings.EnableVimMode && mode != VimModes.Insert ? Visibility.Visible : Visibility.Collapsed; | ||
|
|
||
| _vimModeIndicator.Background = mode switch |
Contributor
There was a problem hiding this comment.
P1: Functional regression: Vim mode indicator no longer displays the mode label text. The label variable ('NORMAL', 'VISUAL', 'V-LINE') is computed but never assigned to any UI element. The VimModeIndicator Border has no child TextBlock and no Text property, so users cannot distinguish which Vim mode is active.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Flow.Launcher/VimMode/VimManager.cs, line 139:
<comment>Functional regression: Vim mode indicator no longer displays the mode label text. The `label` variable ('NORMAL', 'VISUAL', 'V-LINE') is computed but never assigned to any UI element. The `VimModeIndicator` Border has no child TextBlock and no Text property, so users cannot distinguish which Vim mode is active.</comment>
<file context>
@@ -132,21 +132,17 @@ private void ApplyModeUI(VimModes mode)
+ _vimModeIndicator.Visibility = _settings.EnableVimMode && mode != VimModes.Insert ? Visibility.Visible : Visibility.Collapsed;
- if (_vimModeText.Parent is Border border)
+ _vimModeIndicator.Background = mode switch
{
- border.Background = mode switch
</file context>
Contributor
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
added 8 commits
June 19, 2026 00:43
Author
|
@cubic-dev-ai review |
Contributor
@namefailed I have started the AI code review. It will take a few minutes to complete. |
added 3 commits
June 20, 2026 01:23
… leaks - Disabling Vim mode while in a non-Insert mode no longer blocks typing: PreviewTextInput now respects EnableVimMode, and toggling the setting off forces a return to Insert mode and clears the overlays. - The multi-key 'g' prefix (gu/gU/g~/gv/g_) is now triggered by lowercase 'g' instead of Shift+G, matching the documented bindings. - Visual-mode text objects (vi(, va", viw, ...) are reachable again; the unreachable insert-at-boundary cases that shadowed them were removed. - Wire up gu/gU in Visual mode (previously documented but not implemented). - Reset transient command state (count, pending operator, g-prefix, awaited text object) on Escape and on mode entry so commands like 3dd don't leak count. - 'p' now honors counts, leaves the caret on the last pasted character, and is repeatable with '.'. - Guard MoveEndWord/MoveEndWordBig against empty text (no longer return -1). - Add unit tests for BIG-word motions, bracket matching, and all text objects. - Reconcile README: dot indicator (not pill), document '.' and counts, and add a Known Limitations section.
Second audit pass. Found and fixed a class of off-by-one bugs in
operator + motion combinations:
- Inclusive forward motions were treated as exclusive, so de/df/dt/d%
(and ce/ye/gue/...) left the final character behind. Introduced a
MotionInclusivity model + VimMotionEngine.OperatorRange to compute the
affected range per Vim's inclusive/exclusive rules, and tagged e/E/f/t
as inclusive-forward and % as an inclusive pair (both brackets, either
direction).
- The gu/gU/g~ branches always extended the range by one, making guw/g~w
over-inclusive on exclusive motions; they now respect motion inclusivity.
Consolidated the three near-identical branches into one.
- A count typed before r/f/t/;/, leaked into the next command; it is now
consumed. Added count support: r{char} replaces N chars, f/t/;/, jump to
the Nth occurrence.
- Reset the count after every completed operator motion.
- Add unit tests for OperatorRange covering exclusive, inclusive-forward
(incl. backward find and failed find), and inclusive-pair cases.
…rk CI Prepares the upstream PR for review by removing fork-only artifacts so the change touches feature files only: - Restore README.md to match upstream exactly. The branch had rebranded it to a "Vim Edition" README, deleting upstream's badges and content. - Move the Vim documentation to a dedicated Flow.Launcher/VimMode/README.md co-located with the feature: modes, keybindings, counts/repeat, the engine architecture, and known limitations. - Drop the fork-specific "vim-mode" branch trigger from dotnet.yml. - Revert incidental packages.lock.json drift (Flow.Launcher.Plugin 5.0.0 -> 5.3.1) that came from a local restore. - Restore the UTF-8 BOM on MainWindow.xaml.cs to avoid an encoding-only diff. - Fix stray indentation on two VimMotionEngine text-object methods.
namefailed
pushed a commit
to namefailed/flowlauncher-vim-fork
that referenced
this pull request
Jun 20, 2026
dev is the fork's public/distribution branch; vim-mode stays pristine for the upstream PR (Flow-Launcher#4541), so the two intentionally diverge by these files. - Brand README.md as "Flow Launcher - Vim Edition (fork)": marks it clearly as an unofficial fork, links to upstream PR Flow-Launcher#4541, documents download & install (via Releases), building from source, and the branch layout. - Add .github/workflows/fork-release.yml: builds the installer + portable zip and publishes them to a GitHub Release (manual dispatch, or pushing a v* tag), so the latest fork build is downloadable without a login.
The mode is shown as a small color-coded dot rather than a NORMAL/VISUAL text label, deliberately, so the indicator never overlaps the query text or alters Flow Launcher's search-bar layout (and non-Vim users see no change). Documented in both VimManager.ApplyModeUI and the Vim mode README; a text label can be added later if preferred. Addresses the reviewer note about the indicator not showing mode text.
Author
|
Thanks for the reviews. The earlier findings have been addressed in the latest commits — opt-in default ( On the mode indicator: it intentionally uses a small color-coded dot rather than a Could you re-review the current head? @coderabbitai review |
Contributor
@namefailed I have started the AI code review. It will take a few minutes to complete. |
Contributor
|
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Flow.Launcher/VimMode/VimManager.cs`:
- Around line 583-587: The cases for Key.A and Key.I in the VimManager class
only check for _pendingCommand values of "d", "c", or "y" when setting
_awaitingTextObject, which prevents "gu" and "gU" commands from consuming text
objects properly. Add "gu" and "gU" to the conditions in both the Key.A case and
Key.I case (around lines 583-587) by extending the _pendingCommand equality
checks to include these two commands. Additionally, apply the same fix to the
HandleTextObjectKey method (around lines 963-978) where identical conditions
exist, ensuring that all places where text object entry is guarded have
consistent support for all four pending commands: "d", "c", "y", "gu", and "gU".
- Around line 488-498: The issue is that when repeating the `x` command with a
count (e.g., `3x`), the stored `_lastChangeLen` value should be used during
repetition instead of calling GetCount() which defaults to 1. When a counted
deletion like the one shown (storing `_lastChangeLen = n`) is executed, ensure
that the RepeatLastChange() method uses the previously stored `_lastChangeLen`
value rather than recalling GetCount(). Apply the same fix pattern to the `r`
command and the other similar locations mentioned (around lines 1049-1053,
1114-1120, 1180-1188) so that repeating any counted edit with `.` preserves the
original count instead of defaulting to single repetitions.
- Around line 56-90: Insert-mode edits (direct typing or pasting into the
TextBox) bypass the PushUndo() method and are not captured in the undo history.
To fix this, hook into the TextBox's TextChanged event handler to detect when
text is modified during Insert mode, and push the current text and caret state
to the undo stack before the change occurs. You may need to track whether an
edit is in progress to avoid duplicate snapshots, and ensure that only
user-initiated changes in Insert mode trigger undo snapshots, not programmatic
changes made by SetText() or undo/redo operations.
In `@Flow.Launcher/VimMode/VimMotionEngine.cs`:
- Around line 413-440: The TextObjectQuote method receives caret from
TextBox.CaretIndex which can equal text.Length, but string indices only range
from 0 to text.Length - 1. This causes the quote-pair matching condition `caret
>= first && caret <= i` to fail when the caret is at the end of the text, even
if it's within a quoted region. Clamp the caret value at the beginning of the
TextObjectQuote method using Math.Min(caret, text.Length - 1) before using it in
the comparison logic to ensure the caret position never exceeds the last valid
string index.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b7204071-ceaa-43c2-abd1-8c03270d2167
📒 Files selected for processing (11)
Flow.Launcher.Infrastructure/UserSettings/Settings.csFlow.Launcher.Test/VimEngineTest.csFlow.Launcher.Test/VimMotionEngineTest.csFlow.Launcher/Languages/en.xamlFlow.Launcher/MainWindow.xamlFlow.Launcher/MainWindow.xaml.csFlow.Launcher/SettingPages/Views/SettingsPaneGeneral.xamlFlow.Launcher/VimMode/README.mdFlow.Launcher/VimMode/VimEngine.csFlow.Launcher/VimMode/VimManager.csFlow.Launcher/VimMode/VimMotionEngine.cs
added 2 commits
June 20, 2026 17:08
Normal-mode P pastes the clipboard before the cursor (p pastes after). Small Vim-completeness addition backported from the multi-line fork; documented in the Vim mode README.
…at EOL Addresses three issues from code review: - `u` now reverts the whole Insert session. Entering Insert via i/I/a/A pushes an undo snapshot first, so after `i`<text>`<Esc>` pressing `u` removes the typed text (change-operators already snapshot via SetText, so they are unaffected and need no second snapshot). - `.` after a counted `x`/`r` repeats the count. `r` now stores its length and RepeatLastChange falls back to the stored count for `x`/`r` instead of defaulting to 1, so `3x`/`3rx` then `.` repeat 3 chars. - `ci"`/`di"` work at end-of-query. TextObjectQuote clamps the caret to the last index (CaretIndex can equal text.Length), so a quoted region ending the query still matches. Covered by new unit-test cases.
Author
|
Latest commit addresses the open review findings: Fixed
Not changed (already handled / intentional)
All 35 Vim unit tests pass; AppVeyor is green. |
Yank operations (y{motion}, yiw/yi", yy, Y, and Visual-mode y) now briefly flash
an accent highlight over the copied text, like Neovim's on-yank highlight, so
you get visual confirmation the yank happened. Drawn on a VimYankFlash canvas
overlay (one rectangle per line of the range) and faded out over 300ms; purely
visual — it never touches the text, caret, or selection, and only fires on pure
yanks, never on cuts (x/d/c/s).
- gg / G jump to the start / end of the query (Normal and Visual; work as
operator targets, e.g. dgg / dG).
- Ctrl-A / Ctrl-X increment / decrement the number at or after the cursor,
count-aware (5 Ctrl-A adds 5), Normal mode only so Insert-mode select-all is
unaffected. Pure logic in VimMotionEngine.ChangeNumber (+ unit tests).
- ib/ab and iB/aB block-object aliases for i(/a( and i{/a{.
- Counts on word text objects: 2daw / 3iw extend through additional words.
On show, the non-Insert branch cleared the selection before switching to Insert, wiping the select-all the launcher sets up on reopen — so dismissing in Normal mode reopened with an unselected query. Switch to Insert then SelectAll, matching the Insert-mode behaviour, so the first keystroke replaces.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Adds an opt-in Vim editing mode to the search bar. Toggle it in Settings → General → "Enable Advanced Vim Mode". It is off by default — when disabled, the search bar behaves exactly as before.
Modes
A small color-coded dot at the left of the box shows the current mode (hidden in Insert).
Normal mode
h l w W b B e E 0 ^ $ %,f/F/t/T{char}with;/,repeat, andj/kto move through the resultsx X s S r{char} ~ dd cc D C Y p,gu/gU, undou/ redoCtrl+Rd c y gucombined withiw/aw,i"/a",i'/a',i(/a(,i[/a[,i{/a{(e.g.diw,ci",ya().repeats the last change; most motions/operators accept a count (3w,5x,d3w,3f,)i I a A v VVisual mode
Extend the selection with any motion, then operate on it:
x/d,y,c/s,r{char},~,gu/gU.i/astart a text object,oswaps the selection ends,Escreturns to Normal.Notes for reviewers
EnableVimModedefaults tofalse).Ctrl/Altchords pass through untouched, so existing hotkeys are unaffected.VimMotionEngine(caret/range math), aVimEnginemode state machine, and aVimManagerintegration layer.VimManagerisIDisposableand is torn down inMainWindow.Dispose.Flow.Launcher/VimMode/README.md.