Add support for "launchUrl" in launchSettings.json#17634
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds support for a separate browser launch URL (launchUrl) in .NET launch profiles so the debugger can open a specific page instead of the base applicationUrl.
Changes:
- Extend
LaunchProfilewith optionallaunchUrl. - Use
launchUrl(fallback toapplicationUrl) when constructingserverReadyAction. - Update launch profile tests to validate parsing/propagation of
launchUrl.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| extension/src/test/launchProfiles.test.ts | Adds a launchUrl field to the test launchSettings input and asserts it is parsed into the profile. |
| extension/src/debugger/launchProfiles.ts | Extends the LaunchProfile interface with the new launchUrl property and documentation. |
| extension/src/debugger/languages/dotnet.ts | Prefer launchUrl over applicationUrl when configuring serverReadyAction browser opening behavior. |
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 17634Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 17634" |
|
@microsoft-github-policy-service agree |
adamint
left a comment
There was a problem hiding this comment.
Reviewed by three parallel agents focused on URL-parsing correctness, call-site fit, and contract/UX. The core direction is right and consistent with dotnet run / Visual Studio semantics, and the bug-fix path works for the issue's headline case (absolute launchUrl with wildcard applicationUrl). However, the unguarded new URL() introduces a real regression for some previously-tolerated applicationUrl values, the absolute-launchUrl override silently broadens the URI-scheme contract that VS Code's openExternally will hand to the OS, and the new tests don't actually exercise the bug from issue #17633 (a no-op implementation would pass them).
Blockers / High
new URL(launchUrl, uriFormat)throwsERR_INVALID_URLfor previously-toleratedapplicationUrlvalues, aborting the entire debug session. Pre-PR, an unparseable / empty / leading-semicolonapplicationUrlwas silently passed through asserverReadyAction.uriFormat— VS Code's serverReadyAction would just fail to open a browser. Post-PR, when both an invalidapplicationUrland alaunchUrlare present,new URL(...)throws. There is no try/catch; the exception propagates throughextension/src/debugger/languages/dotnet.ts:312→ the awaitedcreateDebugSessionConfigurationCallback→extension/src/dcp/AspireDcpServer.ts:173, which responds to DCP with HTTP 500 and the resource fails to start. Reproducible cases verified with Node 24:";https://localhost:5001"+"/abc"," "+"/abc","not-a-url"+"/abc"— all throw. Critically, an absolutelaunchUrldoes NOT save the user: WHATWGnew URL()requires the base to be parseable whenever a base is supplied, regardless of whether the first arg is absolute (new URL('https://abs.com', 'malformed')throws). (Inline comment.)
Medium
-
No scheme validation on
launchUrl— arbitrary URI schemes are passed tovscode.env.openExternal. BecauselaunchUrlis resolved withnew URL(launchUrl, base), any absolutelaunchUrlcompletely overrides the http(s) base and is returned verbatim. Verified:new URL('javascript:alert(1)', 'https://localhost:5001').href === 'javascript:alert(1)'; same fordata:,file:,vscode-insiders://command/.... The result becomesserverReadyAction.uriFormat, which VS Code feeds toopenExternalwhen the debug-console pattern matches. The extension's other URL-opening helpers (AspireAppHostTreeProvider.openInExternalBrowser,EndpointUrlItem) all funnel throughisLinkableUrlinextension/src/utils/urlSchemes.ts; the new launchUrl code path bypasses that.launchSettings.jsonis normally trusted, so this is not critical — but it silently broadens the contract from "open an http(s) URL" to "open any URI scheme" and is inconsistent with the rest of the extension. The regexpatternalready constrains matches tohttps?://, so non-http(s)uriFormatis semantically inconsistent with it anyway. (Inline comment.) -
The added tests do not actually exercise the bug from issue #17633. Both new tests use
applicationUrl = 'https://localhost:5001;http://localhost:5000'with launchUrls like'https://localhost:5001/some/path'or'/some/path'— i.e. thelaunchUrleither equals or resolves cleanly against the firstapplicationUrlentry. A no-op implementation that returnedapplicationUrl.split(';')[0]followed by simple concatenation would pass them. The issue's motivating scenario isapplicationUrl: "http://*:80/;https://*:443/"+launchUrl: "https://mywebsite.localhost"; verified that the fix works for it (new URL('https://mywebsite.localhost', 'http://*:80/').href === 'https://mywebsite.localhost/'), but that exact scenario should be pinned in a test so a future refactor (or finding #1's fallback logic) can't silently re-break it. (Inline comment on the absolute-launchUrl test.)
Low
-
Interface comment says "relative" but the implementation handles both.
// The relative URL to launch in the browser.is copied verbatim from the SchemaStore description, but the function explicitly accepts both relative and absolutelaunchUrlvalues — and the issue #17633 fix relies on the absolute case. A future maintainer reading only the interface comment would be justified in adding anif (looksRelative(launchUrl))guard and re-breaking the issue. One-line addition like// May be absolute (e.g. "https://my.localhost"); when relative, it is resolved against the first applicationUrl entry.resolves this without re-litigating the schemastore-wording debate. (Inline comment.) -
No call-site integration test in
extension/src/test/dotnetDebugger.test.ts. The existing integration test "applies launch profile settings to debug configuration" (~line 514) verifiesserverReadyAction.uriFormatis populated fromapplicationUrl, but no analogous assertion was added thatlaunchUrlfrom the profile is plumbed throughdotnet.ts:312intoserverReadyAction.uriFormat. The new unit tests exercise the helper in isolation; if a future refactor ofdotnet.tsdropsbaseProfile?.launchUrlfrom the third positional argument (easy mistake with three optional positional args), no test will fail. -
Test gap: relative
launchUrlwith the issue's wildcardapplicationUrl. The "relative launchUrl" test uses a "nice"https://localhost:5001base. For the issue's actual'http://*:80/'base + a relativelaunchUrl: '/dashboard', the implementation produceshttp://*/dashboard(port stripped because*parses as host and80is the default forhttp:). Verified with Node. That URL is not openable by any browser, so users hitting the issue with a relativelaunchUrlstill see broken behavior post-PR. The PR doesn't necessarily have to fix this case, but the test for "relative launchUrl" gives a false impression of completeness; either document the limitation or skip resolution when the base host is unresolvable.
Verified safe (not flagged)
- Parity with
dotnet run/ Visual Studio: usingapplicationUrl.split(';')[0]as the base matches bothdotnet runand Visual Studio's behavior of taking the first applicationUrl. Also matches Aspire's ownProjectResourceBuilderExtensionsabsolute-vs-relative semantics. - Apphost branch is correctly excluded: the dashboard URL is opened separately via
AspireDebugSession.openDashboardfrom DCP-reported URLs, not from launchSettings — so theif (!launchOptions.isApphost)guard is intentional andlaunchUrlon an AppHost's own launchSettings.json is silently ignored, consistent with that flow. - Only one production call site: confirmed by grep,
dotnet.ts:312is the sole non-test caller ofdetermineServerReadyAction. No other language debuggers (python.ts,go.ts,node.ts,azureFunctions.ts,browser.ts,cli.ts) referenceserverReadyAction,launchBrowser,applicationUrl, orlaunchUrl— serverReadyAction is .NET-specific in this codebase (relies on ASP.NET Core's\bNow listening on:log pattern), so issue #17633 has no cross-language analog. - No other consumer of
LaunchProfile.applicationUrlopens a browser at a wrong URL: the "Open in external/integrated browser" actions onEndpointUrlItemuse DCP-reported resource URLs, not launch-profileapplicationUrl. The user-visible bug from #17633 has no second surface that this PR silently leaves broken. - Signature change is wire-compatible: adding
launchUrl?: stringas a third optional positional parameter is non-breaking; existing callers type-check understrict: true. (Mild design smell — three optional positional args is awkward — but no concrete bug; an options-object refactor would be a reasonable future improvement.) yarn compile-testsandyarn lintpass cleanly with no warnings against PR changes.- WHATWG URL edge cases verified safe (with Node 24): query strings, fragments, protocol-relative URLs, IPv6 wildcard bases (
http://[::]:5000),0.0.0.0bases, whitespacelaunchUrl, and empty-stringlaunchUrlall behave as expected (no crash, sensible result). - Env-var expansion in
launchUrl: not expanded by VS ordotnet run; not expanding here is correct. - Aspire runtime port rebinding vs. static
uriFormat: pre-existing limitation, not made worse by this PR. - VS Code
serverReadyAction.uriFormat %ssubstitution: not used pre-PR either; behavior preserved. - Test wiring:
extension/.vscode-test.mjspicks up newsuite/testblocks inlaunchProfiles.test.ts; they will run in CI. - SchemaStore type fidelity:
launchUrlis"type": "string"with no further constraints; theLaunchProfile.launchUrl?: stringtyping is faithful.
|
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.
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ot/pr17634-review-fixes
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
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.
|
adamint
left a comment
There was a problem hiding this comment.
Final review: changes look good. CI is green after rerun.
|
Thanks @adamint! |
|
Hey @adamint, out of curiosity what did you use to generate the "Reviewed by three parallel agents focused on URL-parsing correctness, call-site fit, and contract/UX." comment above? It's much more detailed than what copilot does. |
Thanks for contributing! This will be available for use by Tuesday. |
It's largely relying on the code-review skill in our repo. For an automated review, I prompt copilot to review with a list of models and that skill and mention that particular attention should be paid to specific areas depending on the PR. It's a relatively new workflow for me and works really well! Certainly uses a lot of tokens though... |
Description
Use
launchUrlas the value set forserverReadyAction.uriFormatif it's set inlaunchSettings.jsonwhen launching the debugger.Fixes #17633
Checklist
<remarks />and<code />elements on your triple slash comments?