Merged
Conversation
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThe PR adds a UI-driven package extension configuration panel, refactors extension entrypoint discovery to be manifest-driven and asynchronous (with npm root resolution), removes the separate package-extension item type from the unified list, adds a new 'c' key binding to configure package extensions, and updates types, UI flows, and tests accordingly. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as UI/Command Handler
participant Discovery as Extension Discovery
participant Config as Config Panel
participant Settings as Settings Manager
participant Restart as Restart Handler
User->>UI: Press 'c' to configure package
activate UI
UI->>Discovery: resolveManifestExtensionEntrypoints(packageRoot)
activate Discovery
Discovery->>Discovery: Parse package.json pi.extensions
Discovery->>Discovery: Resolve globs & directories
Discovery-->>UI: Return entrypoint list
deactivate Discovery
UI->>Config: configurePackageExtensions(pkg, ctx, pi)
activate Config
Config->>Config: buildPackageConfigRows(entries)
Config->>Config: Check file existence and build UI rows
Config->>Config: Show interactive selection panel
User->>Config: Toggle entries (stage)
Config->>Config: Track staged changes
User->>Config: Save changes
Config->>Settings: applyPackageExtensionChanges(rows, staged, ...)
activate Settings
Settings->>Settings: Read settings.json
Settings->>Settings: Apply +/- markers to pi.extensions
Settings->>Settings: Write updated settings.json
Settings-->>Config: Confirm changes applied
deactivate Settings
Config->>Restart: promptRestartForPackageConfig()
activate Restart
User->>Restart: Confirm restart
Restart->>UI: Trigger UI reload (ctx.reload)
deactivate Restart
Config-->>UI: Return changed count & reload status
deactivate Config
deactivate UI
UI-->>User: Configuration complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 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 unit tests (beta)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ui/unified.ts (1)
273-309:⚠️ Potential issue | 🟡 MinorNormalize path separators before duplicate detection.
On Windows,localPath.startsWith(pkgResolvedLower + "/")can fail whenlocalPathcontains backslashes, letting duplicate package rows through. Normalize both sides to forward slashes before comparisons.🔧 Suggested fix
- localPaths.add(entry.activePath.toLowerCase()); + localPaths.add(entry.activePath.replace(/\\/g, "/").toLowerCase()); items.push({ type: "local", id: entry.id, @@ - const pkgResolvedLower = pkg.resolvedPath?.toLowerCase() ?? ""; + const pkgResolvedLower = pkg.resolvedPath?.replace(/\\/g, "/").toLowerCase() ?? "";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/unified.ts` around lines 273 - 309, The duplicate-detection fails on Windows because comparisons mix backslashes and forward slashes; normalize path separators to forward slashes before any comparisons. When building localPaths (and when computing pkgSourceLower/pkgResolvedLower inside the installedPackages loop) replace backslashes with "/" and lowercase the result, then use those normalizedLocalPath and normalizedPkgResolved strings for equality, startsWith, and localDir comparisons (referencing localPaths, localPath, installedPackages, pkgSourceLower, pkgResolvedLower). Ensure the localPaths Set stores normalized paths and that all comparisons use the normalized variables to reliably detect duplicates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/packages/extensions.ts`:
- Around line 366-423: The directory token normalization step can leave a
trailing slash (e.g., "src/") which later produces double slashes and prevents
matches; inside resolveManifestExtensionEntries after computing pattern (the
result of normalizeRelativePath) trim any trailing path separator(s) from
pattern (e.g., remove a trailing "/" or "\\") before using isSafeRelativePath,
hasGlobMagic, selectDirectoryFiles, and isExtensionEntrypointPath so directory
lookups and pattern matching work correctly.
- Around line 209-219: The Node.js-only API used by matchesFilterPattern
(matchesGlob) may be missing on Node <22.5.0; update by adding a safe fallback:
in the module that defines/uses matchesGlob, detect availability (typeof
matchesGlob === 'function' or try importing path.matchesGlob) and if missing,
import a small dependency (picomatch or minimatch) and provide a local
matchesGlob fallback function used by matchesFilterPattern; alternatively,
tighten package.json engine to ">=22.5.0" and document the change—ensure you
reference and update the matchesFilterPattern function to call the safe
matchesGlob wrapper instead of relying on the built-in directly.
---
Outside diff comments:
In `@src/ui/unified.ts`:
- Around line 273-309: The duplicate-detection fails on Windows because
comparisons mix backslashes and forward slashes; normalize path separators to
forward slashes before any comparisons. When building localPaths (and when
computing pkgSourceLower/pkgResolvedLower inside the installedPackages loop)
replace backslashes with "/" and lowercase the result, then use those
normalizedLocalPath and normalizedPkgResolved strings for equality, startsWith,
and localDir comparisons (referencing localPaths, localPath, installedPackages,
pkgSourceLower, pkgResolvedLower). Ensure the localPaths Set stores normalized
paths and that all comparisons use the normalized variables to reliably detect
duplicates.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
README.mdsrc/packages/extensions.tssrc/packages/install.tssrc/packages/management.tssrc/types/index.tssrc/ui/footer.tssrc/ui/help.tssrc/ui/package-config.tssrc/ui/unified.tstest/package-config.test.tstest/package-extensions.test.tstest/unified-items.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/package-extensions.test.ts (2)
113-152:⚠️ Potential issue | 🟡 MinorMissing
try/finally— temp directory not cleaned up on failure.
cwdis created withmkdtempbut there is notry/finallyblock. If any assertion throws, the directory is leaked. All other new tests in this file usetry/finallyfor cleanup.🔧 Suggested fix
void test("discoverPackageExtensions reads manifest entrypoints and project filter state", async () => { const cwd = await mkdtemp(join(tmpdir(), "pi-extmgr-cwd-")); const pkgRoot = join(cwd, "vendor", "demo"); + try { await mkdir(pkgRoot, { recursive: true }); // ... rest of setup and assertions ... const discovered = await discoverPackageExtensions(installed, cwd); assert.equal(discovered.length, 1); assert.equal(discovered[0]?.extensionPath, "index.ts"); assert.equal(discovered[0]?.state, "disabled"); + } finally { + await rm(cwd, { recursive: true, force: true }); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/package-extensions.test.ts` around lines 113 - 152, Wrap the test "discoverPackageExtensions reads manifest entrypoints and project filter state" body in a try/finally: create cwd with mkdtemp as before, then move all setup, calls to discoverPackageExtensions and assertions into the try block, and in the finally block remove the temporary directory (cwd) recursively (e.g., using rm or rmdir with recursive option) to ensure cleanup on failure; reference the test name, the cwd variable, mkdtemp call, and the final call to discoverPackageExtensions so you can find where to add the try/finally and the recursive deletion.
311-364:⚠️ Potential issue | 🟡 Minor
finallyblock only restores the env var —cwdandagentDirare never cleaned up.Both temp directories are created via
mkdtempbut neither is removed in thefinallyblock. On test failure they are permanently leaked.🔧 Suggested fix
} finally { if (oldAgentDir === undefined) { delete process.env.PI_CODING_AGENT_DIR; } else { process.env.PI_CODING_AGENT_DIR = oldAgentDir; } + await rm(cwd, { recursive: true, force: true }); + await rm(agentDir, { recursive: true, force: true }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/package-extensions.test.ts` around lines 311 - 364, The finally block for the test "setPackageExtensionState converts string package entries and keeps latest marker" only restores process.env.PI_CODING_AGENT_DIR but does not remove the temporary directories created with mkdtemp (cwd and agentDir), leaking temp files on failure; update the finally block to also remove both temp dirs (cwd and agentDir) using an async recursive rm/rmdir (e.g., fs.promises.rm with { recursive: true, force: true } or equivalent) awaited before exiting the finally, while keeping the existing logic that restores or deletes process.env.PI_CODING_AGENT_DIR and preserving the order so env restoration happens after cleanup.
♻️ Duplicate comments (1)
package.json (1)
59-61: Correct engine bump forpath.matchesGlob.
>=22.5.0is the exact minimum needed forpath.matchesGlobon the v22.x line.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 59 - 61, The package.json engines.node field must require Node >=22.5.0 to guarantee availability of path.matchesGlob; update the "engines" -> "node" value to ">=22.5.0" (edit the package.json engines.entry) so the project explicitly enforces the correct minimum Node version.
🧹 Nitpick comments (1)
src/ui/unified.ts (1)
758-779: Consider adding adefaultguard to theselectionswitch.If the TypeScript types ever widen (or a new
PackageActionKeyis added without a correspondingcase), execution silently falls through toapplyToggleChangesFromManagerand applies local toggle changes — incorrect for a package action context.🛡️ Proposed defensive default
case "details": { ... return false; } + default: + return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/unified.ts` around lines 758 - 779, The switch on selection in the package-action handling block (inside unified.ts where configurePackageExtensions, updatePackageWithOutcome, removePackageWithOutcome and the "details" branch are used) lacks a default guard; add a default case that either throws an explicit error or returns false (and logs) to prevent accidental fall-through to applyToggleChangesFromManager when selection widens; implement an exhaustive-check pattern (e.g., assertUnreachable(selection)) or a clear default return to ensure any unhandled PackageActionKey is caught at runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/packages/extensions.ts`:
- Around line 52-56: The execFileAsync call that invokes npm (the invocation of
execFileAsync("npm", ["root", "-g"], { timeout: 2_000, windowsHide: true }))
will silently fail on Windows when only npm.cmd exists; update that call (in
src/packages/extensions.ts where execFileAsync is used) to add shell: true to
the options object so .cmd/.bat invocations work cross-platform, preserving the
existing timeout and windowsHide settings.
---
Outside diff comments:
In `@test/package-extensions.test.ts`:
- Around line 113-152: Wrap the test "discoverPackageExtensions reads manifest
entrypoints and project filter state" body in a try/finally: create cwd with
mkdtemp as before, then move all setup, calls to discoverPackageExtensions and
assertions into the try block, and in the finally block remove the temporary
directory (cwd) recursively (e.g., using rm or rmdir with recursive option) to
ensure cleanup on failure; reference the test name, the cwd variable, mkdtemp
call, and the final call to discoverPackageExtensions so you can find where to
add the try/finally and the recursive deletion.
- Around line 311-364: The finally block for the test "setPackageExtensionState
converts string package entries and keeps latest marker" only restores
process.env.PI_CODING_AGENT_DIR but does not remove the temporary directories
created with mkdtemp (cwd and agentDir), leaking temp files on failure; update
the finally block to also remove both temp dirs (cwd and agentDir) using an
async recursive rm/rmdir (e.g., fs.promises.rm with { recursive: true, force:
true } or equivalent) awaited before exiting the finally, while keeping the
existing logic that restores or deletes process.env.PI_CODING_AGENT_DIR and
preserving the order so env restoration happens after cleanup.
---
Duplicate comments:
In `@package.json`:
- Around line 59-61: The package.json engines.node field must require Node
>=22.5.0 to guarantee availability of path.matchesGlob; update the "engines" ->
"node" value to ">=22.5.0" (edit the package.json engines.entry) so the project
explicitly enforces the correct minimum Node version.
---
Nitpick comments:
In `@src/ui/unified.ts`:
- Around line 758-779: The switch on selection in the package-action handling
block (inside unified.ts where configurePackageExtensions,
updatePackageWithOutcome, removePackageWithOutcome and the "details" branch are
used) lacks a default guard; add a default case that either throws an explicit
error or returns false (and logs) to prevent accidental fall-through to
applyToggleChangesFromManager when selection widens; implement an
exhaustive-check pattern (e.g., assertUnreachable(selection)) or a clear default
return to ensure any unhandled PackageActionKey is caught at runtime.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.mdpackage.jsonsrc/packages/extensions.tssrc/ui/unified.tstest/package-extensions.test.tstest/unified-items.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
Comment on lines
+52
to
+56
| try { | ||
| const { stdout } = await execFileAsync("npm", ["root", "-g"], { | ||
| timeout: 2_000, | ||
| windowsHide: true, | ||
| }); |
There was a problem hiding this comment.
execFile("npm", ...) will silently fail on Windows where only npm.cmd is in PATH.
On Windows, .bat and .cmd files are not executable on their own without a terminal, and therefore cannot be launched using child_process.execFile(). npm on many Windows installations (e.g., Chocolatey, older official installers) ships only as npm.cmd, making this call fail. The error is caught and globalNpmRootCache is set to null, so global npm root discovery is silently disabled on those setups, falling back to PI_PACKAGE_DIR-based resolution only.
Add shell: true to cover .cmd-only environments:
🔧 Suggested fix
const { stdout } = await execFileAsync("npm", ["root", "-g"], {
timeout: 2_000,
windowsHide: true,
+ shell: process.platform === "win32",
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/packages/extensions.ts` around lines 52 - 56, The execFileAsync call that
invokes npm (the invocation of execFileAsync("npm", ["root", "-g"], { timeout:
2_000, windowsHide: true })) will silently fail on Windows when only npm.cmd
exists; update that call (in src/packages/extensions.ts where execFileAsync is
used) to add shell: true to the options object so .cmd/.bat invocations work
cross-platform, preserving the existing timeout and windowsHide settings.
Merged
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.
Closes #2
Summary by CodeRabbit
New Features
Documentation