fix: skip installed npm globals and prevent nvim lua typechange - #1380
Conversation
- Skip already-installed npm packages by comparing versions from node_modules package.json, preventing bun hang on resolution - Reduce batch size from 10 to 5 for more reliable installs - Remove recursive=true from nvim lua home.file to prevent home-manager from replacing repo files with Nix store symlinks - Add tests for version checking and skip-installed logic Closes #1379 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Disabled knowledge base sources:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR changes npm-global installation to parse name=version specs, check installed versions in ~/.bun global node_modules, and install only missing/outdated packages in batches of 5 with failure logging. Tests updated for the new flow. Also removes Changes
Sequence DiagramsequenceDiagram
participant Script as install-npm-globals.sh
participant PkgJSON as package.json
participant NodeMods as ~/.bun/.../node_modules
participant Bun as bun add --global
Script->>PkgJSON: Read dependencies as name=versionSpec
Script->>Script: Compute wanted_ver (from semver)
loop per dependency
Script->>NodeMods: Check if package dir exists
alt exists
Script->>NodeMods: Read package.json -> installed_ver
Script->>Script: Compare installed_ver vs wanted_ver
alt match
Script->>Script: Mark as skip
else
Script->>Script: Add to MISSING
end
else
Script->>Script: Add to MISSING
end
end
Script->>Script: Chunk MISSING into batches (size 5)
loop per batch
Script->>Bun: bun add --global [batch]
alt success
Bun->>NodeMods: Install packages
else failure
Script->>Script: Log "Batch install failed"
end
end
alt MISSING was empty
Script->>Script: Print "All npm global packages already installed"
else
Script->>Script: Report installed/updated packages
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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;DRImproved What changed?
Description generated by Mesa. Update settings |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the global npm package installation script to check for existing versions before installing, reducing redundant operations. It also updates the batch size for bun and adds error reporting. Additionally, the recursive flag was removed from the Neovim lua configuration in the Nix module. Review feedback highlights that the regex used for version parsing may be too aggressive for complex semver strings and suggests redirecting error messages to stderr for improved logging.
| dep="${entry%%=*}" | ||
| wanted="${entry#*=}" | ||
| # Extract minimum version from semver spec (e.g. "^2.1.92" -> "2.1.92") | ||
| wanted_ver=$(echo "$wanted" | sed 's/[^0-9.]//g') |
There was a problem hiding this comment.
The sed command s/[^0-9.]//g is too aggressive and may incorrectly parse semver ranges like ^2.1.92 as 2.1.92 but also ~1.2.3-beta as 1.2.3 or similar. While it works for simple versions, it is safer to use a more robust approach to extract the version string, such as using jq to parse the version or a more specific regex.
| BATCH+=("$dep") | ||
| if [ "${#BATCH[@]}" -ge "$BATCH_SIZE" ]; then | ||
| bun add --global "${BATCH[@]}" 2>/dev/null || true | ||
| bun add --global "${BATCH[@]}" 2>/dev/null || echo "Batch install failed: ${BATCH[*]}" |
There was a problem hiding this comment.
Using echo for error reporting in a script that uses set -e (via set -euo pipefail) is good, but consider redirecting the error message to stderr using >&2 to ensure it is captured correctly by logging systems or when the output is redirected.
| bun add --global "${BATCH[@]}" 2>/dev/null || echo "Batch install failed: ${BATCH[*]}" | |
| bun add --global "${BATCH[@]}" 2>/dev/null || echo "Batch install failed: ${BATCH[*]}" >&2 |
| done | ||
| if [ "${#BATCH[@]}" -gt 0 ]; then | ||
| bun add --global "${BATCH[@]}" 2>/dev/null || true | ||
| bun add --global "${BATCH[@]}" 2>/dev/null || echo "Batch install failed: ${BATCH[*]}" |
There was a problem hiding this comment.
Similar to the previous batch install, redirect the error message to stderr for consistency.
| bun add --global "${BATCH[@]}" 2>/dev/null || echo "Batch install failed: ${BATCH[*]}" | |
| bun add --global "${BATCH[@]}" 2>/dev/null || echo "Batch install failed: ${BATCH[*]}" >&2 |
There was a problem hiding this comment.
Pull request overview
This PR updates the npm-globals Home Manager module to avoid re-installing already-installed Bun/npm global packages (to prevent bun hangs during make switch), reduces install batch size for reliability, and adjusts the Neovim Home Manager file mapping to avoid git typechange on Lua config files.
Changes:
- Add “skip already installed” logic for Bun global installs and reduce batch size from 10 → 5.
- Remove
recursive = truefrom Neovim Luahome.filemapping to prevent typechange issues. - Expand ShellSpec coverage for the new install/skip behavior and logging strings.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
home-manager/modules/npm-globals/install-npm-globals.sh |
Adds version/installed checks, builds a missing list, installs in smaller batches, and logs skips/failures. |
spec/npm_globals_spec.sh |
Adds assertions (via greps) for trustedDependencies handling, skip-installed logic, batch size, and reporting strings. |
home-manager/programs/neovim/default.nix |
Removes recursive directory linking for ~/.config/nvim/lua to avoid unwanted type changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| DEPS=$(jq -r '.dependencies | to_entries[] | "\(.key)=\(.value)"' "$PACKAGE_JSON" 2>/dev/null || true) | ||
| MISSING=() | ||
| if [ -n "$DEPS" ]; then | ||
| BATCH_SIZE=10 | ||
| while IFS= read -r entry; do | ||
| dep="${entry%%=*}" | ||
| wanted="${entry#*=}" | ||
| # Extract minimum version from semver spec (e.g. "^2.1.92" -> "2.1.92") | ||
| wanted_ver=$(echo "$wanted" | sed 's/[^0-9.]//g') | ||
| installed_ver="" | ||
| pkg_json="${GLOBAL_MODULES}/${dep}/package.json" | ||
| if [ -f "$pkg_json" ]; then | ||
| installed_ver=$(jq -r '.version // empty' "$pkg_json" 2>/dev/null || true) | ||
| fi | ||
| if [ -n "$installed_ver" ] && [ -n "$wanted_ver" ] && [ "$installed_ver" = "$wanted_ver" ]; then | ||
| echo "$dep@$installed_ver already installed, skipping" | ||
| elif [ -n "$installed_ver" ]; then | ||
| echo "$dep@$installed_ver installed, want $wanted_ver, updating" | ||
| MISSING+=("$dep") | ||
| else | ||
| MISSING+=("$dep") | ||
| fi | ||
| done <<<"$DEPS" | ||
| fi | ||
|
|
||
| # Install missing packages in small batches | ||
| if [ "${#MISSING[@]}" -gt 0 ]; then | ||
| echo "Installing ${#MISSING[@]} missing packages..." | ||
| BATCH_SIZE=5 | ||
| BATCH=() | ||
| while IFS= read -r dep; do | ||
| for dep in "${MISSING[@]}"; do | ||
| BATCH+=("$dep") | ||
| if [ "${#BATCH[@]}" -ge "$BATCH_SIZE" ]; then | ||
| bun add --global "${BATCH[@]}" 2>/dev/null || true | ||
| bun add --global "${BATCH[@]}" 2>/dev/null || echo "Batch install failed: ${BATCH[*]}" | ||
| BATCH=() | ||
| fi | ||
| done <<<"$DEPS" | ||
| done | ||
| if [ "${#BATCH[@]}" -gt 0 ]; then | ||
| bun add --global "${BATCH[@]}" 2>/dev/null || true | ||
| bun add --global "${BATCH[@]}" 2>/dev/null || echo "Batch install failed: ${BATCH[*]}" | ||
| fi |
There was a problem hiding this comment.
The install step ignores the requested version/range from package.json: MISSING stores only dep, and bun add --global "${BATCH[@]}" installs latest. That makes the earlier version check ineffective and can cause repeated installs/updates even when the installed version already satisfies the requested spec. Consider carrying the original spec into the install list (e.g., install name@<spec>), and base the skip/update decision on the same spec you pass to bun.
| # Extract minimum version from semver spec (e.g. "^2.1.92" -> "2.1.92") | ||
| wanted_ver=$(echo "$wanted" | sed 's/[^0-9.]//g') | ||
| installed_ver="" | ||
| pkg_json="${GLOBAL_MODULES}/${dep}/package.json" | ||
| if [ -f "$pkg_json" ]; then | ||
| installed_ver=$(jq -r '.version // empty' "$pkg_json" 2>/dev/null || true) | ||
| fi | ||
| if [ -n "$installed_ver" ] && [ -n "$wanted_ver" ] && [ "$installed_ver" = "$wanted_ver" ]; then | ||
| echo "$dep@$installed_ver already installed, skipping" | ||
| elif [ -n "$installed_ver" ]; then | ||
| echo "$dep@$installed_ver installed, want $wanted_ver, updating" | ||
| MISSING+=("$dep") |
There was a problem hiding this comment.
wanted_ver=$(echo "$wanted" | sed 's/[^0-9.]//g') is not a valid way to interpret semver specs. For common ranges like ^1.2.3, an installed 1.2.4 (which satisfies the range) will be treated as needing an update; for specs like *, latest, workspace:*, or prereleases (1.2.3-beta.1), this produces empty/incorrect versions and can lead to always-updating behavior and confusing logs. Use a comparison that checks whether the installed version satisfies the requested semver range (or avoid parsing and delegate to the package manager).
| # Build list of packages that need installing or updating | ||
| GLOBAL_MODULES="${HOME}/.bun/install/global/node_modules" | ||
| DEPS=$(jq -r '.dependencies | to_entries[] | "\(.key)=\(.value)"' "$PACKAGE_JSON" 2>/dev/null || true) | ||
| MISSING=() |
There was a problem hiding this comment.
GLOBAL_MODULES is introduced here but the same variable is redefined later in the script for the override/dedup logic. Consider defining it once near the top (or making it readonly) to avoid accidental divergence if one path is changed later.
![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
🤖 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/modules/npm-globals/install-npm-globals.sh`:
- Around line 79-85: The bun batch-install blocks currently swallow errors via
"bun add ... || echo ...", so introduce a FAILED flag (e.g., FAILED=0) before
the loop, replace each "|| echo ..." with logic that sets FAILED=1 on failure
(so the failure is recorded) while still printing the error message, and after
the final batch check examine FAILED and exit non-zero (exit 1) if any batch
failed; update both occurrences that run "bun add --global \"${BATCH[@]}\"" and
ensure the BATCH reset (BATCH=()) remains unchanged.
- Around line 53-64: The code strips prerelease/build metadata by using
wanted_ver=$(echo "$wanted" | sed 's/[^0-9.]//g'); instead, preserve the full
semver (including prerelease/dev identifiers) by only removing leading range
operators and optional leading "v". Replace the wanted_ver assignment with a
safe normalization that strips only leading ^ ~ > < = and whitespace (e.g.
wanted_ver=$(echo "$wanted" | sed 's/^[\^~><=[:space:]]*//; s/^v//')) and ensure
you apply the same normalization to installed_ver (from package.json) before the
equality check in the install-npm-globals.sh logic so comparisons like in the
if/elif that reference wanted_ver and installed_ver correctly detect
prerelease/dev versions.
In `@spec/npm_globals_spec.sh`:
- Around line 83-134: The new ShellSpec tests only grep for literal strings
(e.g., 'installed_ver', 'wanted_ver', 'MISSING', 'updating', 'All npm global
packages already installed') and thus don't verify actual behavior of
install-npm-globals.sh; update the spec to run the script with controlled
fixtures instead: create a temporary HOME and a fake global node_modules
structure and fixture package.json, then execute install-npm-globals.sh and
assert real outcomes (zero installs and "All npm global packages already
installed" when all deps present; exactly one install and appropriate
"updating"/batch install output when one dep is missing). Keep the existing
string checks for BATCH_SIZE=5 and 'bun add --global' but replace the greps for
internal variable names with executable ShellSpec examples that source or run
install-npm-globals.sh against the fixtures to validate skip/update logic.
🪄 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: 4e43727a-841b-4543-b99f-e2ad84451fdb
📒 Files selected for processing (3)
home-manager/modules/npm-globals/install-npm-globals.shhome-manager/programs/neovim/default.nixspec/npm_globals_spec.sh
💤 Files with no reviewable changes (1)
- home-manager/programs/neovim/default.nix
| # Extract minimum version from semver spec (e.g. "^2.1.92" -> "2.1.92") | ||
| wanted_ver=$(echo "$wanted" | sed 's/[^0-9.]//g') | ||
| installed_ver="" | ||
| pkg_json="${GLOBAL_MODULES}/${dep}/package.json" | ||
| if [ -f "$pkg_json" ]; then | ||
| installed_ver=$(jq -r '.version // empty' "$pkg_json" 2>/dev/null || true) | ||
| fi | ||
| if [ -n "$installed_ver" ] && [ -n "$wanted_ver" ] && [ "$installed_ver" = "$wanted_ver" ]; then | ||
| echo "$dep@$installed_ver already installed, skipping" | ||
| elif [ -n "$installed_ver" ]; then | ||
| echo "$dep@$installed_ver installed, want $wanted_ver, updating" | ||
| MISSING+=("$dep") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify which dependency specs are mangled by the current normalization logic.
python - <<'PY'
import json, re, pathlib
pkg = pathlib.Path("package.json")
deps = json.loads(pkg.read_text()).get("dependencies", {})
print("name\tspec\tcurrent_wanted_ver")
for name, spec in deps.items():
wanted_ver = re.sub(r"[^0-9.]", "", spec) # mirrors Line 54
if wanted_ver != spec.lstrip("^~"): # quick signal of lossy transform
print(f"{name}\t{spec}\t{wanted_ver}")
PYRepository: shunkakinoki/dotfiles
Length of output: 275
🏁 Script executed:
# Check file existence and line count
wc -l "home-manager/modules/npm-globals/install-npm-globals.sh"Repository: shunkakinoki/dotfiles
Length of output: 124
🏁 Script executed:
# Read lines 45-90 to see the context and verify the code snippet
sed -n '45,90p' "home-manager/modules/npm-globals/install-npm-globals.sh"Repository: shunkakinoki/dotfiles
Length of output: 1708
🏁 Script executed:
# Check if file exists at the stated path
ls -la "home-manager/modules/npm-globals/install-npm-globals.sh"Repository: shunkakinoki/dotfiles
Length of output: 169
Semver parsing loses prerelease and dev version information, causing unnecessary package updates.
Line 54's sed pattern s/[^0-9.]//g strips all non-numeric characters, corrupting dependency specs like ^0.1.0-alpha.1743007075 (becomes 0.1.0.1743007075), ^0.0.1775390627-g406649 (becomes 0.0.1775390627406649), and ^7.0.0-dev.20260405.1 (becomes 7.0.0.20260405.1). Line 60's strict equality check then compares the installed version against these corrupted values, causing packages with prerelease or dev versions to be incorrectly identified as needing updates.
Preserve the full version spec without lossy character stripping to ensure accurate version comparison.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@home-manager/modules/npm-globals/install-npm-globals.sh` around lines 53 -
64, The code strips prerelease/build metadata by using wanted_ver=$(echo
"$wanted" | sed 's/[^0-9.]//g'); instead, preserve the full semver (including
prerelease/dev identifiers) by only removing leading range operators and
optional leading "v". Replace the wanted_ver assignment with a safe
normalization that strips only leading ^ ~ > < = and whitespace (e.g.
wanted_ver=$(echo "$wanted" | sed 's/^[\^~><=[:space:]]*//; s/^v//')) and ensure
you apply the same normalization to installed_ver (from package.json) before the
equality check in the install-npm-globals.sh logic so comparisons like in the
if/elif that reference wanted_ver and installed_ver correctly detect
prerelease/dev versions.
| bun add --global "${BATCH[@]}" 2>/dev/null || echo "Batch install failed: ${BATCH[*]}" | ||
| BATCH=() | ||
| fi | ||
| done <<<"$DEPS" | ||
| done | ||
| if [ "${#BATCH[@]}" -gt 0 ]; then | ||
| bun add --global "${BATCH[@]}" 2>/dev/null || true | ||
| bun add --global "${BATCH[@]}" 2>/dev/null || echo "Batch install failed: ${BATCH[*]}" | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "install-npm-globals.sh" --exec head -100 {}Repository: shunkakinoki/dotfiles
Length of output: 3494
🏁 Script executed:
sed -n '80,120p' home-manager/modules/npm-globals/install-npm-globals.shRepository: shunkakinoki/dotfiles
Length of output: 1653
Batch install errors are silently suppressed, leaving packages half-installed.
The bun add ... || echo ... pattern bypasses set -e. When bun add fails, the || catches it and executes echo, which returns 0—so the overall expression succeeds and the script continues without detecting the failure. No subsequent code validates installation success, so downstream operations proceed on incomplete state.
Apply the proposed fix: track failures with a FAILED variable, and exit non-zero at the end of the installation block if any batch failed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@home-manager/modules/npm-globals/install-npm-globals.sh` around lines 79 -
85, The bun batch-install blocks currently swallow errors via "bun add ... ||
echo ...", so introduce a FAILED flag (e.g., FAILED=0) before the loop, replace
each "|| echo ..." with logic that sets FAILED=1 on failure (so the failure is
recorded) while still printing the error message, and after the final batch
check examine FAILED and exit non-zero (exit 1) if any batch failed; update both
occurrences that run "bun add --global \"${BATCH[@]}\"" and ensure the BATCH
reset (BATCH=()) remains unchanged.
| Describe 'skip already installed' | ||
| It 'checks global node_modules for installed packages' | ||
| When run bash -c "grep 'GLOBAL_MODULES=' '$SCRIPT' | head -1" | ||
| The output should include '.bun/install/global/node_modules' | ||
| End | ||
|
|
||
| It 'reads installed version from package.json' | ||
| When run bash -c "grep 'installed_ver' '$SCRIPT'" | ||
| The output should include 'installed_ver' | ||
| End | ||
|
|
||
| It 'extracts wanted version from semver spec' | ||
| When run bash -c "grep 'wanted_ver' '$SCRIPT'" | ||
| The output should include 'wanted_ver' | ||
| End | ||
|
|
||
| It 'skips when installed version matches wanted' | ||
| When run bash -c "grep 'already installed, skipping' '$SCRIPT'" | ||
| The output should include 'already installed, skipping' | ||
| End | ||
|
|
||
| It 'detects when update is needed' | ||
| When run bash -c "grep 'updating' '$SCRIPT'" | ||
| The output should include 'updating' | ||
| End | ||
|
|
||
| It 'builds a MISSING array of packages to install' | ||
| When run bash -c "grep 'MISSING' '$SCRIPT'" | ||
| The output should include 'MISSING' | ||
| End | ||
| End | ||
|
|
||
| Describe 'batch installation' | ||
| It 'uses batch size of 5' | ||
| When run bash -c "grep 'BATCH_SIZE=5' '$SCRIPT'" | ||
| The output should include 'BATCH_SIZE=5' | ||
| End | ||
|
|
||
| It 'uses bun add --global in batches' | ||
| When run bash -c "grep 'bun add --global' '$SCRIPT'" | ||
| The output should include 'bun add --global' | ||
| End | ||
|
|
||
| It 'batches packages to avoid bun resolution hangs' | ||
| When run bash -c "grep 'BATCH_SIZE' '$SCRIPT'" | ||
| The output should include 'BATCH_SIZE' | ||
| It 'reports batch failures' | ||
| When run bash -c "grep 'Batch install failed' '$SCRIPT'" | ||
| The output should include 'Batch install failed' | ||
| End | ||
|
|
||
| It 'parses dependencies with jq' | ||
| When run bash -c "grep 'dependencies' '$SCRIPT'" | ||
| The output should include 'dependencies' | ||
| It 'reports when all packages are already installed' | ||
| When run bash -c "grep 'All npm global packages already installed' '$SCRIPT'" | ||
| The output should include 'All npm global packages already installed' | ||
| End |
There was a problem hiding this comment.
These new cases validate strings, not behavior, so skip/update logic is effectively untested.
The added specs (installed_ver, wanted_ver, MISSING, updating, All npm global packages already installed) only assert text exists in the script. They do not execute install-npm-globals.sh with controlled fixtures (HOME, fake global node_modules, fixture package.json), so regressions in actual decision logic will still pass.
If you want, I can draft a fixture-based ShellSpec block that covers:
- all deps preinstalled => zero installs, skip message;
- one dep removed => exactly one dep installed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spec/npm_globals_spec.sh` around lines 83 - 134, The new ShellSpec tests only
grep for literal strings (e.g., 'installed_ver', 'wanted_ver', 'MISSING',
'updating', 'All npm global packages already installed') and thus don't verify
actual behavior of install-npm-globals.sh; update the spec to run the script
with controlled fixtures instead: create a temporary HOME and a fake global
node_modules structure and fixture package.json, then execute
install-npm-globals.sh and assert real outcomes (zero installs and "All npm
global packages already installed" when all deps present; exactly one install
and appropriate "updating"/batch install output when one dep is missing). Keep
the existing string checks for BATCH_SIZE=5 and 'bun add --global' but replace
the greps for internal variable names with executable ShellSpec examples that
source or run install-npm-globals.sh against the fixtures to validate
skip/update logic.
Summary
make switchrecursive = truefrom nvim luahome.fileto prevent home-manager from replacing repo files with Nix store symlinks (which caused git typechange status)Closes #1379
Test plan
make build && make switch- npm globals should skip already-installed packagesgit statusshould show no typechange onhome-manager/programs/neovim/lua/config/filesshellspec spec/npm_globals_spec.shpasses (27 examples, 0 failures)🤖 Generated with Claude Code
Summary by cubic
Skip already-installed global
npmpackages and shrink batch installs to reducebunhangs duringmake switch. Also stophome-managerfrom symlinking Neovim Lua files to avoid git typechange; closes #1379.~/.bun/install/global/node_modules/*/package.jsonto wanted frompackage.jsonwith simplified semver extraction; install in batches of 5 and report batch failures; added shellspec tests.recursive = truefrom Neovim Luahome.filesohome-managerdoesn't replace repo files with Nix store symlinks.bashdirectory toPATHin mocks.Written for commit a123e8b. Summary will update on new commits.