Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces a new stringbuffer utility module providing Neovim Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
lua/fzf-lua/utils.lua (1)
21-26: Consider replacing committed debug override comments with an explicit dev toggle.These commented flag overrides are easy to accidentally uncomment and are noisy in a core utility module. Prefer a guarded debug mechanism (e.g., env var or local dev config) instead of committed commented assignments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/fzf-lua/utils.lua` around lines 21 - 26, The file contains commented-out debug flag overrides (M.__HAS_NVIM_010, M.__HAS_NVIM_0102, M.__HAS_NVIM_011, M.__HAS_NVIM_0116, M.__HAS_NVIM_012, M.__IS_WINDOWS) which should be removed and replaced with a guarded dev toggle; add a single explicit development switch (e.g., local DEV_MODE = os.getenv("FZFLUA_DEV") == "1" or a local dev_config table) and when DEV_MODE is true set those M.__HAS_* and M.__IS_WINDOWS values from that toggle or a non-committed dev config, keeping the production defaults intact in the module (update places that reference M.__HAS_NVIM_010 etc. to rely on the computed values).lua/fzf-lua/make_entry.lua (1)
3-3: Drop this stray no-op comment.It reads like disabled control flow rather than documentation, so it is likely to confuse the next person skimming the module header.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/fzf-lua/make_entry.lua` at line 3, Remove the stray no-op comment at the top of the module (the line "-- do return { file = nil } end") in lua/fzf-lua/make_entry.lua; simply delete that commented-out control-flow-looking line so the module header is clear and no misleading disabled code remains, leaving the rest of the file and any functions in make_entry.lua unchanged.lua/fzf-lua/lib/stringbuffer.lua (1)
26-109: Use a shim-local type name for the fallback annotations.The current
---@class vim._core.stringbufferdocs are what trigger the duplicate/invisible-field warnings in the lint job, because those fields are package-private in Neovim's internal type. Giving the fallback its own local class name would keep the docs accurate and quiet the pipeline without changing behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/fzf-lua/lib/stringbuffer.lua` around lines 26 - 109, The file currently uses the internal annotation `---@class vim._core.stringbuffer` which triggers lint warnings; change that fallback to a shim-local class name (e.g. `---@class stringbuffer_fallback`) and update any related annotations to use that local name (replace the top `---@class ...` and the `---@param buf vim._core.stringbuffer` before `M.len`), leaving all runtime code (StrBuffer, M.new, M.len, etc.) unchanged so behavior stays identical while silencing the duplicate/invisible-field warnings.
🤖 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/lib/stringbuffer.lua`:
- Around line 26-110: The fallback StrBuffer is missing the length metamethod
and a ref method used by libuv code; add a __len metamethod on StrBuffer that
returns self.len - self.skip_ptr (so `#strbuf` works) and implement
StrBuffer:ref() to return a compatible reference (e.g., return self or the
internal buffer view) so calls to strbuf:ref() succeed; ensure these are added
alongside existing methods (StrBuffer.__index, StrBuffer:tostring,
StrBuffer:_peak, M.new, M.len) so the fallback behaves like
vim._core.stringbuffer.
In `@lua/fzf-lua/libuv.lua`:
- Around line 236-240: The code appends the character "a" when stdout closes
without a trailing EOL which doesn't help split_lines() (it only looks for
EOL_data), so change the behavior to append the actual EOL delimiter instead of
"a". Specifically, in the output_pipe:is_closing() branch, when len > 0 and
ref[len - 1] ~= EOL_byte, call strbuf:put(...) with the real EOL (use EOL_data
or string.char(EOL_byte)) so split_lines() will treat the final partial buffer
as a complete line, then continue to increment write_cb_count and call
work_ctx:queue(strbuf:get()) as before. Ensure you reference the existing
symbols output_pipe:is_closing(), ref, len, EOL_byte/EOL_data, strbuf:put,
work_ctx:queue and split_lines().
- Around line 128-129: The finish function contains a synchronous
os.execute("notify-send " .. write_cb_count) debug hook that runs on every spawn
completion; remove this call (or wrap it behind a dedicated debug flag) so
finish(code, sig, from, pid) no longer shells out or depends on Linux-only
notify-send; locate the finish function in lua/fzf-lua/libuv.lua and either
delete the os.execute line or gate it behind an opt-in debug variable before
merging.
---
Nitpick comments:
In `@lua/fzf-lua/lib/stringbuffer.lua`:
- Around line 26-109: The file currently uses the internal annotation `---@class
vim._core.stringbuffer` which triggers lint warnings; change that fallback to a
shim-local class name (e.g. `---@class stringbuffer_fallback`) and update any
related annotations to use that local name (replace the top `---@class ...` and
the `---@param buf vim._core.stringbuffer` before `M.len`), leaving all runtime
code (StrBuffer, M.new, M.len, etc.) unchanged so behavior stays identical while
silencing the duplicate/invisible-field warnings.
In `@lua/fzf-lua/make_entry.lua`:
- Line 3: Remove the stray no-op comment at the top of the module (the line "--
do return { file = nil } end") in lua/fzf-lua/make_entry.lua; simply delete that
commented-out control-flow-looking line so the module header is clear and no
misleading disabled code remains, leaving the rest of the file and any functions
in make_entry.lua unchanged.
In `@lua/fzf-lua/utils.lua`:
- Around line 21-26: The file contains commented-out debug flag overrides
(M.__HAS_NVIM_010, M.__HAS_NVIM_0102, M.__HAS_NVIM_011, M.__HAS_NVIM_0116,
M.__HAS_NVIM_012, M.__IS_WINDOWS) which should be removed and replaced with a
guarded dev toggle; add a single explicit development switch (e.g., local
DEV_MODE = os.getenv("FZFLUA_DEV") == "1" or a local dev_config table) and when
DEV_MODE is true set those M.__HAS_* and M.__IS_WINDOWS values from that toggle
or a non-committed dev config, keeping the production defaults intact in the
module (update places that reference M.__HAS_NVIM_010 etc. to rely on the
computed values).
🪄 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: 1b398cfb-092b-4b3a-ad6a-21a30204e8c1
📒 Files selected for processing (4)
lua/fzf-lua/lib/stringbuffer.lualua/fzf-lua/libuv.lualua/fzf-lua/make_entry.lualua/fzf-lua/utils.lua
|
Since all vim function ( |
|
This indeed make st significantly faster, I will try if it's possible to pass the serialized options to worker. |
Manage to make nvim-web-devicons work. It's 3~4x faster on large repo. _G.time = vim.uv.hrtime()
require('fzf-lua').grep({
search = '',
multiprocess = false,
file_icons = true,
path_shorten = 1,
keymap = { fzf = { load = 'abort' } },
winopts = { on_close = function() print((vim.uv.hrtime() - _G.time) * 1e-9) end },
})Before: |
that’s impressive improvement |
278b4ee to
4622a38
Compare
45c2757 to
c268217
Compare
|
is there no stringbuffer in core?(almost certainly) this could be useful there, what do you think 🤔 |
Yes, currently a shim is needed for PUC lua ("vim._core.stringbuffer" is useful but it missing method like Related issue here: neovim/neovim#39316 |
Try to fix #2657 (comment).
cannot work correctly now...
Not sure why it missing some entries.should be correct, I change the file so the grep result is changed...Summary by CodeRabbit
Refactor
Chores