feat(extmgr): adopt Pi package APIs and async loaders#10
Merged
Conversation
Replace package discovery and update checks built on parsed CLI output with a structured package catalog over Pi's package manager APIs. Add reusable async loader flows across the unified manager, remote browsing, and package configuration screens, and harden cancellation, removal, and status handling around the new flows.
📝 WalkthroughWalkthroughAdds a PackageCatalog abstraction and test mocks, replaces CLI-based package discovery/operations with catalog-backed APIs, threads AbortSignal cancellation through network and discovery flows, and introduces runTaskWithLoader for cancellable loader UI around long-running package tasks. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI (runTaskWithLoader)
participant Catalog as PackageCatalog
participant PM as PackageManager (Default)
participant Settings as SettingsManager
UI->>Catalog: install(source, scope, onProgress) [with AbortSignal]
Catalog->>PM: perform install (progress callback)
PM-->>Catalog: progress events
Catalog->>UI: onProgress -> setMessage
PM-->>Catalog: install result / error
Catalog->>Settings: update sources & flush
Catalog-->>UI: resolve / throw
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ui/remote.ts (1)
135-137:⚠️ Potential issue | 🟡 MinorCheck abort status before caching package info.
Line 135 catches
AbortErrorlike any other fetch failure, converting it toundefinedinstead of re-throwing. IfexecNpm()finishes first and the user cancels while weekly downloads are loading,Promise.all()still resolves successfully, and the code caches a partially loaded payload (with "Weekly downloads: unknown") at line 190 without checking if the signal was aborted. Add a check forsignal?.abortedbeforepackageInfoCache.set(), or re-throwAbortErrorfromfetchWeeklyDownloads.Also applies to: 155-160, 189-190
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/remote.ts` around lines 135 - 137, The fetchWeeklyDownloads catch currently swallows AbortError causing partial payloads to be cached; modify fetchWeeklyDownloads (and the try/catch blocks around lines referencing execNpm and packageInfoCache.set) so that if the AbortSignal is aborted you re-throw the AbortError (or at minimum return early) instead of returning undefined, and before calling packageInfoCache.set(...) check signal?.aborted and skip caching when aborted; ensure the change is applied to the other similar catch sites noted (around the execNpm flow and the packageInfoCache.set call).
🧹 Nitpick comments (2)
test/rpc-mode.test.ts (1)
89-119: Cover the non-deduped installed-list contract in this test.
showInstalledPackagesList()is supposed to request installed packages with deduping disabled, but this fixture only contains one package. That means the test still passes if the call accidentally falls back to the catalog's default dedupe behavior, becausemockPackageCatalog()only diverges once the same identity exists in both scopes. Please add a global+project duplicate here and assert both rows are surfaced.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/rpc-mode.test.ts` around lines 89 - 119, The test doesn't verify that showInstalledPackagesList requests installed packages with deduping disabled because mockPackageCatalog only has one package; add a duplicate package with the same identity in both global and project scopes so the installed-list call must surface both rows, update the mockPackageCatalog call in this test to include two entries with identical name/version but different scope (e.g., scope: "global" and scope: "project"), and assert after runResolvedCommand that notifications (or the output inspected) contain both entries (e.g., both "demo-pkg" rows) and that customCallCount() remains 0 to confirm dedupe was disabled.test/helpers/package-catalog.ts (1)
28-29: Let the mock reflect installs/removals between calls.
removePackageInternal()and the other catalog-backed flows now re-query the catalog after a mutation, but this helper always serves the originalpackages/updatessnapshots. That makes it easy for success-path tests to pass while post-remove or post-update behavior is wrong. Keeping mutable local copies and applying default install/remove/update state changes here would make the tests much more representative.Also applies to: 34-61
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/helpers/package-catalog.ts` around lines 28 - 29, The helper currently uses immutable snapshots (packages and updates) so subsequent calls don't reflect installs/removals; change it to keep mutable copies (e.g., let packages = [...(options?.packages ?? [])] and let updates = [...(options?.updates ?? [])]) and mutate those arrays inside the helper's install/update/remove handlers so subsequent catalog queries return updated state; specifically update the handlers that call removePackageInternal and any catalog-backed flows to modify these mutable arrays (add on install, remove on delete, replace on update) so tests see the post-mutation catalog state.
🤖 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/install.ts`:
- Around line 208-221: The install flow currently calls
getPackageCatalog(ctx.cwd).install(...) inside runTaskWithLoader but does not
wait for the newly installed package to become discoverable, so downstream UI
prompts (cache clear, success log, reload prompt) run before
list/status/discovery can see it; fix by adding a post-install readiness wait —
either call a new or existing method on the PackageCatalog (e.g.,
getPackageCatalog(ctx.cwd).waitForDiscoverable(normalized) or
waitForPackageDiscoverable(normalized)) after
getPackageCatalog(...).install(...) and before clearing
caches/logging/prompting, or alternatively move that readiness guarantee into
PackageCatalog.install itself (ensure PackageCatalog.install waits for
discoverability before resolving). Ensure the symbol names referenced are
getPackageCatalog, PackageCatalog.install and the package identifier variable
normalized.
In `@src/packages/management.ts`:
- Around line 59-70: The preflight call to
getPackageCatalog(ctx.cwd).checkForAvailableUpdates() is made outside the
try/catch so exceptions bypass the existing error handling path; move the call
that computes updates (the checkForAvailableUpdates() invocation used to set
updates/hasUpdate for updateIdentity) into the same try/catch block that wraps
the actual update logic so any thrown errors trigger the existing
notify(...,"error"), logPackageUpdate(...), clearUpdatesAvailable(...), and
updateExtmgrStatus(ctx, pi) flows; ensure both spots (around
updateIdentity/hasUpdate and the similar call at lines 114–120) are relocated
into their corresponding try/catch blocks and that you still call
clearUpdatesAvailable and updateExtmgrStatus in the catch/finally paths when
appropriate.
In `@src/ui/async-task.ts`:
- Around line 88-95: The cancel callback currently calls done() directly which
can race with the task's .then()/.catch() and cause double completion; move the
finished guard (the finished boolean/guard setup referenced as finished) so it
is defined before calling createLoaderComponent, and change the cancel handler
to invoke finish(TASK_ABORTED) instead of done() so all completion paths funnel
through the single finish(...) completion path (ensure you update the cancel
callback passed into createLoaderComponent accordingly and keep references to
createLoaderComponent, done, finish, TASK_ABORTED, and the finished guard
consistent).
In `@test/search-regression.test.ts`:
- Around line 15-64: The helper captureCustomComponent currently polls up to 20
ticks for done() (doneValue) which causes flaky tests and may skip
component.dispose() when onMatch throws; replace the polling loop by creating a
real completion Promise (e.g., let completion = new Promise(resolve => done =
resolve) or similar) and pass its resolver as the done parameter to the factory,
then await that Promise (or Promise.race it with a short timeout) instead of the
for-loop; also ensure component.dispose() is invoked in a finally block around
awaiting completion and invoking onMatch so cleanup always runs even if onMatch
throws.
---
Outside diff comments:
In `@src/ui/remote.ts`:
- Around line 135-137: The fetchWeeklyDownloads catch currently swallows
AbortError causing partial payloads to be cached; modify fetchWeeklyDownloads
(and the try/catch blocks around lines referencing execNpm and
packageInfoCache.set) so that if the AbortSignal is aborted you re-throw the
AbortError (or at minimum return early) instead of returning undefined, and
before calling packageInfoCache.set(...) check signal?.aborted and skip caching
when aborted; ensure the change is applied to the other similar catch sites
noted (around the execNpm flow and the packageInfoCache.set call).
---
Nitpick comments:
In `@test/helpers/package-catalog.ts`:
- Around line 28-29: The helper currently uses immutable snapshots (packages and
updates) so subsequent calls don't reflect installs/removals; change it to keep
mutable copies (e.g., let packages = [...(options?.packages ?? [])] and let
updates = [...(options?.updates ?? [])]) and mutate those arrays inside the
helper's install/update/remove handlers so subsequent catalog queries return
updated state; specifically update the handlers that call removePackageInternal
and any catalog-backed flows to modify these mutable arrays (add on install,
remove on delete, replace on update) so tests see the post-mutation catalog
state.
In `@test/rpc-mode.test.ts`:
- Around line 89-119: The test doesn't verify that showInstalledPackagesList
requests installed packages with deduping disabled because mockPackageCatalog
only has one package; add a duplicate package with the same identity in both
global and project scopes so the installed-list call must surface both rows,
update the mockPackageCatalog call in this test to include two entries with
identical name/version but different scope (e.g., scope: "global" and scope:
"project"), and assert after runResolvedCommand that notifications (or the
output inspected) contain both entries (e.g., both "demo-pkg" rows) and that
customCallCount() remains 0 to confirm dedupe was disabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7cd4b112-2d15-4621-9a0c-e2ad221c3c8a
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (24)
README.mdpackage.jsonsrc/packages/catalog.tssrc/packages/discovery.tssrc/packages/install.tssrc/packages/management.tssrc/ui/async-task.tssrc/ui/package-config.tssrc/ui/remote.tssrc/ui/unified.tssrc/utils/auto-update.tssrc/utils/network.tssrc/utils/npm-exec.tssrc/utils/package-source.tssrc/utils/status.tssrc/utils/ui-helpers.tstest/auto-update.test.tstest/command-orchestration.test.tstest/discovery-parser.test.tstest/helpers/package-catalog.tstest/install-remove.test.tstest/rpc-mode.test.tstest/search-regression.test.tstest/unified-items.test.ts
Comment on lines
+208
to
+221
| await runTaskWithLoader( | ||
| ctx, | ||
| { | ||
| title: "Install Package", | ||
| message: `Installing ${normalized}...`, | ||
| cancellable: false, | ||
| }, | ||
| async ({ setMessage }) => { | ||
| await getPackageCatalog(ctx.cwd).install(normalized, scope, (event) => { | ||
| setMessage(getProgressMessage(event, `Installing ${normalized}...`)); | ||
| }); | ||
| return undefined; | ||
| } | ||
| ); |
There was a problem hiding this comment.
Reintroduce a post-install visibility check before prompting reload.
PackageCatalog.install() only waits for packageManager.install() and settingsManager.flush() in src/packages/catalog.ts:114-124. The old discoverability wait is gone, so the success path below can clear caches, log success, and prompt reload before list/status/discovery can actually see the new package, which makes fresh installs look broken until a later manual reload. Please keep the readiness wait here or move that guarantee into the catalog.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/packages/install.ts` around lines 208 - 221, The install flow currently
calls getPackageCatalog(ctx.cwd).install(...) inside runTaskWithLoader but does
not wait for the newly installed package to become discoverable, so downstream
UI prompts (cache clear, success log, reload prompt) run before
list/status/discovery can see it; fix by adding a post-install readiness wait —
either call a new or existing method on the PackageCatalog (e.g.,
getPackageCatalog(ctx.cwd).waitForDiscoverable(normalized) or
waitForPackageDiscoverable(normalized)) after
getPackageCatalog(...).install(...) and before clearing
caches/logging/prompting, or alternatively move that readiness guarantee into
PackageCatalog.install itself (ensure PackageCatalog.install waits for
discoverability before resolving). Ensure the symbol names referenced are
getPackageCatalog, PackageCatalog.install and the package identifier variable
normalized.
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/packages/management.ts (1)
388-395:⚠️ Potential issue | 🟠 MajorWait for removal state to settle before computing
remaining.
src/packages/catalog.ts:126-142returns fromremove()afterpackageManager.remove(...)and settings flush, but nothing here waits for discovery to stop reporting the removed package. The immediate reread on Lines 388-390 can therefore leaveremainingtemporarily non-empty, which produces a false “Still installed” warning and skipsclearUpdatesAvailable(...).💡 Proposed fix
- const remaining = (await getInstalledPackagesAllScopesForRemoval(ctx)).filter( - (p) => packageIdentity(p.source) === identity - ); + const expectedRemainingCount = matching.length - successfulTargets.length; + let remaining: InstalledPackage[] = []; + + for (let attempt = 0; attempt < 5; attempt += 1) { + remaining = (await getInstalledPackagesAllScopesForRemoval(ctx)).filter( + (p) => packageIdentity(p.source) === identity + ); + if (remaining.length === expectedRemainingCount) { + break; + } + await new Promise((resolve) => setTimeout(resolve, 200)); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/packages/management.ts` around lines 388 - 395, The current removal flow reads installed packages immediately after calling remove() and settings flush, which can race with discovery; modify the removal handler (the block using getInstalledPackagesAllScopesForRemoval, packageIdentity, notifyRemovalSummary, clearUpdatesAvailable) to wait for discovery to settle before computing `remaining` — implement a short polling loop or await a discovery-stable helper (e.g., retry calling getInstalledPackagesAllScopesForRemoval with a small delay until the set of matching packages is empty or unchanged for N iterations or a timeout) and then call notifyRemovalSummary and, if failures.length === 0 and remaining.length === 0, call clearUpdatesAvailable(pi, ctx, [identity]); ensure the logic still respects the existing `failures` variable and times out to avoid hanging.
🤖 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/ui/remote.ts`:
- Around line 502-505: The early return when text is falsy prevents returning to
the package menu; instead of returning immediately in the block that calls
notify(ctx, `Loading ${packageName} details was cancelled.`, "info"), call
showPackageDetails(ctx, packageName) (or the same call used below) so the UI
navigates back to the package-details menu after a cancelled load—remove the
return and invoke showPackageDetails with the same ctx and packageName (and
preserve any other relevant state variables) so cancellation routes the user
back into the details flow.
- Around line 376-383: The code treats an empty cached results array as a cache
miss because it checks allPackages.length === 0; change the control flow so a
valid cache (even with results: []) is honored: use isCacheValid(query) +
getSearchCache() to set allPackages (from cache.results) and then decide whether
to hit the loader based on whether allPackages is undefined/null (or track a
cacheHit flag) rather than checking length. Update references in this block
(isCacheValid(query), getSearchCache(), and the allPackages variable) so only a
truly missing/unset allPackages triggers the remote fetch.
---
Outside diff comments:
In `@src/packages/management.ts`:
- Around line 388-395: The current removal flow reads installed packages
immediately after calling remove() and settings flush, which can race with
discovery; modify the removal handler (the block using
getInstalledPackagesAllScopesForRemoval, packageIdentity, notifyRemovalSummary,
clearUpdatesAvailable) to wait for discovery to settle before computing
`remaining` — implement a short polling loop or await a discovery-stable helper
(e.g., retry calling getInstalledPackagesAllScopesForRemoval with a small delay
until the set of matching packages is empty or unchanged for N iterations or a
timeout) and then call notifyRemovalSummary and, if failures.length === 0 and
remaining.length === 0, call clearUpdatesAvailable(pi, ctx, [identity]); ensure
the logic still respects the existing `failures` variable and times out to avoid
hanging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4fffc70a-dfd9-4ac0-acf9-1abcbf36f225
📒 Files selected for processing (7)
src/packages/management.tssrc/ui/async-task.tssrc/ui/remote.tstest/helpers/package-catalog.tstest/install-remove.test.tstest/rpc-mode.test.tstest/search-regression.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- test/search-regression.test.ts
- test/install-remove.test.ts
- test/helpers/package-catalog.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/remote-ui.test.ts (1)
52-68: Optionally assert mock queues are fully consumed to catch hidden UI flow drift.Right now, extra/missing
custom/selectcalls could still pass. Adding queue-length assertions makes this test stricter and more diagnostic.✅ Optional test hardening
try { await browseRemotePackages(ctx, "demo", pi); assert.equal(selectPrompts.filter((title) => title === "demo-pkg").length, 2); assert.ok( notifications.some((entry) => entry.message.includes("Loading demo-pkg details was cancelled.") ) ); + assert.equal(customResults.length, 0, "Unexpected number of ctx.ui.custom calls"); + assert.equal(selectResults.length, 0, "Unexpected number of ctx.ui.select calls"); } finally { clearSearchCache(); }Also applies to: 73-78
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/remote-ui.test.ts` around lines 52 - 68, Add assertions after the test run to ensure the mock queues were fully consumed: verify customResults.length === 0 and selectResults.length === 0 (and any other mock arrays used around lines 73-78) to catch extra/missing UI calls; locate the mocks assigned to (ctx.ui as ...).custom and (ctx.ui as ...).select and assert the corresponding arrays (customResults, selectResults and any other enqueue arrays) are empty and optionally check selectPrompts length/contents for expected prompts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/remote-ui.test.ts`:
- Around line 52-68: Add assertions after the test run to ensure the mock queues
were fully consumed: verify customResults.length === 0 and selectResults.length
=== 0 (and any other mock arrays used around lines 73-78) to catch extra/missing
UI calls; locate the mocks assigned to (ctx.ui as ...).custom and (ctx.ui as
...).select and assert the corresponding arrays (customResults, selectResults
and any other enqueue arrays) are empty and optionally check selectPrompts
length/contents for expected prompts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c90bc504-9a37-43ae-9b26-dfcd1639c2b5
📒 Files selected for processing (2)
src/ui/remote.tstest/remote-ui.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ui/remote.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.
Replace package discovery and update checks built on parsed CLI output with a structured package catalog over Pi's package manager APIs.
Add reusable async loader flows across the unified manager, remote browsing, and package configuration screens, and harden cancellation, removal, and status handling around the new flows.
Summary by CodeRabbit
New Features
Improvements
Documentation