Move progress prompt title to options - #19382
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 19382Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 19382" |
There was a problem hiding this comment.
Pull request overview
Reorders progress-prompt parameters to consistently use nullable title before required message.
Changes:
- Updates C# and ATS progress-prompt APIs.
- Migrates C# and TypeScript callers and tests.
- Regenerates the TypeScript API snapshot.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
tests/Shared/TestInteractionService.cs |
Updates the test implementation signature. |
tests/PolyglotAppHosts/Aspire.Hosting/TypeScript/apphost.mts |
Migrates the TypeScript test AppHost. |
tests/Aspire.Hosting.Tests/InteractionServiceTests.cs |
Updates progress-prompt tests. |
tests/Aspire.Hosting.CodeGeneration.TypeScript.Tests/Snapshots/TwoPassScanningGeneratedAspire.verified.ts |
Updates generated TypeScript API output. |
src/Aspire.Hosting/InteractionService.cs |
Reorders implementation parameters. |
src/Aspire.Hosting/IInteractionService.cs |
Changes the public experimental API. |
src/Aspire.Hosting/Ats/InteractionExports.cs |
Reorders the exported ATS capability. |
src/Aspire.Hosting/ApplicationModel/ResourceCommandService.cs |
Migrates the internal caller. |
playground/TypeScriptAppHost/apphost.mts |
Updates the TypeScript sample. |
playground/Stress/Stress.AppHost/InteractionCommands.cs |
Updates C# stress scenarios. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Mitch Denny (mitchdenny)
left a comment
There was a problem hiding this comment.
Went through the full diff plus the ATS compat tooling. The change itself is correct and complete — I traced the suppression story through tools/TypeScriptApiCompat/AtsCompatibilityComparer.cs and it holds up: capability parameters are matched by name, so removing title produces exactly one capability-parameter-removed diagnostic, and the shared parameter order (message, options, cancellationToken) is unchanged so capability-parameter-order-changed correctly doesn't fire. The new Title is nullable, so it lands in ATS as optional and rightly needs no dto-property-added-required suppression, unlike the CommandOptions.Progress precedent a few lines up in the same file. Every call site is migrated and all five language snapshots are consistent.
Two test-coverage gaps though: the title now flows through hand-written mappings in two places that nothing asserts, so either could be dropped without turning CI red. Details inline. Third comment is a non-blocking API note.
The progress dialog title now reaches the interaction through two hand-written mappings that nothing asserted: CommandProgressOptions.Title -> ProgressInteractionOptions.Title in ResourceCommandService, and InteractionProgressOptions.Title -> ProgressInteractionOptions.Title in the ATS export. Either could be dropped without failing a test, silently losing the title for command progress dialogs and for every polyglot AppHost. Both tests were verified to fail when the corresponding mapping is removed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Mitch Denny (mitchdenny)
left a comment
There was a problem hiding this comment.
Approving. The API shape change is correct and complete — I traced the ATS compat story through tools/TypeScriptApiCompat/AtsCompatibilityComparer.cs and it holds up: capability parameters are matched by name, so removing title yields exactly one capability-parameter-removed diagnostic, and the shared parameter order (message, options, cancellationToken) is unchanged so capability-parameter-order-changed correctly does not fire. The new Title is nullable and therefore optional in ATS, so it rightly needs no dto-property-added-required suppression, unlike the CommandOptions.Progress precedent. The CP0006 target update is right and leaves no stale entry. Every call site is migrated and all five language snapshots are consistent.
The two test-coverage gaps I raised are addressed in 6b0afcb, which I pushed to this branch. Both new assertions were verified to fail when the corresponding title mapping is removed, so they are real regression tests rather than happy-path padding.
CI on the previous head was fully green; the run for the new commit is still in progress.
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
beb7e41
into
microsoft:main
|
/backport to release/13.5 |
|
Started backporting to |
Description
Makes progress prompt APIs consistent by keeping
messageas the only required positional argument and exposing nullabletitlethrough progress options. Native C# callers useProgressInteractionOptions.Title, while polyglot callers useInteractionProgressOptions.title.The C# API, ATS export, generated TypeScript, Python, Java, Go, and Rust APIs, tests, and samples now use this shape consistently.
User-facing usage
C# AppHost:
TypeScript AppHost:
Compatibility
PromptProgressAsyncand the generatedpromptProgresscapability are new and unshipped. The compatibility suppression declares the intentional pre-release removal of the positional polyglottitleparameter after it moved intoInteractionProgressOptions; no released callers are affected.Validation:
dotnet build src/Aspire.Hosting/Aspire.Hosting.csproj --no-restorePromptProgressAsync_*tests (7 passed)javacis not installeddotnet build playground/Stress/Stress.AppHost/Stress.AppHost.csproj --no-restoregit diff --checkFixes # (issue)
Checklist
<remarks />and<code />elements on your triple slash comments?