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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 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 |
phanen
force-pushed
the
worker
branch
7 times, most recently
from
278b4ee to
4622a38
Compare
phanen
force-pushed
the
worker
branch
3 times, most recently
from
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 |
FYI, I previously added support for |
4 participants
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