Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a cross-platform npm execution helper and migrates npm calls to it; implements a robust shell-style tokenizer; extends git-source recognition and local-path normalization (preserving case for non-Windows paths); augments package update/remove flows with per-target results and refines auto-update timer handling; tests updated/added. Changes
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 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/packages/management.ts (1)
365-392:⚠️ Potential issue | 🟡 MinorPartial-failure scenario skips the removal-completion wait.
When some targets succeed and some fail,
failures.length === 0isfalse, so thewaitForConditionblock (line 368) is skipped entirely—yetsuccessfulRemovalCount > 0means the code falls through toconfirmReloadat line 394 without waiting for the successful removals to be fully flushed.I understand that waiting for all targets (including failed ones) would cause a guaranteed timeout, but you could filter the wait to only the successful targets. Low severity since the existing
confirmReloadprompt itself gives the system a moment, and the prior code likely had the same gap.Proposed fix: wait only for successfully removed targets
- const successfulRemovalCount = targets.length - failures.length; + const successfulRemovalCount = targets.length - failures.length; + const succeededTargets = targets.filter( + (_, i) => !failures.some((f) => f.includes(targets[i]?.source ?? "")) + ); // Wait for selected targets to disappear from their target scopes before reloading. - if (successfulRemovalCount > 0 && failures.length === 0 && targets.length > 0) { + if (succeededTargets.length > 0) { notify(ctx, "Waiting for removal to complete...", "info"); const isRemoved = await waitForCondition( async () => { const installedChecks = await Promise.all( - targets.map((target) => + succeededTargets.map((target) => isSourceInstalled(target.source, ctx, pi, { scope: target.scope, })Note: This sketch assumes you can correlate failures back to specific targets. A cleaner approach would be to return per-target results from
executeRemovalTargetsinstead of just failure strings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/packages/management.ts` around lines 365 - 392, The wait-for-removal block currently skips when any failures exist; change it to wait only for the successfully removed targets by computing successfulTargets = targets.filter(t => t is not represented in failures) and then call waitForCondition using successfulTargets (instead of targets) and isSourceInstalled to check each successful target's scope; keep the same retry options and same warning on timeout, and leave the existing early-return behavior that calls updateExtmgrStatus/returns NO_PACKAGE_MUTATION_OUTCOME when successfulRemovalCount === 0.
♻️ Duplicate comments (1)
src/packages/discovery.ts (1)
120-131: Duplicate of the normalization logic inmanagement.ts:normalizeLocalSourceIdentity.Already flagged in the
management.tsreview — the Windows-path heuristic and conditional lowercasing here are identical. Both should be consolidated into a shared utility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/packages/discovery.ts` around lines 120 - 131, The Windows-path heuristic and conditional lowercasing in normalizeSourceIdentity duplicate logic already present in management.ts:normalizeLocalSourceIdentity; extract the shared behavior into a new utility (e.g., normalizeLocalSourceIdentity or normalizePathIdentity) and have normalizeSourceIdentity call that utility after calling sanitizeListSourceSuffix and getPackageSourceKind, reusing the same Windows-path detection (regex /^[a-zA-Z]:\//, startsWith("//"), or source.includes("\\")) and conditional toLowerCase logic; ensure the utility exports the function and update both normalizeSourceIdentity and the function in management.ts to import and call the single shared implementation (keeping references to sanitizeListSourceSuffix and getPackageSourceKind where needed).
🧹 Nitpick comments (4)
test/command-tokenizer.test.ts (1)
5-44: Optional: add coverage for empty quoted args.Given the tokenizer now handles quotes, a small test for
""/''would lock in empty‑argument behavior and prevent future regressions.✅ Example test addition
void test("tokenizeArgs supports quoted values", () => { assert.deepEqual(tokenizeArgs('install "./extensions/My Cool Extension" --project'), [ "install", "./extensions/My Cool Extension", "--project", ]); assert.deepEqual(tokenizeArgs("install 'git@github.com:user/my repo.git'"), [ "install", "git@github.com:user/my repo.git", ]); + + assert.deepEqual(tokenizeArgs('cmd "" --flag'), ["cmd", "", "--flag"]); + assert.deepEqual(tokenizeArgs("cmd '' --flag"), ["cmd", "", "--flag"]); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/command-tokenizer.test.ts` around lines 5 - 44, Add tests to command-tokenizer.test.ts that assert tokenizeArgs correctly returns an empty string token for empty quoted arguments: e.g., assert.deepEqual(tokenizeArgs('cmd ""'), ["cmd", ""]); and for single quotes assert.deepEqual(tokenizeArgs("cmd ''"), ["cmd", ""]); also add a mixed case such as assert.deepEqual(tokenizeArgs('cmd "" --flag'), ["cmd", "", "--flag"]); reference the tokenizeArgs calls in existing tests to follow the same style and include both double- and single-quoted empty arg cases.src/utils/format.ts (1)
127-140: Minor: doubleunwrapQuotedSourceinvocation innormalizePackageSource.Line 128 unwraps the source, then line 135 calls
isPackageSource(trimmed), which unwraps again internally (line 121). Harmless since it's idempotent, but you could avoid the double work by using the internalisGitLikeSource/isLocalPathSourcechecks directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/format.ts` around lines 127 - 140, normalizePackageSource currently calls unwrapQuotedSource and then calls isPackageSource(trimmed) which unwraps again; avoid the redundant unwrap by replacing the isPackageSource call with the internal checks (isGitLikeSource and isLocalPathSource) or by adding an overload that accepts an already-unwrapped string. Concretely, in normalizePackageSource use unwrapQuotedSource(source) once, then test trimmed with isGitLikeSource(trimmed) and isLocalPathSource(trimmed) (or call a new isPackageSourceUnwrapped helper) before returning trimmed or `npm:${trimmed}`, keeping the existing GIT_PATTERNS.gitSsh check intact.src/packages/management.ts (1)
154-160: Extract duplicated Windows path normalization logic to a shared utility.The Windows path detection heuristics (
/^[a-zA-Z]:\//.test(),startsWith("//"), and backslash presence check) along with the lowercase-for-Windows / preserve-case-for-Unix logic are identical betweennormalizeLocalSourceIdentity(lines 154–160) andnormalizeSourceIdentityinsrc/packages/discovery.ts(lines 120–127). Consider extracting this into a shared helper (e.g.,src/utils/package-source.ts) to maintain the heuristic in one place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/packages/management.ts` around lines 154 - 160, Duplicate Windows path normalization logic should be extracted into a shared helper: create a utility (e.g., normalizePackageSourceIdentity) in a new module (suggested name src/utils/package-source.ts) that encapsulates the heuristic (/^[a-zA-Z]:\//.test, startsWith("//"), and backslash presence) and returns lowercased Windows paths or original casing for non-Windows; then replace the inline logic in normalizeLocalSourceIdentity and normalizeSourceIdentity to call this new helper and update their imports accordingly so behavior is unchanged but the heuristic is maintained in one place.test/discovery-parser.test.ts (1)
206-216: Test name slightly outgrows its new scope — optional nit.The block at line 197 is named
"normalizePackageSource preserves git and local path sources", but the three new assertions here also cover quote-unwrapping behaviour ("..."and'...'). Consider splitting the quote-unwrapping cases into their owntest(...)block (e.g.,"normalizePackageSource unwraps quoted sources") to keep test intent discoverable from the name.The assertions themselves are correct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/discovery-parser.test.ts` around lines 206 - 216, The test "normalizePackageSource preserves git and local path sources" now includes assertions that exercise quote-unwrapping behavior; split those quoted-source assertions into their own test to keep intent clear. Locate the existing test block that calls normalizePackageSource (the assertions with '"./extensions/My Cool Extension.ts"' and "'@scope/pkg'"), remove those two assertions from the current test and create a new test named something like "normalizePackageSource unwraps quoted sources" that asserts normalizePackageSource('"./extensions/My Cool Extension.ts"') -> "./extensions/My Cool Extension.ts" and normalizePackageSource("'@scope/pkg'") -> "npm:`@scope/pkg`". Ensure both tests still import/require normalizePackageSource and run in the same test file.
🤖 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/management.ts`:
- Around line 44-46: The current isUpToDateOutput function uses a too-broad
/\bpinned\b/i which can match "pinned" anywhere; update the pinned check to only
match when "pinned" appears as the primary status at the start of a line (e.g.
use a multiline-anchored pattern like /^\s*pinned\b/im or an equivalent
anchored/line-start regex) so that phrases like "pinned dependency Y skipped" do
not count as up-to-date; modify the return expression in isUpToDateOutput to
replace the /\bpinned\b/i test with the anchored variant.
In `@src/utils/command.ts`:
- Around line 5-76: The tokenizeArgs function currently drops empty quoted
tokens because pushCurrent only pushes when current.length > 0; update
token-start tracking so empty quoted arguments are preserved: add a boolean
(e.g., tokenStarted or sawQuote) that is set when starting a token (on seeing
non-space, opening single-quote inSingleQuote set, or opening double-quote
inDoubleQuote set) and clear when pushCurrent is called; change pushCurrent to
push the current string even if empty when tokenStarted is true (and then reset
tokenStarted), and ensure entering quotes sets tokenStarted so inputs like "" or
'' produce an empty token in the returned array. Reference: tokenizeArgs,
pushCurrent, inSingleQuote, inDoubleQuote.
---
Outside diff comments:
In `@src/packages/management.ts`:
- Around line 365-392: The wait-for-removal block currently skips when any
failures exist; change it to wait only for the successfully removed targets by
computing successfulTargets = targets.filter(t => t is not represented in
failures) and then call waitForCondition using successfulTargets (instead of
targets) and isSourceInstalled to check each successful target's scope; keep the
same retry options and same warning on timeout, and leave the existing
early-return behavior that calls updateExtmgrStatus/returns
NO_PACKAGE_MUTATION_OUTCOME when successfulRemovalCount === 0.
---
Duplicate comments:
In `@src/packages/discovery.ts`:
- Around line 120-131: The Windows-path heuristic and conditional lowercasing in
normalizeSourceIdentity duplicate logic already present in
management.ts:normalizeLocalSourceIdentity; extract the shared behavior into a
new utility (e.g., normalizeLocalSourceIdentity or normalizePathIdentity) and
have normalizeSourceIdentity call that utility after calling
sanitizeListSourceSuffix and getPackageSourceKind, reusing the same Windows-path
detection (regex /^[a-zA-Z]:\//, startsWith("//"), or source.includes("\\")) and
conditional toLowerCase logic; ensure the utility exports the function and
update both normalizeSourceIdentity and the function in management.ts to import
and call the single shared implementation (keeping references to
sanitizeListSourceSuffix and getPackageSourceKind where needed).
---
Nitpick comments:
In `@src/packages/management.ts`:
- Around line 154-160: Duplicate Windows path normalization logic should be
extracted into a shared helper: create a utility (e.g.,
normalizePackageSourceIdentity) in a new module (suggested name
src/utils/package-source.ts) that encapsulates the heuristic
(/^[a-zA-Z]:\//.test, startsWith("//"), and backslash presence) and returns
lowercased Windows paths or original casing for non-Windows; then replace the
inline logic in normalizeLocalSourceIdentity and normalizeSourceIdentity to call
this new helper and update their imports accordingly so behavior is unchanged
but the heuristic is maintained in one place.
In `@src/utils/format.ts`:
- Around line 127-140: normalizePackageSource currently calls unwrapQuotedSource
and then calls isPackageSource(trimmed) which unwraps again; avoid the redundant
unwrap by replacing the isPackageSource call with the internal checks
(isGitLikeSource and isLocalPathSource) or by adding an overload that accepts an
already-unwrapped string. Concretely, in normalizePackageSource use
unwrapQuotedSource(source) once, then test trimmed with isGitLikeSource(trimmed)
and isLocalPathSource(trimmed) (or call a new isPackageSourceUnwrapped helper)
before returning trimmed or `npm:${trimmed}`, keeping the existing
GIT_PATTERNS.gitSsh check intact.
In `@test/command-tokenizer.test.ts`:
- Around line 5-44: Add tests to command-tokenizer.test.ts that assert
tokenizeArgs correctly returns an empty string token for empty quoted arguments:
e.g., assert.deepEqual(tokenizeArgs('cmd ""'), ["cmd", ""]); and for single
quotes assert.deepEqual(tokenizeArgs("cmd ''"), ["cmd", ""]); also add a mixed
case such as assert.deepEqual(tokenizeArgs('cmd "" --flag'), ["cmd", "",
"--flag"]); reference the tokenizeArgs calls in existing tests to follow the
same style and include both double- and single-quoted empty arg cases.
In `@test/discovery-parser.test.ts`:
- Around line 206-216: The test "normalizePackageSource preserves git and local
path sources" now includes assertions that exercise quote-unwrapping behavior;
split those quoted-source assertions into their own test to keep intent clear.
Locate the existing test block that calls normalizePackageSource (the assertions
with '"./extensions/My Cool Extension.ts"' and "'@scope/pkg'"), remove those two
assertions from the current test and create a new test named something like
"normalizePackageSource unwraps quoted sources" that asserts
normalizePackageSource('"./extensions/My Cool Extension.ts"') ->
"./extensions/My Cool Extension.ts" and normalizePackageSource("'@scope/pkg'")
-> "npm:`@scope/pkg`". Ensure both tests still import/require
normalizePackageSource and run in the same test file.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/index.tssrc/packages/discovery.tssrc/packages/install.tssrc/packages/management.tssrc/ui/remote.tssrc/ui/unified.tssrc/utils/auto-update.tssrc/utils/command.tssrc/utils/format.tssrc/utils/npm-exec.tssrc/utils/package-source.tstest/auto-update.test.tstest/command-tokenizer.test.tstest/discovery-parser.test.tstest/install-remove.test.tstest/npm-exec.test.tstest/unified-items.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/packages/management.ts (1)
159-180:getPackageSourceKindis called twice on the same input.Lines 165 and 171 both call
getPackageSourceKind(source). Consider computing it once and using a variable.♻️ Suggested refactor
function packageIdentity(source: string, fallbackName?: string): string { const npm = parseNpmSource(source); if (npm?.name) { return `npm:${npm.name}`; } - if (getPackageSourceKind(source) === "git") { + const kind = getPackageSourceKind(source); + + if (kind === "git") { const gitSpec = source.startsWith("git:") ? source.slice(4) : source; const { repo } = splitGitRepoAndRef(gitSpec); return `git:${repo}`; } - if (getPackageSourceKind(source) === "local") { + if (kind === "local") { return `src:${normalizeLocalSourceIdentity(source)}`; } if (fallbackName) { return `name:${fallbackName}`; } return `src:${source}`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/packages/management.ts` around lines 159 - 180, The packageIdentity function calls getPackageSourceKind(source) twice; compute it once into a local variable (e.g., const kind = getPackageSourceKind(source)) at the top of the function and use that variable in the subsequent conditionals instead of calling the function again; update references in the git and local branches (the blocks using getPackageSourceKind(source) === "git" and === "local") to use kind so the logic and return values from packageIdentity remain identical.src/utils/format.ts (1)
60-91: Git source detection logic is duplicated acrossformat.tsandpackage-source.ts.Both
isGitLikeSourcehere (regex-based) andgetPackageSourceKindinpackage-source.ts(string prefix-based) maintain parallel lists of git URL prefixes. If a new prefix is added to one but not the other, they'll silently diverge. Consider consolidating into a single source of truth — e.g., haveisGitLikeSourcedelegate togetPackageSourceKind(source) === "git".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/format.ts` around lines 60 - 91, The git detection logic in isGitLikeSource duplicates the prefixes in getPackageSourceKind; remove the duplicated regex table and make isGitLikeSource delegate to getPackageSourceKind (or a new shared helper) instead: import or call getPackageSourceKind(source) and return getPackageSourceKind(source) === "git" (or move the GIT_PATTERNS into a single exported helper used by both isGitLikeSource and getPackageSourceKind) so there’s a single source of truth for git detection.src/utils/package-source.ts (1)
50-56:normalizeLocalSourceIdentity— logic is sound but consider trailing-slash normalization.The function correctly lowercases only Windows-style paths while preserving case for Unix paths. One edge case: paths with or without trailing slashes (e.g.
/opt/ext/vs/opt/ext) would produce different identities. If this is used for deduplication, trailing slash inconsistency could cause false mismatches. Consider stripping trailing slashes if that scenario is possible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/package-source.ts` around lines 50 - 56, normalizeLocalSourceIdentity should also normalize trailing slashes to avoid identity mismatches; after computing normalized and looksWindowsPath, strip trailing slashes from normalized (e.g. remove / or multiple trailing /) except when the path is exactly "/" or a Windows drive root like "C:/", then return the possibly-lowercased, trimmed value—update the function using the existing normalized and looksWindowsPath symbols to implement this safe-trimming logic.
🤖 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/management.ts`:
- Around line 159-180: The packageIdentity function calls
getPackageSourceKind(source) twice; compute it once into a local variable (e.g.,
const kind = getPackageSourceKind(source)) at the top of the function and use
that variable in the subsequent conditionals instead of calling the function
again; update references in the git and local branches (the blocks using
getPackageSourceKind(source) === "git" and === "local") to use kind so the logic
and return values from packageIdentity remain identical.
In `@src/utils/format.ts`:
- Around line 60-91: The git detection logic in isGitLikeSource duplicates the
prefixes in getPackageSourceKind; remove the duplicated regex table and make
isGitLikeSource delegate to getPackageSourceKind (or a new shared helper)
instead: import or call getPackageSourceKind(source) and return
getPackageSourceKind(source) === "git" (or move the GIT_PATTERNS into a single
exported helper used by both isGitLikeSource and getPackageSourceKind) so
there’s a single source of truth for git detection.
In `@src/utils/package-source.ts`:
- Around line 50-56: normalizeLocalSourceIdentity should also normalize trailing
slashes to avoid identity mismatches; after computing normalized and
looksWindowsPath, strip trailing slashes from normalized (e.g. remove / or
multiple trailing /) except when the path is exactly "/" or a Windows drive root
like "C:/", then return the possibly-lowercased, trimmed value—update the
function using the existing normalized and looksWindowsPath symbols to implement
this safe-trimming logic.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/packages/discovery.tssrc/packages/management.tssrc/utils/command.tssrc/utils/format.tssrc/utils/package-source.tstest/command-tokenizer.test.tstest/discovery-parser.test.tstest/install-remove.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/utils/command.ts
- test/command-tokenizer.test.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
Bug Fixes
Tests