Sync Blazor templates with aspnetcore net11-pre4 - #35265
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 35265Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 35265" |
8a99d66 to
72b817b
Compare
Apply upstream changes from dotnet/aspnetcore BlazorWeb-CSharp template (release/10.0 → main/net11-pre4) to both maui-blazor and maui-blazor-solution templates: - ReconnectModal.razor: Resume button ordering + resume-failed class - ReconnectModal.razor.js: Show resume-failed state instead of reload - ReconnectModal.razor.css: Fix &[open] CSS nesting indentation - Web.Client Program.cs/Main.cs: Use AddAuthenticationStateDeserialization() - template.json: Add onlyIf localhost constraint on port replacers - template.json: Add *.razor.js to copyOnly for co-located JS files - index.html: Add SampleContent-guarded NavMenu.razor.js script refs Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
72b817b to
ce11f67
Compare
This comment has been minimized.
This comment has been minimized.
|
/review -b feature/refactor-copilot-yml |
This comment has been minimized.
This comment has been minimized.
AI code review for net11.0 targetVerdict: LGTM Template-only sync of the Blazor ( Key findings
CI note: Most legs green. The Confidence: High. This is an automated, non-approval review comment, not a GitHub approval/request-changes. A human must make the merge decision. |
kubaflo
left a comment
There was a problem hiding this comment.
Cross-model synthesis — dotnet/maui PR #35265 (Sync Blazor templates with aspnetcore net11-pre4)
Verdict: NEEDS_CHANGES (confidence: medium)
gpt-5.5's lone finding is CONFIRMED real, not a false positive. The added index.html module reference (maui-blazor/wwwroot/index.html:30 and maui-blazor-solution/MauiApp.1/wwwroot/index.html:25) is a deferred type="module" script that evaluates during the initial WebView page load; because MAUI Hybrid has no prerendering and uses autostart="false", the NavMenu has not been rendered into #app yet, so NavMenu.razor.js's one-shot getElementById("nav-scrollable") returns null, the if guard short-circuits (no null-ref, but no listener attaches), and it never re-runs — the mobile nav auto-collapse that net10's inline onclick provided is not restored in generated Hybrid apps. This is not mitigated by defer/DOMContentLoaded/event-delegation and does NOT match the upstream mechanism (which works only via SSR prerendering, absent in Hybrid); the pattern exists in no shipping branch (net10.0, net11.0 base, release/10.0.1xx, main). The other 8 changed lines (copyOnly **/*.razor.js, onlyIf localhost port guard, AddAuthenticationStateDeserialization(), ReconnectModal razor/css/js) are verified correct as the 3 other models found.
CI: maui-pr is RED — Build Analysis + RunOniOS_*Release/TrimFull(_CoreCLR) + AOT macOS legs fail; these exercise the plain MAUI template (not these Blazor files) and read as shared infra/flaky (Blazor/Build/Samples/MultiProject/WindowsTemplates legs pass). Verdict is driven by the confirmed finding, not CI.
Use document-level event delegation so the generated Hybrid templates attach the collapse behavior even when the module loads before Blazor renders the NavMenu. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 2 findings
See inline comments for details.
Use the existing WebAssembly package version token for the auth package so generated auth-enabled Web.Client projects can call AddAuthenticationStateDeserialization. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Only forward delegated nav clicks to the hidden checkbox when the mobile menu is already open, so desktop nav clicks do not leave the menu pre-opened after a later resize. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
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:
6a393d2. 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: ⚠️ SKIPPED
No tests were detected in this PR.
Recommendation: Add tests to verify the fix using the write-tests-agent.
📋 Pre-Flight — Context & Validation
Issue: #35265 - MAUI Blazor Hybrid NavMenu collapse timing
PR: #35265 - Fix MAUI Blazor Hybrid NavMenu collapse
Platforms Affected: Android (requested), plus all MAUI Blazor Hybrid hosts generated from these templates
Files Changed: 12 implementation/template, 0 test
Key Findings
- GitHub CLI is unauthenticated, so PR metadata/comments/checks were gathered from the local squashed PR commit and public patch content instead of authenticated GitHub APIs.
- The PR fix changes NavMenu collapse from one-shot element binding to guarded document-level delegation because Hybrid can load
NavMenu.razor.jsbefore Blazor renders the menu. - Gate result was already skipped because no tests were detected; no gate rerun was performed.
- Impacted UI categories: Template / Blazor Hybrid generated app navigation; no canonical UI test category was selected because this is a template JavaScript behavior change, not a Controls UITest category.
Code Review Summary
Verdict: NEEDS_DISCUSSION
Confidence: low
Errors: 0 | Warnings: 1 | Suggestions: 1
Key code review findings:
- ⚠ No regression coverage for generated Hybrid NavMenu collapse when the module loads before the rendered menu exists.
- ℹ The duplicate script-loading surfaces are safe due to the global guard but would benefit from clarification or simplification.
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #35265 | Load NavMenu module from Hybrid index.html and use guarded document-level delegated click handling that only toggles when the checkbox is checked. |
NavMenu.razor.js, index.html, template config and upstream sync files |
Original PR |
🔬 Code Review — Deep Analysis
Code Review — PR #35265
Independent Assessment
What this changes: The PR syncs MAUI Blazor templates with upstream ASP.NET Core template changes and fixes the Hybrid NavMenu collapse behavior by copying colocated .razor.js files, loading NavMenu.razor.js from Hybrid index.html, and changing the NavMenu script to a guarded document-level delegated click handler.
Inferred motivation: In MAUI Blazor Hybrid, blazor.webview.js is loaded with autostart="false" and #app initially contains Loading...; a module loaded from index.html can run before Blazor renders #nav-scrollable, so the previous one-shot element lookup attached no listener.
Reconciliation with PR Narrative
Author claims: Public patch commits describe syncing upstream Blazor template changes and fixing MAUI Blazor Hybrid NavMenu collapse by using document-level event delegation so the behavior attaches even when the module loads before Blazor renders the menu.
Agreement/disagreement: Local code agrees with that root cause. The final PR version also avoids opening the menu from desktop nav clicks by forwarding only when .navbar-toggler.checked is true.
Prior Review Reconciliation
| Prior ❌ Error Finding | Source | Status | Evidence |
|---|---|---|---|
NavMenu script can run before #nav-scrollable exists and fail to attach collapse behavior |
Public patch commit narrative / local expert review | ✅ Fixed | Both NavMenu.razor.js files now use document.addEventListener("click", ...) and resolve elements when clicks occur. |
| Auth-enabled Web.Client template used an incompatible hard-coded WebAssembly auth package version | Public patch commit narrative / local expert review | ✅ Fixed | MauiApp.1.Web.Client.csproj now uses MS_COMPONENTS_WEBASSEMBLY_VERSION. |
Blast Radius Assessment
- Runs for all instances: Yes, but only generated
maui-blazorandmaui-blazor-webprojects with sample content load the NavMenu script. - Startup impact: Yes, Hybrid
index.htmlnow imports the NavMenu module before Blazor renders app content. - Static/shared state: Yes,
globalThis.__mauiNavMenuCollapseInitializedsuppresses duplicate document handlers across duplicate module imports.
CI Status
- Required-check result: undetermined;
ghis unauthenticated in this environment. - Classification: undetermined.
- Action taken: capped confidence; gate was already skipped because no tests were detected.
Findings
⚠️ Warning — No regression coverage for the Hybrid NavMenu timing bug
The PR adds no test proving generated Hybrid apps collapse the mobile nav when NavMenu.razor.js loads before Blazor renders #nav-scrollable. This risk applies to src/Templates/src/templates/maui-blazor/Components/Layout/NavMenu.razor.js and src/Templates/src/templates/maui-blazor-solution/MauiApp.1.Shared/Layout/NavMenu.razor.js.
💡 Suggestion — Consider documenting or simplifying the two script-loading surfaces
Hybrid index.html loads the NavMenu script early, while the component also retains a script tag. The guarded global flag makes this safe, but future maintainers may need context on why both surfaces exist.
Failure-Mode Probing
- Module loads before NavMenu exists: current PR succeeds because it binds to
documentand resolves targets at click time. - Desktop nav click when checkbox is unchecked: current PR does not call
click()because it requires anHTMLInputElementwithchecked === true. - Duplicate imports from
index.htmland component script tag: current PR suppresses duplicate listeners with aglobalThisflag.
Verdict: NEEDS_DISCUSSION
Confidence: low (CI/check status unavailable and no regression test was added.)
Summary: The PR’s delegated handler is directionally sound for the Hybrid timing problem. The main remaining risk is lack of regression coverage for generated template behavior.
🛠️ Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix | Scoped late binding with requestAnimationFrame; wait for rendered NavMenu elements, then attach listener directly to #nav-scrollable. |
✅ PASS | 2 JS files | Avoids document-level delegation but adds a polling loop and depends on stable element identity. Not better than PR. |
| 2 | try-fix | MutationObserver late binding; observe until NavMenu appears, bind direct listener, disconnect. |
✅ PASS | 2 JS files | Avoids permanent document click listener but is more complex and depends on document.body. Not better than PR. |
| 3 | try-fix | Delegated direct state reset; keep timing-safe delegation but set navToggler.checked = false instead of calling click(). |
✅ PASS | 2 JS files | Plausible simplification that avoids synthetic click side effects, but assumes no future checkbox click/change listeners are needed. |
| PR | PR #35265 | Load NavMenu module from Hybrid index.html and use guarded document-level delegated click handling that only toggles when the checkbox is checked. |
12 template files | Original PR; simplest robust timing fix among scoped alternatives. |
Cross-Pollination
| Model/Reviewer | Round | New Ideas? | Details |
|---|---|---|---|
| maui-expert-reviewer | 1 | Yes | Suggested late startup hook, MutationObserver/init-on-render, Blazor-native collapse state, and direct checked = false reset as alternatives. |
| local test loop | 2 | No | After testing the three materially different viable strategies, remaining Blazor-native/component-script approaches were not pursued because they either reintroduce the original dynamically-rendered script timing risk or diverge substantially from upstream template structure. |
Exhausted: Yes
Selected Fix: PR #35265 — The tested alternatives pass the local timing simulation and template build, but none is demonstrably better overall. Candidate #3 is the strongest alternative if avoiding synthetic checkbox clicks is preferred, but the PR’s click() path better preserves checkbox event semantics if future template code observes those events.
Test Command Used
node <session>/files/navmenu-*.mjs <NavMenu.razor.js>
dotnet build src/Templates/src/Microsoft.Maui.Templates.csproj --no-restore --nologo --verbosity:minimal
Candidate #1 initially hit a missing restore asset, then passed after dotnet build src/Templates/src/Microsoft.Maui.Templates.csproj restored assets. Candidates #2 and #3 used the restored assets and passed with --no-restore.
Environment Notes
ghwas unauthenticated, so PR comments/checks could not be fetched via GitHub CLI.EstablishBrokenBaseline.ps1was blocked by pre-existing dirty files outside the PR/template files, so baseline setup was recorded as blocked and the loop continued with isolated patches against the local PR state.- Gate verification was not rerun, per instruction; gate was already skipped because no tests were detected.
📝 Recommended PR Title & Description
Assessment: ✏️ Recommend updating — the description is detailed but stale: it lists App.razor/BasePath, Error.razor, _Imports.razor, and other changes that are not present in the actual diff.
Recommended title
[Templates] Blazor Hybrid: Sync MAUI Blazor templates with aspnetcore net11-pre4
Recommended description
## Description
Syncs both MAUI Blazor templates (`maui-blazor` and `maui-blazor-web`) with upstream `dotnet/aspnetcore` Blazor template changes for net11-pre4.
## Changes
### Both templates
- **NavMenu.razor.js**: Use a guarded document-level delegated click handler so the mobile NavMenu collapse logic still works when the module loads before Blazor renders `#nav-scrollable`.
- **index.html**: Load `NavMenu.razor.js` from the Hybrid host when sample content is included.
- **template.json**: Add `**/*.razor.js` to `copyOnly` so the template engine emits co-located JavaScript files.
### `maui-blazor-web` / multi-project template
- **template.json**: Add `onlyIf` localhost constraints on the Kestrel port replacers.
- **Web.Client csproj**: Use `MS_COMPONENTS_WEBASSEMBLY_VERSION` for `Microsoft.AspNetCore.Components.WebAssembly.Authentication`.
- **Web.Client Program.cs / Program.Main.cs**: Replace `PersistentAuthenticationStateProvider` registration with `AddAuthenticationStateDeserialization()`.
- **ReconnectModal.razor**: Update resume failure UI so users can retry or reload.
- **ReconnectModal.razor.js**: Show resume-failed state on resume exceptions instead of immediately reloading.
- **ReconnectModal.razor.css**: Fix `&[open]` CSS nesting formatting.
### Not synced / not included
- Account/Identity pages and data/migrations that are not part of the MAUI templates.
- Localization files and other generated template assets.
## Testing
Gate was skipped because no tests were detected. Android is the target platform for validating the Hybrid NavMenu collapse timing behavior.
🏁 Report — Final Recommendation
Comparative Fix Report
Candidate ranking
| Rank | Candidate | Regression evidence | Assessment |
|---|---|---|---|
| 1 | pr |
Gate skipped; no tests detected | Best overall. The delegated handler is the simplest timing-safe fix, preserves checkbox click/change semantics by using navToggler.click(), and aligns with the template sync intent. Expert review found no actionable code findings. |
| 2 | pr-plus-reviewer |
Gate skipped; no tests detected | Production-code-identical to pr because expert reviewer feedback was coverage-only. Equivalent behavior, but not a distinct stronger fix. |
| 3 | try-fix-3 |
✅ Passed Node timing simulation and template build | Strongest alternate. It keeps timing-safe delegation but directly assigns navToggler.checked = false; this avoids synthetic clicks, but is a narrower behavior bet if future template code observes checkbox click/change events. |
| 4 | try-fix-1 |
✅ Passed Node timing simulation and template build | Correct but less attractive. requestAnimationFrame late binding avoids document delegation, but adds a recurring retry loop until render and assumes the initially found elements remain the right binding targets. |
| 5 | try-fix-2 |
✅ Passed Node timing simulation and template build | Correct but more complex. MutationObserver late binding avoids a permanent document listener, but adds observer lifecycle complexity and depends on document.body being available. |
No candidate failed regression tests, so no failing candidate was ranked above a passing one.
Winning candidate: pr
The raw PR fix should win. It directly solves the Hybrid script-load timing issue by moving from startup-time DOM lookup to event-time DOM lookup, while the global guard prevents duplicate handler registration from the NavMenu.razor and index.html module references. The alternatives demonstrate viable implementations, but each introduces a tradeoff that is not clearly better than the PR: polling, mutation observation, or direct state assignment that bypasses checkbox event semantics.
Notes for the PR author
The remaining weakness is test coverage, not the fix shape. The PR should ideally add or document regression coverage for the Android/Hybrid case where NavMenu.razor.js loads before Blazor renders #nav-scrollable; gate verification was skipped because no tests were detected.
🧭 Next Steps — review latest findings
No alternative fix was selected for this run. Review the session findings and CI results before merging.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
✓ Clarified the intentional dual ℹ For the test-gap warning: this remains a template JavaScript behavior change rather than a Controls UITest scenario. I checked the template integration coverage path; it can build/run generated templates, but it does not provide a reliable normal-infra regression hook for this DOM timing/click behavior, so I did not fabricate a test. |
|
Closing as we need to redo this with things from aspnetcore pre 7 |
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
Syncs both MAUI Blazor templates (
maui-blazorandmaui-blazor-web) with upstream changes fromdotnet/aspnetcoreBlazorWeb-CSharp template (release/10.0→main/net11-pre4).Changes
Both templates
onclickhandler withid="nav-scrollable"+ JS module (CSP compliance)**/*.razor.jstocopyOnlyso template engine emits co-located JS filesmaui-blazor(standalone)<script>reference for NavMenu.razor.jsmaui-blazor-solution(multi-project)<base href="/" />→<BasePath />component&[open]CSS nesting indentation[PersistentState]on RequestId,public,??=assignment@using Microsoft.AspNetCore.Components.EndpointsonlyIflocalhost constraint on port replacersPersistentAuthenticationStateProvider→AddAuthenticationStateDeserialization()Not synced (intentional)