fix: declarative build for telescope-fzf-native.nvim - #867
Conversation
Add home-manager activation script to compile libfzf.so on switch, pcall guard for fzf extension loading, and e2e init.lua loading test. Entire-Checkpoint: 9fc65125b062
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
Summary of ChangesHello @shunkakinoki, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the robustness and maintainability of the Neovim configuration by introducing a declarative build mechanism for telescope-fzf-native.nvim, ensuring its native component is compiled reliably. It also improves error handling for plugin loading and adds a comprehensive end-to-end test to validate the entire init.lua configuration, preventing unexpected startup issues. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Mesa DescriptionTL;DRThis PR introduces a declarative build for What changed?
Description generated by Mesa. Update settings |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a declarative build process for the telescope-fzf-native.nvim plugin, which is a solid improvement for configuration robustness. The changes are well-structured, including a Nix home-manager activation script, a defensive wrapper in the Lua configuration, and a new end-to-end test. I've identified a small bug in the Nix script where the library extension is hardcoded, which will cause issues on macOS. I also found that the new test case captures errors but doesn't assert on them, making it less effective. Addressing these points will make this contribution even better.
| nvimPackLockJson = ./nvim-pack-lock.json; | ||
| packDir = "$HOME/.local/share/nvim/site/pack"; | ||
| buildTools = if pkgs.stdenv.isDarwin then [ pkgs.gnumake pkgs.clang ] else [ pkgs.gnumake pkgs.gcc ]; | ||
| libExt = "so"; |
There was a problem hiding this comment.
The dynamic library extension is hardcoded to so. On macOS, the extension for shared libraries is .dylib. This will cause the activation script to fail to detect an already built library on macOS, leading to unnecessary rebuilds. The libExt variable should be set conditionally based on the operating system.
libExt = if pkgs.stdenv.isDarwin then "dylib" else "so";
| describe("e2e init.lua loading", function() | ||
| it("should load full config without Lua errors", function() | ||
| -- Capture any Lua errors that occur during init.lua sourcing | ||
| local errors = {} |
There was a problem hiding this comment.
The errors table is initialized here to capture errors from vim.notify, but its contents are never checked later in the test. This means that non-fatal errors that are reported via vim.notify will be missed. Please add an assertion to check if this table is empty after sourcing init.lua. For example: assert.is_empty(errors, "init.lua produced errors via vim.notify: " .. table.concat(errors, "\\n"))
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Neovim activation that finds and optionally builds the telescope-fzf-native native plugin, an e2e test that sources init.lua and captures Lua errors, keymap and UI tweaks, and adds build tools to the dev environment and test runner. Changes
Sequence Diagram(s)sequenceDiagram
participant Activation as Activation Hook
participant Env as PATH Env
participant Locator as Pack Locator
participant FS as Filesystem
participant Builder as Make Builder
Activation->>Env: Prepend buildTools (gnumake, clang/gcc)
Activation->>Locator: Search `packDir` for telescope-fzf-native.nvim (opt/start)
Locator-->>FS: Return candidate plugin path(s)
FS->>FS: Check for built lib `build/libfzf.${libExt}`
alt built lib missing
FS->>Builder: Run `make -C <path> clean all`
Builder-->>Activation: Build finished
else built lib present
FS-->>Activation: Skip build
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 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.
Pull request overview
This pull request adds declarative build support for the telescope-fzf-native.nvim Neovim plugin, which requires native compilation. The PR addresses the E5113 error that occurs when the fzf extension fails to load due to missing native library.
Changes:
- Added home-manager activation script to compile
libfzf.soon system activation when the library is missing - Wrapped telescope fzf extension loading in
pcallfor defensive error handling - Added end-to-end test to verify full init.lua loads without errors
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| home-manager/programs/neovim/default.nix | Added activation script to build telescope-fzf-native plugin and defined build tools/library extension variables |
| home-manager/programs/neovim/lua/config/telescope.lua | Wrapped fzf extension loading in pcall to handle build failures gracefully |
| home-manager/programs/neovim/tests/plugins_spec.lua | Added e2e test that sources full init.lua to catch plugin loading errors |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| it("should load full config without Lua errors", function() | ||
| -- Capture any Lua errors that occur during init.lua sourcing | ||
| local errors = {} | ||
| local orig_notify = vim.notify | ||
| vim.notify = function(msg, level) | ||
| if level == vim.log.levels.ERROR then | ||
| table.insert(errors, msg) | ||
| end | ||
| end | ||
|
|
||
| -- Locate init.lua relative to this test file | ||
| local tests_dir = debug.getinfo(1, "S").source:sub(2):match("(.*/)") | ||
| local nvim_dir = vim.fn.fnamemodify(tests_dir, ":h") | ||
| local init_lua = nvim_dir .. "/init.lua" | ||
|
|
||
| local ok, err = pcall(function() | ||
| vim.cmd("source " .. init_lua) | ||
| end) | ||
|
|
||
| vim.notify = orig_notify | ||
|
|
||
| -- If pcall caught an error, the config has a loading problem | ||
| if not ok then | ||
| -- Allow known CI-only failures (missing plugins in test env) | ||
| -- but flag unexpected errors | ||
| assert.is_true( | ||
| ok, | ||
| "init.lua raised an error: " .. tostring(err) | ||
| ) | ||
| end |
There was a problem hiding this comment.
Sourcing the full init.lua in a test can have significant side effects, including loading all plugins, setting up autocommands, and potentially interfering with other tests. The test runs within a test suite that uses minimal_init.lua to isolate tests, but then sources the full config which could override test isolation settings. Consider whether this test should run in complete isolation (separate test file run independently) or use a more targeted approach to test specific plugin loading without sourcing the entire configuration.
| it("should load full config without Lua errors", function() | |
| -- Capture any Lua errors that occur during init.lua sourcing | |
| local errors = {} | |
| local orig_notify = vim.notify | |
| vim.notify = function(msg, level) | |
| if level == vim.log.levels.ERROR then | |
| table.insert(errors, msg) | |
| end | |
| end | |
| -- Locate init.lua relative to this test file | |
| local tests_dir = debug.getinfo(1, "S").source:sub(2):match("(.*/)") | |
| local nvim_dir = vim.fn.fnamemodify(tests_dir, ":h") | |
| local init_lua = nvim_dir .. "/init.lua" | |
| local ok, err = pcall(function() | |
| vim.cmd("source " .. init_lua) | |
| end) | |
| vim.notify = orig_notify | |
| -- If pcall caught an error, the config has a loading problem | |
| if not ok then | |
| -- Allow known CI-only failures (missing plugins in test env) | |
| -- but flag unexpected errors | |
| assert.is_true( | |
| ok, | |
| "init.lua raised an error: " .. tostring(err) | |
| ) | |
| end | |
| it("should have an init.lua file present", function() | |
| -- Locate init.lua relative to this test file without sourcing it, | |
| -- to avoid side effects in the minimal test environment. | |
| local tests_dir = debug.getinfo(1, "S").source:sub(2):match("(.*/)") | |
| local nvim_dir = vim.fn.fnamemodify(tests_dir, ":h") | |
| local init_lua = nvim_dir .. "/init.lua" | |
| -- Assert that init.lua exists and is readable. | |
| local readable = vim.fn.filereadable(init_lua) | |
| assert.is_true( | |
| readable == 1, | |
| "init.lua is missing or not readable at path: " .. tostring(init_lua) | |
| ) |
| nvimPackLockJson = ./nvim-pack-lock.json; | ||
| packDir = "$HOME/.local/share/nvim/site/pack"; | ||
| buildTools = if pkgs.stdenv.isDarwin then [ pkgs.gnumake pkgs.clang ] else [ pkgs.gnumake pkgs.gcc ]; | ||
| libExt = "so"; |
There was a problem hiding this comment.
On macOS/Darwin, shared libraries typically use the .dylib extension instead of .so. The libExt variable should be set conditionally based on the platform, similar to how buildTools is set. This will cause the build check to fail on macOS even when the library is successfully built, as the script will look for libfzf.so instead of libfzf.dylib.
| libExt = "so"; | |
| libExt = if pkgs.stdenv.isDarwin then "dylib" else "so"; |
| telescope.load_extension("gh") | ||
| telescope.load_extension("fzf") | ||
| pcall(telescope.load_extension, "fzf") | ||
| telescope.load_extension("ui-select") |
There was a problem hiding this comment.
Only the "fzf" extension is wrapped in pcall, while "gh" and "ui-select" extensions are called directly without error handling. For consistency, if the fzf extension requires defensive pcall handling due to potential build issues, consider whether the other extensions should also be wrapped in pcall, or alternatively, document why only fzf needs this special handling.
| done | ||
| if [ -n "$fzf_dir" ] && [ ! -f "$fzf_dir/build/libfzf.${libExt}" ]; then | ||
| echo "Building telescope-fzf-native.nvim in $fzf_dir..." | ||
| $DRY_RUN_CMD make -C "$fzf_dir" clean all |
There was a problem hiding this comment.
The activation script doesn't ensure the build directory exists before running make. While make typically creates its output directories, it's safer to explicitly create the build directory or verify it exists after the build completes. Additionally, there's no error handling if the make command fails - the script will silently continue even if compilation fails.
| $DRY_RUN_CMD make -C "$fzf_dir" clean all | |
| build_dir="$fzf_dir/build" | |
| $DRY_RUN_CMD mkdir -p "$build_dir" | |
| if ! $DRY_RUN_CMD make -C "$fzf_dir" clean all; then | |
| echo "Error: Failed to build telescope-fzf-native.nvim in $fzf_dir" >&2 | |
| exit 1 | |
| fi |
| end) | ||
|
|
||
| vim.notify = orig_notify | ||
|
|
There was a problem hiding this comment.
The test captures errors in the errors table by overriding vim.notify, but never checks this table. If errors are logged via vim.notify rather than thrown as exceptions, they would be silently ignored. Consider adding an assertion to check that the errors table is empty after restoring the original notify function.
| -- Ensure no error-level notifications were emitted during init.lua loading | |
| if #errors > 0 then | |
| assert.is_true( | |
| false, | |
| "init.lua logged error notifications via vim.notify:\n" | |
| .. table.concat(errors, "\n") | |
| ) | |
| end |
| if not ok then | ||
| -- Allow known CI-only failures (missing plugins in test env) | ||
| -- but flag unexpected errors | ||
| assert.is_true( | ||
| ok, | ||
| "init.lua raised an error: " .. tostring(err) | ||
| ) | ||
| end |
There was a problem hiding this comment.
The conditional check if not ok then assert.is_true(ok, ...) is redundant because if ok is false, the assertion will always fail. The assertion could be moved outside the conditional block to be more straightforward. Additionally, the comment mentions "Allow known CI-only failures" but the code doesn't actually allow any failures - it always asserts that ok must be true.
| if not ok then | |
| -- Allow known CI-only failures (missing plugins in test env) | |
| -- but flag unexpected errors | |
| assert.is_true( | |
| ok, | |
| "init.lua raised an error: " .. tostring(err) | |
| ) | |
| end | |
| assert.is_true( | |
| ok, | |
| "init.lua raised an error: " .. tostring(err) | |
| ) |
Entire-Checkpoint: ffed18b6dbd7
![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 (2)
home-manager/programs/neovim/lua/config/telescope.lua (1)
43-43: Silentpcalldiscards the error — consider surfacing a warning.The pcall itself is the right call, but swallowing the return value means users silently get unaccelerated sorting with no diagnostic when the native library is missing or fails to load. LazyVim's reference implementation captures the result and emits a targeted warning/error.
✨ Suggested improvement
-pcall(telescope.load_extension, "fzf") +local fzf_ok, fzf_err = pcall(telescope.load_extension, "fzf") +if not fzf_ok then + vim.notify("telescope-fzf-native: " .. tostring(fzf_err), vim.log.levels.WARN) +end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@home-manager/programs/neovim/lua/config/telescope.lua` at line 43, The current pcall(telescope.load_extension, "fzf") silently discards errors; change it to capture pcall's return values (e.g., local ok, err = pcall(telescope.load_extension, "fzf")) and, if not ok, surface a warning using the Neovim notifier (e.g., vim.notify or vim.notify_once) with a clear message mentioning telescope.load_extension and "fzf" plus the err text so users know why the fzf native sorter failed to load.home-manager/programs/neovim/default.nix (1)
40-53: Activation won't rebuild after a plugin update.The guard
[ ! -f "$fzf_dir/build/libfzf.so" ]is correct for the initial build, but iftelescope-fzf-native.nvimis updated to a new version (the existinglibfzf.sostays in place), the activation will silently skip the rebuild and the stale binary remains loaded. Consider also checking a content hash or the plugin's revision file, or documenting that a manualmake clean allis required after updates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@home-manager/programs/neovim/default.nix` around lines 40 - 53, The activation currently skips rebuilding when libfzf.${libExt} already exists, so updates to telescope-fzf-native.nvim won't trigger a rebuild; update the condition in home.activation.buildNvimNativePlugins to also rebuild when the plugin directory is newer than the built library by changing the check around fzf_dir and libfzf.${libExt} to something like: test rebuild if libfzf.${libExt} is missing OR if "$fzf_dir" is newer than "$fzf_dir/build/libfzf.${libExt}" (use the -nt test on fzf_dir vs the built file) and still run $DRY_RUN_CMD make -C "$fzf_dir" clean all when that test passes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@home-manager/programs/neovim/tests/plugins_spec.lua`:
- Around line 85-91: The test monkey-patches vim.notify to collect ERROR
messages into the local errors table but never asserts against it, and the pcall
result (ok) is unconditionally asserted via assert.is_true(ok,…), contradicting
the "allow CI-only failures" intent; update the test around the init.lua
sourcing to either remove the vim.notify override or add an assertion like
assert(`#errors` == 0) after the pcall, and change the assert.is_true(ok, ...)
logic to pattern-match known benign CI errors from the pcall error (ok, err =
pcall(...)) and only fail the test for unexpected errors—use the errors table
and error pattern matching together to allow known missing-plugin messages while
failing on other errors (referencing errors, vim.notify, ok, and the pcall that
sources init.lua).
- Line 99: The call using vim.cmd("source " .. init_lua) can break when init_lua
contains spaces; update the code that constructs the source command (the vim.cmd
call referencing init_lua) to escape the filename first (use vim.fn.fnameescape
on init_lua) or otherwise quote/escape the path before concatenation so the
sourced path is treated as a single argument by :source.
- Around line 94-96: tests_dir is being computed with a trailing slash and then
passed into vim.fn.fnamemodify via nvim_dir = vim.fn.fnamemodify(tests_dir,
":h"), which returns the same directory instead of its parent, causing init_lua
to point to tests/init.lua; fix by deriving the parent directory from the source
file itself (use debug.getinfo(1, "S").source:sub(2) as the file path) and call
vim.fn.fnamemodify on that with a double :h (or ":h:h") to get the grandparent
neovim directory, then build init_lua from that nvim_dir variable (replace the
tests_dir → nvim_dir computation with this approach referencing debug.getinfo,
tests_dir, nvim_dir, and init_lua).
---
Nitpick comments:
In `@home-manager/programs/neovim/default.nix`:
- Around line 40-53: The activation currently skips rebuilding when
libfzf.${libExt} already exists, so updates to telescope-fzf-native.nvim won't
trigger a rebuild; update the condition in
home.activation.buildNvimNativePlugins to also rebuild when the plugin directory
is newer than the built library by changing the check around fzf_dir and
libfzf.${libExt} to something like: test rebuild if libfzf.${libExt} is missing
OR if "$fzf_dir" is newer than "$fzf_dir/build/libfzf.${libExt}" (use the -nt
test on fzf_dir vs the built file) and still run $DRY_RUN_CMD make -C "$fzf_dir"
clean all when that test passes.
In `@home-manager/programs/neovim/lua/config/telescope.lua`:
- Line 43: The current pcall(telescope.load_extension, "fzf") silently discards
errors; change it to capture pcall's return values (e.g., local ok, err =
pcall(telescope.load_extension, "fzf")) and, if not ok, surface a warning using
the Neovim notifier (e.g., vim.notify or vim.notify_once) with a clear message
mentioning telescope.load_extension and "fzf" plus the err text so users know
why the fzf native sorter failed to load.
| local errors = {} | ||
| local orig_notify = vim.notify | ||
| vim.notify = function(msg, level) | ||
| if level == vim.log.levels.ERROR then | ||
| table.insert(errors, msg) | ||
| end | ||
| end |
There was a problem hiding this comment.
errors table is collected but never asserted; the "allow CI failures" comment is a no-op.
Two issues in the same block:
-
Dead monitoring code:
vim.notifyis monkey-patched to collectERROR-level messages intoerrors, buterrorsis never checked after the pcall. Anyvim.notify(…, ERROR)call duringinit.luasourcing is silently ignored. Either assert#errors == 0or remove the monkey-patching. -
Misleading comment vs. actual behaviour: The comment on line 106 says "allow known CI-only failures (missing plugins in test env)", but the code on line 108 calls
assert.is_true(ok, …)which unconditionally fails the test on any pcall error — there is no actual allowlist. If the intent is genuinely to tolerate known-missing-plugin errors in CI, the error message needs to be pattern-matched.
✨ Suggested improvement
local ok, err = pcall(function()
vim.cmd("source " .. vim.fn.fnameescape(init_lua))
end)
vim.notify = orig_notify
+ -- Assert no ERROR-level notifications were fired
+ assert.are.same({}, errors, "init.lua emitted ERROR notifications: " .. vim.inspect(errors))
+
-- If pcall caught an error, the config has a loading problem
if not ok then
- -- Allow known CI-only failures (missing plugins in test env)
- -- but flag unexpected errors
- assert.is_true(
- ok,
- "init.lua raised an error: " .. tostring(err)
- )
+ -- Tolerate "module not found" errors in CI (no plugins installed)
+ local is_missing_module = tostring(err):find("module .* not found") ~= nil
+ assert.is_true(is_missing_module,
+ "init.lua raised an unexpected error: " .. tostring(err))
endAlso applies to: 104-112
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@home-manager/programs/neovim/tests/plugins_spec.lua` around lines 85 - 91,
The test monkey-patches vim.notify to collect ERROR messages into the local
errors table but never asserts against it, and the pcall result (ok) is
unconditionally asserted via assert.is_true(ok,…), contradicting the "allow
CI-only failures" intent; update the test around the init.lua sourcing to either
remove the vim.notify override or add an assertion like assert(`#errors` == 0)
after the pcall, and change the assert.is_true(ok, ...) logic to pattern-match
known benign CI errors from the pcall error (ok, err = pcall(...)) and only fail
the test for unexpected errors—use the errors table and error pattern matching
together to allow known missing-plugin messages while failing on other errors
(referencing errors, vim.notify, ok, and the pcall that sources init.lua).
| local init_lua = nvim_dir .. "/init.lua" | ||
|
|
||
| local ok, err = pcall(function() | ||
| vim.cmd("source " .. init_lua) |
There was a problem hiding this comment.
Missing fnameescape in vim.cmd("source …").
If init_lua contains spaces (common in home directories on macOS/Linux), the unquoted path will be split by :source at the first space.
🛡️ Proposed fix
- vim.cmd("source " .. init_lua)
+ vim.cmd("source " .. vim.fn.fnameescape(init_lua))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@home-manager/programs/neovim/tests/plugins_spec.lua` at line 99, The call
using vim.cmd("source " .. init_lua) can break when init_lua contains spaces;
update the code that constructs the source command (the vim.cmd call referencing
init_lua) to escape the filename first (use vim.fn.fnameescape on init_lua) or
otherwise quote/escape the path before concatenation so the sourced path is
treated as a single argument by :source.
Entire-Checkpoint: 4b6a54ef1df8
Entire-Checkpoint: 305bd7276e83
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
home-manager/programs/neovim/lua/config/keymaps.lua (1)
210-217:<leader>ffand<leader>fpare now identical — consider differentiating or removing one.The
require('fff').find_files()call is the correct public API perdmtrKovalenko/fff.nvim's documentation. However, both<leader>ffand<leader>fpnow invoke the exact same function with no distinguishing arguments, making one of them redundant. If<leader>fpwas previously mapped to a different action (e.g.,open()with a path argument), consider whether the intended distinction has been lost.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@home-manager/programs/neovim/lua/config/keymaps.lua` around lines 210 - 217, The two mappings using keymap("n", "<leader>ff", ...) and keymap("n", "<leader>fp", ...) both call require("fff").find_files() and are therefore redundant; either remove one mapping or restore the intended alternate behavior for "<leader>fp". If the intent was to open a specific path or use a different API, change the "<leader>fp" handler to call the appropriate fff method (e.g., require("fff").open(path) or require("fff").find_files({root = "..."}) per the plugin API) and update the desc accordingly so "<leader>ff" remains the generic file picker and "<leader>fp" becomes the distinct path/open variant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@home-manager/programs/neovim/lua/config/keymaps.lua`:
- Around line 43-50: The mapping using vim.api.nvim_tabpage_list_wins(0)
incorrectly counts floating windows; update the anonymous function bound by
keymap("n", "<leader>q", ...) to compute non-floating windows by filtering the
list from vim.api.nvim_tabpage_list_wins(0) with
vim.api.nvim_win_get_config(win).relative == "" (or equivalent check) and use
that count for the conditional (if non_floating_count > 1 then vim.cmd("close")
else vim.cmd("Bdelete")). Leave the rest of the mapping and opts intact.
In `@home-manager/programs/neovim/lua/config/settings.lua`:
- Line 106: The fillchars configuration uses an invalid key; update the call to
vim.opt.fillchars:append by removing the unrecognized "vertsplit" key so only
valid keys are set (e.g., keep vert = " " or add vertleft/vertright/verthoriz if
needed). Locate the vim.opt.fillchars:append invocation and remove "vertsplit"
from the table to avoid startup errors or silent ignores.
---
Nitpick comments:
In `@home-manager/programs/neovim/lua/config/keymaps.lua`:
- Around line 210-217: The two mappings using keymap("n", "<leader>ff", ...) and
keymap("n", "<leader>fp", ...) both call require("fff").find_files() and are
therefore redundant; either remove one mapping or restore the intended alternate
behavior for "<leader>fp". If the intent was to open a specific path or use a
different API, change the "<leader>fp" handler to call the appropriate fff
method (e.g., require("fff").open(path) or require("fff").find_files({root =
"..."}) per the plugin API) and update the desc accordingly so "<leader>ff"
remains the generic file picker and "<leader>fp" becomes the distinct path/open
variant.
| -- @keymap <leader>q: Close current window/split, or delete buffer if last window | ||
| keymap("n", "<leader>q", function() | ||
| if #vim.api.nvim_tabpage_list_wins(0) > 1 then | ||
| vim.cmd("close") | ||
| else | ||
| vim.cmd("Bdelete") | ||
| end | ||
| end, opts) |
There was a problem hiding this comment.
nvim_tabpage_list_wins counts floating windows, causing close to fire incorrectly.
nvim_tabpage_list_wins(0) returns every window handle in the current tabpage, including floating windows (completion menus, LSP hover docs, Telescope pickers, etc.). When any float is open alongside a single real split, #wins > 1 is true and vim.cmd("close") executes against the editing window instead of the float, unexpectedly closing it rather than deleting the buffer.
Filter to non-floating windows before the count:
🐛 Proposed fix – exclude floating windows
keymap("n", "<leader>q", function()
- if `#vim.api.nvim_tabpage_list_wins`(0) > 1 then
+ local real_wins = vim.tbl_filter(function(w)
+ return vim.api.nvim_win_get_config(w).relative == ""
+ end, vim.api.nvim_tabpage_list_wins(0))
+ if `#real_wins` > 1 then
vim.cmd("close")
else
vim.cmd("Bdelete")
end
end, opts)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@home-manager/programs/neovim/lua/config/keymaps.lua` around lines 43 - 50,
The mapping using vim.api.nvim_tabpage_list_wins(0) incorrectly counts floating
windows; update the anonymous function bound by keymap("n", "<leader>q", ...) to
compute non-floating windows by filtering the list from
vim.api.nvim_tabpage_list_wins(0) with vim.api.nvim_win_get_config(win).relative
== "" (or equivalent check) and use that count for the conditional (if
non_floating_count > 1 then vim.cmd("close") else vim.cmd("Bdelete")). Leave the
rest of the mapping and opts intact.
Entire-Checkpoint: 1dfaa3a49b3d
Entire-Checkpoint: 2a90720ce63b
Entire-Checkpoint: 412109469711
The activation script now reliably builds libfzf.so on every make switch, so pcall is no longer needed and would silently swallow real errors. Entire-Checkpoint: d0c38f6422f5
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="home-manager/programs/neovim/lua/config/telescope.lua">
<violation number="1">
P2: Reintroduce the `pcall` wrapper so a missing/failed native build doesn’t throw during startup. Directly calling `load_extension("fzf")` makes Neovim fail to start when `libfzf.so` is missing or the build fails.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
The vertsplit fillchar was removed in newer Neovim versions, causing E474 in CI. The vert key already covers vertical separators. Entire-Checkpoint: bc8df04da4bb
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@home-manager/programs/neovim/lua/config/settings.lua`:
- Line 106: The change to use vim.opt.fillchars:append({ vert = " " }) is
correct given laststatus = 2; leave the fillchars append as-is (no vertsplits
keys needed) and keep the existing setting in settings.lua, referencing the
vim.opt.fillchars:append call and the laststatus setting already present.
Add gnumake and gcc to devenv.nix and a pre-build step in run_tests.sh that detects a missing libfzf.so and compiles it before running tests. Entire-Checkpoint: 2f300b6106d9
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
home-manager/programs/neovim/run_tests.sh (1)
38-39: Two minor observability/resilience gaps in the build step.
- The "Building..." message omits
$d, making it hard to know which instance is being compiled when multiple matches exist.- There's no pre-flight check that
make(or a C compiler) is available; underset -e, a missingmakewill abort the test runner with an opaque error rather than an actionable one.🔧 Suggested improvement
- echo -e "${YELLOW}Building telescope-fzf-native.nvim...${NC}" - make -C "$d" clean all + echo -e "${YELLOW}Building telescope-fzf-native.nvim at $d...${NC}" + if ! command -v make &>/dev/null; then + echo -e "${RED}Error: 'make' not found in PATH; cannot build telescope-fzf-native.nvim${NC}" + exit 1 + fi + make -C "$d" clean all🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@home-manager/programs/neovim/run_tests.sh` around lines 38 - 39, The build step prints a generic "Building..." message and calls make -C "$d" clean all without checking prerequisites; update the echo to include the target directory variable $d (so the message shows which instance is being built) and add a pre-flight check before invoking make that verifies make (and optionally a C compiler like cc/gcc/clang) exists (use command -v or which) and emit a clear error message and exit if missing; keep the make invocation as make -C "$d" clean all so failures still surface under set -e but now fail with an actionable pre-check if tooling is absent.devenv.nix (1)
10-11: Consider mirroring the Darwin/non-Darwin compiler split fromhome-manager/programs/neovim/default.nix.
pkgs.gccis added unconditionally here, but according to the AI summary, the home-manager Neovim config usesclangon Darwin andgcconly on non-Darwin. Adding GCC unconditionally means macOS CI builds telescope-fzf-native with GCC while production uses clang — a subtle toolchain inconsistency, and it brings in a large unnecessary closure on Darwin.♻️ Suggested platform-conditional fix
- pkgs.gnumake - pkgs.gcc + pkgs.gnumake + ] ++ (if pkgs.stdenv.hostPlatform.isDarwin then [ + pkgs.clang + ] else [ + pkgs.gcc + ]) ++ [🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@devenv.nix` around lines 10 - 11, The dev environment unconditionally adds pkgs.gcc which pulls a large closure on Darwin and mismatches production that uses clang; change the platform handling so that pkgs.gcc is only added for non‑Darwin systems and use pkgs.clang on Darwin (check the platform via pkgs.stdenv.isDarwin or builtins.currentSystem like the Neovim config does), keeping pkgs.gnumake unchanged; update the dependency list where pkgs.gcc is referenced to conditionally select pkgs.gcc or pkgs.clang to avoid enforcing GCC on macOS CI.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@home-manager/programs/neovim/run_tests.sh`:
- Line 35: The PACK_DIR variable in run_tests.sh is hardcoded and ignores
XDG_DATA_HOME, causing the glob to miss Neovim packages when XDG_DATA_HOME is
set; update PACK_DIR to respect XDG_DATA_HOME (e.g. use the value of
XDG_DATA_HOME with a fallback to $HOME/.local/share and append /nvim/site/pack)
so the build glob matches the correct path, and ensure subsequent logic (the
glob/loop that uses PACK_DIR) still behaves correctly when the directory is
absent.
---
Nitpick comments:
In `@devenv.nix`:
- Around line 10-11: The dev environment unconditionally adds pkgs.gcc which
pulls a large closure on Darwin and mismatches production that uses clang;
change the platform handling so that pkgs.gcc is only added for non‑Darwin
systems and use pkgs.clang on Darwin (check the platform via
pkgs.stdenv.isDarwin or builtins.currentSystem like the Neovim config does),
keeping pkgs.gnumake unchanged; update the dependency list where pkgs.gcc is
referenced to conditionally select pkgs.gcc or pkgs.clang to avoid enforcing GCC
on macOS CI.
In `@home-manager/programs/neovim/run_tests.sh`:
- Around line 38-39: The build step prints a generic "Building..." message and
calls make -C "$d" clean all without checking prerequisites; update the echo to
include the target directory variable $d (so the message shows which instance is
being built) and add a pre-flight check before invoking make that verifies make
(and optionally a C compiler like cc/gcc/clang) exists (use command -v or which)
and emit a clear error message and exit if missing; keep the make invocation as
make -C "$d" clean all so failures still surface under set -e but now fail with
an actionable pre-check if tooling is absent.
| export PLENARY_DIR | ||
|
|
||
| # Build telescope-fzf-native if libfzf.so is missing | ||
| PACK_DIR="$HOME/.local/share/nvim/site/pack" |
There was a problem hiding this comment.
PACK_DIR ignores $XDG_DATA_HOME, silently skipping the build step when it's set.
The path is hardcoded to $HOME/.local/share/nvim/site/pack. If $XDG_DATA_HOME is set (a common Nix/dotfiles pattern), Neovim uses $XDG_DATA_HOME/nvim/site/pack instead, the glob never matches, and the build step is silently skipped.
🐛 Proposed fix
-PACK_DIR="$HOME/.local/share/nvim/site/pack"
+PACK_DIR="${XDG_DATA_HOME:-$HOME/.local/share}/nvim/site/pack"📝 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.
| PACK_DIR="$HOME/.local/share/nvim/site/pack" | |
| PACK_DIR="${XDG_DATA_HOME:-$HOME/.local/share}/nvim/site/pack" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@home-manager/programs/neovim/run_tests.sh` at line 35, The PACK_DIR variable
in run_tests.sh is hardcoded and ignores XDG_DATA_HOME, causing the glob to miss
Neovim packages when XDG_DATA_HOME is set; update PACK_DIR to respect
XDG_DATA_HOME (e.g. use the value of XDG_DATA_HOME with a fallback to
$HOME/.local/share and append /nvim/site/pack) so the build glob matches the
correct path, and ensure subsequent logic (the glob/loop that uses PACK_DIR)
still behaves correctly when the directory is absent.
In CI the plugin directory doesn't exist until vim.pack installs it during neovim startup. Add a headless nvim bootstrap step to install plugins first, then build libfzf.so. Entire-Checkpoint: 0adb16f41c39
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
home-manager/programs/neovim/run_tests.sh (1)
37-42: Duplicated glob pattern and detection loop can be consolidated.The identical glob
"$PACK_DIR"/*/opt/telescope-fzf-native.nvim "$PACK_DIR"/*/start/telescope-fzf-native.nvimappears in two separate loops. A single loop can handle both the bootstrap-detection and the build step.♻️ Proposed refactor
-# Bootstrap plugins so vim.pack installs them before tests run -PACK_DIR="$HOME/.local/share/nvim/site/pack" -FZF_NATIVE_FOUND=false -for d in "$PACK_DIR"/*/opt/telescope-fzf-native.nvim "$PACK_DIR"/*/start/telescope-fzf-native.nvim; do - if [ -d "$d" ]; then - FZF_NATIVE_FOUND=true - break - fi -done - -if [ "$FZF_NATIVE_FOUND" = false ]; then +# Bootstrap plugins so vim.pack installs them before tests run +PACK_DIR="${XDG_DATA_HOME:-$HOME/.local/share}/nvim/site/pack" +case "$(uname -s)" in + Darwin) LIB_EXT="dylib" ;; + *) LIB_EXT="so" ;; +esac + +FZF_NATIVE_GLOB=("$PACK_DIR"/*/opt/telescope-fzf-native.nvim "$PACK_DIR"/*/start/telescope-fzf-native.nvim) + +FZF_NATIVE_FOUND=false +for d in "${FZF_NATIVE_GLOB[@]}"; do + [ -d "$d" ] && FZF_NATIVE_FOUND=true && break +done + +if [ "$FZF_NATIVE_FOUND" = false ]; then echo -e "${YELLOW}Bootstrapping plugins (first run)...${NC}" - nvim --headless -u "$NVIM_DIR/init.lua" -c 'qall!' 2>/dev/null || true + if ! nvim --headless -u "$NVIM_DIR/init.lua" -c 'qall!'; then + echo -e "${YELLOW}Warning: plugin bootstrap returned non-zero; continuing...${NC}" + fi fi # Build telescope-fzf-native if the native library is missing -for d in "$PACK_DIR"/*/opt/telescope-fzf-native.nvim "$PACK_DIR"/*/start/telescope-fzf-native.nvim; do - if [ -d "$d" ] && [ ! -f "$d/build/libfzf.so" ]; then +for d in "${FZF_NATIVE_GLOB[@]}"; do + if [ -d "$d" ] && [ ! -f "$d/build/libfzf.$LIB_EXT" ]; then echo -e "${YELLOW}Building telescope-fzf-native.nvim...${NC}" make -C "$d" clean all fi doneAlso applies to: 50-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@home-manager/programs/neovim/run_tests.sh` around lines 37 - 42, The duplicate glob and loops detecting and building telescope-fzf-native.nvim should be consolidated: create a single list/array of paths using the two patterns "$PACK_DIR"/*/opt/telescope-fzf-native.nvim and "$PACK_DIR"/*/start/telescope-fzf-native.nvim, then iterate that list once; inside the loop check if the entry is a directory, set FZF_NATIVE_FOUND=true and perform the build step (the logic currently in the second loop) for that directory (referencing PACK_DIR, FZF_NATIVE_FOUND and the existing build commands), and then break or continue as appropriate so you no longer duplicate the glob or loop logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@home-manager/programs/neovim/run_tests.sh`:
- Around line 44-47: The current bootstrap command hides Neovim stderr and
forces success (`2>/dev/null || true`), so failures (e.g. bad init.lua or
network problems) are swallowed; remove the stderr redirection and the `||
true`, run `nvim --headless -u "$NVIM_DIR/init.lua" -c 'qall!'` normally,
capture its exit status ($?) and if non-zero echo a clear warning that includes
the exit code and a note to check "$NVIM_DIR/init.lua" and plugin/network issues
(referencing FZF_NATIVE_FOUND, NVIM_DIR, and init.lua), so bootstrap errors
surface instead of being silently ignored.
---
Duplicate comments:
In `@home-manager/programs/neovim/run_tests.sh`:
- Line 35: The PACK_DIR variable in run_tests.sh ignores XDG_DATA_HOME causing
bootstrap/build to be skipped; update how PACK_DIR is computed to respect
XDG_DATA_HOME (fall back to $HOME/.local/share if unset) so PACK_DIR points to
"${XDG_DATA_HOME:-$HOME/.local/share}/nvim/site/pack" and use that updated
PACK_DIR in the subsequent bootstrap/build checks (references: PACK_DIR in
run_tests.sh).
---
Nitpick comments:
In `@home-manager/programs/neovim/run_tests.sh`:
- Around line 37-42: The duplicate glob and loops detecting and building
telescope-fzf-native.nvim should be consolidated: create a single list/array of
paths using the two patterns "$PACK_DIR"/*/opt/telescope-fzf-native.nvim and
"$PACK_DIR"/*/start/telescope-fzf-native.nvim, then iterate that list once;
inside the loop check if the entry is a directory, set FZF_NATIVE_FOUND=true and
perform the build step (the logic currently in the second loop) for that
directory (referencing PACK_DIR, FZF_NATIVE_FOUND and the existing build
commands), and then break or continue as appropriate so you no longer duplicate
the glob or loop logic.
| if [ "$FZF_NATIVE_FOUND" = false ]; then | ||
| echo -e "${YELLOW}Bootstrapping plugins (first run)...${NC}" | ||
| nvim --headless -u "$NVIM_DIR/init.lua" -c 'qall!' 2>/dev/null || true | ||
| fi |
There was a problem hiding this comment.
Bootstrap failures are silently swallowed, leaving the build loop with no plugins to compile.
2>/dev/null || true discards all Neovim stderr output and forces a zero exit even on failure. If bootstrapping fails (network issue, config error, bad init.lua), the build loop at Line 50 finds nothing, skips silently, and the test suite proceeds without libfzf.so — producing a cryptic failure rather than an actionable message.
Preserve stderr and surface non-zero exits as warnings at a minimum:
🐛 Proposed fix
- nvim --headless -u "$NVIM_DIR/init.lua" -c 'qall!' 2>/dev/null || true
+ if ! nvim --headless -u "$NVIM_DIR/init.lua" -c 'qall!'; then
+ echo -e "${YELLOW}Warning: plugin bootstrap returned non-zero; continuing...${NC}"
+ fi📝 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 [ "$FZF_NATIVE_FOUND" = false ]; then | |
| echo -e "${YELLOW}Bootstrapping plugins (first run)...${NC}" | |
| nvim --headless -u "$NVIM_DIR/init.lua" -c 'qall!' 2>/dev/null || true | |
| fi | |
| if [ "$FZF_NATIVE_FOUND" = false ]; then | |
| echo -e "${YELLOW}Bootstrapping plugins (first run)...${NC}" | |
| if ! nvim --headless -u "$NVIM_DIR/init.lua" -c 'qall!'; then | |
| echo -e "${YELLOW}Warning: plugin bootstrap returned non-zero; continuing...${NC}" | |
| fi | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@home-manager/programs/neovim/run_tests.sh` around lines 44 - 47, The current
bootstrap command hides Neovim stderr and forces success (`2>/dev/null ||
true`), so failures (e.g. bad init.lua or network problems) are swallowed;
remove the stderr redirection and the `|| true`, run `nvim --headless -u
"$NVIM_DIR/init.lua" -c 'qall!'` normally, capture its exit status ($?) and if
non-zero echo a clear warning that includes the exit code and a note to check
"$NVIM_DIR/init.lua" and plugin/network issues (referencing FZF_NATIVE_FOUND,
NVIM_DIR, and init.lua), so bootstrap errors surface instead of being silently
ignored.
vim.pack needs to run during nvim startup to properly add opt packages to the runtimepath. Sourcing init.lua within a plenary session skips this, causing module-not-found errors for treesitter and others. Spawn a separate headless nvim process for the e2e test instead. Entire-Checkpoint: 3a2fa5733e9e
Summary
buildNvimNativePluginshome-manager activation script that compileslibfzf.soonmake switchwhen missingtelescope.load_extension("fzf")inpcallas defense-in-depthinit.luaheadlessly to catch plugin loading errorsTest plan
make buildsucceedsmake switchsucceeds and activation script compileslibfzf.sonvimstarts without E5113 errorls ~/.local/share/nvim/site/pack/core/opt/telescope-fzf-native.nvim/build/showslibfzf.soSummary by cubic
Declarative build for telescope-fzf-native.nvim so libfzf.so compiles on switch and in CI before tests, with plugin bootstrap and a subprocess e2e check to ensure init.lua loads cleanly.
Bug Fixes
Refactors
Written for commit 2794bed. Summary will update on new commits.