feat(nvim): nvm-default package #49
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce a new Lua module named Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
config/nvim/lua/nvm-default/init.lua (3)
1-7: Add comprehensive module documentation.Consider adding more detailed documentation including:
- Configuration options
- Requirements (NVM installation)
- Example usage
-- Get nvm default version and use it in node_host_prog -- and g.copilot_node_command. +-- +-- This module automatically configures Neovim to use the default Node.js version +-- from NVM. It requires a working NVM installation. +-- +-- Usage: +-- require('nvm-default').setup({ +-- nvm_path = "~/.nvm", -- optional: custom NVM installation path +-- notify_level = "info" -- optional: notification level +-- }) local M = {}
40-43: Normalize paths in options.Consider normalizing paths to handle different formats (e.g., "~", relative paths) consistently.
M.defaults = { - nvm_path = os.getenv("NVM_DIR") or os.getenv("HOME") .. "/.nvm", + nvm_path = vim.fn.expand(os.getenv("NVM_DIR") or "~/.nvm"), notify_level = vim.g.nvm_default_notify_level or "info", }
79-79: Consider PATH modification implications.Modifying PATH could affect other plugins. Consider using absolute paths for binaries instead.
- vim.env.PATH = node_dir_bin .. ":" .. vim.env.PATH + -- Remove PATH modification and use absolute paths in node_host_prog and copilot_node_command
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
config/nvim/init.lua(1 hunks)config/nvim/lua/nvm-default/init.lua(1 hunks)
🔇 Additional comments (1)
config/nvim/init.lua (1)
61-61: LGTM! Verify setup order dependencies.
The placement of the nvm-default setup is logically sound, occurring after plugin initialization but before keymaps.
Let's verify if any plugins depend on Node.js being available:
✅ Verification successful
Setup order is correct and necessary for Copilot functionality
The placement of nvm-default setup is not only logical but necessary as it configures node_host_prog and copilot_node_command which are required by the Copilot plugin. The code shows that:
- Copilot is configured in
lua/plugins/blink.lua nvm-defaultproperly sets up Node.js paths before they're needed- The setup occurs after plugin initialization which is the correct order
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for plugins that might require Node.js
rg -l "node_host_prog|copilot_node_command|require.*node" config/nvim/lua/plugins/
Length of output: 84
Script:
#!/bin/bash
# Let's check for any Node.js related dependencies in the entire config directory
rg -i "node|npm|nvm" config/nvim/
# Also check if there are any copilot configurations since it's a common Node.js dependent plugin
rg -i "copilot" config/nvim/
Length of output: 4691
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
config/nvim/lua/nvm-default/init.lua (2)
100-102: Consider more robust PATH modification.The current PATH modification might lead to duplicate entries if setup is called multiple times. Consider checking if the path already exists before adding it.
if options.add_to_path then - vim.env.PATH = current_nvm_node_bin_path .. ":" .. vim.env.PATH + if not vim.env.PATH:find(vim.fn.escape(current_nvm_node_bin_path, "%-%."), 1, true) then + vim.env.PATH = current_nvm_node_bin_path .. ":" .. vim.env.PATH + end end
94-95: Enhance error message with installation instructions.The error message could be more helpful by including instructions for installing missing components.
- n("Missing required files:\n- " .. table.concat(missing, "\n- "), "error") + n("Missing required files:\n- " .. table.concat(missing, "\n- ") .. + "\n\nPlease ensure Node.js is installed via NVM and run:\n" .. + "npm install -g neovim", "error")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
config/nvim/lua/nvm-default/init.lua(1 hunks)
🔇 Additional comments (5)
config/nvim/lua/nvm-default/init.lua (5)
1-16: LGTM! Well-documented module header.
The documentation clearly explains the purpose, requirements, and usage of the module. Installation instructions and configuration options are well-documented.
18-21: LGTM! Clean module initialization.
The module initialization follows Lua best practices, and the version string is properly formatted for release automation.
23-44: LGTM! Well-implemented helper functions.
The helper functions are well-implemented with proper type annotations and error handling. The run_command function uses the safe vim.fn.system API as recommended.
46-57: LGTM! Well-structured type definitions and defaults.
The type definitions are clear and the defaults are properly configured with appropriate fallbacks using environment variables and global settings.
64-67: 🛠️ Refactor suggestion
Verify NVM installation before usage.
The code should verify that the NVM installation (nvm.sh) exists before attempting to use it.
local nvm_path = options.nvm_path
+ local nvm_sh = nvm_path .. "/nvm.sh"
+
+ if not vim.fn.filereadable(nvm_sh) then
+ n("NVM installation not found at " .. nvm_sh, "error")
+ return
+ end
+
local node_version =
- run_command(string.format(". %s/nvm.sh && nvm version default", nvm_path)) or
- run_command(string.format(". %s/nvm.sh && nvm version node", nvm_path))
+ run_command(string.format(". %s && nvm version default", nvm_sh)) or
+ run_command(string.format(". %s && nvm version node", nvm_sh))✅ Verification successful
Based on the code analysis, I can now generate the final response:
The NVM path verification suggestion is valid but needs refinement
The code already handles the NVM path intelligently by:
- Using
NVM_DIRenvironment variable as the primary source - Falling back to
~/.nvmas a default - Expanding the path using
vim.fn.expand()
However, the review suggestion to verify the existence of nvm.sh is still valuable, but should be implemented with these considerations in mind.
local nvm_path = options.nvm_path
local nvm_sh = nvm_path .. "/nvm.sh"
if vim.fn.filereadable(vim.fn.expand(nvm_sh)) ~= 1 then
n("NVM installation not found at " .. nvm_sh, "error")
return
end
local node_version =
run_command(string.format(". %s && nvm version default", nvm_sh)) or
run_command(string.format(". %s && nvm version node", nvm_sh))🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if nvm.sh exists in common installation paths
# Common NVM installation paths
paths=(
"$HOME/.nvm/nvm.sh"
"$HOME/.config/nvm/nvm.sh"
"/usr/local/nvm/nvm.sh"
)
for path in "${paths[@]}"; do
echo "Checking $path"
[ -f "$path" ] && echo "Found nvm.sh at $path"
done
Length of output: 533
Script:
#!/bin/bash
# Let's check how nvm_path is being set and used in the codebase
rg -A 5 "nvm_path" --type lua
Length of output: 2570
Summary by CodeRabbit
New Features
Bug Fixes