Conversation
…gs errors earlier
📝 WalkthroughWalkthroughThe PR standardizes package identity handling, adds manifest-driven extension entrypoint discovery and standalone-install validation (tar, deps, timeouts), extends cache clearing to in-memory caches, records new history actions, and refactors UIs to use custom UI wrappers with read-only fallbacks. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Installer as Installer
participant Fetch as Fetch (with Timeout)
participant Tar as Tar Utility
participant Manifest as Manifest Reader
participant Deps as Dependency Validator
participant FS as File System
participant History as History Logger
User->>Installer: request install (URL/local)
Installer->>Tar: ensure tar available
alt tar unavailable
Tar-->>Installer: error
Installer-->>User: abort with actionable error
else tar available
Installer->>Fetch: download with timeout
alt download times out
Fetch-->>Installer: timeout error
Installer-->>User: abort with timeout message
else download success
Installer->>FS: extract to temp dir
Installer->>Manifest: read package.json / discover entrypoints
Manifest-->>Installer: manifest + entrypoints
Installer->>Deps: validate runtime deps present in tar
alt deps missing
Deps-->>Installer: unresolved deps error
Installer->>FS: cleanup temp artifacts
Installer-->>User: abort with dependency error
else deps ok
Installer->>FS: move package into place
Installer->>History: log install success
Installer->>FS: cleanup temp artifacts
Installer-->>User: install success
end
end
end
sequenceDiagram
participant User
participant UIHandler as UI Handler
participant Capability as UI Capability Detector
participant CustomUI as Custom UI
participant Fallback as Read-only Fallback
User->>UIHandler: request interactive UI (configure/browse)
UIHandler->>Capability: getUICapability(ctx)
alt custom UI available
Capability-->>CustomUI: runCustomUI(open)
CustomUI-->>UIHandler: interactive result
UIHandler-->>User: show interactive UI
else custom UI unavailable
Capability-->>Fallback: show read-only lists / warn
Fallback-->>User: emit warning and display packages
UIHandler-->>User: read-only view (no config)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 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: 6
🧹 Nitpick comments (9)
README.md (1)
159-160: Prefer source-agnostic wording for local install constraints.“Bundled in the tarball” is a bit too implementation-specific here since the documented local sources also include folders and single files. “Bundled with the package contents” or similar would be clearer and less confusing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 159 - 160, Update the second bullet's wording to be source-agnostic: replace the phrase "bundled in the tarball" with something like "bundled with the package contents" (or "included with the package contents") so the sentence reads that the local installer "requires `tar` on `PATH`, and rejects packages whose runtime `dependencies` are not already bundled with the package contents"; keep the rest of the bullet (references to `~/.pi/agent/extensions/{package}/`, runnable standalone layouts, and requiring `tar`) unchanged.src/commands/history.ts (1)
191-192: Derive the--actionhelp text fromHISTORY_ACTIONS.This list now has two sources of truth. The next action change can easily update parsing but leave the help stale.
♻️ Proposed fix
function showHistoryHelp(ctx: ExtensionCommandContext): void { + const actionList = HISTORY_ACTIONS.join(" | "); const lines = [ "Usage: /extensions history [options]", "", "Options:", " --limit <n> Maximum entries to show (default: 20)", " --action <type> Filter by action", - " extension_toggle | extension_delete | package_install | package_update | package_remove | cache_clear | auto_update_config", + ` ${actionList}`, " --success Show only successful entries", " --failed Show only failed entries", " --package <q> Filter by package/source/extension id",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/history.ts` around lines 191 - 192, Replace the hard-coded `--action` help text with a generated string derived from the `HISTORY_ACTIONS` array so there isn't a second source of truth; locate the help lines for the `--action <type>` option in the help array and build the second line by joining `HISTORY_ACTIONS` (e.g., `HISTORY_ACTIONS.join(' | ')`) or formatting it the same way the current literals are, ensuring the help output preserves the existing spacing/indentation and uses the `HISTORY_ACTIONS` identifier.src/ui/unified.ts (1)
63-69: Use the shared notifier in the no-custom-UI fallback.
hasCustomUI(ctx)is also false whenctx.hasUIis false, so this branch now bypasses the helper that already knows how to surface messages in headless/RPC mode. Routing this throughnotify(...)keeps the fallback visible beforeshowInteractiveFallback()prints the read-only lists.Possible fix
import { parseChoiceByLabel } from "../utils/command.js"; import { getPackageSourceKind, normalizePackageIdentity } from "../utils/package-source.js"; import { hasCustomUI, runCustomUI } from "../utils/mode.js"; +import { notify } from "../utils/notify.js"; import { getSettingsListSelectedIndex } from "../utils/settings-list.js"; import { UI } from "../constants.js"; import { configurePackageExtensions } from "./package-config.js"; @@ if (!hasCustomUI(ctx)) { - ctx.ui.notify( + notify( + ctx, "The unified extensions manager requires the full interactive TUI. Showing read-only local and installed package lists instead.", "warning" ); await showInteractiveFallback(ctx, pi);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/unified.ts` around lines 63 - 69, The branch that runs when hasCustomUI(ctx) is false currently calls ctx.ui.notify directly, which bypasses the shared headless/RPC notifier; instead route the warning through the shared notifier helper before calling showInteractiveFallback so the message is surfaced in headless/RPC mode (i.e., replace the direct ctx.ui.notify call with the shared notifier invocation used elsewhere, then await showInteractiveFallback(ctx, pi) as before); reference symbols: hasCustomUI, ctx.ui.notify, showInteractiveFallback.test/helpers/mocks.ts (1)
22-30: Let tests drive theui.custom()outcome.The new callers distinguish a real cancel/result from a degraded custom UI, but this harness always resolves
ctx.ui.custom()toundefined. That means it can only exercise the fallback path, not the happy path for the new custom UI flows.Possible fix
export interface MockHarnessOptions { cwd?: string; hasUI?: boolean; hasCustomUI?: boolean; execImpl?: ExecImpl; inputResult?: string; selectResult?: string; confirmResult?: boolean; confirmImpl?: (title: string, message?: string) => boolean | Promise<boolean>; + customUIResult?: unknown; + customUIImpl?: (factory: unknown) => unknown | Promise<unknown>; } @@ custom: options.hasUI && options.hasCustomUI !== false - ? (_factory: unknown) => { + ? (factory: unknown) => { customCalls += 1; - return Promise.resolve(undefined); + if (options.customUIImpl) { + return Promise.resolve(options.customUIImpl(factory)); + } + return Promise.resolve(options.customUIResult); } : undefined,Also applies to: 150-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/helpers/mocks.ts` around lines 22 - 30, The harness always resolves ctx.ui.custom() to undefined, preventing tests from exercising the success path; add a customizable handler (e.g., a new MockHarnessOptions field named customImpl or customResult) similar to confirmImpl so tests can specify the resolved value or async function, then update the harness code that supplies ctx.ui.custom() to call and/or return that customImpl result (falling back to undefined only when not provided). Reference MockHarnessOptions and ctx.ui.custom() (and mirror the confirmImpl pattern) so callers can simulate real results, cancellations, or degraded behavior.src/utils/mode.ts (1)
53-70: Avoid overloadingundefinedas the degraded-UI signal.This helper now forces every caller to invent its own cancel sentinel to avoid a misleading warning/fallback on normal user cancel. A small result union here would keep cancellation semantics inside
runCustomUI()instead of pushing that contract into each call site.One possible API shape
export async function runCustomUI<T>( ctx: AnyContext, featureName: string, - open: () => Promise<T | undefined>, + open: () => Promise< + | { type: "ok"; value: T } + | { type: "cancel" } + | { type: "unavailable" } + >, fallbackMessage?: string ): Promise<T | undefined> { if (!requireCustomUI(ctx, featureName, fallbackMessage)) { return undefined; } const result = await open(); - if (result !== undefined) { - return result; + if (result.type === "ok") { + return result.value; + } + if (result.type === "cancel") { + return undefined; } const suffix = fallbackMessage ? ` ${fallbackMessage}` : ""; notify(ctx, `${featureName} requires the full interactive TUI.${suffix}`, "warning"); return undefined; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/mode.ts` around lines 53 - 70, runCustomUI currently overloads undefined for three meanings (no-custom-ui, user-cancel, and fallback-needed) which forces callers to invent sentinels; change its return type to a discriminated union (e.g., Promise<{status:"ok", value:T} | {status:"cancel"} | {status:"needs-tui"}), update runCustomUI to map open() results into those three statuses (return {status:"cancel"} when the user cancelled without emitting the notify, return {status:"needs-tui"} when custom UI is unavailable and emit the existing notify, and return {status:"ok", value} when open yields a value), and update all callers of runCustomUI to handle the new union instead of treating undefined specially; keep the function name runCustomUI and the notify call but ensure notify is only invoked for the needs-tui status.src/packages/install.ts (1)
149-155: Consider swallowing errors in cleanup to avoid masking original failures.If
rmthrows (e.g., permission denied), it could mask the original error that triggered cleanup. Consider wrapping in try-catch or using{ force: true }semantics more defensively.🛡️ Defensive cleanup approach
async function cleanupStandaloneTempArtifacts(tempDir: string, extractDir?: string): Promise<void> { - if (extractDir) { - await rm(extractDir, { recursive: true, force: true }); - } - - await rm(tempDir, { recursive: true, force: true }); + try { + if (extractDir) { + await rm(extractDir, { recursive: true, force: true }); + } + await rm(tempDir, { recursive: true, force: true }); + } catch { + // Ignore cleanup errors to avoid masking the original failure + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/packages/install.ts` around lines 149 - 155, The cleanupStandaloneTempArtifacts function currently awaits rm calls that can throw and mask original errors; wrap each rm invocation (the extractDir rm and the tempDir rm) in try-catch (or use Promise.allSettled) so deletion failures are swallowed or logged as warnings instead of rethrowing. Specifically, update cleanupStandaloneTempArtifacts to catch errors from rm(...) and call a logger.warn/console.warn with context (including the path) or ignore them, ensuring the function never throws during cleanup.src/packages/discovery.ts (1)
288-290: Clarify intent ofparseInstalledPackagesOutputAllScopes.This function returns raw parsed results without deduplication, unlike
parseInstalledPackagesOutput. Consider adding a doc comment to clarify when to use each variant, since callers might inadvertently use the wrong one.📝 Add clarifying documentation
+/** + * Parse installed packages output without deduplication. + * Use this when you need to inspect all entries including duplicates + * (e.g., for isSourceInstalled scope-specific checks). + * For deduplicated results honoring project precedence, use parseInstalledPackagesOutput. + */ export function parseInstalledPackagesOutputAllScopes(text: string): InstalledPackage[] { return parseInstalledPackagesOutputInternal(text); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/packages/discovery.ts` around lines 288 - 290, Add a short doc comment above parseInstalledPackagesOutputAllScopes explaining that it returns raw parsed results from parseInstalledPackagesOutputInternal without deduplication or scope-merging, and note that parseInstalledPackagesOutput is the deduplicating/normalized variant callers should prefer for user-facing lists; reference the function names parseInstalledPackagesOutputAllScopes, parseInstalledPackagesOutput, and parseInstalledPackagesOutputInternal in the comment so maintainers know the behavioral difference and when to use each.src/utils/auto-update.ts (1)
57-80: Redundant config save wheninitialDelayMs > 0.When
initialDelayMs > 0, the config is saved with the samenextCheckvalue that was just read. This write has no effect unlessstartTimermodifies state that needs persisting. Consider removing this redundant save or clarifying intent with a comment.♻️ Remove redundant save or add clarifying comment
if (initialDelayMs > 0 && nextCheck !== undefined) { - saveAutoUpdateConfig(pi, { - ...config, - nextCheck, - }); + // Config already has nextCheck set; no need to re-save }Alternatively, if this save is intentional (e.g., to ensure config is persisted after timer start), add a comment explaining why.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/auto-update.ts` around lines 57 - 80, The code saves the same nextCheck back into config redundantly after computing initialDelayMs; remove the conditional saveAutoUpdateConfig(pi, { ...config, nextCheck }) block (or if the save is required for persistence after startTimer, replace it with a one-line comment clarifying that intent). Update the area around startTimer, initialDelayMs, nextCheck, and saveAutoUpdateConfig so there is no unnecessary write unless there is a documented side-effect requiring persistence.src/ui/package-config.ts (1)
211-213: Potential index out-of-bounds whensettingsItemsis empty.If
settingsItemsis empty,selectedIndexdefaults to0, andsettingsItems[0]?.idwould beundefined. The fallback?? settingsItems[0]?.idis alsoundefinedin this case. WhileselectedRowwould then beundefinedand the toggle block is skipped, consider adding an early return when there are no items.🛡️ Guard against empty items
handleInput(data: string) { if (matchesKey(data, Key.ctrl("s")) || data === "s" || data === "S") { done({ type: "save" }); return; } + if (settingsItems.length === 0) { + return; + } + const selectedIndex = getSettingsListSelectedIndex(settingsList) ?? 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/package-config.ts` around lines 211 - 213, The code calculates selectedIndex and selectedId without guarding for an empty settingsItems array, which can yield undefined and risk out-of-bounds access; update the logic in the block using getSettingsListSelectedIndex, selectedIndex, settingsItems, selectedId and selectedRow to first check if settingsItems.length === 0 and return early (or skip further processing) when empty, otherwise compute selectedIndex (e.g., clamp to valid range) and derive selectedId and selectedRow from rowById as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 111-113: Clarify the README to remove the apparent contradiction
about duration formats: update the description for `/extensions auto-update
<duration>` and the parenthetical after "history keeps `30m`/`24h` style
durations for lookback windows" to state explicitly whether `1mo` is accepted
for history/lookbacks (i.e., either note those are only examples and `1mo` is
also valid, or state that `1mo` is not supported), and make the `--since ...
1mo` example consistent with that wording so users see a single authoritative
rule; reference the `/extensions auto-update <duration>` entry and the `--since`
example when editing so both lines align.
In `@src/packages/install.ts`:
- Line 276: The call in installFromUrl uses TIMEOUTS.fetchPackageInfo for file
downloads which is too short; update the fetchWithTimeout call in installFromUrl
to use a longer timeout constant (either TIMEOUTS.packageInstall or a new
dedicated constant) instead of TIMEOUTS.fetchPackageInfo so large or slow
downloads won't prematurely fail; change the argument passed to fetchWithTimeout
and adjust any related tests or constants as needed.
In `@src/utils/package-source.ts`:
- Around line 71-75: The git branch only strips a plain "git:" prefix, so specs
like "git+https://..." remain different and break identity matching; update the
logic in the git branch (where gitSpec is computed and splitGitRepoAndRef is
called) to strip both "git:" and "git+..." prefixes (e.g. remove a leading
"git+" if present, e.g. via a regex like /^git\+?:/), then proceed to call
splitGitRepoAndRef(gitSpec) and normalize repo with repo.replace(/\\/g,
"/").toLowerCase() as before.
In `@src/utils/settings-list.ts`:
- Around line 5-11: The helper getSettingsListSelectedIndex currently accepts
any JS number (including NaN, Infinity, and fractions); change the guard to only
accept true integers by replacing the typeof check with a Number.isInteger check
on selectable.selectedIndex (i.e., return selectable.selectedIndex only if
Number.isInteger(selectable.selectedIndex)); reference the
SelectableListLike.selectedIndex field when updating the conditional and keep
returning undefined otherwise.
In `@src/utils/settings.ts`:
- Around line 52-62: The sanitizeUpdateIdentities function currently only
filters entries by prefix using isUpdateIdentity, which allows non-canonical
forms to persist; update it to map each retained entry through the existing
normalizePackageIdentity() function after sanitizeStringArray(value) and
filtering, and return the array of normalized identities (or undefined if
empty). Keep the initial filtering with isUpdateIdentity to reject irrelevant
values, but ensure you call normalizePackageIdentity on each value before
returning so stored identities match the canonical form used by
clearUpdatesAvailable and other consumers.
In `@test/install-remove.test.ts`:
- Around line 612-649: The mocked globalThis.fetch in the "installFromUrl aborts
stalled downloads instead of hanging forever" test can hang if the abort wiring
regresses; change the mock to fail-fast by adding a fallback timeout: inside the
fetch mock (the anonymous function used to replace fetch), start a short timer
via the saved originalSetTimeout that rejects the promise with a distinct
timeout Error (e.g., name "TimeoutError") if the abort event never fires, and
clear that timer when the abort handler runs (or when the promise settles); this
keeps the test deterministic while still allowing the abort flow to reject with
an AbortError when signalled and ensures you still restore globalThis.fetch and
globalThis.setTimeout in the finally block.
---
Nitpick comments:
In `@README.md`:
- Around line 159-160: Update the second bullet's wording to be source-agnostic:
replace the phrase "bundled in the tarball" with something like "bundled with
the package contents" (or "included with the package contents") so the sentence
reads that the local installer "requires `tar` on `PATH`, and rejects packages
whose runtime `dependencies` are not already bundled with the package contents";
keep the rest of the bullet (references to `~/.pi/agent/extensions/{package}/`,
runnable standalone layouts, and requiring `tar`) unchanged.
In `@src/commands/history.ts`:
- Around line 191-192: Replace the hard-coded `--action` help text with a
generated string derived from the `HISTORY_ACTIONS` array so there isn't a
second source of truth; locate the help lines for the `--action <type>` option
in the help array and build the second line by joining `HISTORY_ACTIONS` (e.g.,
`HISTORY_ACTIONS.join(' | ')`) or formatting it the same way the current
literals are, ensuring the help output preserves the existing
spacing/indentation and uses the `HISTORY_ACTIONS` identifier.
In `@src/packages/discovery.ts`:
- Around line 288-290: Add a short doc comment above
parseInstalledPackagesOutputAllScopes explaining that it returns raw parsed
results from parseInstalledPackagesOutputInternal without deduplication or
scope-merging, and note that parseInstalledPackagesOutput is the
deduplicating/normalized variant callers should prefer for user-facing lists;
reference the function names parseInstalledPackagesOutputAllScopes,
parseInstalledPackagesOutput, and parseInstalledPackagesOutputInternal in the
comment so maintainers know the behavioral difference and when to use each.
In `@src/packages/install.ts`:
- Around line 149-155: The cleanupStandaloneTempArtifacts function currently
awaits rm calls that can throw and mask original errors; wrap each rm invocation
(the extractDir rm and the tempDir rm) in try-catch (or use Promise.allSettled)
so deletion failures are swallowed or logged as warnings instead of rethrowing.
Specifically, update cleanupStandaloneTempArtifacts to catch errors from rm(...)
and call a logger.warn/console.warn with context (including the path) or ignore
them, ensuring the function never throws during cleanup.
In `@src/ui/package-config.ts`:
- Around line 211-213: The code calculates selectedIndex and selectedId without
guarding for an empty settingsItems array, which can yield undefined and risk
out-of-bounds access; update the logic in the block using
getSettingsListSelectedIndex, selectedIndex, settingsItems, selectedId and
selectedRow to first check if settingsItems.length === 0 and return early (or
skip further processing) when empty, otherwise compute selectedIndex (e.g.,
clamp to valid range) and derive selectedId and selectedRow from rowById as
before.
In `@src/ui/unified.ts`:
- Around line 63-69: The branch that runs when hasCustomUI(ctx) is false
currently calls ctx.ui.notify directly, which bypasses the shared headless/RPC
notifier; instead route the warning through the shared notifier helper before
calling showInteractiveFallback so the message is surfaced in headless/RPC mode
(i.e., replace the direct ctx.ui.notify call with the shared notifier invocation
used elsewhere, then await showInteractiveFallback(ctx, pi) as before);
reference symbols: hasCustomUI, ctx.ui.notify, showInteractiveFallback.
In `@src/utils/auto-update.ts`:
- Around line 57-80: The code saves the same nextCheck back into config
redundantly after computing initialDelayMs; remove the conditional
saveAutoUpdateConfig(pi, { ...config, nextCheck }) block (or if the save is
required for persistence after startTimer, replace it with a one-line comment
clarifying that intent). Update the area around startTimer, initialDelayMs,
nextCheck, and saveAutoUpdateConfig so there is no unnecessary write unless
there is a documented side-effect requiring persistence.
In `@src/utils/mode.ts`:
- Around line 53-70: runCustomUI currently overloads undefined for three
meanings (no-custom-ui, user-cancel, and fallback-needed) which forces callers
to invent sentinels; change its return type to a discriminated union (e.g.,
Promise<{status:"ok", value:T} | {status:"cancel"} | {status:"needs-tui"}),
update runCustomUI to map open() results into those three statuses (return
{status:"cancel"} when the user cancelled without emitting the notify, return
{status:"needs-tui"} when custom UI is unavailable and emit the existing notify,
and return {status:"ok", value} when open yields a value), and update all
callers of runCustomUI to handle the new union instead of treating undefined
specially; keep the function name runCustomUI and the notify call but ensure
notify is only invoked for the needs-tui status.
In `@test/helpers/mocks.ts`:
- Around line 22-30: The harness always resolves ctx.ui.custom() to undefined,
preventing tests from exercising the success path; add a customizable handler
(e.g., a new MockHarnessOptions field named customImpl or customResult) similar
to confirmImpl so tests can specify the resolved value or async function, then
update the harness code that supplies ctx.ui.custom() to call and/or return that
customImpl result (falling back to undefined only when not provided). Reference
MockHarnessOptions and ctx.ui.custom() (and mirror the confirmImpl pattern) so
callers can simulate real results, cancellations, or degraded behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 808686eb-6dae-4fa3-ba9a-4f46cb1bb005
📒 Files selected for processing (30)
README.mdsrc/commands/auto-update.tssrc/commands/cache.tssrc/commands/history.tssrc/commands/registry.tssrc/packages/discovery.tssrc/packages/extensions.tssrc/packages/install.tssrc/packages/management.tssrc/ui/package-config.tssrc/ui/remote.tssrc/ui/unified.tssrc/utils/auto-update.tssrc/utils/history.tssrc/utils/mode.tssrc/utils/package-source.tssrc/utils/settings-list.tssrc/utils/settings.tssrc/utils/status.tssrc/utils/timer.tssrc/utils/ui-helpers.tstest/auto-update.test.tstest/cache-history.test.tstest/discovery-parser.test.tstest/helpers/mocks.tstest/install-remove.test.tstest/package-config.test.tstest/package-extensions.test.tstest/rpc-mode.test.tstest/unified-items.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/packages/install.ts (1)
149-164: Consider usingallSettledfor more robust cleanup.The current
Promise.allapproach will short-circuit if one deletion fails. While the individualtry/catchblocks prevent exceptions from propagating, usingPromise.allSettledwould be more semantically correct for cleanup operations where all attempts should complete.Optional refactor
async function cleanupStandaloneTempArtifacts(tempDir: string, extractDir?: string): Promise<void> { const paths = [extractDir, tempDir].filter((path): path is string => Boolean(path)); - await Promise.all( + await Promise.allSettled( paths.map(async (path) => { try { await rm(path, { recursive: true, force: true }); } catch (error) { console.warn( `[extmgr] Failed to remove temporary standalone install artifact at ${path}:`, error ); } }) ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/packages/install.ts` around lines 149 - 164, The cleanupStandaloneTempArtifacts function uses Promise.all to run removals; change it to Promise.allSettled so each rm call is awaited to completion regardless of others (keeping the existing try/catch per-path). Locate cleanupStandaloneTempArtifacts and replace the Promise.all(...) wrapper with Promise.allSettled(...) over the same paths.map(async (path) => { try { await rm(...) } catch (error) { console.warn(...) } }); this ensures all rm operations finish without short-circuiting while preserving the rm call and error logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/packages/install.ts`:
- Around line 149-164: The cleanupStandaloneTempArtifacts function uses
Promise.all to run removals; change it to Promise.allSettled so each rm call is
awaited to completion regardless of others (keeping the existing try/catch
per-path). Locate cleanupStandaloneTempArtifacts and replace the
Promise.all(...) wrapper with Promise.allSettled(...) over the same
paths.map(async (path) => { try { await rm(...) } catch (error) {
console.warn(...) } }); this ensures all rm operations finish without
short-circuiting while preserving the rm call and error logging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 91cdcbcd-db0e-460d-aaba-2d884166e020
📒 Files selected for processing (13)
README.mdsrc/commands/history.tssrc/packages/discovery.tssrc/packages/install.tssrc/ui/unified.tssrc/utils/auto-update.tssrc/utils/package-source.tssrc/utils/settings-list.tssrc/utils/settings.tstest/auto-update.test.tstest/discovery-parser.test.tstest/install-remove.test.tstest/settings-list.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- test/auto-update.test.ts
- src/utils/auto-update.ts
- src/utils/settings-list.ts
ayagmar
changed the title
This was referenced Mar 24, 2026
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.
Summary
- degraded custom UI / RPC behavior
- auto-update scheduling and normalized update markers
- standalone install cleanup and dependency rejection
- stalled URL download abort behavior
- parser identity handling
- package config safety
- unified item update matching
Summary by CodeRabbit
New Features
Bug Fixes & Improvements