feat(nvim): add diff keymaps and comprehensive lua tests - #1404
Merged
Conversation
- Fix vscode-diff keymap (was calling non-existent diff() fn) - Add keymaps: gD (explorer), gH (HEAD), gr (revision), gf (files) - Add fugitive diff keymaps: gs (Gvdiffsplit), gS (vs HEAD) - Add gitsigns: hn/hN (next/prev hunk) - Add oil side-by-side dir browser keymap (<leader>-) - Add 80+ new plenary tests across utils, settings, terminal, keymaps, ui, completion specs
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
|
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:
📝 WalkthroughWalkthroughAdded multiple Neovim keybindings (vscode-diff variants, gitsigns, Fugitive, Oil), a full-config reload binding, a :NvimPluginsInstall user command, extended native-plugin build/bootstrap logic, and broad test coverage additions for completion, keymaps, settings, terminal, UI, and utils. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Keymap as Neovim Keymap
participant UI as vim.ui.input
participant VSD as vscode-diff.commands
participant PKG as package.loaded
participant RC as $MYVIMRC
participant NOT as vim.notify
User->>Keymap: Press <leader>gr / <leader>gf / <leader>gH
Keymap->>UI: Prompt for revision or file paths (if required)
UI-->>Keymap: Returns input
alt input(s) non-empty
Keymap->>VSD: Call vscode_diff({ fargs = { ... } })
VSD-->>Keymap: Open diff UI/buffers
else empty input
Keymap-->>User: No-op
end
User->>Keymap: Press <leader>R
Keymap->>PKG: Clear modules matching ^config%.
Keymap->>RC: Source $MYVIMRC
Keymap->>NOT: Show info notification
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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 |
Mesa DescriptionTL;DRFixes a broken What changed?
Description generated by Mesa. Update settings |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request adds keybindings for Git and directory management and introduces unit tests for completion, settings, and UI components. Feedback highlights a conflict for the gs mapping and notes that several test suites validate local mock structures rather than the actual application state. An improvement to the SSH environment test assertion was also suggested.
Comment on lines
+183
to
+184
| -- @keymap <leader>gs: Fugitive vertical diff split (current file vs index) | ||
| keymap("n", "<leader>gs", ":Gvdiffsplit<cr>", opts) |
Contributor
There was a problem hiding this comment.
The keymap <leader>gs is already defined at line 137 for opening the Git status in a new tab (:tab Git<cr>). This new mapping will overwrite the existing one. Consider using a different key combination for the Fugitive vertical diff split, such as <leader>gv, to avoid this conflict.
-- @keymap <leader>gv: Fugitive vertical diff split (current file vs index)
keymap("n", "<leader>gv", ":Gvdiffsplit<cr>", opts)
|
|
||
| -- Tests for terminal.lua global functions | ||
| -- Simulates the module logic without requiring toggleterm | ||
| describe("terminal globals pattern", function() |
Contributor
There was a problem hiding this comment.
This test block reimplements the internal logic of the terminal management system within the test file itself. This approach tests a mock implementation rather than the actual code used in the application, which can lead to false positives and fails to catch regressions in the real logic. It is recommended to refactor the terminal logic into a testable module and require it here.
| vim.fn.setenv("SSH_TTY", "") | ||
| -- is_ssh checks non-nil, empty string is still non-nil | ||
| -- just verify it returns boolean | ||
| assert.is_boolean(utils.is_ssh()) |
Contributor
There was a problem hiding this comment.
The test for the non-SSH case should explicitly assert that is_ssh() returns false. Currently, it only checks that the return type is a boolean. Since vim.fn.setenv with an empty string removes the environment variable in Neovim, os.getenv will return nil, and the function should evaluate to false.
assert.is_false(utils.is_ssh())
| end) | ||
| end) | ||
|
|
||
| describe("cmp source configuration pattern", function() |
Contributor
There was a problem hiding this comment.
These tests verify local table structures defined within the test functions rather than testing the actual completion.lua configuration or its side effects on the Neovim environment. This results in tests that pass regardless of the actual application state, providing no real validation of the configuration.
| end) | ||
| end) | ||
|
|
||
| describe("lualine config structure", function() |
Contributor
There was a problem hiding this comment.
Similar to the completion tests, these 'pattern' tests only validate that Lua tables can be constructed with specific keys and values. They do not test the actual UI configuration of the Neovim instance. Consider testing the actual state of Neovim (e.g., using vim.api or vim.opt) after the configuration is applied.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (3)
home-manager/programs/neovim/tests/settings_spec.lua (1)
258-261: Make theseverity_sortassertion strict to match the test intent.Line 260 currently checks
~= false, which is looser than the test name and can hide misconfiguration. Assert explicit enablement instead.Proposed test tightening
it("should sort by severity", function() local config = vim.diagnostic.config() - assert.is_true(config.severity_sort ~= false) + assert.is_true(config.severity_sort == true) end)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@home-manager/programs/neovim/tests/settings_spec.lua` around lines 258 - 261, The test currently checks severity_sort with a loose inequality (config.severity_sort ~= false); update the assertion to be strict by asserting the value is true (e.g., use assert.is_true(config.severity_sort) or assert.equals(true, config.severity_sort)) so the test name "should sort by severity" is enforced; locate the test that calls vim.diagnostic.config() and replace the existing assertion on config.severity_sort accordingly.home-manager/programs/neovim/tests/ui_spec.lua (1)
63-153: These tests are self-fulfilling and don’t validate real UI config.From Line 63 onward, most cases assert locally constructed literals (
config = { ... },theme_fn = function() ... end) rather than values loaded fromlua/ui.lua. This can pass even when the actual plugin setup is broken, so coverage is inflated but not protective.Please bind assertions to real module output (or captured
setup(...)calls) instead of testing placeholder tables/functions.Example direction to make tests meaningful
- describe("nvim-tree config pattern", function() - it("should support view width config", function() - local config = { view = { width = 30 } } - assert.equals(30, config.view.width) - end) - end) + describe("nvim-tree config", function() + it("sets expected view width in real setup", function() + local captured + package.loaded["nvim-tree"] = { + setup = function(opts) captured = opts end, + } + + package.loaded["ui"] = nil + require("ui") + + assert.is_table(captured) + assert.equals(30, captured.view.width) + end) + end)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@home-manager/programs/neovim/tests/ui_spec.lua` around lines 63 - 153, Many tests create local placeholder tables/functions (e.g., local config = {...}, theme_fn = function()... end) instead of exercising the real UI module; update each failing spec to require and assert against the actual module exports (e.g., local ui = require("ui") or require("lua.ui")), call ui.setup(...) if present and assert on ui.options, ui.sections (for lualine), ui.view or ui.actions (for nvim-tree), ui.input (for dressing), ui.render/stages (for notify), or call ui.theme() instead of theme_fn; if the module only exposes a setup function, use a spy/mock to capture the setup(...) argument and assert its structure rather than using a locally constructed table.home-manager/programs/neovim/tests/terminal_spec.lua (1)
201-224: Kill-flow assertions are weaker than the runtime contract.The kill tests only verify length/reset, but the real logic also enforces post-kill index adjustment and showing the next terminal when one remains. Add assertions for
current_indexand which terminal is open after removal to lock in that behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@home-manager/programs/neovim/tests/terminal_spec.lua` around lines 201 - 224, The tests for kill-flow (in the spec around the create_term/show_term logic) only assert term_sequence length and reset but miss verifying current_index adjustment and which terminal is shown after removal; update both tests to assert current_index equals the expected value after removals and that the visible/active terminal matches the expected id by checking term_sequence[current_index] (or 0 when empty) and ensuring show_term was called for that id (or that no terminal is shown), referencing the existing symbols term_sequence, current_index, terms, show_term, and create_term to locate where to add the extra assertions.
🤖 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 183-184: The duplicate keymap defines "<leader>gs" twice (first
mapped to ":tab Git<cr>" and later to ":Gvdiffsplit<cr>") so the second mapping
in the keymap(...) call overwrites the Git status mapping; change the second
mapping to a different key (e.g., use "<leader>gv" or "<leader>gi") in the
keymap("n", "<leader>gs", ":Gvdiffsplit<cr>", opts) call so it no longer
collides with the earlier mapping, and keep the original ":tab Git<cr>" mapping
intact.
In `@home-manager/programs/neovim/tests/completion_spec.lua`:
- Around line 145-155: The test's mock for the expand callback should call into
the LuaSnip API instead of returning the string; update the
snippet_config.expand mock to invoke require("luasnip").lsp_expand(args.body)
(or use a spy/mocked function on require("luasnip").lsp_expand to assert it was
called with args.body) so the test mirrors the real behavior of expand and
verifies that lsp_expand is invoked rather than simply returning args.body.
- Around line 93-143: The tests currently assert against hardcoded local tables;
load the real module under test (completion.lua) and assert against its exported
config values instead (e.g., use the module's cmp_sources, cmdline_sources,
filetype_sources/gitcommit pattern, mappings, nav_keys, and copilot_config
exports) so the specs validate the actual produced configuration; replace the
local literals (variables named sources, cmdline_sources, gitcommit_sources,
mappings, nav_keys, config) with references to the corresponding exports from
completion.lua and assert the expected names/values on those real objects.
In `@home-manager/programs/neovim/tests/keymaps_spec.lua`:
- Around line 354-365: The test "all gitsigns keymaps should be silent and
noremap" currently doesn't assert that each expected key was found; add a local
found flag inside the outer loop (e.g., before iterating keymaps), set found =
true when a matching map is located in vim.api.nvim_get_keymap("n") (same place
you assert silent and noremap) and break, then after the inner loop assert that
found is true with a clear message like km.lhs .. " should be present"; this
ensures each expected gitsigns keymap exists as well as having silent and
noremap set.
- Around line 234-245: The test "fugitive diff keymaps should be silent and
noremap" iterates keys but never verifies each key was present; add a per-key
found flag and assert it. For the test function (the it block with that
description) declare a local found = false before iterating keymaps, set found =
true when km.lhs == key, keep the existing asserts for km.silent and km.noremap,
break out of the inner loop, and after the inner loop add assert.is_true(found,
key .. " should be mapped") to fail when a key is missing.
- Around line 195-206: The test iterates keys but never fails if a keymap isn't
found; update the "all vscode-diff keymaps should have function callbacks" test
to track a local found flag for each key (similar to the previous test), set
found=true when km.lhs:match(key) is true and you assert km.callback is a
function, then after the inner loop assert that found is true with a message
like key .. " should be defined"; use the existing variables keys, keymaps, km,
km.lhs, and km.callback to locate and implement the change.
- Around line 273-284: In the "hunk navigation keymaps should be silent and
noremap" test, add a local found flag for each key in the outer loop, set found
= true when km.lhs == key inside the inner loop (alongside the existing
assertions for km.silent and km.noremap), and after the inner loop assert that
found is true (e.g., assert.is_true(found, key .. " should be mapped")) so the
test fails when a key is missing; reference the existing variables keys,
keymaps, km and km.lhs to locate where to add the flag and assertion.
- Around line 181-193: The test loop over keys currently breaks when a matching
keymap is found but does not assert that a match existed, allowing missing
keymaps to pass silently; modify the test (the it block "all vscode-diff keymaps
should be silent and noremap" that iterates over keys and keymaps) to track a
found flag for each key (e.g., local found = false) set it true when
km.lhs:match(key) is true, perform the existing assertions, break, and after the
inner loop assert that found is true (using assert.is_true or assert.equals(1,
found and 1 or 0)) so the test fails when a keymap is missing.
In `@home-manager/programs/neovim/tests/terminal_spec.lua`:
- Around line 50-113: The test creates a local reimplementation of the terminal
state machine (reset_state, make_term, create_term, index_of, close_other_terms,
show_term, cycle) instead of exercising the production module; replace this
duplicated logic by importing the actual terminal module (or the shared state
layer) used by home-manager/programs/neovim/lua/config/terminal.lua and call its
exported functions/state in the specs (or refactor the production code to expose
a dependency-free/state-only interface that both tests and runtime require).
Ensure tests stub any external dependencies from the module, initialize/reset
the module's state via its public API (instead of reset_state), and invoke its
cycle/show_term/create_term equivalents so the spec validates the real
implementation rather than a local copy.
In `@home-manager/programs/neovim/tests/ui_spec.lua`:
- Around line 91-99: The test "auto-dark-mode pattern" mutates vim.o.background
and restores a hardcoded "dark"; change the spec (the it block) to capture the
original value (local original = vim.o.background) before calling
vim.api.nvim_set_option_value("background", ...) and ensure you restore it at
the end (e.g., vim.api.nvim_set_option_value("background", original, {}) or
vim.o.background = original) even if an assertion fails (wrap mutations in a
pcall or use a finally-style restore or move restore into an after_each for the
describe) so the global editor state is safely preserved.
In `@home-manager/programs/neovim/tests/utils_spec.lua`:
- Around line 209-220: The test for utils.is_ssh is wrong: it only asserts a
boolean and may leave environment variables mutated; update the spec to
assert.is_false(utils.is_ssh()) when both SSH_CLIENT and SSH_TTY are cleared,
and restore the original environment correctly by setting each var back to its
previous value if it existed or unsetting it if it did not (use
vim.fn.setenv("SSH_CLIENT", nil) / vim.fn.setenv("SSH_TTY", nil) or equivalent).
Ensure the test clears the env with vim.fn.setenv("SSH_CLIENT", "") and
"SSH_TTY" before the assertion and always restores both vars in the teardown
logic so no env state leaks to other tests.
---
Nitpick comments:
In `@home-manager/programs/neovim/tests/settings_spec.lua`:
- Around line 258-261: The test currently checks severity_sort with a loose
inequality (config.severity_sort ~= false); update the assertion to be strict by
asserting the value is true (e.g., use assert.is_true(config.severity_sort) or
assert.equals(true, config.severity_sort)) so the test name "should sort by
severity" is enforced; locate the test that calls vim.diagnostic.config() and
replace the existing assertion on config.severity_sort accordingly.
In `@home-manager/programs/neovim/tests/terminal_spec.lua`:
- Around line 201-224: The tests for kill-flow (in the spec around the
create_term/show_term logic) only assert term_sequence length and reset but miss
verifying current_index adjustment and which terminal is shown after removal;
update both tests to assert current_index equals the expected value after
removals and that the visible/active terminal matches the expected id by
checking term_sequence[current_index] (or 0 when empty) and ensuring show_term
was called for that id (or that no terminal is shown), referencing the existing
symbols term_sequence, current_index, terms, show_term, and create_term to
locate where to add the extra assertions.
In `@home-manager/programs/neovim/tests/ui_spec.lua`:
- Around line 63-153: Many tests create local placeholder tables/functions
(e.g., local config = {...}, theme_fn = function()... end) instead of exercising
the real UI module; update each failing spec to require and assert against the
actual module exports (e.g., local ui = require("ui") or require("lua.ui")),
call ui.setup(...) if present and assert on ui.options, ui.sections (for
lualine), ui.view or ui.actions (for nvim-tree), ui.input (for dressing),
ui.render/stages (for notify), or call ui.theme() instead of theme_fn; if the
module only exposes a setup function, use a spy/mock to capture the setup(...)
argument and assert its structure rather than using a locally constructed table.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 45f3cb34-79b0-41e9-9464-35e34aebb9c9
📒 Files selected for processing (7)
home-manager/programs/neovim/lua/config/keymaps.luahome-manager/programs/neovim/tests/completion_spec.luahome-manager/programs/neovim/tests/keymaps_spec.luahome-manager/programs/neovim/tests/settings_spec.luahome-manager/programs/neovim/tests/terminal_spec.luahome-manager/programs/neovim/tests/ui_spec.luahome-manager/programs/neovim/tests/utils_spec.lua
Comment on lines
+183
to
+184
| -- @keymap <leader>gs: Fugitive vertical diff split (current file vs index) | ||
| keymap("n", "<leader>gs", ":Gvdiffsplit<cr>", opts) |
There was a problem hiding this comment.
Duplicate keymap <leader>gs overwrites Git status mapping.
Line 137 already defines <leader>gs as :tab Git<cr> (open Git status in new tab). This second definition at line 184 overwrites it with :Gvdiffsplit<cr>, breaking the original functionality.
Use a different key for the Fugitive diff split, such as <leader>gv or <leader>gi.
🐛 Proposed fix: rename to avoid conflict
--- `@keymap` <leader>gs: Fugitive vertical diff split (current file vs index)
-keymap("n", "<leader>gs", ":Gvdiffsplit<cr>", opts)
+-- `@keymap` <leader>gi: Fugitive vertical diff split (current file vs index)
+keymap("n", "<leader>gi", ":Gvdiffsplit<cr>", opts)📝 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.
Suggested change
| -- @keymap <leader>gs: Fugitive vertical diff split (current file vs index) | |
| keymap("n", "<leader>gs", ":Gvdiffsplit<cr>", opts) | |
| -- `@keymap` <leader>gi: Fugitive vertical diff split (current file vs index) | |
| keymap("n", "<leader>gi", ":Gvdiffsplit<cr>", 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 183 - 184,
The duplicate keymap defines "<leader>gs" twice (first mapped to ":tab Git<cr>"
and later to ":Gvdiffsplit<cr>") so the second mapping in the keymap(...) call
overwrites the Git status mapping; change the second mapping to a different key
(e.g., use "<leader>gv" or "<leader>gi") in the keymap("n", "<leader>gs",
":Gvdiffsplit<cr>", opts) call so it no longer collides with the earlier
mapping, and keep the original ":tab Git<cr>" mapping intact.
Comment on lines
+93
to
+143
| describe("cmp source configuration pattern", function() | ||
| it("should define expected source names", function() | ||
| local sources = { "nvim_lsp", "copilot", "luasnip", "buffer", "path" } | ||
| for _, s in ipairs(sources) do | ||
| assert.is_string(s) | ||
| assert.is_true(#s > 0) | ||
| end | ||
| end) | ||
|
|
||
| it("should support cmdline source names", function() | ||
| local cmdline_sources = { "buffer", "path", "cmdline" } | ||
| for _, s in ipairs(cmdline_sources) do | ||
| assert.is_string(s) | ||
| end | ||
| end) | ||
|
|
||
| it("should support filetype-specific source pattern", function() | ||
| local gitcommit_sources = { { name = "git" }, { name = "buffer" } } | ||
| assert.equals("git", gitcommit_sources[1].name) | ||
| assert.equals("buffer", gitcommit_sources[2].name) | ||
| end) | ||
| end) | ||
|
|
||
| describe("cmp mapping patterns", function() | ||
| it("should support scroll_docs mapping pattern", function() | ||
| local mappings = { ["<C-b>"] = "scroll_docs(-4)", ["<C-f>"] = "scroll_docs(4)" } | ||
| assert.is_not_nil(mappings["<C-b>"]) | ||
| assert.is_not_nil(mappings["<C-f>"]) | ||
| end) | ||
|
|
||
| it("should support abort and confirm pattern", function() | ||
| local mappings = { ["<C-e>"] = "abort", ["<CR>"] = "confirm" } | ||
| assert.equals("abort", mappings["<C-e>"]) | ||
| assert.equals("confirm", mappings["<CR>"]) | ||
| end) | ||
|
|
||
| it("should support tab/shift-tab navigation pattern", function() | ||
| local nav_keys = { "<Tab>", "<S-Tab>" } | ||
| for _, k in ipairs(nav_keys) do | ||
| assert.is_string(k) | ||
| end | ||
| end) | ||
| end) | ||
|
|
||
| describe("copilot config pattern", function() | ||
| it("should disable suggestion panel by default", function() | ||
| local config = { suggestion = { enabled = false }, panel = { enabled = false } } | ||
| assert.is_false(config.suggestion.enabled) | ||
| assert.is_false(config.panel.enabled) | ||
| end) | ||
| end) |
There was a problem hiding this comment.
These assertions are self-referential and don’t validate completion.lua.
The new specs assert hardcoded local tables, so they pass even if real cmp sources/mappings/copilot config regress. Please assert against captured config produced by loading the module under test.
Suggested refactor (bind tests to real config instead of literals)
describe("cmp source configuration pattern", function()
- it("should define expected source names", function()
- local sources = { "nvim_lsp", "copilot", "luasnip", "buffer", "path" }
- for _, s in ipairs(sources) do
- assert.is_string(s)
- assert.is_true(`#s` > 0)
- end
- end)
+ it("should configure expected source names in cmp.setup", function()
+ local captured = {}
+ local stub_cmp = {
+ setup = function(cfg) captured.main = cfg end,
+ setup_cmdline = function(mode, cfg) captured[mode] = cfg end,
+ config = { sources = function(...) return { ... } end },
+ }
+
+ package.loaded["cmp"] = stub_cmp
+ package.loaded["completion"] = nil
+ require("completion")
+
+ assert.is_table(captured.main)
+ assert.is_table(captured.main.sources)
+ local names = vim.tbl_map(function(s) return s.name end, captured.main.sources)
+ assert.is_true(vim.tbl_contains(names, "nvim_lsp"))
+ assert.is_true(vim.tbl_contains(names, "copilot"))
+ assert.is_true(vim.tbl_contains(names, "luasnip"))
+ end)
end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@home-manager/programs/neovim/tests/completion_spec.lua` around lines 93 -
143, The tests currently assert against hardcoded local tables; load the real
module under test (completion.lua) and assert against its exported config values
instead (e.g., use the module's cmp_sources, cmdline_sources,
filetype_sources/gitcommit pattern, mappings, nav_keys, and copilot_config
exports) so the specs validate the actual produced configuration; replace the
local literals (variables named sources, cmdline_sources, gitcommit_sources,
mappings, nav_keys, config) with references to the corresponding exports from
completion.lua and assert the expected names/values on those real objects.
Comment on lines
+145
to
+155
| describe("luasnip pattern", function() | ||
| it("should support lsp_expand callback pattern", function() | ||
| local snippet_config = { | ||
| expand = function(args) | ||
| return args.body | ||
| end, | ||
| } | ||
| assert.is_function(snippet_config.expand) | ||
| assert.equals("test", snippet_config.expand({ body = "test" })) | ||
| end) | ||
| end) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how snippet expand callback is implemented in the real completion config (read-only).
set -euo pipefail
# Locate completion.lua
fd -i 'completion.lua' | sed -n '1,20p'
# Inspect snippet expand usage and luasnip/cmp wiring with context
rg -n -C3 'snippet|expand|lsp_expand|luasnip|cmp\.setup' $(fd -i 'completion.lua')Repository: shunkakinoki/dotfiles
Length of output: 1683
Fix the expand callback mock to properly test LuaSnip integration.
The test currently returns args.body, but the actual expand function calls require("luasnip").lsp_expand(args.body) (a side-effect with no return). The mock should invoke luasnip.lsp_expand() instead of returning a string, or mock/spy on the call to verify it's invoked correctly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@home-manager/programs/neovim/tests/completion_spec.lua` around lines 145 -
155, The test's mock for the expand callback should call into the LuaSnip API
instead of returning the string; update the snippet_config.expand mock to invoke
require("luasnip").lsp_expand(args.body) (or use a spy/mocked function on
require("luasnip").lsp_expand to assert it was called with args.body) so the
test mirrors the real behavior of expand and verifies that lsp_expand is invoked
rather than simply returning args.body.
Comment on lines
+273
to
+284
| it("hunk navigation keymaps should be silent and noremap", function() | ||
| local keymaps = vim.api.nvim_get_keymap("n") | ||
| for _, key in ipairs(keys) do | ||
| for _, km in ipairs(keymaps) do | ||
| if km.lhs == key then | ||
| assert.equals(1, km.silent, key .. " should be silent") | ||
| assert.equals(1, km.noremap, key .. " should be noremap") | ||
| break | ||
| end | ||
| end | ||
| end | ||
| end) |
There was a problem hiding this comment.
Same pattern issue in hunk navigation keymaps test.
Add a found flag and assertion for each key.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@home-manager/programs/neovim/tests/keymaps_spec.lua` around lines 273 - 284,
In the "hunk navigation keymaps should be silent and noremap" test, add a local
found flag for each key in the outer loop, set found = true when km.lhs == key
inside the inner loop (alongside the existing assertions for km.silent and
km.noremap), and after the inner loop assert that found is true (e.g.,
assert.is_true(found, key .. " should be mapped")) so the test fails when a key
is missing; reference the existing variables keys, keymaps, km and km.lhs to
locate where to add the flag and assertion.
Comment on lines
+354
to
+365
| it("all gitsigns keymaps should be silent and noremap", function() | ||
| local keymaps = vim.api.nvim_get_keymap("n") | ||
| for _, km in ipairs(git_keys) do | ||
| for _, m in ipairs(keymaps) do | ||
| if m.lhs == km.lhs then | ||
| assert.equals(1, m.silent, km.lhs .. " should be silent") | ||
| assert.equals(1, m.noremap, km.lhs .. " should be noremap") | ||
| break | ||
| end | ||
| end | ||
| end | ||
| end) |
There was a problem hiding this comment.
Same pattern issue in gitsigns existing keymaps test.
Add a found flag and assertion for each key.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@home-manager/programs/neovim/tests/keymaps_spec.lua` around lines 354 - 365,
The test "all gitsigns keymaps should be silent and noremap" currently doesn't
assert that each expected key was found; add a local found flag inside the outer
loop (e.g., before iterating keymaps), set found = true when a matching map is
located in vim.api.nvim_get_keymap("n") (same place you assert silent and
noremap) and break, then after the inner loop assert that found is true with a
clear message like km.lhs .. " should be present"; this ensures each expected
gitsigns keymap exists as well as having silent and noremap set.
Comment on lines
+50
to
+113
| describe("terminal globals pattern", function() | ||
| -- Replicate terminal.lua's internal state and logic for unit testing | ||
| local term_counter, term_sequence, terms, current_index | ||
|
|
||
| local function reset_state() | ||
| term_counter = 0 | ||
| term_sequence = {} | ||
| terms = {} | ||
| current_index = 0 | ||
| end | ||
|
|
||
| local function make_term(id) | ||
| return { | ||
| count = id, | ||
| _open = false, | ||
| is_open = function(self) return self._open end, | ||
| open = function(self) self._open = true end, | ||
| close = function(self) self._open = false end, | ||
| } | ||
| end | ||
|
|
||
| local function create_term() | ||
| term_counter = term_counter + 1 | ||
| local id = term_counter | ||
| terms[id] = make_term(id) | ||
| table.insert(term_sequence, id) | ||
| return id | ||
| end | ||
|
|
||
| local function index_of(count) | ||
| for idx, value in ipairs(term_sequence) do | ||
| if value == count then return idx end | ||
| end | ||
| end | ||
|
|
||
| local function close_other_terms(except_id) | ||
| for id, term in pairs(terms) do | ||
| if id ~= except_id and term:is_open() then term:close() end | ||
| end | ||
| end | ||
|
|
||
| local function show_term(id) | ||
| local term = terms[id] | ||
| if not term then return end | ||
| close_other_terms(id) | ||
| if not term:is_open() then term:open() end | ||
| current_index = index_of(id) or 0 | ||
| end | ||
|
|
||
| local function cycle(step) | ||
| local len = #term_sequence | ||
| if len == 0 then | ||
| local first = create_term() | ||
| show_term(first) | ||
| return | ||
| end | ||
| if current_index == 0 then | ||
| current_index = 1 | ||
| else | ||
| current_index = ((current_index - 1 + step) % len) + 1 | ||
| end | ||
| show_term(term_sequence[current_index]) | ||
| end | ||
|
|
There was a problem hiding this comment.
These tests are disconnected from the production implementation.
Line 50 through Line 113 re-implement the terminal state machine locally instead of exercising home-manager/programs/neovim/lua/config/terminal.lua. This can pass even when the real module regresses, because the spec validates its own model. Please route both runtime and tests through one shared, dependency-free state layer (or directly test exported module behavior with stubs).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@home-manager/programs/neovim/tests/terminal_spec.lua` around lines 50 - 113,
The test creates a local reimplementation of the terminal state machine
(reset_state, make_term, create_term, index_of, close_other_terms, show_term,
cycle) instead of exercising the production module; replace this duplicated
logic by importing the actual terminal module (or the shared state layer) used
by home-manager/programs/neovim/lua/config/terminal.lua and call its exported
functions/state in the specs (or refactor the production code to expose a
dependency-free/state-only interface that both tests and runtime require).
Ensure tests stub any external dependencies from the module, initialize/reset
the module's state via its public API (instead of reset_state), and invoke its
cycle/show_term/create_term equivalents so the spec validates the real
implementation rather than a local copy.
Comment on lines
+91
to
+99
| describe("auto-dark-mode pattern", function() | ||
| it("should support background option dark/light", function() | ||
| vim.api.nvim_set_option_value("background", "dark", {}) | ||
| assert.equals("dark", vim.o.background) | ||
| vim.api.nvim_set_option_value("background", "light", {}) | ||
| assert.equals("light", vim.o.background) | ||
| vim.api.nvim_set_option_value("background", "dark", {}) | ||
| end) | ||
|
|
There was a problem hiding this comment.
background option mutation is not safely restored, causing test-order coupling.
Line 93-Line 98 mutate global editor state and restore to a hardcoded "dark" value, not the pre-test value. This can leak state into later specs and create flaky behavior.
Safer restore pattern
describe("auto-dark-mode pattern", function()
it("should support background option dark/light", function()
+ local original_bg = vim.o.background
vim.api.nvim_set_option_value("background", "dark", {})
assert.equals("dark", vim.o.background)
vim.api.nvim_set_option_value("background", "light", {})
assert.equals("light", vim.o.background)
- vim.api.nvim_set_option_value("background", "dark", {})
+ vim.api.nvim_set_option_value("background", original_bg, {})
end)📝 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.
Suggested change
| describe("auto-dark-mode pattern", function() | |
| it("should support background option dark/light", function() | |
| vim.api.nvim_set_option_value("background", "dark", {}) | |
| assert.equals("dark", vim.o.background) | |
| vim.api.nvim_set_option_value("background", "light", {}) | |
| assert.equals("light", vim.o.background) | |
| vim.api.nvim_set_option_value("background", "dark", {}) | |
| end) | |
| describe("auto-dark-mode pattern", function() | |
| it("should support background option dark/light", function() | |
| local original_bg = vim.o.background | |
| vim.api.nvim_set_option_value("background", "dark", {}) | |
| assert.equals("dark", vim.o.background) | |
| vim.api.nvim_set_option_value("background", "light", {}) | |
| assert.equals("light", vim.o.background) | |
| vim.api.nvim_set_option_value("background", original_bg, {}) | |
| end) | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@home-manager/programs/neovim/tests/ui_spec.lua` around lines 91 - 99, The
test "auto-dark-mode pattern" mutates vim.o.background and restores a hardcoded
"dark"; change the spec (the it block) to capture the original value (local
original = vim.o.background) before calling
vim.api.nvim_set_option_value("background", ...) and ensure you restore it at
the end (e.g., vim.api.nvim_set_option_value("background", original, {}) or
vim.o.background = original) even if an assertion fails (wrap mutations in a
pcall or use a finally-style restore or move restore into an after_each for the
describe) so the global editor state is safely preserved.
Comment on lines
+209
to
+220
| it("should return false when no SSH env vars set", function() | ||
| local old_client = os.getenv("SSH_CLIENT") | ||
| local old_tty = os.getenv("SSH_TTY") | ||
| -- Clear SSH env vars for test | ||
| vim.fn.setenv("SSH_CLIENT", "") | ||
| vim.fn.setenv("SSH_TTY", "") | ||
| -- is_ssh checks non-nil, empty string is still non-nil | ||
| -- just verify it returns boolean | ||
| assert.is_boolean(utils.is_ssh()) | ||
| if old_client then vim.fn.setenv("SSH_CLIENT", old_client) end | ||
| if old_tty then vim.fn.setenv("SSH_TTY", old_tty) end | ||
| end) |
There was a problem hiding this comment.
is_ssh false-case test is not actually testing the false path and may leak env state.
The test name says “return false,” but it only asserts boolean. Also, when old_client/old_tty were originally unset, restoration is skipped, leaving mutated env vars for later tests.
Suggested fix
- it("should return false when no SSH env vars set", function()
- local old_client = os.getenv("SSH_CLIENT")
- local old_tty = os.getenv("SSH_TTY")
- -- Clear SSH env vars for test
- vim.fn.setenv("SSH_CLIENT", "")
- vim.fn.setenv("SSH_TTY", "")
- -- is_ssh checks non-nil, empty string is still non-nil
- -- just verify it returns boolean
- assert.is_boolean(utils.is_ssh())
- if old_client then vim.fn.setenv("SSH_CLIENT", old_client) end
- if old_tty then vim.fn.setenv("SSH_TTY", old_tty) end
- end)
+ it("should return false when SSH env vars are missing", function()
+ local original_getenv = os.getenv
+ os.getenv = function(name)
+ if name == "SSH_CLIENT" or name == "SSH_TTY" then
+ return nil
+ end
+ return original_getenv(name)
+ end
+
+ local ok, result = pcall(utils.is_ssh)
+ os.getenv = original_getenv
+
+ assert.is_true(ok)
+ assert.is_false(result)
+ end)📝 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.
Suggested change
| it("should return false when no SSH env vars set", function() | |
| local old_client = os.getenv("SSH_CLIENT") | |
| local old_tty = os.getenv("SSH_TTY") | |
| -- Clear SSH env vars for test | |
| vim.fn.setenv("SSH_CLIENT", "") | |
| vim.fn.setenv("SSH_TTY", "") | |
| -- is_ssh checks non-nil, empty string is still non-nil | |
| -- just verify it returns boolean | |
| assert.is_boolean(utils.is_ssh()) | |
| if old_client then vim.fn.setenv("SSH_CLIENT", old_client) end | |
| if old_tty then vim.fn.setenv("SSH_TTY", old_tty) end | |
| end) | |
| it("should return false when SSH env vars are missing", function() | |
| local original_getenv = os.getenv | |
| os.getenv = function(name) | |
| if name == "SSH_CLIENT" or name == "SSH_TTY" then | |
| return nil | |
| end | |
| return original_getenv(name) | |
| end | |
| local ok, result = pcall(utils.is_ssh) | |
| os.getenv = original_getenv | |
| assert.is_true(ok) | |
| assert.is_false(result) | |
| end) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@home-manager/programs/neovim/tests/utils_spec.lua` around lines 209 - 220,
The test for utils.is_ssh is wrong: it only asserts a boolean and may leave
environment variables mutated; update the spec to
assert.is_false(utils.is_ssh()) when both SSH_CLIENT and SSH_TTY are cleared,
and restore the original environment correctly by setting each var back to its
previous value if it existed or unsetting it if it did not (use
vim.fn.setenv("SSH_CLIENT", nil) / vim.fn.setenv("SSH_TTY", nil) or equivalent).
Ensure the test clears the env with vim.fn.setenv("SSH_CLIENT", "") and
"SSH_TTY" before the assertion and always restores both vars in the teardown
logic so no env state leaks to other tests.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds/repairs several Neovim git/diff-related keymaps (notably switching <leader>gD to vscode-diff.commands) and significantly expands the Lua test suite under home-manager/programs/neovim/tests to cover more config/util patterns.
Changes:
- Fix
<leader>gDby switching from a non-existentrequire("vscode-diff").diff()call torequire("vscode-diff.commands").vscode_diff(...)and add related diff keymaps. - Add new git navigation/diff mappings (vscode-diff, fugitive diff, gitsigns hunk nav) and an Oil “two dirs side-by-side” mapping.
- Add many new plenary/busted specs across
utils,terminal,settings,keymaps,ui, andcompletion.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| home-manager/programs/neovim/lua/config/keymaps.lua | Updates/adds git/diff/hunk/Oil keymaps; fixes broken vscode-diff mapping. |
| home-manager/programs/neovim/tests/utils_spec.lua | Adds tests for additional config.utils helpers (smart_delete, yank_shift, etc.). |
| home-manager/programs/neovim/tests/ui_spec.lua | Adds UI-related “pattern” tests (lualine/auto-dark-mode/etc.). |
| home-manager/programs/neovim/tests/terminal_spec.lua | Adds simulated tests for terminal global-management logic. |
| home-manager/programs/neovim/tests/settings_spec.lua | Extends assertions for configured vim options and diagnostic settings. |
| home-manager/programs/neovim/tests/keymaps_spec.lua | Adds keymap API tests for new git/diff-related mappings (using test-only mappings). |
| home-manager/programs/neovim/tests/completion_spec.lua | Adds completion “pattern” tests for cmp/copilot/luasnip configuration shapes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+183
to
+184
| -- @keymap <leader>gs: Fugitive vertical diff split (current file vs index) | ||
| keymap("n", "<leader>gs", ":Gvdiffsplit<cr>", opts) |
There was a problem hiding this comment.
<leader>gs is mapped twice in this file: earlier to :tab Git and here to :Gvdiffsplit. The later mapping will override the earlier one, so the Git status tab keymap becomes unreachable and the inline documentation for gs becomes contradictory. Pick one behavior and either remove the other mapping or move one of them to a different lhs.
Suggested change
| -- @keymap <leader>gs: Fugitive vertical diff split (current file vs index) | |
| keymap("n", "<leader>gs", ":Gvdiffsplit<cr>", opts) | |
| -- @keymap <leader>gd: Fugitive vertical diff split (current file vs index) | |
| keymap("n", "<leader>gd", ":Gvdiffsplit<cr>", opts) |
| @@ -139,9 +139,32 @@ keymap("n", "<leader>gs", ":tab Git<cr>", opts) | |||
| keymap("n", "<F9>", ":tab Git mergetool<cr>", opts) | |||
| -- @keymap <leader>gg: Open LazyGit | |||
There was a problem hiding this comment.
The comment says this is the <leader>gg LazyGit keymap, but the actual mapping is for <leader>lg. Please update the annotation so the documented key matches the real keymap.
Suggested change
| -- @keymap <leader>gg: Open LazyGit | |
| -- @keymap <leader>lg: Open LazyGit |
| require("oil").open(b) | ||
| end) | ||
| end) | ||
| end, { desc = "Oil: open two dirs side by side" }) |
There was a problem hiding this comment.
This mapping only sets desc and does not reuse the shared opts (notably silent=true). For consistency with the rest of this file (and to avoid command-line noise during vim.ui.input flows), consider merging opts into this mapping’s options while keeping desc.
Suggested change
| end, { desc = "Oil: open two dirs side by side" }) | |
| end, vim.tbl_extend("force", opts, { desc = "Oil: open two dirs side by side" })) |
Comment on lines
+212
to
+219
| -- Clear SSH env vars for test | ||
| vim.fn.setenv("SSH_CLIENT", "") | ||
| vim.fn.setenv("SSH_TTY", "") | ||
| -- is_ssh checks non-nil, empty string is still non-nil | ||
| -- just verify it returns boolean | ||
| assert.is_boolean(utils.is_ssh()) | ||
| if old_client then vim.fn.setenv("SSH_CLIENT", old_client) end | ||
| if old_tty then vim.fn.setenv("SSH_TTY", old_tty) end |
There was a problem hiding this comment.
This test’s name says it verifies is_ssh() returns false when no SSH env vars are set, but it sets SSH_CLIENT/SSH_TTY to empty strings (which are still non-nil) and then only asserts the result is a boolean. Either rename the test to match what it checks, or actually unset the env vars so the behavior is meaningfully validated.
Suggested change
| -- Clear SSH env vars for test | |
| vim.fn.setenv("SSH_CLIENT", "") | |
| vim.fn.setenv("SSH_TTY", "") | |
| -- is_ssh checks non-nil, empty string is still non-nil | |
| -- just verify it returns boolean | |
| assert.is_boolean(utils.is_ssh()) | |
| if old_client then vim.fn.setenv("SSH_CLIENT", old_client) end | |
| if old_tty then vim.fn.setenv("SSH_TTY", old_tty) end | |
| local uv = vim.uv or vim.loop | |
| uv.os_unsetenv("SSH_CLIENT") | |
| uv.os_unsetenv("SSH_TTY") | |
| assert.is_false(utils.is_ssh()) | |
| if old_client ~= nil then | |
| vim.fn.setenv("SSH_CLIENT", old_client) | |
| else | |
| uv.os_unsetenv("SSH_CLIENT") | |
| end | |
| if old_tty ~= nil then | |
| vim.fn.setenv("SSH_TTY", old_tty) | |
| else | |
| uv.os_unsetenv("SSH_TTY") | |
| end |
Comment on lines
+48
to
+56
| -- Tests for terminal.lua global functions | ||
| -- Simulates the module logic without requiring toggleterm | ||
| describe("terminal globals pattern", function() | ||
| -- Replicate terminal.lua's internal state and logic for unit testing | ||
| local term_counter, term_sequence, terms, current_index | ||
|
|
||
| local function reset_state() | ||
| term_counter = 0 | ||
| term_sequence = {} |
There was a problem hiding this comment.
These tests re-implement terminal state management logic locally instead of exercising config/terminal.lua (e.g., SpawnTerminal, CycleNextTerm, KillCurrentTerminal). This can easily diverge from the real implementation and still pass. Consider stubbing toggleterm/Terminal:new and requiring the real module so the tests validate production behavior.
Comment on lines
+63
to
+88
| describe("lualine config structure", function() | ||
| it("should support lualine sections pattern", function() | ||
| local config = { | ||
| options = { theme = "dracula", component_separators = "", section_separators = "" }, | ||
| sections = { | ||
| lualine_a = { "mode" }, | ||
| lualine_b = { "branch", "diff" }, | ||
| lualine_c = { "filename" }, | ||
| lualine_x = { "encoding", "fileformat", "filetype" }, | ||
| lualine_y = { "progress" }, | ||
| lualine_z = { "location" }, | ||
| }, | ||
| } | ||
| assert.is_table(config.sections) | ||
| assert.is_table(config.sections.lualine_a) | ||
| assert.equals("mode", config.sections.lualine_a[1]) | ||
| end) | ||
|
|
||
| it("should support dynamic theme function pattern", function() | ||
| local theme_fn = function() | ||
| return vim.o.background == "dark" and "dracula" or "auto" | ||
| end | ||
| assert.is_function(theme_fn) | ||
| local result = theme_fn() | ||
| assert.is_string(result) | ||
| end) |
There was a problem hiding this comment.
This block constructs local config tables/functions and asserts properties about them, but it does not import or inspect config/ui.lua. As written, these tests will continue to pass even if the real UI configuration changes or breaks. Consider requiring the UI module (or exposing the relevant config tables) so the assertions are tied to the actual config.
Comment on lines
+93
to
+135
| describe("cmp source configuration pattern", function() | ||
| it("should define expected source names", function() | ||
| local sources = { "nvim_lsp", "copilot", "luasnip", "buffer", "path" } | ||
| for _, s in ipairs(sources) do | ||
| assert.is_string(s) | ||
| assert.is_true(#s > 0) | ||
| end | ||
| end) | ||
|
|
||
| it("should support cmdline source names", function() | ||
| local cmdline_sources = { "buffer", "path", "cmdline" } | ||
| for _, s in ipairs(cmdline_sources) do | ||
| assert.is_string(s) | ||
| end | ||
| end) | ||
|
|
||
| it("should support filetype-specific source pattern", function() | ||
| local gitcommit_sources = { { name = "git" }, { name = "buffer" } } | ||
| assert.equals("git", gitcommit_sources[1].name) | ||
| assert.equals("buffer", gitcommit_sources[2].name) | ||
| end) | ||
| end) | ||
|
|
||
| describe("cmp mapping patterns", function() | ||
| it("should support scroll_docs mapping pattern", function() | ||
| local mappings = { ["<C-b>"] = "scroll_docs(-4)", ["<C-f>"] = "scroll_docs(4)" } | ||
| assert.is_not_nil(mappings["<C-b>"]) | ||
| assert.is_not_nil(mappings["<C-f>"]) | ||
| end) | ||
|
|
||
| it("should support abort and confirm pattern", function() | ||
| local mappings = { ["<C-e>"] = "abort", ["<CR>"] = "confirm" } | ||
| assert.equals("abort", mappings["<C-e>"]) | ||
| assert.equals("confirm", mappings["<CR>"]) | ||
| end) | ||
|
|
||
| it("should support tab/shift-tab navigation pattern", function() | ||
| local nav_keys = { "<Tab>", "<S-Tab>" } | ||
| for _, k in ipairs(nav_keys) do | ||
| assert.is_string(k) | ||
| end | ||
| end) | ||
| end) |
There was a problem hiding this comment.
The new “cmp source/mapping pattern” tests only validate locally-constructed tables/strings, not the actual config/completion.lua configuration. That means they won’t catch regressions (e.g., a source name typo or mapping change) in the real completion setup. Consider requiring config.completion with stubbed cmp/copilot/luasnip modules (or exposing the config table) so the assertions are grounded in production code.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
5 issues found across 7 files
Prompt for AI agents (unresolved 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/keymaps.lua">
<violation number="1" location="home-manager/programs/neovim/lua/config/keymaps.lua:184">
P2: `<leader>gs` is mapped twice in normal mode; the new mapping overrides the existing Git status keymap and changes behavior unexpectedly.</violation>
</file>
<file name="home-manager/programs/neovim/tests/ui_spec.lua">
<violation number="1" location="home-manager/programs/neovim/tests/ui_spec.lua:93">
P2: This test leaks global state by restoring `background` to a hardcoded value instead of the original pre-test value.</violation>
</file>
<file name="home-manager/programs/neovim/tests/utils_spec.lua">
<violation number="1" location="home-manager/programs/neovim/tests/utils_spec.lua:209">
P3: This test is named "should return false when no SSH env vars set" but only asserts `is_boolean`, not `is_false`. Additionally, if `old_client`/`old_tty` were originally `nil`, the conditional restore is skipped, leaking the empty-string env vars into subsequent tests.</violation>
<violation number="2" location="home-manager/programs/neovim/tests/utils_spec.lua:218">
P2: This test does not fully restore SSH environment variables when they were originally unset, causing cross-test environment leakage.</violation>
</file>
<file name="home-manager/programs/neovim/tests/keymaps_spec.lua">
<violation number="1" location="home-manager/programs/neovim/tests/keymaps_spec.lua:181">
P2: This test silently passes if a keymap is not found. The inner loop `break`s on match but there's no tracking of whether a match occurred. If a keymap is missing, no assertion fires. Add a `found` flag and assert it after the inner loop.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| -- @keymap <leader>hN: Prev hunk (Gitsigns) | ||
| keymap("n", "<leader>hN", ":Gitsigns prev_hunk<cr>", opts) | ||
| -- @keymap <leader>gs: Fugitive vertical diff split (current file vs index) | ||
| keymap("n", "<leader>gs", ":Gvdiffsplit<cr>", opts) |
Contributor
There was a problem hiding this comment.
P2: <leader>gs is mapped twice in normal mode; the new mapping overrides the existing Git status keymap and changes behavior unexpectedly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/programs/neovim/lua/config/keymaps.lua, line 184:
<comment>`<leader>gs` is mapped twice in normal mode; the new mapping overrides the existing Git status keymap and changes behavior unexpectedly.</comment>
<file context>
@@ -153,6 +176,14 @@ keymap("n", "<leader>hr", ":Gitsigns reset_hunk<cr>", opts)
+-- @keymap <leader>hN: Prev hunk (Gitsigns)
+keymap("n", "<leader>hN", ":Gitsigns prev_hunk<cr>", opts)
+-- @keymap <leader>gs: Fugitive vertical diff split (current file vs index)
+keymap("n", "<leader>gs", ":Gvdiffsplit<cr>", opts)
+-- @keymap <leader>gS: Fugitive diff split vs HEAD
+keymap("n", "<leader>gS", ":Gvdiffsplit HEAD<cr>", opts)
</file context>
Comment on lines
+93
to
+97
| vim.api.nvim_set_option_value("background", "dark", {}) | ||
| assert.equals("dark", vim.o.background) | ||
| vim.api.nvim_set_option_value("background", "light", {}) | ||
| assert.equals("light", vim.o.background) | ||
| vim.api.nvim_set_option_value("background", "dark", {}) |
Contributor
There was a problem hiding this comment.
P2: This test leaks global state by restoring background to a hardcoded value instead of the original pre-test value.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/programs/neovim/tests/ui_spec.lua, line 93:
<comment>This test leaks global state by restoring `background` to a hardcoded value instead of the original pre-test value.</comment>
<file context>
@@ -59,4 +59,96 @@ describe("ui", function()
+
+ describe("auto-dark-mode pattern", function()
+ it("should support background option dark/light", function()
+ vim.api.nvim_set_option_value("background", "dark", {})
+ assert.equals("dark", vim.o.background)
+ vim.api.nvim_set_option_value("background", "light", {})
</file context>
Suggested change
| vim.api.nvim_set_option_value("background", "dark", {}) | |
| assert.equals("dark", vim.o.background) | |
| vim.api.nvim_set_option_value("background", "light", {}) | |
| assert.equals("light", vim.o.background) | |
| vim.api.nvim_set_option_value("background", "dark", {}) | |
| local original_background = vim.o.background | |
| vim.api.nvim_set_option_value("background", "dark", {}) | |
| assert.equals("dark", vim.o.background) | |
| vim.api.nvim_set_option_value("background", "light", {}) | |
| assert.equals("light", vim.o.background) | |
| vim.api.nvim_set_option_value("background", original_background, {}) |
Comment on lines
+218
to
+219
| if old_client then vim.fn.setenv("SSH_CLIENT", old_client) end | ||
| if old_tty then vim.fn.setenv("SSH_TTY", old_tty) end |
Contributor
There was a problem hiding this comment.
P2: This test does not fully restore SSH environment variables when they were originally unset, causing cross-test environment leakage.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/programs/neovim/tests/utils_spec.lua, line 218:
<comment>This test does not fully restore SSH environment variables when they were originally unset, causing cross-test environment leakage.</comment>
<file context>
@@ -91,19 +91,160 @@ describe("utils", function()
+ -- is_ssh checks non-nil, empty string is still non-nil
+ -- just verify it returns boolean
+ assert.is_boolean(utils.is_ssh())
+ if old_client then vim.fn.setenv("SSH_CLIENT", old_client) end
+ if old_tty then vim.fn.setenv("SSH_TTY", old_tty) end
+ end)
</file context>
Suggested change
| if old_client then vim.fn.setenv("SSH_CLIENT", old_client) end | |
| if old_tty then vim.fn.setenv("SSH_TTY", old_tty) end | |
| vim.env.SSH_CLIENT = old_client | |
| vim.env.SSH_TTY = old_tty |
Comment on lines
+181
to
+192
| it("all vscode-diff keymaps should be silent and noremap", function() | ||
| local keys = { "gD_test", "gH_test", "gr_test", "gf_test" } | ||
| local keymaps = vim.api.nvim_get_keymap("n") | ||
| for _, key in ipairs(keys) do | ||
| for _, km in ipairs(keymaps) do | ||
| if km.lhs:match(key) then | ||
| assert.equals(1, km.silent, key .. " should be silent") | ||
| assert.equals(1, km.noremap, key .. " should be noremap") | ||
| break | ||
| end | ||
| end | ||
| end |
Contributor
There was a problem hiding this comment.
P2: This test silently passes if a keymap is not found. The inner loop breaks on match but there's no tracking of whether a match occurred. If a keymap is missing, no assertion fires. Add a found flag and assert it after the inner loop.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/programs/neovim/tests/keymaps_spec.lua, line 181:
<comment>This test silently passes if a keymap is not found. The inner loop `break`s on match but there's no tracking of whether a match occurred. If a keymap is missing, no assertion fires. Add a `found` flag and assert it after the inner loop.</comment>
<file context>
@@ -119,6 +119,282 @@ describe("keymaps", function()
+ assert.is_true(km ~= "")
+ end)
+
+ it("all vscode-diff keymaps should be silent and noremap", function()
+ local keys = { "gD_test", "gH_test", "gr_test", "gf_test" }
+ local keymaps = vim.api.nvim_get_keymap("n")
</file context>
Suggested change
| it("all vscode-diff keymaps should be silent and noremap", function() | |
| local keys = { "gD_test", "gH_test", "gr_test", "gf_test" } | |
| local keymaps = vim.api.nvim_get_keymap("n") | |
| for _, key in ipairs(keys) do | |
| for _, km in ipairs(keymaps) do | |
| if km.lhs:match(key) then | |
| assert.equals(1, km.silent, key .. " should be silent") | |
| assert.equals(1, km.noremap, key .. " should be noremap") | |
| break | |
| end | |
| end | |
| end | |
| it("all vscode-diff keymaps should be silent and noremap", function() | |
| local keys = { "gD_test", "gH_test", "gr_test", "gf_test" } | |
| local keymaps = vim.api.nvim_get_keymap("n") | |
| for _, key in ipairs(keys) do | |
| local found = false | |
| for _, km in ipairs(keymaps) do | |
| if km.lhs:match(key) then | |
| assert.equals(1, km.silent, key .. " should be silent") | |
| assert.equals(1, km.noremap, key .. " should be noremap") | |
| found = true | |
| break | |
| end | |
| end | |
| assert.is_true(found, key .. " keymap not found") | |
| end | |
| end) |
| assert.is_boolean(result) | ||
| end) | ||
|
|
||
| it("should return false when no SSH env vars set", function() |
Contributor
There was a problem hiding this comment.
P3: This test is named "should return false when no SSH env vars set" but only asserts is_boolean, not is_false. Additionally, if old_client/old_tty were originally nil, the conditional restore is skipped, leaking the empty-string env vars into subsequent tests.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/programs/neovim/tests/utils_spec.lua, line 209:
<comment>This test is named "should return false when no SSH env vars set" but only asserts `is_boolean`, not `is_false`. Additionally, if `old_client`/`old_tty` were originally `nil`, the conditional restore is skipped, leaking the empty-string env vars into subsequent tests.</comment>
<file context>
@@ -91,19 +91,160 @@ describe("utils", function()
+ assert.is_boolean(result)
+ end)
+
+ it("should return false when no SSH env vars set", function()
+ local old_client = os.getenv("SSH_CLIENT")
+ local old_tty = os.getenv("SSH_TTY")
</file context>
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 `@home-manager/programs/neovim/lua/config/keymaps.lua`:
- Around line 257-274: The sequence of synchronous calls in the keymap callback
causes a race because require("oil").open() is asynchronous; change the flow in
the anonymous function bound by keymap("n", "<leader>-", ...) to use oil.open's
callback parameter so the second actions run only after the first buffer is
loaded: after creating the split and focusing the left window, call
require("oil").open(a, {}, function() ... end) and inside that callback run
vim.cmd("wincmd l") and then call require("oil").open(b, {}, function() ... end)
(or finalize there) so operations happen in proper order and avoid the
focus/load race.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3193277b-7661-4c54-a811-6c254acae803
📒 Files selected for processing (4)
home-manager/programs/neovim/lua/config/keymaps.luahome-manager/programs/neovim/tests/keymaps_spec.luahome-manager/programs/neovim/tests/terminal_spec.luahome-manager/programs/neovim/tests/utils_spec.lua
✅ Files skipped from review due to trivial changes (1)
- home-manager/programs/neovim/tests/terminal_spec.lua
🚧 Files skipped from review as they are similar to previous changes (2)
- home-manager/programs/neovim/tests/keymaps_spec.lua
- home-manager/programs/neovim/tests/utils_spec.lua
Comment on lines
+257
to
+274
| -- @keymap <leader>-: Oil split - open two dirs side by side (prompted) | ||
| keymap("n", "<leader>-", function() | ||
| vim.ui.input({ prompt = "Dir A: ", default = vim.fn.expand("%:p:h"), completion = "dir" }, function(a) | ||
| if not a or a == "" then | ||
| return | ||
| end | ||
| vim.ui.input({ prompt = "Dir B: ", completion = "dir" }, function(b) | ||
| if not b or b == "" then | ||
| return | ||
| end | ||
| vim.cmd("vsplit") | ||
| vim.cmd("wincmd h") | ||
| require("oil").open(a) | ||
| vim.cmd("wincmd l") | ||
| require("oil").open(b) | ||
| end) | ||
| end) | ||
| end, { desc = "Oil: open two dirs side by side" }) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does oil.nvim's open() function execute synchronously or asynchronously?
💡 Result:
require("oil").open(dir, opts, cb) is asynchronous with respect to “when the Oil buffer is ready.” It kicks off :edit immediately, then schedules your cb to run later via util.run_after_load(0, function() ... end), i.e. after the Oil buffer has finished loading/populating. Use the cb if you need to do work once Oil is ready. [1], [2]
Sources:
[1] oil.nvim doc/api.md (open(): “cb … Called after the oil buffer is ready”)
[2] oil.nvim lua/oil/init.lua (open(): uses util.run_after_load(..., function() ... cb() ... end))
Timing issue with sequential oil.open() calls—use callbacks instead of scheduling.
oil.open() is asynchronous; it schedules buffer loading and returns immediately. The current sequence oil.open(a) → wincmd l → oil.open(b) executes without waiting for the first buffer to be ready, creating a race condition where window focus may shift before oil finishes loading.
Use oil's callback parameter to ensure the first buffer is fully loaded before proceeding:
🛠️ Proposed fix using oil's callback
vim.cmd("vsplit")
vim.cmd("wincmd h")
-require("oil").open(a)
-vim.cmd("wincmd l")
-require("oil").open(b)
+require("oil").open(a, {}, function()
+ vim.cmd("wincmd l")
+ require("oil").open(b)
+end)Alternatively, if you need to wait for both buffers:
require("oil").open(a, {}, function()
vim.cmd("wincmd l")
require("oil").open(b, {}, function()
-- Both buffers are now loaded
end)
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/keymaps.lua` around lines 257 - 274,
The sequence of synchronous calls in the keymap callback causes a race because
require("oil").open() is asynchronous; change the flow in the anonymous function
bound by keymap("n", "<leader>-", ...) to use oil.open's callback parameter so
the second actions run only after the first buffer is loaded: after creating the
split and focusing the left window, call require("oil").open(a, {}, function()
... end) and inside that callback run vim.cmd("wincmd l") and then call
require("oil").open(b, {}, function() ... end) (or finalize there) so operations
happen in proper order and avoid the focus/load race.
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved 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/tests/keymaps_spec.lua">
<violation number="1" location="home-manager/programs/neovim/tests/keymaps_spec.lua:378">
P2: This test clears every loaded `config.*` module from global `package.loaded`, which can leak state across tests. Restrict unloading to the fake test module (or restore prior entries) to keep the spec isolated.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| before_each(function() | ||
| vim.keymap.set("n", "<leader>R_test", function() | ||
| for name, _ in pairs(package.loaded) do | ||
| if name:match("^config%.") then |
Contributor
There was a problem hiding this comment.
P2: This test clears every loaded config.* module from global package.loaded, which can leak state across tests. Restrict unloading to the fake test module (or restore prior entries) to keep the spec isolated.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/programs/neovim/tests/keymaps_spec.lua, line 378:
<comment>This test clears every loaded `config.*` module from global `package.loaded`, which can leak state across tests. Restrict unloading to the fake test module (or restore prior entries) to keep the spec isolated.</comment>
<file context>
@@ -371,25 +371,28 @@ describe("keymaps", function()
- assert.is_string(cmd)
- end, { desc = "Rebuild and switch Nix config" })
+ for name, _ in pairs(package.loaded) do
+ if name:match("^config%.") then
+ package.loaded[name] = nil
+ end
</file context>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
home-manager/programs/neovim/default.nix (1)
22-22:⚠️ Potential issue | 🔴 Critical
libExtis hardcoded to"so"but macOS uses"dylib".The shared library extension differs by platform: Linux uses
.sowhile macOS uses.dylib. This will cause the native plugin checks and downloads to fail on macOS since the script will look for/write.sofiles that don't match the actual library names.🐛 Proposed fix: set libExt based on platform
- libExt = "so"; + libExt = if pkgs.stdenv.isDarwin then "dylib" else "so";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@home-manager/programs/neovim/default.nix` at line 22, libExt is hardcoded to "so" which breaks macOS; change the libExt binding to select "dylib" on Darwin and "so" elsewhere (e.g., use stdenv.isDarwin ? "dylib" : "so" or equivalent platform check) so all places referencing libExt in this file will use the correct shared-library extension for the current platform.
♻️ Duplicate comments (5)
home-manager/programs/neovim/lua/config/keymaps.lua (2)
277-281:⚠️ Potential issue | 🟡 MinorRace condition with sequential async
oil.open()calls.
oil.open()is asynchronous—it returns immediately while the buffer loads in the background. The current sequence executeswincmd lbefore the first Oil buffer may be ready, creating unpredictable window focus behavior.Use Oil's callback parameter to sequence operations properly.
🛠️ Proposed fix using oil's callback
vim.cmd("vsplit") vim.cmd("wincmd h") -require("oil").open(a) -vim.cmd("wincmd l") -require("oil").open(b) +require("oil").open(a, {}, function() + vim.cmd("wincmd l") + require("oil").open(b) +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/keymaps.lua` around lines 277 - 281, The sequential calls to require("oil").open(a) and require("oil").open(b) race because oil.open is async; instead call vim.cmd("vsplit") and vim.cmd("wincmd h"), then invoke require("oil").open(a, callback) and inside that callback run vim.cmd("wincmd l") and then call require("oil").open(b, callback) (or chain callbacks/promises) so the second oil.open runs only after the first buffer is ready; reference the oil.open function and the vim.cmd "vsplit"/"wincmd h"/"wincmd l" calls when applying the change.
195-196:⚠️ Potential issue | 🔴 CriticalDuplicate keymap
<leader>gsoverwrites Git status mapping.Line 147 defines
<leader>gsas:tab Git<cr>(open Git status in new tab). This second definition at line 196 overwrites it with:Gvdiffsplit<cr>, breaking the original functionality.Use a different key for the Fugitive diff split, such as
<leader>gvor<leader>gi.🐛 Proposed fix: rename to avoid conflict
--- `@keymap` <leader>gs: Fugitive vertical diff split (current file vs index) -keymap("n", "<leader>gs", ":Gvdiffsplit<cr>", opts) +-- `@keymap` <leader>gi: Fugitive vertical diff split (current file vs index) +keymap("n", "<leader>gi", ":Gvdiffsplit<cr>", 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 195 - 196, The keymap "<leader>gs" is defined twice (first as keymap("n", "<leader>gs", ":tab Git<cr>" and later as keymap("n", "<leader>gs", ":Gvdiffsplit<cr>")), so the second definition overwrites the Git status mapping; change the Fugitive diff split mapping in the keymap(...) call to a non-conflicting key (e.g., "<leader>gv" or "<leader>gi") so both mappings coexist—update the second keymap invocation that calls ":Gvdiffsplit<cr>" accordingly.home-manager/programs/neovim/tests/keymaps_spec.lua (3)
275-286:⚠️ Potential issue | 🟡 MinorSame pattern issue: test silently passes if keymap not found.
Add a
foundflag and assertion for each key in the hunk navigation keymaps test.💚 Proposed fix
it("hunk navigation keymaps should be silent and noremap", function() local keymaps = vim.api.nvim_get_keymap("n") for _, key in ipairs(keys) do + local found = false for _, km in ipairs(keymaps) do if km.lhs == key then assert.equals(1, km.silent, key .. " should be silent") assert.equals(1, km.noremap, key .. " should be noremap") + found = true break end end + assert.is_true(found, key .. " keymap not found") end end)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@home-manager/programs/neovim/tests/keymaps_spec.lua` around lines 275 - 286, The test in the "hunk navigation keymaps should be silent and noremap" spec silently passes when a keymap isn't present; add a local found flag inside the outer loop over keys (the keys table used in the test), set found = true when km.lhs == key inside the inner loop over keymaps (km), and after the inner loop assert that found is true (e.g., assert.is_true(found, key .. " should be mapped")) before asserting km.silent and km.noremap so each key is verified to exist and then checked for silent/noremap.
236-247:⚠️ Potential issue | 🟡 MinorTest silently passes if keymap not found.
The loop breaks after finding a match but doesn't track whether a match was actually found. If a keymap is missing from the list, the test passes silently. Add a
foundflag and assertion for each key.💚 Proposed fix: track found state
it("fugitive diff keymaps should be silent and noremap", function() local keymaps = vim.api.nvim_get_keymap("n") for _, key in ipairs(keys) do + local found = false for _, km in ipairs(keymaps) do if km.lhs == key then assert.equals(1, km.silent, key .. " should be silent") assert.equals(1, km.noremap, key .. " should be noremap") + found = true break end end + assert.is_true(found, key .. " keymap not found") end end)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@home-manager/programs/neovim/tests/keymaps_spec.lua` around lines 236 - 247, The test loop in the anonymous it("fugitive diff keymaps should be silent and noremap", function() iterates keys and breaks when a matching km.lhs is found but never asserts that a match was found, so missing keymaps pass silently; update the inner loop to set a local found = false when starting each key, set found = true before breaking when km.lhs == key, and after the inner loop add an assertion (e.g., assert.is_true(found, key .. " should be mapped")) before asserting km.silent and km.noremap (or assert the silent/noremap only when found) so each key fails if not present; reference the existing variables keys, keymaps, km.lhs, km.silent and km.noremap to locate and modify the code.
360-371:⚠️ Potential issue | 🟡 MinorSame pattern issue: test silently passes if keymap not found.
Add a
foundflag and assertion for each key in the gitsigns keymaps test.💚 Proposed fix
it("all gitsigns keymaps should be silent and noremap", function() local keymaps = vim.api.nvim_get_keymap("n") for _, km in ipairs(git_keys) do + local found = false for _, m in ipairs(keymaps) do if m.lhs == km.lhs then assert.equals(1, m.silent, km.lhs .. " should be silent") assert.equals(1, m.noremap, km.lhs .. " should be noremap") + found = true break end end + assert.is_true(found, km.lhs .. " keymap not found") end end)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@home-manager/programs/neovim/tests/keymaps_spec.lua` around lines 360 - 371, The test iterates git_keys but currently silently passes if a keymap isn't present; update the loop in the "all gitsigns keymaps should be silent and noremap" test to set a local found = false for each km in git_keys, set found = true when a matching m.lhs is found, then after the inner loop assert.equals(true, found, km.lhs .. " should be defined") before asserting m.silent and m.noremap (or assert those only when found). Reference the existing variables/function names: git_keys, keymaps (vim.api.nvim_get_keymap), km, m, and the test block "all gitsigns keymaps should be silent and noremap".
🤖 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/default.nix`:
- Around line 107-115: Remove the misleading glob test that uses [ ! -f
"$vsd_dir/libvscode_diff"*".${libExt}" ] (globs don't expand inside [ ]), and
instead first check the directory exists with [ -n "$vsd_dir" ] then perform the
existing ls "$vsd_dir"/libvscode_diff*.${libExt} check to decide whether to run
$DRY_RUN_CMD bash "$vsd_dir/build.sh"; keep the inner ls-based guard, the echo
messages and the build invocation (build.sh / DRY_RUN_CMD) unchanged.
- Around line 77-95: The activation hook that tries to download the fff.nvim
binary uses Linux-only detection (_ldd, setting _triple to -unknown-linux-gnu or
-unknown-linux-musl and libExt="so") so it will pick the wrong asset on macOS;
update the logic around fff_version/_ldd/_triple/libExt/fff_binary to detect
Darwin (uname -s or uname -m) and set a Darwin triple and libExt=".dylib" when
running on macOS, or alternatively short-circuit and skip the activation-time
download on non-Linux hosts; also consider removing or guarding this download
path (since plugins.lua already exposes a build function
require("fff.download").download_or_build_binary()) so you don’t duplicate
download/build behavior at activation time.
---
Outside diff comments:
In `@home-manager/programs/neovim/default.nix`:
- Line 22: libExt is hardcoded to "so" which breaks macOS; change the libExt
binding to select "dylib" on Darwin and "so" elsewhere (e.g., use
stdenv.isDarwin ? "dylib" : "so" or equivalent platform check) so all places
referencing libExt in this file will use the correct shared-library extension
for the current platform.
---
Duplicate comments:
In `@home-manager/programs/neovim/lua/config/keymaps.lua`:
- Around line 277-281: The sequential calls to require("oil").open(a) and
require("oil").open(b) race because oil.open is async; instead call
vim.cmd("vsplit") and vim.cmd("wincmd h"), then invoke require("oil").open(a,
callback) and inside that callback run vim.cmd("wincmd l") and then call
require("oil").open(b, callback) (or chain callbacks/promises) so the second
oil.open runs only after the first buffer is ready; reference the oil.open
function and the vim.cmd "vsplit"/"wincmd h"/"wincmd l" calls when applying the
change.
- Around line 195-196: The keymap "<leader>gs" is defined twice (first as
keymap("n", "<leader>gs", ":tab Git<cr>" and later as keymap("n", "<leader>gs",
":Gvdiffsplit<cr>")), so the second definition overwrites the Git status
mapping; change the Fugitive diff split mapping in the keymap(...) call to a
non-conflicting key (e.g., "<leader>gv" or "<leader>gi") so both mappings
coexist—update the second keymap invocation that calls ":Gvdiffsplit<cr>"
accordingly.
In `@home-manager/programs/neovim/tests/keymaps_spec.lua`:
- Around line 275-286: The test in the "hunk navigation keymaps should be silent
and noremap" spec silently passes when a keymap isn't present; add a local found
flag inside the outer loop over keys (the keys table used in the test), set
found = true when km.lhs == key inside the inner loop over keymaps (km), and
after the inner loop assert that found is true (e.g., assert.is_true(found, key
.. " should be mapped")) before asserting km.silent and km.noremap so each key
is verified to exist and then checked for silent/noremap.
- Around line 236-247: The test loop in the anonymous it("fugitive diff keymaps
should be silent and noremap", function() iterates keys and breaks when a
matching km.lhs is found but never asserts that a match was found, so missing
keymaps pass silently; update the inner loop to set a local found = false when
starting each key, set found = true before breaking when km.lhs == key, and
after the inner loop add an assertion (e.g., assert.is_true(found, key .. "
should be mapped")) before asserting km.silent and km.noremap (or assert the
silent/noremap only when found) so each key fails if not present; reference the
existing variables keys, keymaps, km.lhs, km.silent and km.noremap to locate and
modify the code.
- Around line 360-371: The test iterates git_keys but currently silently passes
if a keymap isn't present; update the loop in the "all gitsigns keymaps should
be silent and noremap" test to set a local found = false for each km in
git_keys, set found = true when a matching m.lhs is found, then after the inner
loop assert.equals(true, found, km.lhs .. " should be defined") before asserting
m.silent and m.noremap (or assert those only when found). Reference the existing
variables/function names: git_keys, keymaps (vim.api.nvim_get_keymap), km, m,
and the test block "all gitsigns keymaps should be silent and noremap".
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 167953f2-1c46-4e44-b15d-9fbd65399687
📒 Files selected for processing (3)
home-manager/programs/neovim/default.nixhome-manager/programs/neovim/lua/config/keymaps.luahome-manager/programs/neovim/tests/keymaps_spec.lua
Comment on lines
+77
to
+95
| fff_version=$(git -C "$fff_dir" rev-parse --short HEAD 2>/dev/null || echo "") | ||
| if [ -n "$fff_version" ]; then | ||
| _arch=$(uname -m) | ||
| _ldd=$(ldd --version 2>&1 || echo "") | ||
| if echo "$_ldd" | grep -q musl; then | ||
| _triple="''${_arch}-unknown-linux-musl" | ||
| else | ||
| _triple="''${_arch}-unknown-linux-gnu" | ||
| fi | ||
| mkdir -p "$fff_dir/target" | ||
| echo "Fetching https://github.com/dmtrKovalenko/fff.nvim/releases/download/$fff_version/''${_triple}.${libExt}" | ||
| $DRY_RUN_CMD curl --fail --location --silent --show-error \ | ||
| -o "$fff_binary" \ | ||
| "https://github.com/dmtrKovalenko/fff.nvim/releases/download/$fff_version/''${_triple}.${libExt}" \ | ||
| && echo "fff.nvim binary downloaded successfully" \ | ||
| || echo "fff.nvim binary download failed (will fall back to build on first use)" | ||
| else | ||
| echo "fff.nvim: could not determine version, skipping download" | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify fff.nvim is not installed as a git repo
fd -t d 'fff.nvim' ~/.local/share/nvim/site/pack 2>/dev/null | head -1 | xargs -I{} sh -c 'test -d "{}/.git" && echo "Has .git" || echo "No .git directory"'Repository: shunkakinoki/dotfiles
Length of output: 47
🏁 Script executed:
fd -t f 'plugins.lua' home-manager/Repository: shunkakinoki/dotfiles
Length of output: 116
🏁 Script executed:
cat -n home-manager/programs/neovim/default.nix | sed -n '60,100p'Repository: shunkakinoki/dotfiles
Length of output: 2062
🏁 Script executed:
cat -n home-manager/programs/neovim/lua/config/plugins.lua 2>/dev/null | head -50Repository: shunkakinoki/dotfiles
Length of output: 2503
🏁 Script executed:
rg -i "vim.pack" home-manager/ --type lua -B 2 -A 2Repository: shunkakinoki/dotfiles
Length of output: 2477
🏁 Script executed:
rg -i "libext" home-manager/programs/neovim/default.nix -B 5 -A 5Repository: shunkakinoki/dotfiles
Length of output: 2213
🏁 Script executed:
web_search
vim.pack.add clones git repository or downloads tarballRepository: shunkakinoki/dotfiles
Length of output: 166
🌐 Web query:
Neovim vim.pack.add behavior git clone vs tarball download
💡 Result:
vim.pack.add() installs plugins by running Git operations (a “partial blobless git clone”, then git checkout to the requested version)—it is not a “download a tarball/zip and extract it” mechanism. It also states that src can be any URI format supported by git clone (and it stores that src verbatim in the lockfile). [1]
So in practice:
- If a plugin is missing on disk,
vim.pack.add()will clone the repo (partial/blobless) and then checkout the targetversion. [1] - If you give it something that isn’t clonable by
git clone(like a GitHub “tarball URL”), it won’t work unless that URL is still valid input togit clone(generally it isn’t). [1]
Sources
[1] Neovim :help pack / vim.pack.add() documentation (neovim.io)
Citations:
🏁 Script executed:
cat -n home-manager/programs/neovim/default.nix | sed -n '1,30p'Repository: shunkakinoki/dotfiles
Length of output: 765
fff.nvim script has macOS compatibility issues.
The script assumes a Linux environment and will fail on macOS:
- Line 80:
ldd --versiondoesn't exist on macOS (only on GNU/Linux) - Lines 82–85: Triple detection hardcodes
-unknown-linux-gnuor-unknown-linux-musl; no Darwin/macOS triple - Line 22:
libExt = "so"is hardcoded for Linux (should be.dylibon macOS)
On macOS, the script will set an invalid triple and attempt to download a Linux binary. The activation hook should either skip the download on non-Linux systems or properly detect Darwin and construct the correct Mach-O binary path.
Additionally, since the plugin already has a build function in plugins.lua (line 30) that calls require("fff.download").download_or_build_binary(), this activation-time download logic may be redundant.
🤖 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 77 - 95, The
activation hook that tries to download the fff.nvim binary uses Linux-only
detection (_ldd, setting _triple to -unknown-linux-gnu or -unknown-linux-musl
and libExt="so") so it will pick the wrong asset on macOS; update the logic
around fff_version/_ldd/_triple/libExt/fff_binary to detect Darwin (uname -s or
uname -m) and set a Darwin triple and libExt=".dylib" when running on macOS, or
alternatively short-circuit and skip the activation-time download on non-Linux
hosts; also consider removing or guarding this download path (since plugins.lua
already exposes a build function
require("fff.download").download_or_build_binary()) so you don’t duplicate
download/build behavior at activation time.
Comment on lines
+107
to
+115
| if [ -n "$vsd_dir" ] && [ ! -f "$vsd_dir/libvscode_diff"*".${libExt}" ] 2>/dev/null; then | ||
| if ls "$vsd_dir"/libvscode_diff*.${libExt} 1>/dev/null 2>&1; then | ||
| : # already built | ||
| else | ||
| echo "Building vscode-diff.nvim native library..." | ||
| $DRY_RUN_CMD bash "$vsd_dir/build.sh" && echo "vscode-diff.nvim built successfully" \ | ||
| || echo "vscode-diff.nvim build failed" | ||
| fi | ||
| fi |
There was a problem hiding this comment.
Shell glob in [ -f ... ] test doesn't expand—condition is misleading.
Line 107's [ ! -f "$vsd_dir/libvscode_diff"*".${libExt}" ] won't work as intended because globs don't expand inside [ ] test brackets. The condition tests for a literal filename containing an asterisk, which will always be true (no such file exists).
The actual guard is the ls check on line 108, which does work correctly. Consider simplifying by removing the misleading outer condition.
♻️ Proposed simplification
- if [ -n "$vsd_dir" ] && [ ! -f "$vsd_dir/libvscode_diff"*".${libExt}" ] 2>/dev/null; then
- if ls "$vsd_dir"/libvscode_diff*.${libExt} 1>/dev/null 2>&1; then
- : # already built
- else
+ if [ -n "$vsd_dir" ]; then
+ if ! ls "$vsd_dir"/libvscode_diff*.${libExt} 1>/dev/null 2>&1; then
echo "Building vscode-diff.nvim native library..."
$DRY_RUN_CMD bash "$vsd_dir/build.sh" && echo "vscode-diff.nvim built successfully" \
|| echo "vscode-diff.nvim build failed"
fi
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.
Suggested change
| if [ -n "$vsd_dir" ] && [ ! -f "$vsd_dir/libvscode_diff"*".${libExt}" ] 2>/dev/null; then | |
| if ls "$vsd_dir"/libvscode_diff*.${libExt} 1>/dev/null 2>&1; then | |
| : # already built | |
| else | |
| echo "Building vscode-diff.nvim native library..." | |
| $DRY_RUN_CMD bash "$vsd_dir/build.sh" && echo "vscode-diff.nvim built successfully" \ | |
| || echo "vscode-diff.nvim build failed" | |
| fi | |
| fi | |
| if [ -n "$vsd_dir" ]; then | |
| if ! ls "$vsd_dir"/libvscode_diff*.${libExt} 1>/dev/null 2>&1; then | |
| echo "Building vscode-diff.nvim native library..." | |
| $DRY_RUN_CMD bash "$vsd_dir/build.sh" && echo "vscode-diff.nvim built successfully" \ | |
| || echo "vscode-diff.nvim build failed" | |
| fi | |
| fi |
🤖 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 107 - 115, Remove the
misleading glob test that uses [ ! -f "$vsd_dir/libvscode_diff"*".${libExt}" ]
(globs don't expand inside [ ]), and instead first check the directory exists
with [ -n "$vsd_dir" ] then perform the existing ls
"$vsd_dir"/libvscode_diff*.${libExt} check to decide whether to run $DRY_RUN_CMD
bash "$vsd_dir/build.sh"; keep the inner ls-based guard, the echo messages and
the build invocation (build.sh / DRY_RUN_CMD) unchanged.
Contributor
There was a problem hiding this comment.
2 issues found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved 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/default.nix">
<violation number="1" location="home-manager/programs/neovim/default.nix:84">
P2: The new binary target logic is Linux-only; on Darwin it still builds a Linux triple and attempts to download an incompatible native library.</violation>
<violation number="2" location="home-manager/programs/neovim/default.nix:86">
P2: `mkdir` is not wrapped with `$DRY_RUN_CMD`, so dry-run still mutates the filesystem.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| else | ||
| _triple="''${_arch}-unknown-linux-gnu" | ||
| fi | ||
| mkdir -p "$fff_dir/target" |
Contributor
There was a problem hiding this comment.
P2: mkdir is not wrapped with $DRY_RUN_CMD, so dry-run still mutates the filesystem.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/programs/neovim/default.nix, line 86:
<comment>`mkdir` is not wrapped with `$DRY_RUN_CMD`, so dry-run still mutates the filesystem.</comment>
<file context>
@@ -59,5 +61,57 @@ in
+ else
+ _triple="''${_arch}-unknown-linux-gnu"
+ fi
+ mkdir -p "$fff_dir/target"
+ echo "Fetching https://github.com/dmtrKovalenko/fff.nvim/releases/download/$fff_version/''${_triple}.${libExt}"
+ $DRY_RUN_CMD curl --fail --location --silent --show-error \
</file context>
Suggested change
| mkdir -p "$fff_dir/target" | |
| $DRY_RUN_CMD mkdir -p "$fff_dir/target" |
| if echo "$_ldd" | grep -q musl; then | ||
| _triple="''${_arch}-unknown-linux-musl" | ||
| else | ||
| _triple="''${_arch}-unknown-linux-gnu" |
Contributor
There was a problem hiding this comment.
P2: The new binary target logic is Linux-only; on Darwin it still builds a Linux triple and attempts to download an incompatible native library.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/programs/neovim/default.nix, line 84:
<comment>The new binary target logic is Linux-only; on Darwin it still builds a Linux triple and attempts to download an incompatible native library.</comment>
<file context>
@@ -59,5 +61,57 @@ in
+ if echo "$_ldd" | grep -q musl; then
+ _triple="''${_arch}-unknown-linux-musl"
+ else
+ _triple="''${_arch}-unknown-linux-gnu"
+ fi
+ mkdir -p "$fff_dir/target"
</file context>
…target - Add :NvimPluginsInstall user command that downloads/builds native plugin binaries (fff.nvim, telescope-fzf-native, vscode-diff.nvim) - Add nvim-plugins-install Makefile target running shell download logic - Wire nvim-plugins-install into switch target before dotagents-sync - Fix run_tests.sh to prefer pack-installed plenary (has test_harness) - Fix minimal_init.lua to add plenary lua path to package.path - Add 5 tests for NvimPluginsInstall command in keymaps_spec.lua
Contributor
There was a problem hiding this comment.
4 issues found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved 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="Makefile">
<violation number="1" location="Makefile:190">
P2: `nvim-plugins-install` runs on all platforms but the fff download path is Linux-only (`*-unknown-linux-*.so`). Guard this block to Linux to avoid repeated failing downloads on macOS.</violation>
</file>
<file name="home-manager/programs/neovim/tests/keymaps_spec.lua">
<violation number="1" location="home-manager/programs/neovim/tests/keymaps_spec.lua:490">
P2: The description test is ineffective: `or ""` makes the assertion always pass, so missing command descriptions won’t be caught.</violation>
</file>
<file name="home-manager/programs/neovim/lua/config/keymaps.lua">
<violation number="1" location="home-manager/programs/neovim/lua/config/keymaps.lua:415">
P2: Architecture detection is incorrect: all non-x64 platforms are treated as aarch64, which can fetch the wrong binary URL.</violation>
<violation number="2" location="home-manager/programs/neovim/lua/config/keymaps.lua:416">
P2: The download target triple is hardcoded to Linux, so this command will request Linux binaries even on non-Linux hosts.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if echo "$$_LDD" | grep -q musl; then \ | ||
| _TRIPLE="$${_ARCH}-unknown-linux-musl"; \ | ||
| else \ | ||
| _TRIPLE="$${_ARCH}-unknown-linux-gnu"; \ |
Contributor
There was a problem hiding this comment.
P2: nvim-plugins-install runs on all platforms but the fff download path is Linux-only (*-unknown-linux-*.so). Guard this block to Linux to avoid repeated failing downloads on macOS.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Makefile, line 190:
<comment>`nvim-plugins-install` runs on all platforms but the fff download path is Linux-only (`*-unknown-linux-*.so`). Guard this block to Linux to avoid repeated failing downloads on macOS.</comment>
<file context>
@@ -168,8 +168,46 @@ setup: nix-setup ## Basic Nix setup (alias for nix-setup).
+ if echo "$$_LDD" | grep -q musl; then \
+ _TRIPLE="$${_ARCH}-unknown-linux-musl"; \
+ else \
+ _TRIPLE="$${_ARCH}-unknown-linux-gnu"; \
+ fi; \
+ mkdir -p "$$FFF_DIR/target"; \
</file context>
| end | ||
| end | ||
| local arch = jit.arch:lower():match("x64") and "x86_64" or "aarch64" | ||
| local triple = arch .. "-unknown-linux-" .. libc |
Contributor
There was a problem hiding this comment.
P2: The download target triple is hardcoded to Linux, so this command will request Linux binaries even on non-Linux hosts.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/programs/neovim/lua/config/keymaps.lua, line 416:
<comment>The download target triple is hardcoded to Linux, so this command will request Linux binaries even on non-Linux hosts.</comment>
<file context>
@@ -373,3 +373,117 @@ wk.add({
+ end
+ end
+ local arch = jit.arch:lower():match("x64") and "x86_64" or "aarch64"
+ local triple = arch .. "-unknown-linux-" .. libc
+ local url = "https://github.com/saghen/blink.cmp/releases/download/" .. tag .. "/" .. triple .. ".so"
+ vim.schedule(function()
</file context>
Comment on lines
+415
to
+416
| local arch = jit.arch:lower():match("x64") and "x86_64" or "aarch64" | ||
| local triple = arch .. "-unknown-linux-" .. libc |
Contributor
There was a problem hiding this comment.
P2: Architecture detection is incorrect: all non-x64 platforms are treated as aarch64, which can fetch the wrong binary URL.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/programs/neovim/lua/config/keymaps.lua, line 415:
<comment>Architecture detection is incorrect: all non-x64 platforms are treated as aarch64, which can fetch the wrong binary URL.</comment>
<file context>
@@ -373,3 +373,117 @@ wk.add({
+ libc = last
+ end
+ end
+ local arch = jit.arch:lower():match("x64") and "x86_64" or "aarch64"
+ local triple = arch .. "-unknown-linux-" .. libc
+ local url = "https://github.com/saghen/blink.cmp/releases/download/" .. tag .. "/" .. triple .. ".so"
</file context>
Suggested change
| local arch = jit.arch:lower():match("x64") and "x86_64" or "aarch64" | |
| local triple = arch .. "-unknown-linux-" .. libc | |
| local arch_map = { x64 = "x86_64", arm64 = "aarch64" } | |
| local arch = arch_map[jit.arch:lower()] | |
| if not arch then | |
| vim.schedule(function() | |
| vim.notify("blink.cmp: unsupported architecture '" .. jit.arch .. "', skipping prebuilt download", vim.log.levels.WARN) | |
| end) | |
| return | |
| end | |
| local triple = arch .. "-unknown-linux-" .. libc |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
home-manager/programs/neovim/lua/config/keymaps.lua (2)
195-196:⚠️ Potential issue | 🟠 Major
<leader>gsstill clobbers the existing Git status mapping.Line 147 already maps
<leader>gsto:tab Git<cr>, so this remap replaces that behavior with:Gvdiffsplit<cr>.Suggested fix
--- `@keymap` <leader>gs: Fugitive vertical diff split (current file vs index) -keymap("n", "<leader>gs", ":Gvdiffsplit<cr>", opts) +-- `@keymap` <leader>gi: Fugitive vertical diff split (current file vs index) +keymap("n", "<leader>gi", ":Gvdiffsplit<cr>", 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 195 - 196, The mapping key "<leader>gs" in keymap("n", "<leader>gs", ":Gvdiffsplit<cr>", opts) duplicates and overwrites the existing "<leader>gs" mapping to ":tab Git<cr>"; remove or change this remap to a non-conflicting key (or make it conditional) — locate the existing mapping that uses key "<leader>gs" (the one that calls ":tab Git<cr>") and either delete or rename the new keymap for Gvdiffsplit, or wrap the new keymap in a check to only set it if no mapping exists.
267-281:⚠️ Potential issue | 🟠 MajorWait for the first Oil buffer before moving to the right split.
oil.open()accepts a callback that runs after the Oil buffer is ready, so the immediatewincmd l+ secondopen()can race the left-hand buffer initialization and target the wrong window. (raw.githubusercontent.com)Suggested fix
vim.cmd("vsplit") vim.cmd("wincmd h") - require("oil").open(a) - vim.cmd("wincmd l") - require("oil").open(b) + require("oil").open(a, {}, function() + vim.cmd("wincmd l") + require("oil").open(b) + end) end) end) end, { desc = "Oil: open two dirs side by side" })🤖 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 267 - 281, The keymap's current flow calls require("oil").open(a) then immediately does vim.cmd("wincmd l") and require("oil").open(b), which can race; instead, call require("oil").open for the first directory (symbol: require("oil").open and the keymap callback defined for "<leader>-") with its completion callback/handler and only after that callback runs do vim.cmd("wincmd l") and call require("oil").open for the second directory (ensure the inner check for b remains). In short: move the right-split navigation and second require("oil").open(b) into the first oil.open's ready/callback so the left Oil buffer is fully initialized before switching windows.home-manager/programs/neovim/tests/keymaps_spec.lua (1)
183-208:⚠️ Potential issue | 🟡 MinorFail the test when an expected keymap is missing.
These loops only assert flags after a match. If a mapping disappears, the inner loop just falls through and the test still passes. The same pattern repeats in the fugitive, hunk-nav, and gitsigns checks below.
Suggested fix
local keys = { "gD_test", "gH_test", "gr_test", "gf_test" } local keymaps = vim.api.nvim_get_keymap("n") for _, key in ipairs(keys) do + local found = false for _, km in ipairs(keymaps) do if km.lhs:match(key) then assert.equals(1, km.silent, key .. " should be silent") assert.equals(1, km.noremap, key .. " should be noremap") + found = true break end end + assert.is_true(found, key .. " should be mapped") end end)Also applies to: 236-246, 275-285, 360-370
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@home-manager/programs/neovim/tests/keymaps_spec.lua` around lines 183 - 208, The tests iterate keys and check properties only when a matching mapping is found, so a missing mapping will silently pass; update each test (the "all vscode-diff keymaps should be silent and noremap" and "all vscode-diff keymaps should have function callbacks" blocks that define local keys and keymaps and loop over km with if km.lhs:match(key)) to track whether a match was found (e.g., set local found = false when entering the inner loop, set found = true inside the if branch before breaking) and after the inner loop assert that found is true with a clear message (e.g., assert.is_true(found, key .. " mapping should exist")), and apply the same pattern to the similar fugitive, hunk-nav, and gitsigns tests so missing mappings fail the test suite.
🤖 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 10-18: Replace the hardcoded PACK_DIR_EARLY resolution so the
script replicates minimal_init.lua's lookup: first query Neovim's data directory
(via nvim/vim.fn.stdpath("data")) and search
data_dir/site/pack/{plugins,core}/{start,opt}/plenary.nvim, then fall back to
$HOME/.local/share/nvim/site/pack/{plugins,core}/{start,opt}/plenary.nvim; set
PLENARY_PACK when a match is found and keep the existing PLENARY_DIR defaulting
behavior (PLENARY_DIR="${PLENARY_DIR:-${PLENARY_PACK:-/tmp/plenary.nvim}}") so
the runner and minimal_init use the same plenary path resolution (refer to
PACK_DIR_EARLY, PLENARY_PACK, and PLENARY_DIR).
In `@home-manager/programs/neovim/tests/keymaps_spec.lua`:
- Around line 122-150: The tests currently re-register _test mappings instead of
loading the real keymaps module, so change each spec (e.g., the block with
register_vscode_diff_keymaps and mappings like "<leader>gD_test",
"<leader>gH_test", "<leader>gr_test", "<leader>gf_test") to mock required plugin
modules (for example the vscode-diff plugin) and then require the actual
home-manager/programs/neovim/lua/config/keymaps.lua, asserting that the real
mappings/commands (including the problematic "<leader>gs" mapping and any
NvimPluginsInstall command) are created; specifically, replace the inline _test
registration with mocks for require("vscode-diff.commands") and other plugins,
call require("config.keymaps") to load the real setup function(s), and assert
vim.keymap.get / vim.fn.exists(":"..command) or equivalent to verify the real
mappings and commands exist.
In `@home-manager/programs/neovim/tests/minimal_init.lua`:
- Around line 29-30: The core pack paths currently use hardcoded
"~/.local/share/nvim/site/pack/core/start/plenary.nvim" and
"~/.local/share/nvim/site/pack/core/opt/plenary.nvim" but lack the portable
stdpath variants used for plugins; update the core pack entries to include
vim.fn.stdpath("data") equivalents (i.e., add vim.fn.stdpath("data") ..
"/site/pack/core/start/plenary.nvim" and vim.fn.stdpath("data") ..
"/site/pack/core/opt/plenary.nvim") alongside the existing vim.fn.expand
fallbacks so core lookup mirrors the plugins lookup strategy.
In `@Makefile`:
- Around line 171-207: The nvim-plugins-install target is advertised to manage
three native plugins but only tries fff.nvim and currently masks failures (the
curl ... || echo ... makes the step succeed even on download failure); update
the recipe so failures cause make to fail: remove the "|| echo ..." fallback and
instead check curl's exit status and exit 1 on failure (or wrap the entire shell
block with "set -e" / "set -o pipefail") for the FFF download path (refer to
variables/idents FFF_DIR, FFF_BINARY, FFF_VERSION and the curl invocation), and
either implement equivalent deterministic download/build logic for the other
advertised plugins (telescope-fzf-native, vscode-diff) or change the target help
text to only advertise fff.nvim so the target behavior matches its description.
- Around line 180-196: The Makefile hardcodes the library extension ".so" and
Linux triple parts; introduce a variable (e.g., libExt or FFF_LIB_EXT) and use
it when constructing FFF_BINARY, the target directory filename, and the download
URL so the extension is consistent with default.nix's libExt pattern; update the
places referencing FFF_BINARY, the curl -o target, and the echoed download URL
(symbols: FFF_BINARY, FFF_DIR, FFF_VERSION, _TRIPLE, _ARCH, _LDD) to use the new
variable instead of literal ".so" while leaving triple logic unchanged.
---
Duplicate comments:
In `@home-manager/programs/neovim/lua/config/keymaps.lua`:
- Around line 195-196: The mapping key "<leader>gs" in keymap("n", "<leader>gs",
":Gvdiffsplit<cr>", opts) duplicates and overwrites the existing "<leader>gs"
mapping to ":tab Git<cr>"; remove or change this remap to a non-conflicting key
(or make it conditional) — locate the existing mapping that uses key
"<leader>gs" (the one that calls ":tab Git<cr>") and either delete or rename the
new keymap for Gvdiffsplit, or wrap the new keymap in a check to only set it if
no mapping exists.
- Around line 267-281: The keymap's current flow calls require("oil").open(a)
then immediately does vim.cmd("wincmd l") and require("oil").open(b), which can
race; instead, call require("oil").open for the first directory (symbol:
require("oil").open and the keymap callback defined for "<leader>-") with its
completion callback/handler and only after that callback runs do vim.cmd("wincmd
l") and call require("oil").open for the second directory (ensure the inner
check for b remains). In short: move the right-split navigation and second
require("oil").open(b) into the first oil.open's ready/callback so the left Oil
buffer is fully initialized before switching windows.
In `@home-manager/programs/neovim/tests/keymaps_spec.lua`:
- Around line 183-208: The tests iterate keys and check properties only when a
matching mapping is found, so a missing mapping will silently pass; update each
test (the "all vscode-diff keymaps should be silent and noremap" and "all
vscode-diff keymaps should have function callbacks" blocks that define local
keys and keymaps and loop over km with if km.lhs:match(key)) to track whether a
match was found (e.g., set local found = false when entering the inner loop, set
found = true inside the if branch before breaking) and after the inner loop
assert that found is true with a clear message (e.g., assert.is_true(found, key
.. " mapping should exist")), and apply the same pattern to the similar
fugitive, hunk-nav, and gitsigns tests so missing mappings fail the test suite.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a683813c-4b8e-433e-9b09-d723841089cb
📒 Files selected for processing (5)
Makefilehome-manager/programs/neovim/lua/config/keymaps.luahome-manager/programs/neovim/run_tests.shhome-manager/programs/neovim/tests/keymaps_spec.luahome-manager/programs/neovim/tests/minimal_init.lua
Comment on lines
+122
to
+150
| describe("vscode-diff keymaps", function() | ||
| local function register_vscode_diff_keymaps() | ||
| local opts = { noremap = true, silent = true } | ||
| vim.keymap.set("n", "<leader>gD_test", function() | ||
| require("vscode-diff.commands").vscode_diff({ fargs = {} }) | ||
| end, opts) | ||
| vim.keymap.set("n", "<leader>gH_test", function() | ||
| require("vscode-diff.commands").vscode_diff({ fargs = { "file", "HEAD" } }) | ||
| end, opts) | ||
| vim.keymap.set("n", "<leader>gr_test", function() | ||
| vim.ui.input({ prompt = "Diff against revision: ", default = "HEAD" }, function(rev) | ||
| if rev and rev ~= "" then | ||
| require("vscode-diff.commands").vscode_diff({ fargs = { "file", rev } }) | ||
| end | ||
| end) | ||
| end, opts) | ||
| vim.keymap.set("n", "<leader>gf_test", function() | ||
| vim.ui.input({ prompt = "File A: ", completion = "file" }, function(a) | ||
| if not a or a == "" then | ||
| return | ||
| end | ||
| vim.ui.input({ prompt = "File B: ", completion = "file" }, function(b) | ||
| if b and b ~= "" then | ||
| require("vscode-diff.commands").vscode_diff({ fargs = { "file", a, b } }) | ||
| end | ||
| end) | ||
| end) | ||
| end, opts) | ||
| end |
There was a problem hiding this comment.
These specs don't exercise home-manager/programs/neovim/lua/config/keymaps.lua.
Every new block re-registers _test mappings or a stub NvimPluginsInstall command instead of loading the real module. That means production regressions in keymaps.lua—like the current <leader>gs collision—still pass here. Mock the plugin modules and assert the mappings/command created by the actual config.
Also applies to: 214-218, 253-257, 291-307, 340-344, 376-383, 467-473
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@home-manager/programs/neovim/tests/keymaps_spec.lua` around lines 122 - 150,
The tests currently re-register _test mappings instead of loading the real
keymaps module, so change each spec (e.g., the block with
register_vscode_diff_keymaps and mappings like "<leader>gD_test",
"<leader>gH_test", "<leader>gr_test", "<leader>gf_test") to mock required plugin
modules (for example the vscode-diff plugin) and then require the actual
home-manager/programs/neovim/lua/config/keymaps.lua, asserting that the real
mappings/commands (including the problematic "<leader>gs" mapping and any
NvimPluginsInstall command) are created; specifically, replace the inline _test
registration with mocks for require("vscode-diff.commands") and other plugins,
call require("config.keymaps") to load the real setup function(s), and assert
vim.keymap.get / vim.fn.exists(":"..command) or equivalent to verify the real
mappings and commands exist.
Comment on lines
+171
to
+207
| .PHONY: nvim-plugins-install | ||
| nvim-plugins-install: ## Download/build missing Neovim native plugin binaries (fff.nvim, telescope-fzf-native, vscode-diff). | ||
| @PACK_DIR="$$HOME/.local/share/nvim/site/pack"; \ | ||
| \ | ||
| FFF_DIR=""; \ | ||
| for d in "$$PACK_DIR"/*/opt/fff.nvim "$$PACK_DIR"/*/start/fff.nvim; do \ | ||
| if [ -d "$$d" ]; then FFF_DIR="$$d"; break; fi; \ | ||
| done; \ | ||
| if [ -n "$$FFF_DIR" ]; then \ | ||
| FFF_BINARY="$$FFF_DIR/target/libfff_nvim.so"; \ | ||
| if [ ! -f "$$FFF_BINARY" ]; then \ | ||
| echo "Downloading fff.nvim native binary..."; \ | ||
| FFF_VERSION=$$(git -C "$$FFF_DIR" rev-parse --short HEAD 2>/dev/null || echo ""); \ | ||
| if [ -n "$$FFF_VERSION" ]; then \ | ||
| _ARCH=$$(uname -m); \ | ||
| _LDD=$$(ldd --version 2>&1 || echo ""); \ | ||
| if echo "$$_LDD" | grep -q musl; then \ | ||
| _TRIPLE="$${_ARCH}-unknown-linux-musl"; \ | ||
| else \ | ||
| _TRIPLE="$${_ARCH}-unknown-linux-gnu"; \ | ||
| fi; \ | ||
| mkdir -p "$$FFF_DIR/target"; \ | ||
| echo "Fetching https://github.com/dmtrKovalenko/fff.nvim/releases/download/$$FFF_VERSION/$${_TRIPLE}.so"; \ | ||
| curl --fail --location --silent --show-error \ | ||
| -o "$$FFF_BINARY" \ | ||
| "https://github.com/dmtrKovalenko/fff.nvim/releases/download/$$FFF_VERSION/$${_TRIPLE}.so" \ | ||
| && echo "fff.nvim binary downloaded successfully" \ | ||
| || echo "fff.nvim binary download failed"; \ | ||
| else \ | ||
| echo "fff.nvim: could not determine version, skipping"; \ | ||
| fi; \ | ||
| else \ | ||
| echo "fff.nvim binary already present"; \ | ||
| fi; \ | ||
| else \ | ||
| echo "fff.nvim plugin directory not found, skipping"; \ | ||
| fi |
There was a problem hiding this comment.
nvim-plugins-install is still best-effort only.
The target advertises all three native plugins, but the recipe only implements fff.nvim, and even that branch masks a failed download with || echo .... Direct calls to make nvim-plugins-install therefore succeed with missing binaries, and switch inherits the same silent-failure mode for the fff.nvim step.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` around lines 171 - 207, The nvim-plugins-install target is
advertised to manage three native plugins but only tries fff.nvim and currently
masks failures (the curl ... || echo ... makes the step succeed even on download
failure); update the recipe so failures cause make to fail: remove the "|| echo
..." fallback and instead check curl's exit status and exit 1 on failure (or
wrap the entire shell block with "set -e" / "set -o pipefail") for the FFF
download path (refer to variables/idents FFF_DIR, FFF_BINARY, FFF_VERSION and
the curl invocation), and either implement equivalent deterministic
download/build logic for the other advertised plugins (telescope-fzf-native,
vscode-diff) or change the target help text to only advertise fff.nvim so the
target behavior matches its description.
Comment on lines
+180
to
+196
| FFF_BINARY="$$FFF_DIR/target/libfff_nvim.so"; \ | ||
| if [ ! -f "$$FFF_BINARY" ]; then \ | ||
| echo "Downloading fff.nvim native binary..."; \ | ||
| FFF_VERSION=$$(git -C "$$FFF_DIR" rev-parse --short HEAD 2>/dev/null || echo ""); \ | ||
| if [ -n "$$FFF_VERSION" ]; then \ | ||
| _ARCH=$$(uname -m); \ | ||
| _LDD=$$(ldd --version 2>&1 || echo ""); \ | ||
| if echo "$$_LDD" | grep -q musl; then \ | ||
| _TRIPLE="$${_ARCH}-unknown-linux-musl"; \ | ||
| else \ | ||
| _TRIPLE="$${_ARCH}-unknown-linux-gnu"; \ | ||
| fi; \ | ||
| mkdir -p "$$FFF_DIR/target"; \ | ||
| echo "Fetching https://github.com/dmtrKovalenko/fff.nvim/releases/download/$$FFF_VERSION/$${_TRIPLE}.so"; \ | ||
| curl --fail --location --silent --show-error \ | ||
| -o "$$FFF_BINARY" \ | ||
| "https://github.com/dmtrKovalenko/fff.nvim/releases/download/$$FFF_VERSION/$${_TRIPLE}.so" \ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "default.nix" | grep -E "neovim"Repository: shunkakinoki/dotfiles
Length of output: 105
🏁 Script executed:
cat -n Makefile | sed -n '170,210p'Repository: shunkakinoki/dotfiles
Length of output: 2018
🏁 Script executed:
find . -name "default.nix" -path "*/neovim/*" -type fRepository: shunkakinoki/dotfiles
Length of output: 107
🏁 Script executed:
cat -n home-manager/programs/neovim/default.nixRepository: shunkakinoki/dotfiles
Length of output: 4546
Use a variable for the library extension instead of hardcoding .so in the Makefile.
Both this Makefile and home-manager/programs/neovim/default.nix hardcode the .so extension and Linux artifacts (*-unknown-linux-musl/gnu). However, default.nix uses a libExt variable pattern for the library name (line 74, 87) while the Makefile hardcodes it directly (line 180, 196). For consistency between these two native plugin installation paths, the Makefile should mirror the variable-based approach used in default.nix.
Note: Both files currently target only Linux. Making this change aligns the code patterns but does not address the underlying platform-awareness gap.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` around lines 180 - 196, The Makefile hardcodes the library
extension ".so" and Linux triple parts; introduce a variable (e.g., libExt or
FFF_LIB_EXT) and use it when constructing FFF_BINARY, the target directory
filename, and the download URL so the extension is consistent with default.nix's
libExt pattern; update the places referencing FFF_BINARY, the curl -o target,
and the echoed download URL (symbols: FFF_BINARY, FFF_DIR, FFF_VERSION, _TRIPLE,
_ARCH, _LDD) to use the new variable instead of literal ".so" while leaving
triple logic unchanged.
- Add desc to every leader keymap in keymaps.lua, telescope.lua, lsp.lua, and autocmds.lua - Add source-scanner test that parses all lua/config/*.lua files and fails if any keymap call with a leader lhs is missing a desc field - Test automatically catches regressions across all config files
Contributor
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved 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/tests/keymaps_spec.lua">
<violation number="1" location="home-manager/programs/neovim/tests/keymaps_spec.lua:531">
P2: Fail the test when temp file creation fails; the current `if f then` guard can silently skip assertions and pass incorrectly.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| local keymap_pattern = "keymap[^(]*%([^,]*,?%s*[\"']([^\"']*<[Ll]eader>[^\"']*)[\"']" | ||
| for _, filepath in ipairs(files) do | ||
| local f = io.open(filepath, "r") | ||
| if f then |
Contributor
There was a problem hiding this comment.
P2: Fail the test when temp file creation fails; the current if f then guard can silently skip assertions and pass incorrectly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/programs/neovim/tests/keymaps_spec.lua, line 531:
<comment>Fail the test when temp file creation fails; the current `if f then` guard can silently skip assertions and pass incorrectly.</comment>
<file context>
@@ -508,4 +508,115 @@ describe("keymaps", function()
+ local keymap_pattern = "keymap[^(]*%([^,]*,?%s*[\"']([^\"']*<[Ll]eader>[^\"']*)[\"']"
+ for _, filepath in ipairs(files) do
+ local f = io.open(filepath, "r")
+ if f then
+ local content = f:read("*a")
+ f:close()
</file context>
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
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.
Summary
<leader>gDkeymap (was calling non-existentvscode-diff.diff()fn, now usesvscode_diff.commands)gD(git explorer),gH(vs HEAD),gr(prompted revision),gf(two files)gs(Gvdiffsplit vs index),gS(vs HEAD)hn/hN(next/prev hunk)<leader>-(prompted, opens two dirs in vsplit)utils,settings,terminal,keymaps,ui,completionspecs (288 total, all passing)Test plan
bash home-manager/programs/neovim/run_tests.sh— all 288 tests pass<leader>gDopens git status explorer<leader>gHdiffs current file vs HEAD<leader>-prompts for two dirs and opens side-by-side oil buffers🤖 Generated with Claude Code
Summary by cubic
Fixes the broken
vscode-diff.nvimkeymap, adds Git diff/hunk and side-by-side dir keymaps, expands:NvimPluginsInstall(blink.cmp,nvim-treesitter), and enforcesdescon all<leader>keymaps with a test.New Features
vscode-diff.nvim<leader>gD(explorer),<leader>gH(file vs HEAD),<leader>gr(prompt rev),<leader>gf(two files);fugitive<leader>gs(Gvdiffsplit vs index),<leader>gS(vs HEAD).gitsignshunk nav:<leader>hn/<leader>hN(next/prev).oil.nvimside-by-side dirs:<leader>-prompts two dirs and opens a vsplit.<leader>Runloadsconfig.*, re-sources$MYVIMRC, and notifies.Refactors
:NvimPluginsInstallhandlesfff.nvim,telescope-fzf-native,vscode-diff.nvim,blink.cmp(prebuilt download), then runsTSInstall all; Makefilenvim-plugins-installruns duringswitch.telescope-fzf-nativeandvscode-diff.nvim; downloadsfff.nvimprebuilt.descto all<leader>keymaps acrosskeymaps.lua,telescope.lua,lsp.lua, and autocmds; new test scans alllua/config/*.luaand fails if any leader keymap lacks adesc.:NvimPluginsInstall;run_tests.shprefers packplenary.nvim;minimal_init.luaaddsplenarytopackage.path; formatted withstylua.Written for commit 014f1b9. Summary will update on new commits.