Conversation
📝 WalkthroughWalkthroughCommand-line mode completion support is added to fzf-lua via mode-aware helper functions in the context module and conditional logic branches in actions and core modules for handling cmdline-specific cursor and line operations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
lua/fzf-lua/ctx.lua (2)
124-131:get_modeis not idempotent — passing an already-normalized value falls through to"default".If a caller passes
"cmdline"(one of your own return values) instead of the raw"c", none of themode == "c"/mode == "t"checks match and the function returns"default"(or"cmdwin"ifgetcmdwintype()happens to be non-empty). Today all in-tree callers pass the raw mode captured in__CTX.mode, so it works, but the helper is easy to misuse (e.g. callingset_cursor(get_mode(), ...)).🧹 Proposed fix — short-circuit normalized inputs
function M.get_mode(mode) mode = mode or vim.api.nvim_get_mode().mode + if mode == "cmdline" or mode == "term" or mode == "cmdwin" or mode == "default" then + return mode + end return (mode == "c" and "cmdline") or (mode == "t" and "term") or (vim.fn.getcmdwintype() ~= "" and "cmdwin") or "default" end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/fzf-lua/ctx.lua` around lines 124 - 131, M.get_mode currently treats already-normalized strings like "cmdline"/"term"/"cmdwin"/"default" as raw modes and can return "default" incorrectly; update M.get_mode to first short-circuit and return mode if it's one of the normalized values ("cmdline","term","cmdwin","default"), then proceed to compute from vim.api.nvim_get_mode() or vim.fn.getcmdwintype(); refer to M.get_mode and callers that may pass __CTX.mode or functions like set_cursor(get_mode(), ...) when making this change.
149-159:get_linecallsget_cursor()without forwardingmode.When
num == nilin a non-cmdline call,M.get_cursor()re-queriesnvim_get_mode(). In the currentM.refreshflow this is fine (mode hasn't changed yet), but it's inconsistent with the rest of the helpers that accept an explicitmode. Forwarding the argument avoids a subtle drift if this helper is later called after a mode switch or from an async context.♻️ Proposed tweak
- if num == nil then num = M.get_cursor()[1] - 1 end + if num == nil then num = M.get_cursor(mode)[1] - 1 end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/fzf-lua/ctx.lua` around lines 149 - 159, The get_line helper calls M.get_cursor() without passing the current mode which can re-query nvim_get_mode(); change the fallback branch in M.get_line so when num == nil it calls M.get_cursor(mode) (i.e. forward the mode argument) to compute the line index, preserving the existing decrement (-1) logic and returning the buffer line as before; update references to M.get_cursor in M.get_line accordingly so behavior is consistent with other helpers that accept an explicit mode (keep the cmdline assertion and return path unchanged).lua/fzf-lua/actions.lua (1)
1268-1283:newline/newcolmay benilwhenopts.completeis a function — guard before use.When
opts.completeis user-supplied (Line 1260-1261), it can legitimately returnnil, nilto indicate "do nothing". The current flow handles this formode ~= "c"via thenewline or opts.__CTX.line/newcol or colfallbacks, but in the cmdline branch theCmdlineEntercallback unconditionally overwrites the cmdline withnewline or opts.__CTX.line— so a user callback returningnilstill clobbers whatever the user has subsequently typed between fzf closing and the next:press. It's niche (MVP), but worth a note alongside the "insert complete" refactor you already flagged.Also, once the
setcmdposoff-by-one is fixed inctx.lua, the expressioncursor[2] + (newcol or col) - colsimplifies to(newcol or col) - 1; consider simplifying for readability:♻️ Optional simplification (after the ctx.lua fix)
- require("fzf-lua.ctx").set_cursor(mode, { cursor[1], cursor[2] + (newcol or col) - col }) + require("fzf-lua.ctx").set_cursor(mode, { cursor[1], (newcol or col) - 1 })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/fzf-lua/actions.lua` around lines 1268 - 1283, The CmdlineEnter autocmd callback unconditionally uses newline/newcol and thus can clobber user-typed cmdline when opts.complete returns nil; modify the callback in the CmdlineEnter block so it checks whether newline or newcol are non-nil before calling vim.fn.setcmdline and require("fzf-lua.ctx").set_cursor (leave the existing fallback to opts.__CTX.line only when newline is intentionally provided), and if both are nil do nothing; optionally, after the ctx.lua setcmdpos off-by-one fix you can simplify the cursor math to use (newcol or col) - 1 when calling set_cursor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lua/fzf-lua/actions.lua`:
- Around line 1248-1256: The no-selection branch feeds ':' into the cmdline with
nvim_feedkeys and immediately calls require("fzf-lua.ctx").set_cursor(mode,
cursor), but set_cursor uses vim.fn.setcmdpos which is a no-op until the cmdline
is actually open; instead mirror the success path and delay the cursor restore
by installing a one-shot CmdlineEnter autocmd that calls
require("fzf-lua.ctx").set_cursor(mode, cursor) (and then removes itself),
rather than calling set_cursor synchronously after nvim_feedkeys; reference the
no-selection block where nvim_feedkeys(':', 'n', true) is used and the
set_cursor function in require("fzf-lua.ctx").
In `@lua/fzf-lua/core.lua`:
- Line 368: Replace the version-specific vim.keycode usage: when checking
opts.__CTX.mode == "c" call the compatibility helper utils.send_ctrl_c() and
pass its result to vim.api.nvim_feedkeys instead of vim.keycode(); specifically
replace the vim.api.nvim_feedkeys(vim.keycode("<c-c>"), "n", false) invocation
with vim.api.nvim_feedkeys(utils.send_ctrl_c(), "n", false) while leaving the
opts.__CTX.mode check and feedkeys flags unchanged.
In `@lua/fzf-lua/ctx.lua`:
- Around line 138-147: M.set_cursor uses vim.fn.setcmdpos with a 0-based column
from M.get_cursor which causes an off-by-one left shift in cmdline mode; adjust
the value passed to setcmdpos by adding 1. Update the call in M.set_cursor to
pass cursor[2] + 1 (or otherwise convert the 0-based column to the 1-based byte
position expected by vim.fn.setcmdpos) while keeping the existing mode checks
and assertions intact.
---
Nitpick comments:
In `@lua/fzf-lua/actions.lua`:
- Around line 1268-1283: The CmdlineEnter autocmd callback unconditionally uses
newline/newcol and thus can clobber user-typed cmdline when opts.complete
returns nil; modify the callback in the CmdlineEnter block so it checks whether
newline or newcol are non-nil before calling vim.fn.setcmdline and
require("fzf-lua.ctx").set_cursor (leave the existing fallback to
opts.__CTX.line only when newline is intentionally provided), and if both are
nil do nothing; optionally, after the ctx.lua setcmdpos off-by-one fix you can
simplify the cursor math to use (newcol or col) - 1 when calling set_cursor.
In `@lua/fzf-lua/ctx.lua`:
- Around line 124-131: M.get_mode currently treats already-normalized strings
like "cmdline"/"term"/"cmdwin"/"default" as raw modes and can return "default"
incorrectly; update M.get_mode to first short-circuit and return mode if it's
one of the normalized values ("cmdline","term","cmdwin","default"), then proceed
to compute from vim.api.nvim_get_mode() or vim.fn.getcmdwintype(); refer to
M.get_mode and callers that may pass __CTX.mode or functions like
set_cursor(get_mode(), ...) when making this change.
- Around line 149-159: The get_line helper calls M.get_cursor() without passing
the current mode which can re-query nvim_get_mode(); change the fallback branch
in M.get_line so when num == nil it calls M.get_cursor(mode) (i.e. forward the
mode argument) to compute the line index, preserving the existing decrement (-1)
logic and returning the buffer line as before; update references to M.get_cursor
in M.get_line accordingly so behavior is consistent with other helpers that
accept an explicit mode (keep the cmdline assertion and return path unchanged).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8eeadd47-5eff-4d52-9a8d-bc38d43dc46b
📒 Files selected for processing (3)
lua/fzf-lua/actions.lualua/fzf-lua/core.lualua/fzf-lua/ctx.lua
| if not selected[1] then | ||
| if opts.__CTX.mode == "i" then | ||
| if mode == "i" then | ||
| vim.cmd [[noautocmd lua vim.api.nvim_feedkeys('i', 'n', true)]] | ||
| elseif mode == "c" then | ||
| vim.cmd [[noautocmd lua vim.api.nvim_feedkeys(':', 'n', true)]] | ||
| require("fzf-lua.ctx").set_cursor(mode, cursor) | ||
| end | ||
| return | ||
| end |
There was a problem hiding this comment.
Race: set_cursor runs before the new cmdline is actually open.
On the "no selection" path you feed ':' via nvim_feedkeys (which is queued, not synchronous) and then immediately call require("fzf-lua.ctx").set_cursor(mode, cursor). At that point the command line hasn't been entered yet, so vim.fn.setcmdpos(...) inside set_cursor is a no-op / error (setcmdpos "Only works when editing the command line"). The caret restore silently fails on abort.
The success path (Line 1274-1282) already solves this correctly with a one-shot CmdlineEnter autocmd — mirror that here.
🐛 Proposed fix
if not selected[1] then
if mode == "i" then
vim.cmd [[noautocmd lua vim.api.nvim_feedkeys('i', 'n', true)]]
elseif mode == "c" then
+ vim.api.nvim_create_autocmd("CmdlineEnter", {
+ once = true,
+ callback = function()
+ require("fzf-lua.ctx").set_cursor(mode, cursor)
+ end,
+ })
vim.cmd [[noautocmd lua vim.api.nvim_feedkeys(':', 'n', true)]]
- require("fzf-lua.ctx").set_cursor(mode, cursor)
end
return
end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lua/fzf-lua/actions.lua` around lines 1248 - 1256, The no-selection branch
feeds ':' into the cmdline with nvim_feedkeys and immediately calls
require("fzf-lua.ctx").set_cursor(mode, cursor), but set_cursor uses
vim.fn.setcmdpos which is a no-op until the cmdline is actually open; instead
mirror the success path and delay the cursor restore by installing a one-shot
CmdlineEnter autocmd that calls require("fzf-lua.ctx").set_cursor(mode, cursor)
(and then removes itself), rather than calling set_cursor synchronously after
nvim_feedkeys; reference the no-selection block where nvim_feedkeys(':', 'n',
true) is used and the set_cursor function in require("fzf-lua.ctx").
| opts.fzf_opts["--preview-window"] = "hidden:right:0" | ||
| end | ||
|
|
||
| if opts.__CTX.mode == "c" then vim.api.nvim_feedkeys(vim.keycode("<c-c>"), "n", false) end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm min Neovim version declared/used and whether vim.keycode is referenced elsewhere
rg -nP '__HAS_NVIM_0\d+' -C1
rg -nP '\bvim\.keycode\b' -C1
rg -nP '\bsend_ctrl_c\b' -C1Repository: ibhagwan/fzf-lua
Length of output: 7542
🏁 Script executed:
# Get full send_ctrl_c() implementation
sed -n '1093,1100p' lua/fzf-lua/utils.lua
# Check what vim.keycode is and verify the version requirement
grep -A10 "function M.send_ctrl_c" lua/fzf-lua/utils.luaRepository: ibhagwan/fzf-lua
Length of output: 624
🏁 Script executed:
# Check if vim.keycode() is version-gated elsewhere in the codebase
rg -B2 -A2 'vim\.keycode' lua/fzf-lua/Repository: ibhagwan/fzf-lua
Length of output: 665
🏁 Script executed:
# Check test file to see version gating
head -50 tests/win_spec.lua
# Search for any Neovim version info or changelog references
rg -i "keycode|0\.10" --max-count=5 lua/fzf-lua/Repository: ibhagwan/fzf-lua
Length of output: 3097
🌐 Web query:
Neovim vim.keycode function introduction version history when added
💡 Result:
The vim.keycode function was added to Neovim in version 0.10. It translates keycodes in a string, serving as a convenient wrapper around vim.api.nvim_replace_termcodes with default parameters (true, true, true). It was introduced via Pull Request #22960, created on April 8, 2023, and merged on April 25, 2023. The feature is documented in the official Neovim Lua documentation and listed under new Lua features in the 0.10 release notes (news-0.10). Prior to this, users relied on the more verbose nvim_replace_termcodes. Example usage: local k = vim.keycode; vim.g.mapleader = k'<space>'.
Citations:
- 1: feat(lua): vim.keycode neovim/neovim#22960
- 2: https://neovim.io/doc/user/lua/
- 3: https://neovim.io/doc/user/news-0.10/
- 4: https://github.com/neovim/neovim/blob/master/runtime/doc/news-0.10.txt
- 5: neovim/neovim@27fb629
Use utils.send_ctrl_c() instead of vim.keycode().
vim.keycode() was introduced in Neovim 0.10, but lua/fzf-lua/utils.lua already provides M.send_ctrl_c() which uses vim.api.nvim_replace_termcodes() (compatible with all supported Neovim versions). This ensures the <C-c> feeding behavior is version-safe and consistent across the codebase.
♻️ Proposed fix
- if opts.__CTX.mode == "c" then vim.api.nvim_feedkeys(vim.keycode("<c-c>"), "n", false) end
+ if opts.__CTX.mode == "c" then utils.send_ctrl_c() end📝 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.
| if opts.__CTX.mode == "c" then vim.api.nvim_feedkeys(vim.keycode("<c-c>"), "n", false) end | |
| if opts.__CTX.mode == "c" then utils.send_ctrl_c() end |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lua/fzf-lua/core.lua` at line 368, Replace the version-specific vim.keycode
usage: when checking opts.__CTX.mode == "c" call the compatibility helper
utils.send_ctrl_c() and pass its result to vim.api.nvim_feedkeys instead of
vim.keycode(); specifically replace the
vim.api.nvim_feedkeys(vim.keycode("<c-c>"), "n", false) invocation with
vim.api.nvim_feedkeys(utils.send_ctrl_c(), "n", false) while leaving the
opts.__CTX.mode check and feedkeys flags unchanged.
| function M.set_cursor(mode, cursor) | ||
| mode = M.get_mode(mode) | ||
| if vim.tbl_contains({ "default", "term", "cmdwin" }, mode) then | ||
| return vim.api.nvim_win_set_cursor(0, cursor) | ||
| end | ||
|
|
||
| assert(mode == "cmdline", "Unsupported mode for setting cursor: " .. mode) | ||
| assert(cursor[1] == 1, "Cursor must be on the first line in cmdline mode") | ||
| vim.fn.setcmdpos(cursor[2]) | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Neovim vim.fn.setcmdpos() is the position 1-based or 0-based?
💡 Result:
Neovim vim.fn.setcmdpos uses 1-based positioning for the command-line cursor.
Citations:
- 1: https://neovim.io/doc/user/cmdline/
- 2: https://neo.vimhelp.org/cmdline.txt.html
- 3: https://vimhelp.org/cmdline.txt.html
- 4: https://neovim.io/doc/user/vimfn/
- 5: https://neo.vimhelp.org/vimfn.txt.html
🏁 Script executed:
# Find and examine the get_cursor function in ctx.lua
rg -A 15 "function M.get_cursor" lua/fzf-lua/ctx.luaRepository: ibhagwan/fzf-lua
Length of output: 587
🏁 Script executed:
# Also examine the set_cursor function more closely with context
rg -B 5 -A 10 "function M.set_cursor" lua/fzf-lua/ctx.luaRepository: ibhagwan/fzf-lua
Length of output: 587
🏁 Script executed:
# Look for usage of set_cursor in actions.lua, particularly in M.complete
rg -B 5 -A 10 "set_cursor" lua/fzf-lua/actions.luaRepository: ibhagwan/fzf-lua
Length of output: 3671
Off-by-one when restoring cmdline cursor via setcmdpos.
get_cursor returns { 1, getcmdpos() - 1 } (0-based column, matching nvim_win_get_cursor's convention), but set_cursor passes cursor[2] straight into vim.fn.setcmdpos, which expects a 1-based byte position (:help setcmdpos() — "The first position is 1."). The cursor will land one byte to the left of where it was captured.
Reproduction: In actions.lua M.complete with cursor[2]=3, the code calculates { cursor[1], cursor[2] + (newcol or col) - col } which simplifies to { cursor[1], newcol - 1 } (0-based), then setcmdpos(newcol - 1) positions the caret one byte before the last inserted character.
Fix
assert(mode == "cmdline", "Unsupported mode for setting cursor: " .. mode)
assert(cursor[1] == 1, "Cursor must be on the first line in cmdline mode")
- vim.fn.setcmdpos(cursor[2])
+ vim.fn.setcmdpos(cursor[2] + 1)
end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lua/fzf-lua/ctx.lua` around lines 138 - 147, M.set_cursor uses
vim.fn.setcmdpos with a 0-based column from M.get_cursor which causes an
off-by-one left shift in cmdline mode; adjust the value passed to setcmdpos by
adding 1. Update the call in M.set_cursor to pass cursor[2] + 1 (or otherwise
convert the 0-based column to the 1-based byte position expected by
vim.fn.setcmdpos) while keeping the existing mode checks and assertions intact.
1 participant
#2677
Altough I think we need to refactor insert complete first
e.g. to make it work with dot-repeat
Summary by CodeRabbit