Improve parameter display in VS Code extension - #17881
Conversation
Show parameter values in the resource tree consistently with the dashboard: mask secrets with a fixed 8-bullet string, truncate long non-secret values, and surface a "Value missing" state. Preserve AppHost command registration order in the CLI include-disabled stream so set-parameter shows before delete-parameter. Fixes microsoft#17193
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 17881Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 17881" |
Mirror the tree-view parameter improvements on the inline apphost CodeLens: - ValueMissing renders as "Value missing" with a warning icon. - Add a value lens showing the parameter value (secrets masked, long values truncated to 80 chars) inline next to the state. - Reuse the exported getParameterValueDescription so masking/truncation matches the tree and dashboard. - Preserve command registration order in the tree Commands node (set-parameter before delete-parameter) to match the dashboard.
…text The CodeLens tooltip was using the raw state string (e.g. 'ValueMissing') while the title already used the localized label. Export getResourceStateDescription and use it in the tooltip so it shows 'Value missing' consistently. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates command ordering semantics between API/UI backchannel streams and improves parameter UX in the VS Code extension (new ValueMissing state + inline parameter value display with masking/truncation).
Changes:
- Preserve AppHost registration order for commands when
includeDisabledCommands: true, while keeping default/API stream alphabetically sorted. - Add
ValueMissingresource state support and humanized display text in tree/tooltips/code lenses. - Display Parameter values inline (tree + code lens), masking secrets and truncating long values.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Aspire.Cli.Tests/Backchannel/ResourceSnapshotMapperTests.cs | Updates/extends tests to validate different command ordering contracts per stream. |
| src/Aspire.Cli/Backchannel/ResourceSnapshotMapper.cs | Implements conditional command ordering depending on includeDisabledCommands. |
| extension/src/views/AspireAppHostTreeProvider.ts | Humanizes ValueMissing, shows/masks/truncates parameter values, and preserves command order in the tree. |
| extension/src/editor/AspireCodeLensProvider.ts | Adds ValueMissing label and an additional parameter value code lens. |
| extension/src/editor/resourceConstants.ts | Adds ValueMissing state and constants for parameter command/property names. |
| extension/src/loc/strings.ts | Adds localized strings for “Value missing” (tree) and code lens label (with warning icon). |
| extension/package.nls.json | Adds NLS entry for parameterValueMissing. |
| extension/loc/xlf/aspire-vscode.xlf | Adds XLF entry for parameterValueMissing. |
| extension/src/test/codeLens.test.ts | Adds unit coverage for ValueMissing code lens label. |
| extension/src/test/aspireCodeLensProvider.test.ts | Adds coverage for parameter value code lens behavior (mask/truncate/missing). |
| extension/src/test/appHostTreeView.test.ts | Adds coverage for warning icon, parameter description behavior, tooltip state, and command ordering in quick pick. |
Move getResourceStateDescription/getParameterValueDescription and the mask/truncation constants out of the AspireAppHostTreeProvider view module into a new extension/src/utils/resourceDisplay.ts, so AspireCodeLensProvider no longer depends on the view layer. Pure move; no behavior change.
|
Re-running the failed jobs in the CI workflow for this pull request because 1 job was identified as retry-safe transient failures in the CI run attempt.
|
|
Tested with the PR CLI artifact |
Adam Ratzman (adamint)
left a comment
There was a problem hiding this comment.
Reviewed with four dedicated passes. I did not find any new, non-duplicate high-confidence issues to raise.
Add a RegistrationOrder field to the command JSON contract carrying each command's AppHost registration index, and sort commands in the VS Code extension by (RegistrationOrder, Name). This makes the resource tree, command quick pick, and CodeLens follow the dashboard's registration ordering instead of the alphabetical JSON key order.
Consolidate the resource command comparator alongside the other resource display helpers and remove the standalone resourceCommands.ts module.
cad60c6
into
microsoft:main
| /// The zero-based index at which the command was registered. | ||
| /// </summary> | ||
| [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] | ||
| public int? RegistrationOrder { get; set; } |
There was a problem hiding this comment.
Rename this to SortOrder. That's the name we use in other places, e.g. urls
| .OrderBy(c => c.Name) | ||
| .Select((command, index) => (command, index)) | ||
| .Where(c => IsCommandVisibleForConsumer(c.command.Visibility, includeDisabledCommands) && IsCommandVisibleToConsumer(c.command.State, includeDisabledCommands)) | ||
| .OrderBy(c => c.command.Name) |
There was a problem hiding this comment.
This orderby seems redundent.
…der and remove redundant OrderBy Co-authored-by: JamesNK <303201+JamesNK@users.noreply.github.com>
|
This is pretty slick |
…ilename globs match nested paths Three automated PRs in the last 14d fired more tests than they needed to: #17672, #17263 [Automated] Update ATS API Surface Area #17534 Move repository skills to .agents They each touched only api/*.txt or only README/skill markdown, but were firing trigger_all or selective:integrations — burning the full ~25min CI critical path. Audit-replay over 161 merged PRs found them, plus a latent C# glob bug where bare-filename patterns silently ignored nested matches. Root causes — two independent bugs: 1. The category-trigger rescue logic in RescueCategoryTriggerFiles built a synthetic union of all category triggerPaths without consulting per-category excludePaths. An ignored file that textually matched some category's glob but was excluded from that category got rescued back to active and then hit fallback_unmatched (worse than staying ignored). For src/Aspire.Hosting.Foundry/api/*.txt this meant ATS-only PRs fired integrations even after `**/api/*.txt` was added to ignorePaths. 2. The four glob analyzers (CriticalFileDetector, IgnorePathFilter, CategoryMapper.CompiledCategory, ProjectMappingResolver.CompiledMapping) handed user-facing patterns directly to FileSystemGlobbing.Matcher. The Matcher anchors bare-filename patterns at the repo root, so `Directory.Build.props` matched only the root file, not `src/Directory.Build.props` or `tests/Directory.Build.props`. The Python audit-replay evaluator (eval_rules.py) already documented and applied a "prepend **/ to bare-filename patterns" rule; the C# analyzers did not, so the two evaluators silently disagreed on ~5 patterns across ignorePaths, triggerAllPaths, and sourceToTestMappings. The fix: - Rescue now passes config.Categories directly to CategoryMapper so CompiledCategory.Matches honors per-category excludes. A file is rescued only when at least one category would actually fire on it. - New PatternNormalization.NormalizeGlob prepends `**/` to any pattern without a path separator. Every glob entry point applies it: the four analyzers above plus ProjectMappingResolver's regex compiler. - Rules: integrations.excludePaths gains `tests/Aspire.Acquisition.Tests/**`, `tests/Infrastructure.Tests/**`, `**/*.md`, `**/api/*.txt`. The same `**/api/*.txt` exclude is added to every category so an ignored ATS file can't be rescued back by any category. ignorePaths gains `**/api/*.txt`. Acquisition mapping's source list gains the missing self-mapping `tests/Aspire.Acquisition.Tests/**` (Templates and Infrastructure mappings already had this; Acquisition was an oversight exposed only after the new exclude was added). Verification: - Audit replay over 161 merged PRs: 4 outcomes change (#17263, #17534, #17549, #17672 all move to `skip`); zero regressions; zero fallback_unmatched. - New AuditFixtureTests xUnit [Theory] replays 28 hand-validated PRs against the live audit rules. Each row is a separate test, so any future rule edit that changes a row's outcome shows up as a visible CI failure. Coverage includes templates (#16447), CLI native build (#17567), extension multi-category (#17881/17698/17772), Hosting-core trigger_all (#17879), polyglot (#17948), and the regression canaries for previous fallback_unmatched cases. - Per-component regression tests pin both bugs: two new tests in EndToEndEvaluationTests for rescue+excludes; two more for bare-filename matching at nested paths. - Three pre-existing analyzer tests had asserted the buggy bare-filename behavior as expected (e.g. `*.md` not matching `docs/guide.md`). Updated with comments explaining the user-intent rule. - Full TestSelector namespace: 290 tests, all pass. No collateral damage on the wider Infrastructure.Tests suite (5 pre-existing baseline failures unchanged). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fixes #17193
Parameters now display consistently in both the resource tree and the inline apphost CodeLens, matching the Aspire dashboard.
What changed
raw
ValueMissingstate string.●●●●●●●●).ellipsis so a row stays readable.
set-parameternow appears beforedelete-parameterinstead of being alphabetically sorted.Before / after
ValueMissing●●●●●●●●the-value…(≤80 chars)