Fix prerendering issues of Brouter (#12830) - #12831
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughBrouter now starts initial navigation through a render-tree sentinel during the first render batch. It tracks route-registration versions and defers rematching until navigation is idle. New tests cover flat, nested, wrapped, late-registered, and prerendered routes. ChangesRouting boot and rematch
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Brouter/Bit.Brouter/Components/Brouter.cs (1)
722-752: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRoute exceptions from the discarded rematch task.
ScheduleLateRegistrationRematchdiscards the task returned byInvokeAsync.ProcessNavigationAsynchandles most failures internally, but it deliberately rethrowsNavigationException(see Line 2549). On this path that exception faults a task nobody observes, so an SSR redirect triggered by a late-registration rematch is lost silently.Wrap the awaited call so the failure reaches
SafeInvokeOnErrorinstead of an unobserved task.♻️ Proposed guard around the deferred navigation
- await ProcessNavigationAsync(CurrentLocation, CurrentLocation, decisionAlreadyMade: false, BrouterNavigationType.Replace); + try + { + await ProcessNavigationAsync(CurrentLocation, CurrentLocation, decisionAlreadyMade: false, BrouterNavigationType.Replace); + } + catch (Exception ex) + { + // Nothing awaits this scheduled task, so an escaping exception (notably the SSR + // NavigationException redirect signal) would otherwise be swallowed. + await SafeInvokeOnError(CurrentLocation, CurrentLocation, ex); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Brouter/Bit.Brouter/Components/Brouter.cs` around lines 722 - 752, Update ScheduleLateRegistrationRematch to catch NavigationException from the awaited ProcessNavigationAsync call and forward it to SafeInvokeOnError, ensuring redirects or other route exceptions from the discarded InvokeAsync task are observed without changing the existing rematch scheduling behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Brouter/Bit.Brouter/Components/Brouter.cs`:
- Around line 2613-2622: Move the _processingNavigation decrement and its
outermost-frame rematch check to the beginning of the pipeline’s finally block,
before any await or other potentially throwing cleanup such as
CompleteViewTransitionAsync. Preserve the existing condition and
ScheduleLateRegistrationRematch behavior so the increment at the pipeline entry
is always balanced, including when cleanup throws.
---
Nitpick comments:
In `@src/Brouter/Bit.Brouter/Components/Brouter.cs`:
- Around line 722-752: Update ScheduleLateRegistrationRematch to catch
NavigationException from the awaited ProcessNavigationAsync call and forward it
to SafeInvokeOnError, ensuring redirects or other route exceptions from the
discarded InvokeAsync task are observed without changing the existing rematch
scheduling behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e5103ee7-1ace-4536-ad76-183be511e5ad
📒 Files selected for processing (14)
src/Brouter/Bit.Brouter/Components/Brouter.cssrc/Brouter/Bit.Brouter/Components/BrouterInitializer.cssrc/Brouter/Tests/Bit.Brouter.Tests/AmbiguousRouteTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/BrouterAuthorizationTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/InitialBatchProbe.razorsrc/Brouter/Tests/Bit.Brouter.Tests/InitialMountContent.razorsrc/Brouter/Tests/Bit.Brouter.Tests/InitialMountDeepWrappedHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/InitialMountHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/InitialMountLateRouteHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/InitialMountNestedHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/InitialMountState.cssrc/Brouter/Tests/Bit.Brouter.Tests/InitialMountTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/InitialMountWrappedHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/InitialMountWrapper.razor
|
@coderabbitai full-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Brouter/Tests/Bit.Brouter.Tests/AmbiguousRouteTests.cs`:
- Around line 117-119: Update the stale member reference in the comment within
AmbiguousRouteTests to use Brouter.RequestLateRegistrationRematch instead of the
removed Brouter.ScheduleLateRegistrationRematch, leaving the surrounding
explanation unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 01ccd235-8bd0-492d-8f1e-32b1bdbd6fc9
📒 Files selected for processing (16)
src/Brouter/Bit.Brouter/Components/Brouter.cssrc/Brouter/Bit.Brouter/Components/BrouterInitializer.cssrc/Brouter/Tests/Bit.Brouter.Tests/AmbiguousRouteTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/BrouterAuthorizationTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/InitialBatchProbe.razorsrc/Brouter/Tests/Bit.Brouter.Tests/InitialMountContent.razorsrc/Brouter/Tests/Bit.Brouter.Tests/InitialMountDeepWrappedHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/InitialMountHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/InitialMountLateRouteHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/InitialMountNestedHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/InitialMountState.cssrc/Brouter/Tests/Bit.Brouter.Tests/InitialMountTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/InitialMountWrappedHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/InitialMountWrapper.razorsrc/Brouter/Tests/Bit.Brouter.Tests/LateRegisteredAuthHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/PrerenderMountTests.cs
1 participant
closes #12830
Summary by CodeRabbit
Bug Fixes
Tests