feat(cli): add a show command - #2153
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Version-history sorting can be incorrect for prerelease SemVer due to semver.coerce() (and there’s also a user-facing identifier error message mismatch), which should be fixed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a new ovsx show CLI command to display extension metadata in a vsce show-like layout, including Open VSX–specific registry fields and an optional version-history table sourced from the v2 query endpoint.
Changes:
- Introduces
cli/src/show.tswith formatted output, notices (deprecation/disputed namespace/not downloadable), and version history aggregation. - Extends the CLI registry client to support version-specific metadata fetches and a best-effort
/api/v2/-/query?includeAllVersions=truecall. - Adds a comprehensive unit test suite for
showusing a local HTTP stub.
File summaries
| File | Description |
|---|---|
| cli/src/show.ts | Implements the show command output, including version history sorting/capping and registry-specific sections. |
| cli/src/show-options.ts | Defines ShowOptions for show command flags (--json, --all-versions, --target). |
| cli/src/registry.ts | Adds queryAllVersions and extends getMetadata to optionally request a specific version. |
| cli/src/main.ts | Wires the new show subcommand into the CLI entrypoint. |
| cli/test/unit/show.spec.ts | Adds unit tests for formatting, filtering internal tags, version history behavior, and best-effort query failure handling. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Switched the version history off the query endpoint in 9994223 — you were right that it was the wrong source, and measuring it made that clear. For
The query endpoint repeats every extension-level field —
The cost is the two columns that endpoint cannot supply. Neither Adding Unrelated, but it showed up while testing against the live registry: |
There was a problem hiding this comment.
🟡 Changes recommended
There are confirmed functional correctness issues (semver sorting behavior, potential NaN output, and --target not applying to version history) plus a notable mismatch with the described version-history parity.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (4)
Previously missed (2) — in code that hasn't changed since the last review.
cli/src/show.ts:328
formatRatingcallsNumber(extension.reviewCount); ifreviewCountis missing in the registry response this becomesNaNand prints"NaN reviews".formatCountalready handlesundefinedsafely, so pass the value through directly.
cli/test/unit/show.spec.ts:113- If the CLI accepts both
namespace.extensionandnamespace/extension, this assertion should be updated alongside the CLI error message; otherwise the test will either fail unnecessarily or allow a misleading message to persist.
cli/src/show.ts:53
- The error message says identifiers must be
namespace.extension, butmatchExtensionIdaccepts bothnamespace.extensionandnamespace/extension(andShowOptionsdocs mention both). This makes the thrown message misleading for users.
const match = matchExtensionId(id);
if (!match) {
throw new Error('The extension identifier must have the form `namespace.extension`.');
}
cli/src/show.ts:140
byNewestFirstusessemver.coerce, which contradicts the comment that non‑semver sorts after valid semver (many invalid strings will be coerced) and also drops prerelease/build metadata (e.g.1.2.0-betacoerces to1.2.0), which can mis-order prereleases relative to releases.
function byNewestFirst(a: VersionSummary, b: VersionSummary): number {
const left = semver.coerce(a.version);
const right = semver.coerce(b.version);
if (left && right) {
return semver.rcompare(left, right);
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
Review feedback on #2153, all four points valid. byNewestFirst used semver.coerce, which drops prerelease identifiers: 1.2.0-alpha.1 coerced to 1.2.0, so a prerelease compared equal to its release and the order was unstable. coerce also accepts input semver itself rejects - 'v1.2' becomes 1.2.0 - which defeated the string fallback and made the comment about non-semver sorting last untrue. semver.valid keeps prerelease identifiers, orders them below their release, and rejects what it should. --target is described as scoping the report, but only the metadata lookup honoured it; the version listing always asked for the unscoped path. The registry serves /api/{ns}/{ext}/{target}/version-references, so the target is passed through and the history table now matches the rest of the output. formatRating wrapped reviewCount in Number(), so a response without one printed 'NaN reviews'. formatCount already handles undefined, so the value goes through directly. The identifier error message named only the dotted form, though matchExtensionId accepts namespace/extension too and ShowOptions documents both. Reworded - and the same wording in get and unpublish is updated with it, since three commands validating identically should not report it differently. Each fix has a test, all three confirmed to fail with the bug put back. Refs #2149 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@copilot why is this PR marked as being not ready in the stacked PR view? |
It’s currently marked not ready because the PR is in a blocked state ( |
There was a problem hiding this comment.
🔵 Needs a closer look
There are a couple of concrete CLI behavior and determinism issues (notably @ empty-suffix handling and stable ordering of version target platforms) that should be addressed before approval.
Review details
Suppressed comments (4)
Previously missed (4) — in code that hasn't changed since the last review.
cli/src/show.ts:82
splitVersion()treats an identifier ending with@as if no version was provided (it returns an empty string, andgetMetadata()then omits the version segment). This silently ignores user input likeovsx show redhat.java@, which should be rejected as invalid.
cli/src/main.ts:122- The
showcommand help/usage only mentions the dot form (namespace.extension), but the CLI now explicitly supportsnamespace/extensiontoo (and the error messages say so). Updating the help text avoids misleading users.
cli/src/show.ts:131 - The order of
targetPlatformsin the version history output depends on the registry response order. Since the API doesn’t promise a stable ordering, this can make CLI output noisy and potentially flaky across registries/runs. Sorting the collectedtargetPlatformsper version makes the output deterministic.
cli/test/unit/show.spec.ts:116 - There’s no unit test for the edge case
namespace.extension@(empty version suffix). SincesplitVersion()currently treats that as “no version”, adding a test will prevent regressions once the empty-suffix validation is fixed.
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
Review feedback on #2153, all four points valid. byNewestFirst used semver.coerce, which drops prerelease identifiers: 1.2.0-alpha.1 coerced to 1.2.0, so a prerelease compared equal to its release and the order was unstable. coerce also accepts input semver itself rejects - 'v1.2' becomes 1.2.0 - which defeated the string fallback and made the comment about non-semver sorting last untrue. semver.valid keeps prerelease identifiers, orders them below their release, and rejects what it should. --target is described as scoping the report, but only the metadata lookup honoured it; the version listing always asked for the unscoped path. The registry serves /api/{ns}/{ext}/{target}/version-references, so the target is passed through and the history table now matches the rest of the output. formatRating wrapped reviewCount in Number(), so a response without one printed 'NaN reviews'. formatCount already handles undefined, so the value goes through directly. The identifier error message named only the dotted form, though matchExtensionId accepts namespace/extension too and ShowOptions documents both. Reworded - and the same wording in get and unpublish is updated with it, since three commands validating identically should not report it differently. Each fix has a test, all three confirmed to fail with the bug put back. Refs #2149 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
8854259 to
7f21726
Compare
splitVersion read `namespace.extension@` as "no version given": the trailing `@` cleared the version to an empty string, and getMetadata appends the version segment only when it is truthy, so the request went out without one and show reported the latest version instead. Silent, and wrong in the case that actually produces this input - a shell variable that did not expand, as in `ovsx show ext@$VERSION` with VERSION unset. A script asking about one version and being told about another is worse than an error, so this is now an error. Reported by Copilot on #2153. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Prints an extension's metadata, following 'vsce show' closely enough that the habit transfers: display name, publisher, downloads and rating, description, a version history table, categories, tags (the internal '__'-prefixed ones filtered out, as the web UI does), a "More Info" block and statistics. '--json' prints the raw metadata. Beyond parity, a Registry block reports what an Open VSX compatible registry knows and the Marketplace has no equivalent for: verified publisher, trusted publishing, pre-release status, extension kind, localized languages, dependencies and bundled extensions. Deprecation leads the output instead, naming the replacement extension where the registry supplies one, since that changes whether you should install the extension at all - as do a disputed namespace and an extension the registry won't serve for download. The version history needs per-version timestamps and pre-release flags, which the metadata response doesn't carry - 'allVersions' is version numbers and links only, and 'allTargetPlatformVersions' is populated on the user and admin endpoints rather than the public one. So it comes from '/api/v2/-/query?includeAllVersions=true', one request for every version, whose one-row-per-target-platform results are collapsed to a row per version. That query is best-effort: a registry that doesn't serve it still gets the rest of the output. Versions are sorted newest first here rather than trusting the query's order, which also means the table's cap drops the oldest versions rather than arbitrary ones. '--all-versions' lists them all, and the count left out is reported rather than truncating silently. Accepts 'namespace.extension@version' the way vsce does, including the 'latest' and 'pre-release' aliases, plus '--target' for a specific target platform. No token needed - both endpoints are public. Closes #2149 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review feedback: the history table was reading
/api/v2/-/query?includeAllVersions=true, which is by far the most
expensive way to get it. For redhat.java on open-vsx.org that response
is 267KB against 11KB for the same information from
/api/{namespace}/{extension}/version-references, because the query
repeats every extension-level field - files, tags, description,
publisher - on all 3817 version/target-platform rows.
Worse, the query pages at 100 rows, so '--all-versions' silently listed
at most 100 of those 3817 rather than all of them, which is not what
the option claims.
version-references returns version and target platform, newest first,
with size/offset paging, so the listing is now complete: one page for
the default table, paging to the end for '--all-versions'. totalSize
counts version/target-platform pairs rather than versions, so paging
runs off what has actually been returned.
The cost is the two columns that endpoint doesn't carry: neither it nor
/versions reports a timestamp or the pre-release flag, so the table is
now Version and Target Platforms. Adding those two fields to
VersionReferenceJson would restore them at no bandwidth cost, and looks
like the right fix rather than making every client pay for the query
endpoint - raised separately.
Refs #2149
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review feedback on #2153, all four points valid. byNewestFirst used semver.coerce, which drops prerelease identifiers: 1.2.0-alpha.1 coerced to 1.2.0, so a prerelease compared equal to its release and the order was unstable. coerce also accepts input semver itself rejects - 'v1.2' becomes 1.2.0 - which defeated the string fallback and made the comment about non-semver sorting last untrue. semver.valid keeps prerelease identifiers, orders them below their release, and rejects what it should. --target is described as scoping the report, but only the metadata lookup honoured it; the version listing always asked for the unscoped path. The registry serves /api/{ns}/{ext}/{target}/version-references, so the target is passed through and the history table now matches the rest of the output. formatRating wrapped reviewCount in Number(), so a response without one printed 'NaN reviews'. formatCount already handles undefined, so the value goes through directly. The identifier error message named only the dotted form, though matchExtensionId accepts namespace/extension too and ShowOptions documents both. Reworded - and the same wording in get and unpublish is updated with it, since three commands validating identically should not report it differently. Each fix has a test, all three confirmed to fail with the bug put back. Refs #2149 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
splitVersion read `namespace.extension@` as "no version given": the trailing `@` cleared the version to an empty string, and getMetadata appends the version segment only when it is truthy, so the request went out without one and show reported the latest version instead. Silent, and wrong in the case that actually produces this input - a shell variable that did not expand, as in `ovsx show ext@$VERSION` with VERSION unset. A script asking about one version and being told about another is worse than an error, so this is now an error. Reported by Copilot on #2153. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3d01995 to
8776e7d
Compare
Closes #2149. Follows
vsce showclosely enough that the habit transfers, then leans into what an Open VSX compatible registry knows and the Marketplace has no equivalent for.Parity
The sections and their order follow
vsce show, including filtering the internal__-prefixed tags (the web UI hides those too).--jsonprints the raw metadata.Identifiers accept
namespace.extension@versionthe way vsce does — an exact version or the registry's ownlatest/pre-releasealiases — plus-t/--target, since Open VSX is target-platform aware in a way vsce'sshowisn't. No token required: both endpoints it uses are public, so this also works on a private registry with no login provider configured.Beyond parity
The
Registry:block is kept separate rather than mixed intoMore Info, so it's obvious which fields are registry specific, and it's omitted entirely when none apply: verified publisher, trusted publishing, pre-release status, extension kind, localized languages, dependencies and bundled extensions.Deprecation is promoted above the metadata instead of being a table row, naming the replacement where the registry supplies one, since it changes whether you should install the extension at all:
A disputed namespace and an extension the registry won't serve for download are surfaced the same way.
Why there is a second request
The version history needs one extra request.
allVersionson the metadata response carries version numbers and links only, andallTargetPlatformVersionsis populated on the user and admin endpoints rather than the public one, so the table comes from/api/{namespace}/{extension}/version-references- one page for the default table, paging to the end for--all-versions. Its one-row-per-target-platform results are collapsed into a row per version, which is also what fills the Targets column.--targetscopes that listing too, via/api/{namespace}/{extension}/{target}/version-references.That listing is best-effort. A registry that doesn't serve it, or errors on it, still gets the whole summary minus the history table; there's a test for that.
The table is Version and Target Platforms only. It initially also carried "Last Updated" and a pre-release marker, sourced from
/api/v2/-/query?includeAllVersions=true, but that was the wrong source: 267KB against 11KB for the same extension, and capped at 100 of 3817 rows, so--all-versionswas quietly incomplete - measurements are in the comment below. Neitherversion-referencesnor/versionsreports a timestamp or the pre-release flag, so those two columns are gone for now. AddingtimestampandpreReleasetoVersionReferenceJsonwould bring them back at no bandwidth cost, which seems better than making every client pull the query endpoint - happy to put that up separately.Versions are sorted newest first here rather than trusting the listing's order, which also means the cap drops the oldest rather than arbitrary ones. Sorting uses
semver.validand notcoerce: coerce drops prerelease identifiers, so1.2.0-alpha.1would compare equal to1.2.0. Versions that aren't valid semver (reachable via mirroring) sort after those that are.--all-versionslists everything, and the number left out is reported rather than truncated silently.Testing
13 new cases in
cli/test/unit/show.spec.tsagainst a local HTTP stub, followingunpublish.spec.ts. Full CLI suite green (70 tests),tscandeslintclean.Worth noting that two bugs in this PR were found by running the command rather than by the tests — the unsorted version history above, and
Downloadsprinting unformatted while the header line used thousands separators. Both are now covered, the sort by a case using 1.9.0/1.10.0/1.2.0 that a string sort would get wrong.One thing to decide
--jsonoverlaps with the existingget --metadata, which already prints the same JSON. I kept it for vsce parity and made the content identical rather than subtly different, but if you would rather there were only one way to get it, it is easy to drop.🤖 Generated with Claude Code