fix(keymaps): resume precompile module path is possible#2676
Merged
Conversation
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 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 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: 1
🤖 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/path.lua`:
- Around line 688-693: The returned entry lacks the required "stripped" field,
breaking file actions that expect "entry.stripped" (see actions.lua:656 and
entry_to_file). In the function that builds the entry (the code returning { path
= source, line = info.linedefined } in path.lua), add a stripped field formatted
as "path:line" (e.g., source .. ":" .. tostring(info.linedefined)) and ensure
any other branches that set path also set stripped the same way so consumers
like keymap_to_entry / entry_to_file receive a proper "stripped" string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Collaborator
Author
linedefined is still missing |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
lua/fzf-lua/path.lua (2)
688-693:⚠️ Potential issue | 🟠 MajorPopulate
strippedwhen returning a keymap file target.This branch still returns
pathwithoutstripped, butlua/fzf-lua/actions.lua:650-660passesentry.strippedintofile_edit/file_split/file_vsplit/file_tabedit. Build apath:linestring here so callback-backed keymaps remain actionable.Proposed fix
if source:match("vim/") and package.preload[source:gsub("%.lua$", ""):gsub("/", ".")] then source = vim.env.VIMRUNTIME .. "/lua/" .. source end - return { path = source, line = info.linedefined } + local line = info.linedefined > 0 and info.linedefined or 1 + return { + path = source, + line = line, + stripped = ("%s:%d"):format(source, line), + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/fzf-lua/path.lua` around lines 688 - 693, The returned table from this branch sets only path and line but not stripped, so callback-backed keymaps break when actions.lua expects entry.stripped; update the return to include stripped built from the resolved source and info.linedefined (e.g., source .. ":" .. info.linedefined) so that entry.stripped is populated for downstream uses in file_edit/file_split/file_vsplit/file_tabedit.
688-693:⚠️ Potential issue | 🟠 MajorPopulate
strippedwhen returning a keymap file target.This branch still returns
pathwithoutstripped, butlua/fzf-lua/actions.lua:650-660passesentry.strippedintofile_edit/file_split/file_vsplit/file_tabedit. Build apath:linestring here so callback-backed keymaps remain actionable.Proposed fix
if source:match("vim/") and package.preload[source:gsub("%.lua$", ""):gsub("/", ".")] then source = vim.env.VIMRUNTIME .. "/lua/" .. source end - return { path = source, line = info.linedefined } + local line = info.linedefined > 0 and info.linedefined or 1 + return { + path = source, + line = line, + stripped = ("%s:%d"):format(source, line), + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/fzf-lua/path.lua` around lines 688 - 693, The branch that handles runtime vim lua files currently returns { path = source, line = info.linedefined } but does not set the stripped field used by callbacks; update the return value in that branch (the block using source:match("vim/") and package.preload) to include stripped constructed as a "path:line" string (e.g., source .. ":" .. info.linedefined) so that downstream callers like file_edit/file_split/file_vsplit/file_tabedit (which use entry.stripped) receive the actionable target.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lua/fzf-lua/path.lua`:
- Around line 688-693: The returned table from this branch sets only path and
line but not stripped, so callback-backed keymaps break when actions.lua expects
entry.stripped; update the return to include stripped built from the resolved
source and info.linedefined (e.g., source .. ":" .. info.linedefined) so that
entry.stripped is populated for downstream uses in
file_edit/file_split/file_vsplit/file_tabedit.
- Around line 688-693: The branch that handles runtime vim lua files currently
returns { path = source, line = info.linedefined } but does not set the stripped
field used by callbacks; update the return value in that branch (the block using
source:match("vim/") and package.preload) to include stripped constructed as a
"path:line" string (e.g., source .. ":" .. info.linedefined) so that downstream
callers like file_edit/file_split/file_vsplit/file_tabedit (which use
entry.stripped) receive the actionable target.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a6235fef-f900-412d-8878-ebfbdca54b97
📒 Files selected for processing (3)
lua/fzf-lua/path.lualua/fzf-lua/profiles/hide.lualua/fzf-lua/providers/nvim.lua
✅ Files skipped from review due to trivial changes (1)
- lua/fzf-lua/profiles/hide.lua
phanen
force-pushed
the
precompile-module-path
branch
from
0807c43 to
376d760
Compare
phanen
force-pushed
the
precompile-module-path
branch
from
376d760 to
36ff649
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
lua/fzf-lua/path.lua (1)
686-694:⚠️ Potential issue | 🟠 MajorMissing
strippedfield will still breakkeymap_{edit,split,vsplit,tabedit}actions; also anchor thevim/match.
actions.lua:654-660passesentry.strippedintofile_<fname>; since this return path (and theverb mapfallback at line 699) only setspath/line, consumers receive{ nil }and the action silently no-ops. This was raised on the previous revision and is unresolved.source:match("vim/")on line 690 is unanchored — any path containingvim/anywhere (e.g., a user plugin at~/.config/nvim/pack/.../vim/foo.lua) will hit theVIMRUNTIMErewrite if a same-named key happens to exist inpackage.preload. Per the linkedexecutor.cbehavior, these chunknames start withvim/, so anchor it.Proposed fix
if vmap.callback then local info = debug.getinfo(vmap.callback, "Sl") if info and info.source and info.linedefined and info.linedefined > 0 then local source = info.source:gsub("^@", "") -- https://github.com/neovim/neovim/blob/64d55b74d83d566975e269bed0810d9008119ddf/src/nvim/lua/executor.c#L671-L680 - if source:match("vim/") and package.preload[source:gsub("%.lua$", ""):gsub("/", ".")] then + if source:match("^vim/") and package.preload[source:gsub("%.lua$", ""):gsub("/", ".")] then source = vim.env.VIMRUNTIME .. "/lua/" .. source .. ".lua" end - return { path = source, line = info.linedefined } + return { + path = source, + line = info.linedefined, + stripped = ("%s:%d"):format(source, info.linedefined), + } end end local cmd = ("verb %smap %s"):format(mode, keymap) local output = vim.split(vim.api.nvim_exec2(cmd, { output = true }).output, "\n") local file, lnum = (output[`#output`] or ""):match("Last set from (.-) line (%d+)") - if file and lnum then return { path = vim.fs.normalize(file), line = utils.tointeger(lnum) or 1 } end + if file and lnum then + file = vim.fs.normalize(file) + lnum = utils.tointeger(lnum) or 1 + return { path = file, line = lnum, stripped = ("%s:%d"):format(file, lnum) } + end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/fzf-lua/path.lua` around lines 686 - 694, The returned table from the debug-info branch in path.lua currently only sets path and line which causes consumers like the file_<fname> actions in actions.lua (which expect entry.stripped) to no-op; update the return to include a stripped field (the basename or the appropriate stripped value used elsewhere) so callers receive entry.stripped, and also tighten the runtime rewrite match by anchoring the pattern to "^vim/" (i.e., change source:match("vim/") to source:match("^vim/")) so only chunknames that start with "vim/" are rewritten to VIMRUNTIME.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lua/fzf-lua/path.lua`:
- Around line 686-694: The returned table from the debug-info branch in path.lua
currently only sets path and line which causes consumers like the file_<fname>
actions in actions.lua (which expect entry.stripped) to no-op; update the return
to include a stripped field (the basename or the appropriate stripped value used
elsewhere) so callers receive entry.stripped, and also tighten the runtime
rewrite match by anchoring the pattern to "^vim/" (i.e., change
source:match("vim/") to source:match("^vim/")) so only chunknames that start
with "vim/" are rewritten to VIMRUNTIME.
Collaborator
Author
|
This will automatically be fixed (hopefully when my upstream bytecode fix PR merged). |
Owner
|
Tysm @phanen! |
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.
Fix #2674
https://github.com/neovim/neovim/blob/64d55b74d83d566975e269bed0810d9008119ddf/src/nvim/lua/executor.c#L671-L680
Summary by CodeRabbit