Conversation
Extract common relative-path, path-identity, and duration parsing helpers, reuse them across local and package discovery, and remove dead utility code.
Filter and cap persisted history entries while scanning session files so large session directories do not require collecting every matching change before applying the limit.
Add inline remote browse search, richer npm metadata caching, package author display, and focused regression coverage for remote flows and shared custom component rendering.
Add contextual unified-manager navigation, cleaner read-only fallbacks, shared package action outcomes, and regression coverage for manager, search, and package-config behavior.
Replace the old CI-triggered publish job with the reusable release-it flow, including workflow inputs, release config, CI helper script, dependencies, and tests.
📝 WalkthroughWalkthroughRelease automation was added and the previous CD workflow removed; shared duration and path utilities were extracted; extension discovery and package install/remove flows were refactored (outcome-based APIs); UIs replaced list components with custom browsers and improved remote/community browsing; cache metadata was extended; extensive tests were added. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UnifiedBrowser as UnifiedManagerBrowser
participant FilterMap as readPackageFilterMap
participant Discovery as discoverExtensions
participant UI as renderManager
participant Actions as handleManagerInput
User->>UnifiedBrowser: open manager
UnifiedBrowser->>FilterMap: load package filter state
FilterMap-->>UnifiedBrowser: filter map
UnifiedBrowser->>Discovery: discover local + installed entries
Discovery-->>UnifiedBrowser: ExtensionEntry[]
UnifiedBrowser->>UI: render list + header summary
User->>Actions: press "/" (search)
Actions->>UnifiedBrowser: set search mode
UnifiedBrowser->>UnifiedBrowser: recompute visible items (token scoring/fuzzy)
UnifiedBrowser->>UI: render filtered results
User->>Actions: select item
Actions->>UnifiedBrowser: set selected item
UnifiedBrowser->>UI: render details panel
sequenceDiagram
participant User
participant RemoteBrowser as RemotePackageBrowser
participant Planner as browseRemotePackages
participant Cache as communityBrowseCache
participant Npm as NpmRegistry
participant UI as renderResults
User->>RemoteBrowser: start browse / search
RemoteBrowser->>Planner: planBrowseQuery(query)
alt community mode (keywords or empty)
Planner->>Cache: lookup community key
alt cache hit
Cache-->>Planner: cached results
else
Planner->>Npm: search community keywords
Npm-->>Planner: raw results
Planner->>Cache: store community results
end
Planner->>Planner: apply local filtering & scoring
else npm lookup
Planner->>Npm: search npm registry
Npm-->>Planner: results
else path-like query
Planner-->>User: emit warning (looks like local path)
end
Planner-->>RemoteBrowser: filtered results
RemoteBrowser->>UI: render package list
User->>RemoteBrowser: install action
RemoteBrowser->>Npm: (install flow) call installPackageWithOutcome
Npm-->>RemoteBrowser: InstallOutcome (installed,reloaded)
RemoteBrowser->>UI: update view based on outcome
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/packages/extensions.ts (1)
479-485:⚠️ Potential issue | 🟡 MinorCallback signature mismatch with
resolveRelativePathSelection.The callback at line 481 accepts only one parameter
(path), butresolveRelativePathSelectionpasses two:(path, allFiles). While JavaScript ignores the extra argument, this violates the function's type contract and differs from the correct usage insrc/extensions/discovery.ts(line 206).To match the expected signature and ensure the path actually exists in the collection:
♻️ Suggested fix for consistent callback signature
async function resolveManifestExtensionEntries( packageRoot: string, entries: string[] ): Promise<string[]> { const allFiles = await collectExtensionFilesFromDir(packageRoot, packageRoot); - return resolveRelativePathSelection(allFiles, entries, (path) => isExtensionEntrypointPath(path)); + return resolveRelativePathSelection(allFiles, entries, (path, files) => + isExtensionEntrypointPath(path) && files.includes(path) + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/packages/extensions.ts` around lines 479 - 485, The callback passed to resolveRelativePathSelection from resolveManifestExtensionEntries currently uses a single parameter (path) but resolveRelativePathSelection invokes the callback with (path, allFiles); update the callback signature to accept both parameters and use the allFiles collection to verify membership and entrypoint status. Specifically, inside resolveManifestExtensionEntries (which calls collectExtensionFilesFromDir and resolveRelativePathSelection), change the predicate to (path, allFiles) => isExtensionEntrypointPath(path) && allFiles.includes(path) (or equivalent existence check) so the callback matches resolveRelativePathSelection's expected (path, allFiles) signature and ensures the path exists in the collected files.
🧹 Nitpick comments (5)
src/ui/footer.ts (1)
11-24: Consider simplifying the conditional.Both branches compute
pendingChangesidentically. The ternary can be simplified:♻️ Simplification
export function buildFooterState( staged: Map<string, State>, byId: Map<string, UnifiedItem>, selectedItem?: UnifiedItem ): FooterState { - return selectedItem - ? { - selectedType: selectedItem.type, - pendingChanges: getPendingToggleChangeCount(staged, byId), - } - : { - pendingChanges: getPendingToggleChangeCount(staged, byId), - }; + return { + selectedType: selectedItem?.type, + pendingChanges: getPendingToggleChangeCount(staged, byId), + }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/footer.ts` around lines 11 - 24, The buildFooterState function uses a ternary where both branches compute pendingChanges the same; simplify by creating the base FooterState with pendingChanges = getPendingToggleChangeCount(staged, byId) and then, if selectedItem is present, add selectedType: selectedItem.type to that object; update buildFooterState to use the single base object and conditional property addition (referencing buildFooterState, selectedItem, and getPendingToggleChangeCount) so the redundant ternary is removed.src/utils/relative-path-selection.ts (1)
11-20: Consider rejecting Windows absolute paths.
isSafeRelativePathvalidates against Unix absolute paths and traversal patterns, but Windows absolute paths likeC:/foo(after normalization fromC:\foo) would pass the checks. If this function is meant to restrict to relative paths only, Windows drive-letter paths should also be rejected.🛡️ Proposed fix
export function isSafeRelativePath(path: string): boolean { return ( path !== "" && path !== ".." && !path.startsWith("/") && + !/^[a-zA-Z]:/.test(path) && !path.startsWith("../") && !path.includes("/../") && !path.endsWith("/..") ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/relative-path-selection.ts` around lines 11 - 20, isSafeRelativePath currently only rejects POSIX absolute/traversal forms but lets Windows absolute paths through; update isSafeRelativePath(path: string) to also reject Windows drive-letter roots and UNC paths by detecting a leading drive-letter + colon (e.g. "C:/" or "C:\") and leading backslashes for UNC (e.g. "\\server\share"), in addition to the existing checks so any path matching a drive-letter colon prefix or starting with backslashes is considered unsafe.src/utils/history.ts (1)
384-385: Minor: Theslice(-limit)is redundant but defensive.Since
pushGlobalHistoryEntryalready enforces the limit, the finalslice(-limit)will always return the full array. It's harmless as defensive code but could be simplified.♻️ Optional simplification
matchedEntries.sort((a, b) => a.change.timestamp - b.change.timestamp); - return limit && limit > 0 ? matchedEntries.slice(-limit) : matchedEntries; + return matchedEntries;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/history.ts` around lines 384 - 385, The final slice(-limit) in the return of the matchedEntries pipeline is redundant because pushGlobalHistoryEntry already enforces the global history length; remove the defensive slicing and simply return matchedEntries after sorting. Update the function that contains matchedEntries.sort(...)/return ... so it returns matchedEntries directly (remove the conditional branch that calls slice(-limit)); reference matchedEntries, pushGlobalHistoryEntry, and the sort line to locate where to remove the unnecessary slice.CHANGELOG.md (1)
1-2: Consider populating the changelog with this PR's changes.The changelog file is empty. Given this PR introduces significant features (release automation, UI/UX improvements, new utilities like duration parsing and path identity), documenting these changes would improve release communication.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 1 - 2, The CHANGELOG.md is empty; populate it with an "Unreleased" section summarizing this PR's changes: add brief bullet points for release automation, UI/UX improvements, and new utilities (duration parsing, path identity), include PR number/author(s) and an expected version or date placeholder, and follow the project's changelog style (e.g., "Added", "Changed", "Fixed") so maintainers can review and promote these notes into a release entry later.src/ui/remote.ts (1)
118-147: Keep community browse on a single cache path.Line 903 prefers
communityBrowseCacheover the existing search cache, sosetSearchCache()andclearSearchCache()no longer fully control thekeywords:pi-packagedata. That split invalidation path is easy to miss and is already leaking into the test cleanup story. I'd collapse this to one cache or route both stores through a shared set/clear helper.Also applies to: 902-916
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/remote.ts` around lines 118 - 147, The code introduces a separate communityBrowseCache that bypasses the existing search cache APIs (setSearchCache/clearSearchCache) causing split invalidation for the "keywords:pi-package" entry; refactor so there's a single cache path: either remove communityBrowseCache and have getCommunityBrowseCache/setCommunityBrowseCache/clearCommunityBrowseCache delegate to the existing search cache functions (use the same query key "keywords:pi-package"/COMMUNITY_BROWSE_QUERY) or consolidate search cache into a shared helper that both code paths call, ensuring setSearchCache/clearSearchCache fully control the community data and no separate communityBrowseCache state remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release.yml:
- Around line 3-19: Add workflow-level concurrency and a branch guard: under the
top-level on:workflow_dispatch block (where release_type and first_release are
defined) add a top-level concurrency key (e.g. concurrency: { group:
"release-${{ github.repository }}-${{ github.ref }}", cancel-in-progress: true
}) to serialize manual runs, and add a job-level guard on the release job (e.g.
if: github.ref == 'refs/heads/main') to ensure releases only run for the
intended branch (or require/validate an explicit branch input and check
github.event.inputs.branch == 'main' in the job if you want a selectable
branch). Reference the workflow_dispatch inputs (release_type, first_release)
and the release job name when adding these keys.
In `@src/packages/discovery.ts`:
- Around line 102-110: The current find callback on pkg.maintainers (the
expression pkg.maintainers?.find(...)) uses an OR so it returns the first
maintainer with either a username or email, causing email-only maintainers to be
selected before later username entries; change the logic to first search for a
maintainer with a non-empty username (use pkg.maintainers?.find(entry =>
entry.username?.trim()) and return that username if found), and only if none is
found search for a maintainer with a non-empty email
(pkg.maintainers?.find(entry => entry.email?.trim()) and return that email);
keep the existing trimming and null checks (maintainer?.username?.trim(),
maintainer?.email?.trim()) when returning values.
In `@src/ui/unified.ts`:
- Around line 1305-1331: The handlers for selection branches
(promptLocalActionSelection, showUnifiedItemDetails,
resolvePendingChangesBeforeLeave) currently use "return false" which exits the
custom UI and forces showInteractiveOnce() to restart, losing staged/selection
state; change these returns so the manager session is preserved (e.g., replace
"return false" with logic that re-renders or continues the current interaction
loop and preserves staged, filter/search and selection state) — specifically
update the branches handling selection === "cancel", selection === "details"
(after calling showUnifiedItemDetails), and pending === "stay" (from
resolvePendingChangesBeforeLeave) to avoid exiting the function and instead
resume the current manager loop or refresh the UI so staged remains intact.
Ensure you preserve the existing variables staged, items, byId and selection
when changing control flow.
- Around line 254-255: The duplicate set is built using entry.activePath only
(in the loop over localEntries adding normalizePathIdentity(entry.activePath) to
localPaths), which misses disabled locals that later use entry.disabledPath;
update the logic in that loop to use the "live" local path (prefer
entry.disabledPath when present, otherwise entry.activePath), i.e. add
normalizePathIdentity(entry.disabledPath || entry.activePath) to localPaths so
disabled locals are correctly deduplicated.
In `@src/utils/cache.ts`:
- Around line 255-268: mergeCachedPackageData currently resets the single shared
timestamp to Date.now() even when it inherits stale fields from existing, which
breaks isCacheValid since inherited fields keep living under the new TTL; change
the merge logic so that inherited fields retain their original freshness rather
than treating the whole record as freshly fetched—either preserve
existing.timestamp when any preserved fields are copied (e.g., if next omitted a
field, keep existing.timestamp for those fields) or store per-field timestamps
(a timestamps map) and update only the timestamps for fields present in next;
update corresponding merge usages around the other noted blocks (the similar
merges at the other ranges) and ensure isCacheValid is adjusted to check the
per-field timestamp or use the preserved existing.timestamp semantics.
In `@src/utils/relative-path-selection.ts`:
- Line 1: The import of matchesGlob from node:path in
src/utils/relative-path-selection.ts is unsafe for Node.js <22.5.0; either
update package.json engines.node to ">=22.20.0" (preferred) or add a
runtime-safe fallback: detect whether path.matchesGlob exists (or try
require('node:path').matchesGlob) before using it and fall back to a bundled
implementation or a stable library like minimatch, ensuring any code that calls
matchesGlob (in this module) uses the chosen fallback function (e.g., local
matchesGlobWrapper) so the module works across older v22 releases.
In `@test/unified-ui.test.ts`:
- Line 121: The test currently hardcodes a POSIX temp path when calling
createMockHarness ({ cwd: "/tmp", hasUI: true }), which breaks on Windows;
update the test to create a portable temporary directory (e.g., using Node's
os.tmpdir() + fs.mkdtempSync or the tmpdir()/mkdtemp helpers already used
elsewhere) and pass that generated dir as the cwd to createMockHarness so the
call to createMockHarness({ cwd, hasUI: true }) uses a platform-agnostic temp
path.
---
Outside diff comments:
In `@src/packages/extensions.ts`:
- Around line 479-485: The callback passed to resolveRelativePathSelection from
resolveManifestExtensionEntries currently uses a single parameter (path) but
resolveRelativePathSelection invokes the callback with (path, allFiles); update
the callback signature to accept both parameters and use the allFiles collection
to verify membership and entrypoint status. Specifically, inside
resolveManifestExtensionEntries (which calls collectExtensionFilesFromDir and
resolveRelativePathSelection), change the predicate to (path, allFiles) =>
isExtensionEntrypointPath(path) && allFiles.includes(path) (or equivalent
existence check) so the callback matches resolveRelativePathSelection's expected
(path, allFiles) signature and ensures the path exists in the collected files.
---
Nitpick comments:
In `@CHANGELOG.md`:
- Around line 1-2: The CHANGELOG.md is empty; populate it with an "Unreleased"
section summarizing this PR's changes: add brief bullet points for release
automation, UI/UX improvements, and new utilities (duration parsing, path
identity), include PR number/author(s) and an expected version or date
placeholder, and follow the project's changelog style (e.g., "Added", "Changed",
"Fixed") so maintainers can review and promote these notes into a release entry
later.
In `@src/ui/footer.ts`:
- Around line 11-24: The buildFooterState function uses a ternary where both
branches compute pendingChanges the same; simplify by creating the base
FooterState with pendingChanges = getPendingToggleChangeCount(staged, byId) and
then, if selectedItem is present, add selectedType: selectedItem.type to that
object; update buildFooterState to use the single base object and conditional
property addition (referencing buildFooterState, selectedItem, and
getPendingToggleChangeCount) so the redundant ternary is removed.
In `@src/ui/remote.ts`:
- Around line 118-147: The code introduces a separate communityBrowseCache that
bypasses the existing search cache APIs (setSearchCache/clearSearchCache)
causing split invalidation for the "keywords:pi-package" entry; refactor so
there's a single cache path: either remove communityBrowseCache and have
getCommunityBrowseCache/setCommunityBrowseCache/clearCommunityBrowseCache
delegate to the existing search cache functions (use the same query key
"keywords:pi-package"/COMMUNITY_BROWSE_QUERY) or consolidate search cache into a
shared helper that both code paths call, ensuring
setSearchCache/clearSearchCache fully control the community data and no separate
communityBrowseCache state remains.
In `@src/utils/history.ts`:
- Around line 384-385: The final slice(-limit) in the return of the
matchedEntries pipeline is redundant because pushGlobalHistoryEntry already
enforces the global history length; remove the defensive slicing and simply
return matchedEntries after sorting. Update the function that contains
matchedEntries.sort(...)/return ... so it returns matchedEntries directly
(remove the conditional branch that calls slice(-limit)); reference
matchedEntries, pushGlobalHistoryEntry, and the sort line to locate where to
remove the unnecessary slice.
In `@src/utils/relative-path-selection.ts`:
- Around line 11-20: isSafeRelativePath currently only rejects POSIX
absolute/traversal forms but lets Windows absolute paths through; update
isSafeRelativePath(path: string) to also reject Windows drive-letter roots and
UNC paths by detecting a leading drive-letter + colon (e.g. "C:/" or "C:\") and
leading backslashes for UNC (e.g. "\\server\share"), in addition to the existing
checks so any path matching a drive-letter colon prefix or starting with
backslashes is considered unsafe.
🪄 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: 3ab81f48-f9c0-48cb-ad98-e17d33f73978
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (45)
.github/workflows/cd.yml.github/workflows/release.yml.release-it.jsonCHANGELOG.mdREADME.mdpackage.jsonscripts/release-ci.tssrc/commands/auto-update.tssrc/commands/history.tssrc/constants.tssrc/extensions/discovery.tssrc/packages/discovery.tssrc/packages/extensions.tssrc/packages/install.tssrc/packages/management.tssrc/types/index.tssrc/ui/footer.tssrc/ui/help.tssrc/ui/remote.tssrc/ui/unified.tssrc/utils/auto-update.tssrc/utils/cache.tssrc/utils/duration.tssrc/utils/format.tssrc/utils/history.tssrc/utils/mode.tssrc/utils/notify.tssrc/utils/package-source.tssrc/utils/path-identity.tssrc/utils/relative-path-selection.tssrc/utils/retry.tssrc/utils/settings.tstest/auto-update.test.tstest/cache-history.test.tstest/duration.test.tstest/helpers/custom-component.tstest/install-remove.test.tstest/local-discovery.test.tstest/metadata-cache.test.tstest/package-config.test.tstest/package-extensions.test.tstest/release-ci.test.tstest/remote-ui.test.tstest/search-regression.test.tstest/unified-ui.test.ts
💤 Files with no reviewable changes (5)
- .github/workflows/cd.yml
- src/utils/format.ts
- src/utils/notify.ts
- src/constants.ts
- src/utils/retry.ts
Preserve unified manager state across detail and stay-in-manager flows, tighten local/package dedupe, and restore clear-on-Escape behavior. Also add per-field cache freshness, consolidate community browse caching, harden manifest/path handling, and gate manual releases to main with workflow concurrency.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/ui/remote.ts (1)
1054-1108:⚠️ Potential issue | 🟡 MinorTreat a cancelled details menu like “Back to results”.
If this select is dismissed,
showPackageDetails()just returns, which also endsbrowseRemotePackages()and drops the current query/page. Esc/cancel here should reopen the previous results the same way the explicit back option does.Suggested fix
const choice = parseChoiceByLabel( PACKAGE_DETAILS_CHOICES, await ctx.ui.select(packageName, Object.values(PACKAGE_DETAILS_CHOICES)) ); + + if (!choice || choice === "back") { + await browseRemotePackages(ctx, previousQuery, pi, previousOffset, browseSource); + return; + } switch (choice) { @@ - case "back": - await browseRemotePackages(ctx, previousQuery, pi, previousOffset, browseSource); - return; default: return; }Also applies to: 1110-1111
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/remote.ts` around lines 1054 - 1108, The viewInfo branch treats a cancelled details fetch as a no-op (it notifies then calls showPackageDetails which returns and ends browseRemotePackages), so change the cancel-handling to behave like the explicit "Back to results": when runTaskWithLoader returns falsy (cancelled) call browseRemotePackages(ctx, previousQuery, pi, previousOffset, browseSource) and return, instead of calling showPackageDetails; also apply the same fix to the similar cancellation path around lines 1110-1111 so any cancelled buildPackageInfoText invocation re-opens browseRemotePackages rather than exiting.src/ui/unified.ts (1)
1234-1260:⚠️ Potential issue | 🟠 Major“Discard changes” is not actually discarding on resume paths.
resolvePendingChangesBeforeLeave()still leavesstageduntouched for the discard branch, and these flows now resume the same manager session. That means discarding before Help/Auto-update, or before local removal and then cancelling the delete confirm, brings the user right back to the same pending toggles.Suggested fix
async function resolvePendingChangesBeforeLeave( items: UnifiedItem[], staged: Map<string, State>, byId: Map<string, UnifiedItem>, ctx: ExtensionCommandContext, pi: ExtensionAPI, destinationLabel: string ): Promise<"continue" | "stay"> { @@ if (choice === "Discard changes") { + staged.clear(); return "continue"; }Also applies to: 1372-1386
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/unified.ts` around lines 1234 - 1260, The resume-returning branches (e.g., the "auto-update" case using promptAutoUpdateWizard, the "help" case calling showHelp, and similar resume flows around local removal) are not actually discarding pending toggles because resolvePendingChangesBeforeLeave() leaves manager.staged untouched; update those branches to call resolvePendingChangesBeforeLeave(ctx, pi) and, if it indicates a discard branch, explicitly clear the staged toggles on the current manager (e.g., reset manager.staged or call the manager's clear method) before returning "resume" so resumed sessions do not see the old pending changes.
🧹 Nitpick comments (2)
test/relative-path-selection.test.ts (1)
8-23: Expand safety tests for POSIX/traversal edge cases.The current suite is Windows-focused. Please also assert behavior for POSIX absolute and traversal tokens so cross-platform safety stays regression-proof (Line 8 and Line 15 test blocks).
Suggested test additions
void test("isSafeRelativePath rejects Windows absolute and UNC paths", () => { assert.equal(isSafeRelativePath("C:/repo/extensions/index.ts"), false); assert.equal(isSafeRelativePath("C:\\repo\\extensions\\index.ts"), false); assert.equal(isSafeRelativePath("\\\\server\\share\\index.ts"), false); + assert.equal(isSafeRelativePath("/repo/extensions/index.ts"), false); + assert.equal(isSafeRelativePath("../extensions/index.ts"), false); + assert.equal(isSafeRelativePath("extensions/../index.ts"), false); assert.equal(isSafeRelativePath("extensions/index.ts"), true); }); void test("resolveRelativePathSelection ignores Windows absolute tokens", () => { const selected = resolveRelativePathSelection( ["extensions/index.ts"], - ["C:/repo/extensions/index.ts", "\\\\server\\share\\index.ts"], + [ + "C:/repo/extensions/index.ts", + "\\\\server\\share\\index.ts", + "/repo/extensions/index.ts", + "../extensions/index.ts", + ], () => true );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/relative-path-selection.test.ts` around lines 8 - 23, Add POSIX absolute and traversal cases to the existing safety tests: call isSafeRelativePath with POSIX absolute paths (e.g., "/repo/extensions/index.ts") and path traversal tokens ("../etc/passwd", "extensions/../../secret") and assert they return false where appropriate, and update the resolveRelativePathSelection test to include those POSIX absolute/traversal paths in the available files array and assert the selected result excludes them (use the same pattern as the Windows checks referencing isSafeRelativePath and resolveRelativePathSelection).src/utils/history.ts (1)
317-330: Consider a heap iflimitcan grow.Once the buffer is full, every accepted entry rescans
entriesto find the current oldest item. That makes this pathO(matches × limit), so larger user-supplied limits can be slower than the old collect-and-sort flow. A min-heap keeps the bounded-memory behavior without the linear rescans.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/history.ts` around lines 317 - 330, The current logic in the block that maintains entries (variables entries, nextEntry, oldestIndex, oldestEntry) does a linear scan to find the oldest entry when the buffer is full, causing O(matches × limit) behavior; replace this with a bounded min-heap (priority queue) keyed by entry.change.timestamp to maintain the oldest element in O(log limit) per insertion. Concretely, replace the array-only approach where you push/scan entries with a heap data structure that supports push and popMin, keep heap size ≤ limit, and on accepting nextEntry push it to the heap and if heap.size > limit pop the oldest; update any callers/iterations that expect an array to either rebuild a stable array from the heap when needed or iterate the heap safely. Ensure the heap compares using entry.change.timestamp and preserve existing semantics for ties and null/undefined entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/ui/remote.ts`:
- Around line 1054-1108: The viewInfo branch treats a cancelled details fetch as
a no-op (it notifies then calls showPackageDetails which returns and ends
browseRemotePackages), so change the cancel-handling to behave like the explicit
"Back to results": when runTaskWithLoader returns falsy (cancelled) call
browseRemotePackages(ctx, previousQuery, pi, previousOffset, browseSource) and
return, instead of calling showPackageDetails; also apply the same fix to the
similar cancellation path around lines 1110-1111 so any cancelled
buildPackageInfoText invocation re-opens browseRemotePackages rather than
exiting.
In `@src/ui/unified.ts`:
- Around line 1234-1260: The resume-returning branches (e.g., the "auto-update"
case using promptAutoUpdateWizard, the "help" case calling showHelp, and similar
resume flows around local removal) are not actually discarding pending toggles
because resolvePendingChangesBeforeLeave() leaves manager.staged untouched;
update those branches to call resolvePendingChangesBeforeLeave(ctx, pi) and, if
it indicates a discard branch, explicitly clear the staged toggles on the
current manager (e.g., reset manager.staged or call the manager's clear method)
before returning "resume" so resumed sessions do not see the old pending
changes.
---
Nitpick comments:
In `@src/utils/history.ts`:
- Around line 317-330: The current logic in the block that maintains entries
(variables entries, nextEntry, oldestIndex, oldestEntry) does a linear scan to
find the oldest entry when the buffer is full, causing O(matches × limit)
behavior; replace this with a bounded min-heap (priority queue) keyed by
entry.change.timestamp to maintain the oldest element in O(log limit) per
insertion. Concretely, replace the array-only approach where you push/scan
entries with a heap data structure that supports push and popMin, keep heap size
≤ limit, and on accepting nextEntry push it to the heap and if heap.size > limit
pop the oldest; update any callers/iterations that expect an array to either
rebuild a stable array from the heap when needed or iterate the heap safely.
Ensure the heap compares using entry.change.timestamp and preserve existing
semantics for ties and null/undefined entries.
In `@test/relative-path-selection.test.ts`:
- Around line 8-23: Add POSIX absolute and traversal cases to the existing
safety tests: call isSafeRelativePath with POSIX absolute paths (e.g.,
"/repo/extensions/index.ts") and path traversal tokens ("../etc/passwd",
"extensions/../../secret") and assert they return false where appropriate, and
update the resolveRelativePathSelection test to include those POSIX
absolute/traversal paths in the available files array and assert the selected
result excludes them (use the same pattern as the Windows checks referencing
isSafeRelativePath and resolveRelativePathSelection).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e2ae37cd-137e-4374-bbf4-430eb827334a
📒 Files selected for processing (19)
.github/workflows/release.ymlCHANGELOG.mdREADME.mdpackage.jsonsrc/packages/discovery.tssrc/packages/extensions.tssrc/ui/footer.tssrc/ui/remote.tssrc/ui/unified.tssrc/utils/cache.tssrc/utils/history.tssrc/utils/relative-path-selection.tstest/metadata-cache.test.tstest/npm-search.test.tstest/package-config.test.tstest/package-extensions.test.tstest/relative-path-selection.test.tstest/unified-items.test.tstest/unified-ui.test.ts
✅ Files skipped from review due to trivial changes (5)
- CHANGELOG.md
- src/packages/discovery.ts
- .github/workflows/release.yml
- README.md
- src/utils/relative-path-selection.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- package.json
- test/metadata-cache.test.ts
- test/package-config.test.ts
- src/packages/extensions.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.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes