fix(extmgr): harden npm reload handling#18
Merged
Conversation
📝 WalkthroughWalkthroughThis PR adds configurable per-cwd npm command/root resolution (including bun-aware root heuristics), integrates settings-derived commands into exec and package-root lookup with caching, updates README with npm prefix guidance, and improves confirmReload to catch and notify reload failures. Changesnpm Command & Root Resolution Configuration
UI Reload Error Handling
🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
test/ui-helpers.test.ts (1)
6-28: ⚡ Quick winConsider adding coverage for the
!confirmedand success paths.The new test covers reload rejection well, but two branches in
confirmReloadremain untested:
- User declines (
confirmresolvesfalse) → should returnfalsewithout callingreload.- Reload succeeds → should return
true.These are straightforward to add:
✅ Suggested additional test cases
+void test("confirmReload returns false when user declines", async () => { + let reloadCalled = false; + const ctx = { + hasUI: true, + ui: { + confirm: () => Promise.resolve(false), + notify: () => {}, + }, + reload: () => { reloadCalled = true; return Promise.resolve(); }, + } as unknown as ExtensionCommandContext; + + const reloaded = await confirmReload(ctx, "Package updated."); + + assert.equal(reloaded, false); + assert.equal(reloadCalled, false); +}); + +void test("confirmReload returns true on successful reload", async () => { + const ctx = { + hasUI: true, + ui: { + confirm: () => Promise.resolve(true), + notify: () => {}, + }, + reload: () => Promise.resolve(), + } as unknown as ExtensionCommandContext; + + const reloaded = await confirmReload(ctx, "Package updated."); + + assert.equal(reloaded, true); +});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/ui-helpers.test.ts` around lines 6 - 28, Add two unit tests for confirmReload: one where ctx.ui.confirm resolves to false and ctx.reload should not be called (assert returned value is false and no reload-related notifications were emitted), and one where ctx.reload resolves successfully (assert returned value is true and any success notification/behavior is as expected). Use the same ExtensionCommandContext-shaped mock object pattern from the existing test, stubbing ui.confirm, ui.notify, and reload to simulate each branch, and verify confirmReload's return value and that notify/reload were called or not accordingly.src/utils/npm-exec.ts (2)
87-90: 💤 Low valueBun global root derivation assumes default
globalBinDir/globalDirrelationship.The path from
bun pm bin -g→path.dirname(binDir)→install/global/node_modulesis correct for bun's defaults, but bun'sglobalDirandglobalBinDirare independently configurable viabunfig.toml. If a user has customized these to non-sibling paths, the derivednode_modulespath will be incorrect and globally installed packages won't be found.This is a narrow edge case, but worth noting in a code comment.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/npm-exec.ts` around lines 87 - 90, The current getRoot implementation in src/utils/npm-exec.ts (getRoot) wrongly assumes bun's globalBinDir and globalDir are siblings by computing path.join(path.dirname(binDir), "install", "global", "node_modules"); update this to not assume that relationship: first try to obtain bun's actual globalDir via a dedicated bun command (or API) and use that to compute the node_modules root, and if such a command isn't available fall back to the existing heuristic but add a clear comment and a logged warning that this is a best-effort guess when bun's globalDir is customized in bunfig.toml; reference the getRoot function and the path.dirname(binDir) derivation so the code reviewer can locate and replace the heuristic with the authoritative query/fallback+warning.
45-47: ⚡ Quick winConsider caching
SettingsManagerto avoid recreating it on every npm invocation.
getSettingsNpmCommandcreates a newSettingsManagerinstance on each call, which occurs insideexecNpmfor every npm operation. Compare this withcatalog.ts, whereSettingsManager.createis called once and cached increateDefaultPackageCatalog. IfSettingsManagerinitialization involves any I/O or startup overhead, consider memoizing bycwdor caching the instance at the call site to improve performance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/npm-exec.ts` around lines 45 - 47, getSettingsNpmCommand currently calls SettingsManager.create(cwd, getAgentDir()) on every invocation (used inside execNpm), causing repeated construction overhead; change to memoize or cache the SettingsManager instance by cwd (or at the call site) and return its getNpmCommand(), e.g. store instances keyed by cwd when calling SettingsManager.create so getSettingsNpmCommand reuses the cached SettingsManager instead of recreating it each time (mirror the approach used in createDefaultPackageCatalog).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 23-28: Add concrete examples for the npmCommand setting in
README.md so users know how to configure Pi for different Node version managers;
include example JSONC entries for mise (e.g., using "mise exec node@22 -- npm"),
for bun (e.g., ["bun"]), and for nvm (e.g., using the npm wrapper on PATH as
["npm"]), reference the npmCommand key and show each example as a commented
JSONC line so readers can copy/paste.
In `@src/utils/npm-exec.ts`:
- Around line 83-92: The bun detection should not rely on configured?.command
=== "bun" because configured.command can be an absolute path or Windows-suffixed
name; update the condition in resolveNpmRootCommand to base the check on the
executable basename (e.g., const cmd = path.basename(configured.command || "");
const normalized = cmd.replace(/\.cmd$/i, ""); if (normalized === "bun") { ...
}) so that "/usr/local/bin/bun" and "bun.cmd" are recognized as bun and the
bun-specific args/getRoot branch (the object with args [...configured.args,
"pm","bin","-g"] and getRoot) is returned instead of falling through to generic
npm root handling.
---
Nitpick comments:
In `@src/utils/npm-exec.ts`:
- Around line 87-90: The current getRoot implementation in src/utils/npm-exec.ts
(getRoot) wrongly assumes bun's globalBinDir and globalDir are siblings by
computing path.join(path.dirname(binDir), "install", "global", "node_modules");
update this to not assume that relationship: first try to obtain bun's actual
globalDir via a dedicated bun command (or API) and use that to compute the
node_modules root, and if such a command isn't available fall back to the
existing heuristic but add a clear comment and a logged warning that this is a
best-effort guess when bun's globalDir is customized in bunfig.toml; reference
the getRoot function and the path.dirname(binDir) derivation so the code
reviewer can locate and replace the heuristic with the authoritative
query/fallback+warning.
- Around line 45-47: getSettingsNpmCommand currently calls
SettingsManager.create(cwd, getAgentDir()) on every invocation (used inside
execNpm), causing repeated construction overhead; change to memoize or cache the
SettingsManager instance by cwd (or at the call site) and return its
getNpmCommand(), e.g. store instances keyed by cwd when calling
SettingsManager.create so getSettingsNpmCommand reuses the cached
SettingsManager instead of recreating it each time (mirror the approach used in
createDefaultPackageCatalog).
In `@test/ui-helpers.test.ts`:
- Around line 6-28: Add two unit tests for confirmReload: one where
ctx.ui.confirm resolves to false and ctx.reload should not be called (assert
returned value is false and no reload-related notifications were emitted), and
one where ctx.reload resolves successfully (assert returned value is true and
any success notification/behavior is as expected). Use the same
ExtensionCommandContext-shaped mock object pattern from the existing test,
stubbing ui.confirm, ui.notify, and reload to simulate each branch, and verify
confirmReload's return value and that notify/reload were called or not
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 11a54f5a-e9bf-41b8-a825-e3f847d59611
📒 Files selected for processing (6)
README.mdsrc/packages/extensions.tssrc/utils/npm-exec.tssrc/utils/ui-helpers.tstest/npm-exec.test.tstest/ui-helpers.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/npm-exec.test.ts (2)
56-56: ⚡ Quick win
getRootinput is effectively a dummy — consider adding a test where the stdout matters.Both bun tests call
resolved.getRoot("/home/alice/.bun/bin\n")but the assertion only confirms theBUN_INSTALL_GLOBAL_DIR-derived path, meaning the stdout argument has no observable effect on the expected output. If the implementation has any path where the stdout fallback is used (e.g., whenBUN_INSTALL_GLOBAL_DIRis unset), that branch is untested. A complementary test withBUN_INSTALL_GLOBAL_DIRunset would give confidence that the fallback behaves correctly.Also applies to: 75-75
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/npm-exec.test.ts` at line 56, The current test always relies on BUN_INSTALL_GLOBAL_DIR so getRoot("/home/alice/.bun/bin\n")'s stdout argument is never exercised; add a complementary test case in test/npm-exec.test.ts that unsets or temporarily clears BUN_INSTALL_GLOBAL_DIR, calls resolved.getRoot with a representative stdout string (e.g., "/home/alice/.bun/bin\n") and asserts the returned path matches the expected fallback derived from that stdout; ensure you restore the environment afterwards and reference the resolved.getRoot function in the new test to cover the alternate branch.
47-83: ⚡ Quick winConsider extracting the shared Bun env-setup into a helper to reduce duplication.
Lines 47–64 and 66–83 are nearly identical: the only difference is the
npmCommandvalue ("/usr/local/bin/bun"vs ``"bun.cmd"). The BUN_INSTALL_GLOBAL_DIR save/capture/restore block is copy-pasted in full, including thetry/finally` teardown.♻️ Suggested refactor
+function withBunGlobalDir<T>(globalDir: string, fn: () => T): T { + const old = process.env.BUN_INSTALL_GLOBAL_DIR; + process.env.BUN_INSTALL_GLOBAL_DIR = globalDir; + try { + return fn(); + } finally { + if (old === undefined) delete process.env.BUN_INSTALL_GLOBAL_DIR; + else process.env.BUN_INSTALL_GLOBAL_DIR = old; + } +} void test("resolveNpmRootCommand detects path-qualified bun commands", () => { - const oldGlobalDir = process.env.BUN_INSTALL_GLOBAL_DIR; - process.env.BUN_INSTALL_GLOBAL_DIR = "/opt/bun/global"; - - try { + withBunGlobalDir("/opt/bun/global", () => { const resolved = resolveNpmRootCommand({ npmCommand: ["/usr/local/bin/bun"] }); assert.equal(resolved.command, "/usr/local/bin/bun"); assert.deepEqual(resolved.args, ["pm", "bin", "-g"]); assert.equal(resolved.getRoot("/home/alice/.bun/bin\n"), "/opt/bun/global/node_modules"); - } finally { - if (oldGlobalDir === undefined) { - delete process.env.BUN_INSTALL_GLOBAL_DIR; - } else { - process.env.BUN_INSTALL_GLOBAL_DIR = oldGlobalDir; - } - } + }); }); void test("resolveNpmRootCommand detects bun.cmd commands", () => { - const oldGlobalDir = process.env.BUN_INSTALL_GLOBAL_DIR; - process.env.BUN_INSTALL_GLOBAL_DIR = "/opt/bun/global"; - - try { + withBunGlobalDir("/opt/bun/global", () => { const resolved = resolveNpmRootCommand({ npmCommand: ["bun.cmd"] }); assert.equal(resolved.command, "bun.cmd"); assert.deepEqual(resolved.args, ["pm", "bin", "-g"]); assert.equal(resolved.getRoot("/home/alice/.bun/bin\n"), "/opt/bun/global/node_modules"); - } finally { - if (oldGlobalDir === undefined) { - delete process.env.BUN_INSTALL_GLOBAL_DIR; - } else { - process.env.BUN_INSTALL_GLOBAL_DIR = oldGlobalDir; - } - } + }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/npm-exec.test.ts` around lines 47 - 83, Extract the duplicated Bun env setup/teardown into a small helper used by both tests ("resolveNpmRootCommand detects path-qualified bun commands" and "resolveNpmRootCommand detects bun.cmd commands"): create a helper (e.g., with a name like withBunGlobalDir or runWithBunGlobalDir) that saves process.env.BUN_INSTALL_GLOBAL_DIR, sets it to "/opt/bun/global", invokes a callback to run the test body (calling resolveNpmRootCommand, assertions, etc.), and restores the original env in a finally block; then replace the copy-pasted save/set/try/finally code in both tests with a call to that helper while preserving the existing calls to resolveNpmRootCommand and assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/npm-exec.test.ts`:
- Line 56: The current test always relies on BUN_INSTALL_GLOBAL_DIR so
getRoot("/home/alice/.bun/bin\n")'s stdout argument is never exercised; add a
complementary test case in test/npm-exec.test.ts that unsets or temporarily
clears BUN_INSTALL_GLOBAL_DIR, calls resolved.getRoot with a representative
stdout string (e.g., "/home/alice/.bun/bin\n") and asserts the returned path
matches the expected fallback derived from that stdout; ensure you restore the
environment afterwards and reference the resolved.getRoot function in the new
test to cover the alternate branch.
- Around line 47-83: Extract the duplicated Bun env setup/teardown into a small
helper used by both tests ("resolveNpmRootCommand detects path-qualified bun
commands" and "resolveNpmRootCommand detects bun.cmd commands"): create a helper
(e.g., with a name like withBunGlobalDir or runWithBunGlobalDir) that saves
process.env.BUN_INSTALL_GLOBAL_DIR, sets it to "/opt/bun/global", invokes a
callback to run the test body (calling resolveNpmRootCommand, assertions, etc.),
and restores the original env in a finally block; then replace the copy-pasted
save/set/try/finally code in both tests with a call to that helper while
preserving the existing calls to resolveNpmRootCommand and assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a568e0e3-2e6c-4df2-897f-469e5213f02e
📒 Files selected for processing (4)
README.mdsrc/utils/npm-exec.tstest/npm-exec.test.tstest/ui-helpers.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- README.md
- test/ui-helpers.test.ts
- src/utils/npm-exec.ts
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
1 participant
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.
Solves #17
Summary by CodeRabbit
Documentation
Bug Fixes