Conversation
📝 WalkthroughWalkthroughThe change modifies preview buffer post-processing in the fzf-lua previewer, making the syntax/markdown/treesitter attachment pipeline run unconditionally during the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Collaborator
Author
|
cc @barrettruth, with this your config can be simplified diff --git a/config/nvim/lua/config/fzf/preview.lua b/config/nvim/lua/config/fzf/preview.lua
index c46621c..1c134a4 100644
--- a/config/nvim/lua/config/fzf/preview.lua
+++ b/config/nvim/lua/config/fzf/preview.lua
@@ -66,65 +66,8 @@ local function make_class(parse)
end
---@param entry_str string
- function P:populate_preview_buf(entry_str)
- if not self.win or not self.win:validate_preview() then
- return
- end
- if self.stop_job then
- self.stop_job()
- self.stop_job = nil
- end
-
- local entry = self:_parse(entry_str)
- self._last_entry = entry_str
- if not entry then
- self:_render({ 'cannot preview:', entry_str })
- return
- end
-
- self._job = vim.system(
- entry.argv,
- { text = true, cwd = self.opts.cwd or vim.uv.cwd() },
- vim.schedule_wrap(function(res)
- if entry_str ~= self._last_entry then
- return
- end
- if not self.win or not self.win:validate_preview() then
- return
- end
- self:_render(self:_format(res))
- end)
- )
- self.stop_job = function()
- if self._job and not self._job:is_closing() then
- self._job:kill(9)
- end
- end
- end
-
- ---@param res vim.SystemCompleted
- ---@return string[]
- function P:_format(res)
- if res.code ~= 0 then
- local lines = { ('git exited %d'):format(res.code), '' }
- vim.list_extend(
- lines,
- vim.split(res.stderr or '', '\n', { plain = true })
- )
- return lines
- end
- return vim.split(res.stdout or '', '\n', { plain = true })
- end
-
- ---@param lines string[]
- function P:_render(lines)
- local buf = self:get_tmp_buffer()
- api.nvim_buf_set_lines(buf, 0, -1, false, lines)
- vim.bo[buf].filetype = 'git'
- self:set_preview_buf(buf)
- if self.win and self.win.update_preview_scrollbar then
- self.win:update_preview_scrollbar()
- end
+ function P:parse_entry(entry_str)
+ return { cmd = self:_parse(entry_str), filetype = 'diff' }
end
return P
@@ -157,29 +100,23 @@ local function parse_status(self, entry_str)
if is_untracked then
local stat = vim.uv.fs_stat(entry.path)
if stat and stat.type == 'directory' then
- return { argv = { 'ls', '-la', entry.path } }
+ return { 'ls', '-la', entry.path }
end
return {
- argv = {
- 'git',
- 'diff',
- '--no-color',
- '--no-index',
- '/dev/null',
- entry.path,
- },
+ 'git',
+ 'diff',
+ '--no-color',
+ '--no-index',
+ '/dev/null',
+ entry.path,
}
end
if is_deleted or is_modified then
- return {
- argv = { 'git', 'diff', '--no-color', 'HEAD', '--', entry.path },
- }
+ return { 'git', 'diff', '--no-color', 'HEAD', '--', entry.path }
end
- return {
- argv = { 'git', 'diff', '--no-color', 'HEAD', '--', entry.path },
- }
+ return { 'git', 'diff', '--no-color', 'HEAD', '--', entry.path }
end
---@param _ fzf.preview.Previewer
@@ -190,7 +127,7 @@ local function parse_commits(_, entry_str)
if not sha then
return nil
end
- return { argv = { 'git', 'show', '--no-color', sha } }
+ return { 'git', 'show', '--no-color', sha }
end
---@param self fzf.preview.Previewer
@@ -201,9 +138,7 @@ local function parse_bcommits(self, entry_str)
if not sha or not self._file then
return nil
end
- return {
- argv = { 'git', 'show', '--no-color', sha, '--', self._file },
- }
+ return { 'git', 'show', '--no-color', sha, '--', self._file }
end
---@param self fzf.preview.Previewer
@@ -228,7 +163,7 @@ local function parse_diff(self, entry_str)
end
table.insert(argv, '--')
table.insert(argv, entry.path)
- return { argv = argv }
+ return argv
end
---@param _ fzf.preview.Previewer
@@ -239,9 +174,7 @@ local function parse_stash(_, entry_str)
if not ref then
return nil
end
- return {
- argv = { 'git', 'stash', 'show', '--patch', '--no-color', ref },
- }
+ return { 'git', 'stash', 'show', '--patch', '--no-color', ref }
end
---@param self fzf.preview.Previewer
@@ -252,9 +185,7 @@ local function parse_blame(self, entry_str)
if not sha or not self._file then
return nil
end
- return {
- argv = { 'git', 'show', '--no-color', sha, '--', self._file },
- }
+ return { 'git', 'show', '--no-color', sha, '--', self._file }
end
local M = {} |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lua/fzf-lua/previewer/builtin.lua (1)
1187-1191: Narrow allow-list for FileType re-dispatch — double-check edge cases.The special case is deliberately scoped to
diff/gitto work aroundfiletype_detect'seventignore("FileType"). Two minor things worth confirming:
- When
entry.filetype == "diff"or"git"(e.g. from_populate_loaded_buffer_previewwhereentry.filetype = vim.bo[entry.bufnr].filetype),filetype_detectis never called, sovim.bo[bufnr].filetypeis still""at the moment this autocmd fires. AnyFileTypeautocmd consumer that reads&filetype(rather than the dispatched pattern) would see an empty value. Core ftplugins generally don't, but plugin-registered ones (fugitive, gitsigns helpers, etc.) might. Consider settingvim.bo[bufnr].filetype = ftimmediately before the dispatch to keep buffer state consistent.- Are there other filetypes that need the same treatment (e.g.
fugitive,gitcommit,diff-adjacent), or is diff/git the complete set from the linked issue? A brief comment documenting why only these two would help future readers.Optional nit: set `&filetype` before re-dispatch
if ft == "diff" or ft == "git" then api.nvim_buf_call(bufnr, function() + vim.bo[bufnr].filetype = ft api.nvim_exec_autocmds("FileType", { pattern = ft, modeline = false }) end) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/fzf-lua/previewer/builtin.lua` around lines 1187 - 1191, The FileType re-dispatch for ft == "diff" or "git" can leave vim.bo[bufnr].filetype empty for autocmd handlers that read &filetype; before calling api.nvim_exec_autocmds set vim.bo[bufnr].filetype = ft to keep buffer state consistent (while still using api.nvim_buf_call(bufnr, ...)); also add a brief comment near the ft/git conditional explaining why only these filetypes are re-dispatched (reference the filetype_detect/eventignore issue) and note to consider related filetypes (e.g., fugitive/gitcommit) if needed so future readers understand the narrow allow-list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lua/fzf-lua/previewer/builtin.lua`:
- Around line 1187-1191: The FileType re-dispatch for ft == "diff" or "git" can
leave vim.bo[bufnr].filetype empty for autocmd handlers that read &filetype;
before calling api.nvim_exec_autocmds set vim.bo[bufnr].filetype = ft to keep
buffer state consistent (while still using api.nvim_buf_call(bufnr, ...)); also
add a brief comment near the ft/git conditional explaining why only these
filetypes are re-dispatched (reference the filetype_detect/eventignore issue)
and note to consider related filetypes (e.g., fugitive/gitcommit) if needed so
future readers understand the narrow allow-list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 55c63076-eed3-4748-80db-7dd635c3922a
📒 Files selected for processing (1)
lua/fzf-lua/previewer/builtin.lua
Contributor
|
Def better and more elegant but the highlighting is not quite as good as my overly-complex approach (lol) because of fzf-lua terminal-pty-like rendering. See the local of highlighting on the index.. lines and +++ lines. 
Would like this in main regardless. |
Collaborator
Author
|
right 'git' seems always better than 'diff' in this case index c96684c2..40599687 100644
@@ -58,7 +58,7 @@ local function make_class(parse)
end
---@param entry_str string
- function P:parse_entry(entry_str) return { cmd = self:_parse(entry_str), filetype = 'diff' } end
+ function P:parse_entry(entry_str) return { cmd = self:_parse(entry_str), filetype = 'git' } end
return P
end |
Contributor
|
@phanen can this be marked as ready? |
Collaborator
Author
|
we can merge it now. i forgot why i skip syntax hl on terminal buffer before, maybe for no reason |
Collaborator
Author
|
wondering if we can emit ft event for all filetype... or is there way to kown "lsp" filetype to skip (maybe via get_autocmd or something) not sure will nvim attach lsp on special buftype? |
Contributor
@phanen so clean! thank you! |
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.
Related #2694 (comment)
Summary by CodeRabbit
Bug Fixes