feat: Shell route templates with {param} path parameters#35110
feat: Shell route templates with {param} path parameters#35110mattleibow wants to merge 10 commits into
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 35110Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 35110" |
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 8 findings
See inline comments for details.
Add relationship table showing how this vision document, the Shell Routing Decomposition spec (the actionable engineering plan), and the Route Templates PR #35110 (working code) fit together. This doc = long-term north star #35348 = concrete API + phased delivery for .NET 11+ #35110 = Phase 2 implementation of route templates Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…tified Cross-references all 17 issues linked from Shell Routing Decomposition (#35348) against this vision doc, the Decomposition spec, and PR #35110. 12 fully covered, 5 with notes: - Navigation queueing (#11307, #17608): covered in Decomposition, added note that INavigationService should queue internally - Auto UI-thread dispatch (#27589): covered in Decomposition, adapters should auto-dispatch - Modal stack management (#12162): gap in both docs — needs explicit API design for PresentationOptions.Modal + ModalStack interaction - Shell Handlers for Android (#32985): prerequisite platform work, not in scope but noted as dependency Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ory fails before tests
When a test category fails because the build or deploy crashed before any
test could run (e.g. CS0246 missing namespace, RS0016 PublicAPI errors),
the AI summary table previously showed '0/1 ✓' — the green-checkmark
'all passed' branch — because no per-test failures were parsed. That's
visually misleading: the row is FAILED but the cell looks healthy.
Two fixes:
1. Tests column distinguishes 'category failed AND no per-test failures
parsed' from 'all tests passed':
- 'build/deploy failed' (no tests at all)
- '0/1 — build/deploy failed before per-test results' (some discovered)
2. New optional 'build_tail' field captures the last 30 lines of stdout
when a category fails with zero per-test failures. The Failed test
details collapsible section then renders it in a code block so
reviewers see the actual compiler error / build crash inline,
instead of having to download the full CopilotLogs artifact.
This was discovered while running the regression-check pipeline against
PRs #35110 (142 RS0016 PublicAPI errors), #35281 (CS0246 NSAttributedString
missing for catalyst), and #35358 — all reported as '0/1 ✓' before the fix.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 15 findings
See inline comments for details.
| @@ -1 +1,143 @@ | |||
| #nullable enable | |||
| #nullable enable | |||
| Microsoft.Maui.Navigation.FakeNavigationService | |||
There was a problem hiding this comment.
[error] Public API Surface — scope creep — This PR adds 142 public API entries under a brand-new Microsoft.Maui.Navigation namespace (INavigationService, INavigationGraphBuilder, ITabBuilder, INavigationAware, INavigationGuard, INavigationParameters/NavigationParameters, NavigationContext, NavigationResult/NavigationStatus, NavigatingEventArgs/NavigatedEventArgs/NavigationFailedEventArgs, PresentationOptions/Presentation/IPlatformOptions, RouteAttribute/RouteParameterAttribute, TabConfiguration, FakeNavigationService, NavigationMode). None of these types are referenced by any of the Shell route-template code, none are wired into MauiAppBuilder, and the only implementation is FakeNavigationService (a test-double). Per the project's API guidance ("new public APIs have clear use cases — no speculative additions" and "adding members to a public interface is a breaking change"), shipping these as public surface in the same PR as Shell route templates locks design choices that haven't been reviewed. Recommend: pull all src/Core/src/Navigation/*.cs and the corresponding PublicAPI.Unshipped.txt lines out of this PR and ship them as a separate, reviewable proposal — or mark them internal until a real consumer exists.
|
|
||
| List<string> matches = new List<string>(); | ||
| List<string> currentSet = new List<string>(_matchedSegments); | ||
| public string GetNextSegmentMatch(string matchMe, IDictionary<string, string> capturedParameters, RouteTemplate template) |
There was a problem hiding this comment.
[error] Code style / Build & MSBuild — Lines 192–368 (the entire new GetNextSegmentMatch(string, IDictionary, RouteTemplate) overload plus its single-arg helper at 194–197) are written at column 0 with no leading tabs, while the surrounding class members use one-tab indentation. This will fail dotnet format (and any whitespace check in CI) and makes the new logic visually unreadable next to the original. Re-indent the block (one extra tab inside the class, plus the usual nested levels). The closing brace at line 368 and the trailing blank line at 370 also need to align with the rest of the file.
| if (!seg.IsParameter) | ||
| continue; | ||
| if (pathParameters.TryGetValue(seg.Value, out var val)) | ||
| { |
There was a problem hiding this comment.
[error] Logic and Correctness / Code style — The if (pathParameters.TryGetValue(seg.Value, out var val)) block has broken indentation: the body opening brace at line 134 and the code at 135–138 sits at the wrong column relative to the if at line 133, and the closing } at 139 doesn't pair visually with anything. The logic appears correct (the braces do pair), but the misalignment looks like a missed merge fix-up and will fail formatting checks. Re-indent so the body is one level deeper than the if.
| @@ -369,10 +369,14 @@ async Task PrepareCurrentStackForBeingReplaced(ShellNavigationRequest request, S | |||
| // Routes match so don't do anything | |||
| if (navIndex < _navStack.Count && Routing.GetRoute(_navStack[navIndex]) == globalRoutes[i]) | |||
There was a problem hiding this comment.
[warning] Logic and Correctness / Regression Prevention — When the existing page in the stack matches the registered template key (e.g. both navigations target product/{sku}), the code calls Routing.SetResolvedRoute(...) but continue;s without re-running ApplyQueryAttributes on the reused page. That means navigating from product/apple → product/banana (or the same template with any new parameter values) reuses the same Page instance with stale [QueryProperty] / IQueryAttributable data — the user sees "apple" while the URL says "banana". The reused-page path further down (around line 421) does call ApplyQueryAttributes and SetResolvedRoute; this fast-path branch needs the same treatment. Add a ShellNavigationManager.ApplyQueryAttributes(_navStack[navIndex], queryData, isLast: i == globalRoutes.Count - 1, isRelativePopping) call before continue. Also add a regression test that navigates twice to the same template with different parameter values and asserts the page received the new sku.
| Assert.NotNull(page.ReceivedQuery); | ||
| Assert.True(page.ReceivedQuery.ContainsKey("sku")); | ||
| Assert.Equal("seed-tomato", page.ReceivedQuery["sku"]); | ||
| } |
There was a problem hiding this comment.
[warning] Regression Prevention — The test suite is thorough (1154 lines, 80+ cases) but is missing the single most likely regression scenario for templated routes: two consecutive navigations to the same template with different parameter values (e.g. await shell.GoToAsync("//main/products/product/apple") then await shell.GoToAsync("//main/products/product/banana"), asserting the page reflects sku="banana"). This is the path that hits the reuse branch in ShellSection.PrepareCurrentStackForBeingReplaced (line 370–377) where Routing.GetRoute(_navStack[navIndex]) == globalRoutes[i] is true for any sku. Without this test, the bug flagged above (stale ApplyQueryAttributes on reuse) regresses silently.
| return false; | ||
|
|
||
| return route.IndexOf("{", StringComparison.Ordinal) >= 0; | ||
| } |
There was a problem hiding this comment.
[warning] Public API / Edge case — ContainsTemplateSyntax returns true for any string containing {. There is no escape syntax ({{ / }}) for embedding a literal { in a route, so Routing.RegisterRoute("foo{bar", ...) will throw ArgumentException from Parse. That may be intentional ({ is reserved) but it isn't documented and there is no test for it. Either (a) document { and } as reserved characters in the XML doc summary on RegisterRoute/RouteTemplate, or (b) treat the segment as literal when IsValidParameterName fails on the inner token. Adding a RegisterRoute_LiteralBraceInRouteThrowsHelpfulError test would lock in whichever choice you pick.
| case "long": | ||
| return long.TryParse(value, out _); | ||
| case "double": | ||
| return double.TryParse(value, System.Globalization.NumberStyles.Any, |
There was a problem hiding this comment.
[suggestion] Logic and Correctness — int.TryParse(value, out _) and long.TryParse(value, out _) use the current culture and NumberStyles.Integer | AllowLeadingSign | AllowLeadingWhite | AllowTrailingWhite. Route values come from URIs, which should be culture-invariant and reject whitespace/sign tricks like " 5 " or "+5". Recommend int.TryParse(value, NumberStyles.Integer, CultureInfo.InvariantCulture, out _) (and NumberStyles.None if you want strict digits-only) for int, long. For bool and guid, the current methods are already culture-independent.
| else if (next != null && RouteTemplate.IsTemplateSegment(split)) | ||
| { | ||
| // Fallback for template segments without parsed RouteTemplate | ||
| var paramName = RouteTemplate.GetSegmentParameterName(split); |
There was a problem hiding this comment.
[suggestion] Complexity Reduction — The else if (next != null && RouteTemplate.IsTemplateSegment(split)) fallback branch (lines 319–331) tests whether the URI segment from the user's nav request (split) literally contains a {...} token. End-user nav URIs don't contain {sku} literals — they contain concrete values like seed-tomato. Confirm with a coverage run whether this branch is ever taken; if not, delete it. If it is taken (e.g. via some internal nav path that re-feeds template strings as URIs), document the scenario in a comment so future readers know.
| NavigationHistory.RemoveAt(NavigationHistory.Count - 1); | ||
| CurrentRoute = NavigationHistory.Count > 0 ? NavigationHistory[NavigationHistory.Count - 1] : ""; | ||
|
|
||
| Navigated?.Invoke(this, new NavigatedEventArgs(CurrentRoute, NavigationMode.Pop)); |
There was a problem hiding this comment.
[suggestion] Public API consistency — GoBackAsync raises Navigated but never raises Navigating (and never honours NavigatingEventArgs.Cancel). NavigateAsync raises both and supports cancel. Tests written against the fake will not catch a real-service bug where a back-navigation is supposed to be cancellable via Navigating.Cancel = true. Either raise Navigating here too (matching NavigateAsync), or document that the fake intentionally skips it.
|
|
||
| // Product routes: product/{sku} with review child that has | ||
| // a default stars value via {stars=5} | ||
| Routing.RegisterRoute("product/{sku}", typeof(ProductPage)); |
There was a problem hiding this comment.
[suggestion] PR scoping — The Sandbox sample is being repurposed into a route-template demo (product/{sku}, review/{stars=5}, order/{orderId:int}, plus 5 new *Page.xaml files and a rewritten MainPage). Sandbox is a shared developer test bed used by everyone working on MAUI; permanently dedicating it to one feature's demo is unusual. Consider either (a) moving these pages into a dedicated sample under src/Controls/samples/Controls.Sample or Controls.Sample.Maui so other developers' sandbox state isn't disrupted, or (b) confirming with the team that this repurposing is intentional. At minimum, ensure App.xaml.cs change at line 2 is intentional and not leftover scratch state.
This comment has been minimized.
This comment has been minimized.
73d24f8 to
a1d46af
Compare
|
/review |
…ory fails before tests
When a test category fails because the build or deploy crashed before any
test could run (e.g. CS0246 missing namespace, RS0016 PublicAPI errors),
the AI summary table previously showed '0/1 ✓' — the green-checkmark
'all passed' branch — because no per-test failures were parsed. That's
visually misleading: the row is FAILED but the cell looks healthy.
Two fixes:
1. Tests column distinguishes 'category failed AND no per-test failures
parsed' from 'all tests passed':
- 'build/deploy failed' (no tests at all)
- '0/1 — build/deploy failed before per-test results' (some discovered)
2. New optional 'build_tail' field captures the last 30 lines of stdout
when a category fails with zero per-test failures. The Failed test
details collapsible section then renders it in a code block so
reviewers see the actual compiler error / build crash inline,
instead of having to download the full CopilotLogs artifact.
This was discovered while running the regression-check pipeline against
PRs #35110 (142 RS0016 PublicAPI errors), #35281 (CS0246 NSAttributedString
missing for catalyst), and #35358 — all reported as '0/1 ✓' before the fix.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds docs/specs/shell-route-templates.md with the full design for additive
{param} path parameters in Shell routes (issue #35107 Proposal A), and a
standalone prototype at prototype/ShellRouteTemplates that demonstrates the
parser, matcher, and precedence rules independently of MAUI internals.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds support for route templates like 'product/{sku}' in Shell
navigation, allowing path parameters to be extracted from URIs and
delivered to pages via [QueryProperty] / IQueryAttributable.
Key changes:
- RouteTemplate.cs: template parser and segment helpers
- Routing.cs: detect and store template routes alongside literal routes
- RouteRequestBuilder.cs: template-aware segment matching with parameter capture
- ShellUriHandler.cs: two-pass matching (literal-first precedence),
template-aware CollapsePath
- RequestDefinition.cs: exposes PathParameters from winning route
- ShellNavigationManager.cs: seeds path params before query string params
All 5551 existing unit tests pass unchanged. 8 new tests added.
Ref: #35107
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…oss, add 9 new tests
Review findings addressed:
- Reject {sku?} optional and {*path} catch-all syntax at registration
time (not yet implemented, was silently accepted)
- Reject duplicate parameters in templates ({id}/{id})
- Fix path parameter loss in SearchForGlobalRoutes code path
- Fix path parameter loss in GenerateRoutePaths tree+global merge
- Fix GlobalRouteItem AddMatch missing _resolvedGlobalRoutes entry
- Fix IndexOutOfRange in RequestDefinition.MakeUriString
- Add ShellSection.GetOrCreateFromRoute resolved-route override for
template routes (CurrentState.Location fix)
9 new tests (17 total):
- CurrentState.Location shows resolved values, not template tokens
- Relative navigation limitation documented
- URL-encoded path parameters decoded correctly
- Second navigation with different value
- Reject optional/catch-all/duplicate template syntax
- IQueryAttributable receives path parameters
- All-template route ambiguity documented
All 5560 existing + new tests pass, 0 regressions.
Sandbox demo added with ProductPage, ReviewPage, and navigation buttons.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…t values
Full route template feature set now implemented:
- {sku?} Optional parameters (match zero or one segment)
- {*path} Catch-all (captures all remaining segments, must be last)
- {id:int} Constraints (int, long, double, bool, guid, alpha)
- {stars=5} Default values (provided when segment absent)
- product-{sku} Mixed segments (literal prefix/suffix around parameter)
- {id:int=1} Constraint + default combined
Sandbox demo updated:
- Products tab: catalog with product/{sku} route
- Orders tab: order list with order/{orderId:int} constrained route
- Product review: review/{stars=5} with default star rating
- Multi-step navigation: product → review with inherited parameters
48 template tests (31 new), 5591 total tests pass, 0 regressions.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
67 template tests now cover all features in various combinations:
- Two params in single route ({cat}/{id})
- Required + optional combos
- Optional + constraint ({id:int?})
- Optional with query string fallback
- Default value + query string interaction (default wins)
- Default value + child page inheritance
- Catch-all with URL encoding
- Catch-all with empty remaining segments
- Mixed segment + constraint (item-{id:int})
- Mixed segment prefix mismatch rejection
- Constraint + literal precedence
- Two different templates in same navigation
- Template + literal route together
- Unregister template route lifecycle
- Constraint unit tests: bool, long, double, guid-reject
5610 total tests pass, 0 regressions.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Bugs fixed (from Opus 4.7 + GPT 5.4 reviews):
- BLOCKING: {name:int?=5} produced default '5?' instead of '5' due to
appending '?' to inner string during constraint parsing. Fixed by using
a separate boolean flag.
- Optional params in middle of route (e.g. a/{b?}/c) now rejected at
registration time (matches ASP.NET Core behavior — unmatchable).
- Default values now validated against constraint at registration time
(e.g. {id:int=hello} is now rejected).
- Multiple brace pairs in mixed segments (e.g. item-{x}-{y}) now rejected.
- IsTemplateSegment now requires { before } (rejects 'foo}bar{baz').
- Sandbox demo fixed to use absolute URIs (relative template nav unsupported).
7 new tests (74 total):
- Parse_ConstraintOptionalAndDefault_Combo — verifies {num:int?=5} combo
- RegisterRoute_RejectsDefaultThatViolatesConstraint
- RegisterRoute_RejectsOptionalInMiddle
- RegisterRoute_RejectsMultipleParamsInMixedSegment
- Parse_MalformedBraces_Rejected
- Parse_EmptyParameterName_Rejected
- Parse_ConstraintWithNoName_Rejected
5617 total tests pass, 0 regressions.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…Uri tokens
Critical fixes from Opus 4.7 + GPT 5.4 round 3:
1. Template lookup bypass (Opus blocking): When CollapsePath stripped
prefix segments, GetNextSegmentMatch looked up the template by the
collapsed key (e.g. '{id:int}') instead of the registered key
('orders/{id:int}'), so constraints/defaults/catch-all were silently
bypassed. Fixed by passing RouteTemplate directly from the caller.
2. SetRoute mutation (Opus blocking, GPT medium): Mutating Page.Route
from template key to resolved value broke stack-reuse comparisons
(Routing.GetRoute(page) == globalRoutes[i] always false), causing
page recreation on re-navigation. Fixed with separate
Routing.ResolvedRouteProperty — Route keeps the template key,
ResolvedRoute stores the resolved URI for CurrentState.Location.
3. RequestDefinition.FullUri (Opus warning): Built from unresolved
template tokens ('{sku}'), causing ShellNavigationSource
misclassification. Fixed to use ResolvedGlobalRoutes when available.
4. Back navigation: Updated .. path and GetNavigationState to use
ResolvedRoute ?? Route for correct URI reconstruction.
3 new tests (77 total):
- TemplateRoute_RenavigationPreservesPageInstance
- Constraint_EnforcedWhenShellContentMatchesPrefix
- Constraint_AcceptedWhenShellContentMatchesPrefix
5620 total tests pass, 0 regressions.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…te params, FullUri
Fixes from Opus 4.7 + GPT 5.4 round 4:
1. IL2111 trimmer error (Opus blocking): ResolvedRouteProperty field
initializer caused trimmer warning→error. Fixed by wrapping in
CreateResolvedRouteProperty() with [UnconditionalSuppressMessage].
2. Stale ResolvedRoute on reused pages (GPT high): When re-navigating
to same template route with different value, the reused page kept
the old ResolvedRoute. Fixed by updating ResolvedRoute in the
page-reuse check at PrepareCurrentStackForBeingReplaced.
3. Intermediate page parameter delivery (GPT high): Non-last pages
in navigation chain now receive route-prefixed path params
(e.g. 'product/{sku}.sku') for prefix-filtered ApplyQueryAttributes.
4. Path params overwriting caller params (Opus medium): Changed from
unconditional overwrite to 'only add if not present', matching
SetQueryStringParameters semantics.
5. RequestDefinition.FullUri (Opus medium): Only substitute resolved
route when the route is actually a template (IsTemplateRoute check),
preventing multi-segment literal route truncation.
6. ExpandOutGlobalRoutes cast (Opus medium): Replaced fragile
'as IDictionary' cast with MergePathParameters() helper.
4 new tests (81 total):
- IntermediatePage_ReceivesOwnPathParameter
- ReusedPage_ResolvedRouteUpdated
- OptionalParam_WithCollapsedPrefix_NavigationSucceeds
- DefaultParam_WithCollapsedPrefix_NavigationSucceeds
5624 total tests pass, 0 regressions.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ram precedence Fixes from Opus 4.7 + GPT 5.4 round 5: 1. Tautological tests fixed: IntermediatePage, DefaultWithChild, OptionalParam_WithCollapsedPrefix, DefaultParam_WithCollapsedPrefix all now assert meaningful conditions instead of always-true checks. 2. Modal-stack reuse path: ResolvedRoute is now also updated in the pop/reapply loop (ShellSection.cs line 417 path), not just the non-modal PrepareCurrentStackForBeingReplaced path. 3. Route-prefixed param seeding uses 'only add if not present' semantics, matching the same precedence rule as unprefixed params. 5624 total tests pass, 0 regressions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The Samples integration tests build with TreatWarningsAsErrors=true. Backing fields for QueryProperty properties in ProductPage, ReviewPage, and OrderDetailPage were non-nullable, causing CS8618 warnings→errors. Fixed by making all backing fields and properties nullable (string?). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
a1d46af to
1e9f8ef
Compare
…ory fails before tests
When a test category fails because the build or deploy crashed before any
test could run (e.g. CS0246 missing namespace, RS0016 PublicAPI errors),
the AI summary table previously showed '0/1 ✓' — the green-checkmark
'all passed' branch — because no per-test failures were parsed. That's
visually misleading: the row is FAILED but the cell looks healthy.
Two fixes:
1. Tests column distinguishes 'category failed AND no per-test failures
parsed' from 'all tests passed':
- 'build/deploy failed' (no tests at all)
- '0/1 — build/deploy failed before per-test results' (some discovered)
2. New optional 'build_tail' field captures the last 30 lines of stdout
when a category fails with zero per-test failures. The Failed test
details collapsible section then renders it in a code block so
reviewers see the actual compiler error / build crash inline,
instead of having to download the full CopilotLogs artifact.
This was discovered while running the regression-check pipeline against
PRs #35110 (142 RS0016 PublicAPI errors), #35281 (CS0246 NSAttributedString
missing for catalyst), and #35358 — all reported as '0/1 ✓' before the fix.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/review -b feature/regression-check |
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 2 findings
See inline comments for details.
| parameters[kvp.Key] = kvp.Value; | ||
| } | ||
|
|
||
| // Also seed route-prefixed keys so intermediate (non-last) pages |
There was a problem hiding this comment.
[major] Navigation & Shell — This assumes prefix-filtered keys will deliver path parameters to intermediate pages, but ApplyQueryAttributes only forwards filtered values for ShellContent/BaseShellItem or the last item. A non-last global-route Page created by ShellSection.GetOrCreateFromRoute (for example product/{sku} before review) is a plain Page, so its [QueryProperty]/IQueryAttributable values are skipped and ProductPage.Sku remains unset. Reapply query attributes directly to each global-route page using that route's own captures, or extend ApplyQueryAttributes to handle non-last Page instances.
| { | ||
| if (!seg.IsParameter) | ||
| continue; | ||
| if (pathParameters.TryGetValue(seg.Value, out var val)) |
There was a problem hiding this comment.
[major] Navigation & Shell — Route-prefixed keys are derived from the merged request-level PathParameters dictionary, which has already lost which route captured each value. With chained templates like a/{id} followed by b/{id}, the flat dictionary contains only one id value, so both a/{id}.id and b/{id}.id can be seeded with the innermost value. Keep captures per GlobalRouteMatch/resolved route, or emit prefixed keys when each route is matched, so each intermediate page receives its own route value.
|
/review -b feature/refactor-copilot-yml |
|
/review -b feature/enhanced-reviewer -p android |
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 2 findings
See inline comments for details.
| // s_routeTemplates so existing literal fast paths are unaffected. | ||
| if (RouteTemplate.ContainsTemplateSyntax(route)) | ||
| { | ||
| var template = RouteTemplate.Parse(route, out var error); |
There was a problem hiding this comment.
[major] Logic and Correctness — The template is parsed after s_routes[route] has already been updated, so an invalid template registration throws but still leaves the bad route in the global route table. Concrete scenario: RegisterRoute("shop/{id}/{id}", ...) throws for the duplicate parameter, but the literal shop/{id}/{id} route remains registered and can affect later matching/duplicate state. Parse/validate the template before mutating s_routes/s_routeTemplates, or roll back s_routes when parsing fails.
| if (capturedParameters != null) | ||
| { | ||
| foreach (var kvp in capturedParameters) | ||
| _pathParameters[kvp.Key] = kvp.Value; |
There was a problem hiding this comment.
[major] Navigation & Shell — Captured path parameters are flattened by parameter name, so chained template routes that reuse a common name corrupt each other. Concrete scenario: category/{id} followed by product/{id} captures books and 42, but this assignment overwrites the first id; later prefix seeding can deliver 42 to both the category and product pages. Keep captures per matched route (or include the route key/index in the key) before creating the unprefixed compatibility parameters.
MauiBot
left a comment
There was a problem hiding this comment.
AI Review Summary
@mattleibow — new AI review results are available based on this last commit:
1e9f8ef.
fix: CS8618 nullable warnings in Sandbox pages breaking CI Samples tests To request a fresh review after new comments or commits, comment/review rerun.
Review Sessions — click to expand
Gate — Test Before & After Fix
Gate Result: ✅ PASSED
Platform: ANDROID · Base: main · Merge base: e904e900
| Test | Without Fix (expect FAIL) | With Fix (expect PASS) |
|---|---|---|
🧪 ShellRouteTemplatesTests ShellRouteTemplatesTests |
🛠️ BUILD ERROR | ✅ PASS — 85s |
🔴 Without fix — 🧪 ShellRouteTemplatesTests: 🛠️ BUILD ERROR · 108s
Determining projects to restore...
Restored /home/vsts/work/1/s/src/TestUtils/src/TestUtils/TestUtils.csproj (in 1.98 sec).
Restored /home/vsts/work/1/s/src/Graphics/src/Graphics/Graphics.csproj (in 232 ms).
Restored /home/vsts/work/1/s/src/Essentials/src/Essentials.csproj (in 3.34 sec).
Restored /home/vsts/work/1/s/src/Core/src/Core.csproj (in 2.09 sec).
Restored /home/vsts/work/1/s/src/Core/maps/src/Maps.csproj (in 8.07 sec).
Restored /home/vsts/work/1/s/src/Controls/src/Xaml/Controls.Xaml.csproj (in 64 ms).
Restored /home/vsts/work/1/s/src/Controls/src/Core/Controls.Core.csproj (in 49 ms).
Restored /home/vsts/work/1/s/src/Controls/Maps/src/Controls.Maps.csproj (in 46 ms).
Restored /home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj (in 1.33 sec).
1 of 10 projects are up-to-date for restore.
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14306854
Graphics -> /home/vsts/work/1/s/artifacts/bin/Graphics/Debug/net10.0/Microsoft.Maui.Graphics.dll
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14306854
Essentials -> /home/vsts/work/1/s/artifacts/bin/Essentials/Debug/net10.0/Microsoft.Maui.Essentials.dll
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14306854
Core -> /home/vsts/work/1/s/artifacts/bin/Core/Debug/net10.0/Microsoft.Maui.dll
Controls.BindingSourceGen -> /home/vsts/work/1/s/artifacts/bin/Controls.BindingSourceGen/Debug/netstandard2.0/Microsoft.Maui.Controls.BindingSourceGen.dll
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14306854
Maps -> /home/vsts/work/1/s/artifacts/bin/Maps/Debug/net10.0/Microsoft.Maui.Maps.dll
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14306854
Controls.Core -> /home/vsts/work/1/s/artifacts/bin/Controls.Core/Debug/net10.0/Microsoft.Maui.Controls.dll
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14306854
Controls.Xaml -> /home/vsts/work/1/s/artifacts/bin/Controls.Xaml/Debug/net10.0/Microsoft.Maui.Controls.Xaml.dll
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14306854
Controls.Maps -> /home/vsts/work/1/s/artifacts/bin/Controls.Maps/Debug/net10.0/Microsoft.Maui.Controls.Maps.dll
TestUtils -> /home/vsts/work/1/s/artifacts/bin/TestUtils/Debug/netstandard2.0/Microsoft.Maui.TestUtils.dll
/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/ShellRouteTemplatesTests.cs(29,24): error CS0117: 'Routing' does not contain a definition for 'IsTemplateRoute' [/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]
/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/ShellRouteTemplatesTests.cs(30,25): error CS0117: 'Routing' does not contain a definition for 'IsTemplateRoute' [/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]
/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/ShellRouteTemplatesTests.cs(32,24): error CS0117: 'Routing' does not contain a definition for 'TryGetRouteTemplate' [/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]
/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/ShellRouteTemplatesTests.cs(129,25): error CS0117: 'Routing' does not contain a definition for 'IsTemplateRoute' [/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]
/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/ShellRouteTemplatesTests.cs(130,25): error CS0117: 'Routing' does not contain a definition for 'TryGetRouteTemplate' [/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]
/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/ShellRouteTemplatesTests.cs(137,24): error CS0117: 'Routing' does not contain a definition for 'IsTemplateRoute' [/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]
/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/ShellRouteTemplatesTests.cs(141,25): error CS0117: 'Routing' does not contain a definition for 'IsTemplateRoute' [/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]
/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/ShellRouteTemplatesTests.cs(211,24): error CS0117: 'Routing' does not contain a definition for 'IsTemplateRoute' [/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]
/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/ShellRouteTemplatesTests.cs(212,24): error CS0117: 'Routing' does not contain a definition for 'TryGetRouteTemplate' [/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]
/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/ShellRouteTemplatesTests.cs(220,24): error CS0117: 'Routing' does not contain a definition for 'IsTemplateRoute' [/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]
/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/ShellRouteTemplatesTests.cs(221,24): error CS0117: 'Routing' does not contain a definition for 'TryGetRouteTemplate' [/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]
/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/ShellRouteTemplatesTests.cs(951,24): error CS0117: 'Routing' does not contain a definition for 'IsTemplateRoute' [/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]
/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/ShellRouteTemplatesTests.cs(954,25): error CS0117: 'Routing' does not contain a definition for 'IsTemplateRoute' [/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]
🟢 With fix — 🧪 ShellRouteTemplatesTests: PASS ✅ · 85s
Determining projects to restore...
All projects are up-to-date for restore.
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14306854
Graphics -> /home/vsts/work/1/s/artifacts/bin/Graphics/Debug/net10.0/Microsoft.Maui.Graphics.dll
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14306854
Essentials -> /home/vsts/work/1/s/artifacts/bin/Essentials/Debug/net10.0/Microsoft.Maui.Essentials.dll
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14306854
Core -> /home/vsts/work/1/s/artifacts/bin/Core/Debug/net10.0/Microsoft.Maui.dll
Controls.BindingSourceGen -> /home/vsts/work/1/s/artifacts/bin/Controls.BindingSourceGen/Debug/netstandard2.0/Microsoft.Maui.Controls.BindingSourceGen.dll
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14306854
Maps -> /home/vsts/work/1/s/artifacts/bin/Maps/Debug/net10.0/Microsoft.Maui.Maps.dll
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14306854
Controls.Core -> /home/vsts/work/1/s/artifacts/bin/Controls.Core/Debug/net10.0/Microsoft.Maui.Controls.dll
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14306854
Controls.Xaml -> /home/vsts/work/1/s/artifacts/bin/Controls.Xaml/Debug/net10.0/Microsoft.Maui.Controls.Xaml.dll
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14306854
Controls.Maps -> /home/vsts/work/1/s/artifacts/bin/Controls.Maps/Debug/net10.0/Microsoft.Maui.Controls.Maps.dll
TestUtils -> /home/vsts/work/1/s/artifacts/bin/TestUtils/Debug/netstandard2.0/Microsoft.Maui.TestUtils.dll
Controls.Core.UnitTests -> /home/vsts/work/1/s/artifacts/bin/Controls.Core.UnitTests/Debug/net10.0/Microsoft.Maui.Controls.Core.UnitTests.dll
Test run for /home/vsts/work/1/s/artifacts/bin/Controls.Core.UnitTests/Debug/net10.0/Microsoft.Maui.Controls.Core.UnitTests.dll (.NETCoreApp,Version=v10.0)
VSTest version 18.0.1 (x64)
Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
[xUnit.net 00:00:00.00] xUnit.net VSTest Adapter v2.8.2+699d445a1a (64-bit .NET 10.0.0)
[xUnit.net 00:00:00.16] Discovering: Microsoft.Maui.Controls.Core.UnitTests
[xUnit.net 00:00:01.47] Discovered: Microsoft.Maui.Controls.Core.UnitTests
[xUnit.net 00:00:01.48] Starting: Microsoft.Maui.Controls.Core.UnitTests
Passed OptionalWithConstraint [100 ms]
Passed OptionalParam_WithCollapsedPrefix_NavigationSucceeds [2 ms]
Passed SatisfiesConstraint_Alpha [1 ms]
Passed Parse_MixedSegmentWithSuffix [< 1 ms]
Passed CatchAll_UrlEncodedSegments [2 ms]
Passed ConstraintWithDefault_AbsentUsesDefault [1 ms]
Passed RegisterRouteTemplate_DetectedAsTemplate [< 1 ms]
Passed MultipleRequiredParamsInChain [2 ms]
Passed Parse_EmptyParameterName_Rejected [< 1 ms]
Passed Parse_CatchAllParameter [< 1 ms]
Passed Parse_ConstraintOptionalAndDefault_Combo [< 1 ms]
Passed OptionalParameter_PresentInUri_DeliveredToPage [1 ms]
Passed Constraint_Alpha_MatchesAlphaOnly [1 ms]
Passed TemplateOnlyRoute_AmbiguousRouteIsDocumentedLimitation [4 ms]
Passed PathParameter_DeliveredViaIQueryAttributable [8 ms]
Passed Constraint_AcceptedWhenShellContentMatchesPrefix [1 ms]
Passed UrlEncodedPathParameter_IsDecoded [3 ms]
Passed OptionalWithQueryStringFallback [5 ms]
Passed RegisterRoute_AcceptsOptionalTemplateSyntax [< 1 ms]
Passed Parse_OptionalParameter [< 1 ms]
Passed Constraint_Int_MatchesNumericValue [1 ms]
Passed CatchAll_CapturesAllRemainingSegments [1 ms]
Passed DefaultValue_AbsentInUri_DefaultDelivered [1 ms]
Passed SatisfiesConstraint_Long [< 1 ms]
Passed Parse_MalformedBraces_Rejected [4 ms]
Passed OptionalParameter_AbsentInUri_NavigationSucceeds [1 ms]
Passed Parse_DefaultValueParameter [2 ms]
Passed SatisfiesConstraint_Int [2 ms]
Passed OptionalWithRequired_MiddleOptionalRejected [1 ms]
Passed SatisfiesConstraint_Bool [< 1 ms]
Passed Constraint_Int_RejectsNonNumeric [3 ms]
Passed Constraint_EnforcedWhenShellContentMatchesPrefix [1 ms]
Passed MultiSegmentTemplate_ChildInheritsParentPathParameter [1 ms]
Passed Constraint_Guid_MatchesValidGuid [1 ms]
Passed DefaultWithQueryStringInteraction [18 ms]
Passed MixedSegment_SuffixAndParameter [2 ms]
Passed TwoParamsInSingleRoute [4 ms]
Passed PathParameterOverridesQueryStringWithSameName [1 ms]
Passed CatchAll_EmptyRemainingSegments [2 ms]
Passed CurrentStateLocation_ShowsResolvedValues [1 ms]
Passed IntermediatePage_ReceivesOwnPathParameter [1 ms]
Passed SatisfiesConstraint_Double [10 ms]
Passed OptionalWithRequired_OptionalAtEnd [1 ms]
Passed MixedSegmentWithConstraint [1 ms]
Passed Parse_ConstraintWithNoName_Rejected [< 1 ms]
Passed DefaultParam_WithCollapsedPrefix_NavigationSucceeds [1 ms]
Passed DefaultWithChildPageInheritance [1 ms]
Passed LiteralRouteUnchanged_RegistersAsLiteral [3 ms]
Passed Routing_Clear_AlsoClearsTemplates [< 1 ms]
Passed RegisterRoute_AcceptsCatchAllTemplateSyntax [< 1 ms]
Passed Constraint_UnknownType_RejectedAtRegistration [< 1 ms]
Passed ConstraintWithLiteralPrecedence [2 ms]
Passed UnregisterTemplateRoute_NoLongerDetected [< 1 ms]
Passed PathParameter_MixedWithUnrelatedQueryString [1 ms]
Passed LiteralRouteWinsOverTemplateRoute [4 ms]
Passed Parse_UnknownConstraint_Rejected [< 1 ms]
Passed RegisterRoute_RejectsDuplicateParameters [< 1 ms]
Passed SatisfiesConstraint_GuidRejectsInvalid [< 1 ms]
Passed Parse_ConstraintAndDefault [< 1 ms]
Passed Parse_ConstrainedParameter [< 1 ms]
Passed SatisfiesConstraint_Guid [< 1 ms]
Passed RegisterRoute_RejectsDefaultThatViolatesConstraint [< 1 ms]
Passed SinglePathParameter_DeliveredViaQueryProperty [1 ms]
Passed MixedSegment_PrefixAndParameter [1 ms]
Passed MixedSegment_PrefixMismatchRejects [2 ms]
Passed SecondNavigation_DifferentValue_DeliveredCorrectly [2 ms]
Passed ConstraintWithDefault_PresentUsesValue [1 ms]
Passed Constraint_Alpha_RejectsNumeric [1 ms]
Passed TemplateRoute_RenavigationPreservesPageInstance [1 ms]
Passed CatchAll_MustBeLastSegment [< 1 ms]
Passed DefaultValue_PresentInUri_OverridesDefault [1 ms]
Passed RegisterRoute_RejectsOptionalInMiddle [< 1 ms]
[xUnit.net 00:00:01.79] Finished: Microsoft.Maui.Controls.Core.UnitTests
Passed RelativeNavigation_WithTemplateRoute [4 ms]
Passed Parse_CatchAllNotLast_Rejected [< 1 ms]
Passed MixedSegment_PrefixSuffixAndParameter [1 ms]
Passed Parse_MixedSegment [< 1 ms]
Passed TemplateAndLiteralRouteTogether [1 ms]
Passed ReusedPage_ResolvedRouteUpdated [1 ms]
Passed CatchAll_SingleSegment [1 ms]
Passed RegisterRoute_RejectsMultipleParamsInMixedSegment [< 1 ms]
Passed TwoDifferentTemplatesSameNavigation [1 ms]
Test Run Successful.
Total tests: 81
Passed: 81
Total time: 2.3669 Seconds
⚠️ Failure Details
- 🛠️ ShellRouteTemplatesTests without fix: build failed before tests could run
/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/ShellRouteTemplatesTests.cs(29,24): error CS0117: 'Routing' does not contain a definition for 'IsTemplateRoute' [/home/vsts/work/1/s/src/Controls/...
📁 Fix files reverted (12 files)
eng/pipelines/ci-copilot.ymlsrc/Controls/samples/Controls.Sample.Sandbox/App.xaml.cssrc/Controls/samples/Controls.Sample.Sandbox/MainPage.xamlsrc/Controls/samples/Controls.Sample.Sandbox/MainPage.xaml.cssrc/Controls/samples/Controls.Sample.Sandbox/SandboxShell.xamlsrc/Controls/samples/Controls.Sample.Sandbox/SandboxShell.xaml.cssrc/Controls/src/Core/Routing.cssrc/Controls/src/Core/Shell/RequestDefinition.cssrc/Controls/src/Core/Shell/RouteRequestBuilder.cssrc/Controls/src/Core/Shell/ShellNavigationManager.cssrc/Controls/src/Core/Shell/ShellSection.cssrc/Controls/src/Core/Shell/ShellUriHandler.cs
New files (not reverted):
src/Controls/samples/Controls.Sample.Sandbox/OrderDetailPage.xamlsrc/Controls/samples/Controls.Sample.Sandbox/OrderDetailPage.xaml.cssrc/Controls/samples/Controls.Sample.Sandbox/OrdersPage.xamlsrc/Controls/samples/Controls.Sample.Sandbox/OrdersPage.xaml.cssrc/Controls/samples/Controls.Sample.Sandbox/ProductPage.xamlsrc/Controls/samples/Controls.Sample.Sandbox/ProductPage.xaml.cssrc/Controls/samples/Controls.Sample.Sandbox/ReviewPage.xamlsrc/Controls/samples/Controls.Sample.Sandbox/ReviewPage.xaml.cssrc/Controls/src/Core/Shell/RouteTemplate.cs
UI Tests — Navigation,Shell
Detected UI test categories: Navigation,Shell
✅ Deep UI tests — 395 passed, 0 failed across 2 categories on platform-pool agent (replaces in-process counts above).
🧪 UI Test Execution Results (deep, platform pool)
| Category | Tests | Snapshot diffs |
|---|---|---|
Navigation |
88/89 ✓ | — |
Shell |
307/307 ✓ | — |
📎 Download drop-deep-uitests artifact (TRX + snapshot diffs) |
Pre-Flight — Context & Validation
Issue: #35107 - Shell route templates with {param} path parameters
PR: #35110 - feat: Shell route templates with {param} path parameters
Platforms Affected: All Shell platforms; testing target: android
Files Changed: 7 implementation, 1 unit test, 1 spec doc, 11 Sandbox sample files
Key Findings
- PR adds Shell route-template parsing/matching for required, optional/default, catch-all, constrained, and mixed path parameters.
- Gate result was provided as passed: tests fail without the PR fix and pass with it. Gate was not re-run.
- Public PR review comments identify route-capture delivery risks for reused templated pages, intermediate pages, and repeated parameter names across chained templates.
- GitHub CLI is unauthenticated in this environment, so PR context was fetched via public GitHub API and local checked-out sources.
Code Review Summary
Verdict: NEEDS_DISCUSSION
Confidence: medium
Errors: 0 | Warnings: 7 | Suggestions: 2
Key code review findings:
⚠️ Spec advertises relative route-template navigation even though the current test suite marks it as a known v1 limitation that throws.⚠️ Spec non-goals list says optional/default, catch-all, constraints, and mixed segments are out of scope even though the PR implements them.⚠️ RouteRequestBuilder.GetNextSegmentMatchtemplate-aware overloads are badly indented and hard to review.⚠️ Reused templated pages can keep stale[QueryProperty]/IQueryAttributablevalues unlessApplyQueryAttributesruns before the reusecontinue.⚠️ Flat request-levelPathParametersloses per-route ownership when chained templates reuse parameter names.⚠️ Intermediate global-routePageparameter delivery is under-verified and may be timing-dependent.⚠️ Numeric and alpha constraints should be culture/URI oriented rather than current-culture and broad Unicode letter matching.- 💡
IntermediatePage_ReceivesOwnPathParameterdoes not assert the intermediate page actually receivessku. - 💡
ShellUriHandler.ExpandOutGlobalRoutesincludes a truncated stale comment.
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #35110 | Add RouteTemplate, literal-first/template-second matching, flat path-parameter seeding, and ResolvedRoute tracking. |
✅ PASSED (Gate) | Routing.cs, RouteTemplate.cs, Shell routing files, tests |
Original PR |
Code Review — Deep Analysis
Code Review — PR #35110
Independent Assessment
What this changes: Introduces {param}-style path parameters for Shell route registrations. A new RouteTemplate class parses and stores template metadata alongside literal routes. The URI matcher uses a two-pass strategy (literals first, templates second) to preserve literal-route priority. Captured values are propagated through ResolvedGlobalRoutes/PathParameters on RouteRequestBuilder and RequestDefinition, seeded into ShellRouteParameters before query-string parameters, and stored as an attached ResolvedRouteProperty on pages for accurate CurrentState.Location construction.
Inferred motivation: Today, Shell parameters travel as query-string entries (?sku=value), which prevents clean deep-linking and multi-page stack pushes with per-page parameters. Path params (product/{sku}) eliminate this gap, matching ASP.NET Core/Blazor conventions.
Reconciliation with PR Narrative
Author claims: Additive opt-in feature. Literal routes unchanged. Supports required, optional, catch-all, constrained, and mixed segments. Path params win over query string. CurrentState.Location shows resolved values.
Agreement: Assessment matches on core claims. Primary divergences are documentation/spec inconsistencies and unresolved behavior for relative navigation and intermediate-page parameter delivery.
Findings
⚠️ Warning — Spec says relative navigation works; implementation throws
docs/specs/shell-route-templates.md §3.3 shows relative navigation with template routes as supported, but ShellRouteTemplatesTests.RelativeNavigation_WithTemplateRoute expects ArgumentException and documents it as a known v1 limitation.
⚠️ Warning — Spec "Non-goals" list is entirely implemented
docs/specs/shell-route-templates.md §1 lists optional parameters, catch-all parameters, constraints, and mixed literal+parameter segments as out of scope, while the implementation and tests support all four.
⚠️ Warning — GetNextSegmentMatch overloads have broken indentation
src/Controls/src/Core/Shell/RouteRequestBuilder.cs around lines 192-370 has new overloads at column 0, unlike surrounding class members. This compiles but is difficult to review and will likely fail formatting.
💡 Suggestion — IntermediatePage_ReceivesOwnPathParameter test does not assert the key claim
The test asserts the intermediate page exists but avoids asserting it received the expected route parameter, leaving intermediate-page delivery under-verified.
💡 Suggestion — ExpandOutGlobalRoutes has a cut-off comment
ShellUriHandler.cs contains a truncated comment around the path-parameter forwarding logic.
Additional public review context
Prior automated review comments on the public PR flagged stronger Shell correctness concerns that are relevant to alternative fixes:
- Reusing an existing templated page in
ShellSection.PrepareCurrentStackForBeingReplacedupdatesResolvedRoutebut skipsApplyQueryAttributes, so navigatingproduct/appletoproduct/bananacan leave stale page state. - Route-prefixed keys are derived from a flat
PathParametersdictionary, so chained templates with repeated parameter names (a/{id}followed byb/{id}) can deliver the innermost value to both pages. - Non-last global-route
Pageinstances may not receive path parameters because prefix filtering does not reliably apply to plain intermediate pages. RouteTemplate.SatisfiesConstraintuses culture-sensitiveint.TryParse/long.TryParseand broadchar.IsLetterforalpha.
Devil's Advocate
The core parser/matcher shape is additive and gate-passing, so broad rewrites carry risk. The most valuable alternatives should target demonstrable correctness holes: stale query attributes on page reuse, per-route capture scoping, and matching precedence/readability. Documentation-only fixes do not satisfy the alternative-fix objective.
Verdict: NEEDS_DISCUSSION
Confidence: medium
Summary: The implementation is promising and gate-passing, but there are unresolved route-capture delivery questions for reused/intermediate pages and repeated parameter names. Alternative candidates should explore smaller scoped capture fixes versus broader route-match representation changes.
Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix | Ordered per-route parameter bags plus scoped ShellSection delivery | ❌ FAILED | 6 files | Exposed deeper matcher bug: chained templates repeatedly matched the first template route. |
| 2 | try-fix | Literal-prefix template scoring plus capture-time route-prefixed parameter keys | ✅ PASSED | 4 files | Fixes matcher re-selection, repeated parameter names, and intermediate plain Page delivery with a smaller change. |
| PR | PR #35110 | Flat path-parameter dictionary seeded into navigation parameters plus template matching | ✅ PASSED (Gate) | Shell routing files + tests | Original PR; gate passed but route ownership issues remain. |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| claude-sonnet-4.6 | 1 | Yes | Per-route ordered bags with direct/scoped delivery. Failed because matching still selected already-consumed templates. |
| claude-sonnet-4.6 | 2 | Yes | Three-pass literal-prefix template scoring plus capture-time prefixed parameter keys. Passed targeted and broader Shell tests. |
| maui-expert-reviewer | 2 review | Yes | Confirmed try-fix-2 is meaningfully better; recommended scoping non-last delivery to Page and strengthening value assertions. Refinements applied and tests passed. |
Exhausted: No — stopped because Candidate #2 passed all executed regression tests and is demonstrably better than the PR's current fix for matcher specificity and repeated parameter-name delivery.
Selected Fix: Candidate #2 — smaller than try-fix-1, preserves the PR architecture, and passes Shell route-template plus broader Shell unit tests.
Report — Final Recommendation
Comparative Report — PR #35110
Candidates evaluated
| Candidate | Regression result | Strengths | Concerns | Rank |
|---|---|---|---|---|
pr-plus-reviewer |
✅ Passed targeted ShellRouteTemplates slice and broader Shell filter (385/385) |
Fixes the raw PR's invalid-template registration mutation, repeated parameter-name corruption, intermediate Page delivery, and template re-selection issue. Superset of the strongest successful try-fix behavior plus expert reviewer feedback. |
Sandbox candidate only; not applied to the PR branch. | 1 |
try-fix-2 |
✅ Passed ShellRouteTemplates (83/83) and broader Shell (384/384) in STEP 5a |
Small, focused fix for literal-prefix template scoring, route-prefixed captures at capture time, repeated parameter names, and intermediate Page delivery. |
Does not include the expert reviewer's invalid-template rollback fix. | 2 |
pr |
✅ Gate passed | Broad implementation for Shell route templates with substantial tests and resolved route tracking. | Expert review found two major correctness issues: invalid template registration can mutate global route state before throwing, and flat path-parameter storage corrupts repeated parameter names across chained templates. Prior review also flagged under-tested intermediate/reused page delivery. | 3 |
try-fix-1 |
❌ Failed ShellRouteTemplates (82/83) |
Explored a principled per-route ordered parameter bag design. | Failed the chained-template repeated-name regression because matching still repeatedly selected the first template route. Per instructions, failed regression candidates rank below passing candidates. | 4 |
Analysis
The raw PR (pr) passes the supplied gate, but the expert review identified correctness bugs that the gate did not cover. The most serious issue is that captured path values are flattened before route ownership is preserved, so chained templates like a/{id}/b/{id} can overwrite the earlier id and deliver the wrong value. The second expert finding is a registration atomicity issue: parsing invalid templates after s_routes mutation leaves bad global state after a thrown RegisterRoute.
try-fix-1 correctly recognized that route ownership matters, but it failed because it did not also fix the matcher's template re-selection behavior. It must rank below every passing candidate.
try-fix-2 is the strongest STEP 5a candidate. It preserves the PR's architecture while fixing matcher specificity and capture-time route scoping, and it passed both targeted and broader Shell tests. However, it predates the expert review's invalid-registration finding.
pr-plus-reviewer wins because it combines the successful try-fix-2 route-selection and capture-scoping changes with the expert reviewer's additional registration atomicity fix. It passed the broader Shell filter after adding focused sandbox regressions for both expert findings.
Winner
pr-plus-reviewer is the single best candidate. It is a PR-derived fix, it passed regression testing, and it covers all known major correctness issues identified by expert review and STEP 5a.
Future Action — review latest findings
No alternative fix was selected for this run. Review the session findings and CI results before merging.
kubaflo
left a comment
There was a problem hiding this comment.
Could you please check the ai's suggestions?
|
/review rerun |
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Description
Adds route template support to Shell navigation, enabling
{param}path parameters inspired by ASP.NET Core / Blazor routing. This is additive and non-breaking — existing literal routes work identically, and the feature is completely dormant unless you register a route containing{.Fixes #35107
How It Works
Registration
Register a route with
{param}segments. Everything inside braces becomes a path parameter:XAML works too —
{is safe because XAML only interprets markup extensions when{is the first character of the attribute value:Navigation
Navigate with values in the path instead of query strings:
Parameter Delivery
Uses existing
[QueryProperty]andIQueryAttributable— no new attributes or interfaces needed:Supported Syntax
{sku}product/{sku}matchesproduct/seed-tomato{sku?}product/{sku?}matchesproductorproduct/seed-tomato{stars=5}review/{stars=5}→ stars is "5" if you navigate to justreview{id:int}order/{id:int}rejectsorder/abc{*path}files/{*path}→docs/report.pdfproduct-{sku}product-seed-tomato→ sku = "seed-tomato"{id:int=1}Constraints:
int,long,double,bool,guid,alphaRoute Matching Precedence
Literals always win over templates (same as ASP.NET Core). Implemented via two-pass matching in
FindAndAddSegmentMatch.Path vs Query String
Path parameters take precedence over query strings with the same name. Both can coexist.
How the Implementation Works
Route Registration (
Routing.cs)When
RegisterRoutereceives a route containing{, it parses aRouteTemplateand stores it in a parallels_routeTemplatesdictionary alongside the existings_routesentry.Route Matching (
ShellUriHandler.cs,RouteRequestBuilder.cs)FindAndAddSegmentMatchuses a two-pass approach: pass 0 checks literal routes only (unchanged), pass 1 checks template routes.GetNextSegmentMatchwalks template segments in parallel with URI segments, checking type (literal, parameter, optional, catch-all, mixed) and applying constraints. TheRouteTemplateis passed directly from the caller to handleCollapsePathprefix stripping.Parameter Delivery (
ShellNavigationManager.cs)Path parameters are seeded into
ShellRouteParametersbeforeSetQueryStringParameters(path wins). Route-prefixed keys are also seeded for intermediate page delivery through Shell's prefix-basedApplyQueryAttributesfiltering.CurrentState.Location (
ShellSection.cs,ShellNavigationManager.cs)Resolved URIs are stored in a separate internal
ResolvedRoutePropertyon the page.GetNavigationStatereadsGetResolvedRoute(page) ?? GetRoute(page). The page'sRoutekeeps the template key for factory lookups and stack-reuse comparisons.Migration
None required. To opt in, change your route string to include
{param}. Everything else stays the same.Behavior Changes
None for existing apps. All new types are
internal. The two-pass matching is a no-op for routes without{.CollapsePathhas one new condition that only triggers for{-prefixed segments.Known Limitations