Improve route lifecycle management in Brouter (#12642)#12643
Open
msynk wants to merge 13 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughBrouter adds unified route lifecycle callbacks, component-level navigation locks, stable lifecycle contexts, staged arrivals, host-aware outlet dispatch, unified parameter binding, and updated demos and tests. ChangesRoute lifecycle management
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Navigation
participant Brouter
participant RouteContext
participant RoutedComponent
Navigation->>Brouter: Start navigation
Brouter->>RouteContext: Run deactivating or renavigating locks
RouteContext->>RoutedComponent: Request cancel, redirect, or allow
Brouter->>RouteContext: Notify departures and stage arrivals
Brouter->>RoutedComponent: Commit rendered content
Brouter->>RouteContext: Flush arrivals after render
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: 4
🧹 Nitpick comments (1)
src/Brouter/InteralDemos/Core/Pages/StickyNotePage.razor (1)
81-87: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
!activation.IsFirstActivationoveris falsefor consistency.
activation.IsFirstActivation is falseworks but is less idiomatic than!activation.IsFirstActivation. Theis falsepattern is typically reserved for nullable bool checks or pattern-matching contexts. This is a minor readability nit.♻️ Suggested refactor
- if (activation.IsFirstActivation is false) _reactivations++; + if (!activation.IsFirstActivation) _reactivations++;🤖 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/InteralDemos/Core/Pages/StickyNotePage.razor` around lines 81 - 87, Update the condition in OnActivated to use the idiomatic negation !activation.IsFirstActivation instead of the is false pattern, preserving the existing reactivation 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/Brouter.cs`:
- Around line 620-632: Decouple _pendingLifecycleFlush from _navVersion by
introducing a separate lifecycle navigation generation that increments only when
navigation starts. Store that generation when staging entries, and in the flush
logic around OnAfterRenderAsync compare each entry against the live generation
inside the loop rather than capturing it once, so revalidation cannot invalidate
valid arrivals and later nodes are skipped only when superseded by a newly
started navigation.
- Around line 645-654: Abort the in-progress navigation immediately when
synchronous lifecycle callbacks supersede its token/version. Add checks directly
after each departure or arrival preparation/notification dispatch in the
affected commit paths, including NotifyChainDepartures and the referenced commit
sections, before mutating _committedChain, error state, calling SetMatched, or
queuing rendering; return through the existing stale-navigation cancellation
path rather than continuing until the late check.
In `@src/Brouter/Bit.Brouter/BrouterOutlet.cs`:
- Around line 114-117: Snapshot _kept with ToArray() before each
lifecycle-dispatch foreach, including the loops around NotifyEntryTeardown and
the highlighted locations near lines 185-191, 215-220, 237-243, and 422-423.
Ensure handlers can mutate _kept without invalidating the active enumeration.
In `@src/Brouter/Bit.Brouter/BrouterRouteBase.cs`:
- Around line 69-76: Update the StateHasChanged calls in both activation-related
paths, including the explicit interface implementation
IBrouterRoute.OnActivatedAsync and the additional occurrence noted in the
comment, to execute only when _disposed is false and IsActive is true. Preserve
the existing lifecycle callback and repaint behavior for active, undisposed
components.
---
Nitpick comments:
In `@src/Brouter/InteralDemos/Core/Pages/StickyNotePage.razor`:
- Around line 81-87: Update the condition in OnActivated to use the idiomatic
negation !activation.IsFirstActivation instead of the is false pattern,
preserving the existing reactivation behavior.
🪄 Autofix (Beta)
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
Run ID: cb2afe75-dc44-4a83-8118-28e74ea3e885
📒 Files selected for processing (21)
src/Brouter/Bit.Brouter/Broute.cssrc/Brouter/Bit.Brouter/Brouter.cssrc/Brouter/Bit.Brouter/BrouterKeepAliveContext.cssrc/Brouter/Bit.Brouter/BrouterOutlet.cssrc/Brouter/Bit.Brouter/BrouterRouteActivation.cssrc/Brouter/Bit.Brouter/BrouterRouteBase.cssrc/Brouter/Bit.Brouter/BrouterRouteContext.cssrc/Brouter/Bit.Brouter/BrouterRouteDeactivation.cssrc/Brouter/Bit.Brouter/BrouterRouteDeactivationReason.cssrc/Brouter/Bit.Brouter/BrouterRouteRenavigation.cssrc/Brouter/Bit.Brouter/BrouterRouteRenderer.cssrc/Brouter/Bit.Brouter/IBrouterRoute.cssrc/Brouter/InteralDemos/Core/AppRouter.razorsrc/Brouter/InteralDemos/Core/Pages/StickyNotePage.razorsrc/Brouter/README.mdsrc/Brouter/Tests/Bit.Brouter.Tests/KeepAliveHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/KeepAliveLifecycle.razorsrc/Brouter/Tests/Bit.Brouter.Tests/KeepAliveTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/LifecyclePage.razorsrc/Brouter/Tests/Bit.Brouter.Tests/LifecycleProbe.razorsrc/Brouter/Tests/Bit.Brouter.Tests/RouteLifecycleTests.cs
💤 Files with no reviewable changes (1)
- src/Brouter/Bit.Brouter/BrouterKeepAliveContext.cs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Brouter/Bit.Brouter/Brouter.cs (1)
2029-2051: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider explicit test coverage for chained/superseded navigations during the pending-UI window.
The
departuresNotified/notifySurvivorsAsRemaininginterplay only fires the "unmount everything" departure notification once per commit that actually shows theNavigatingfragment; a second navigation that supersedes the first while_navigatingis stilltrue(and has no loaders of its own) skips this branch entirely and falls through to the defaultNotifyChainDeparturescall later. This resolves correctly today becauseFireArrivalultimately readsBrouterRouteContext.IsActive(already set by whichever pipeline actually deactivated the content) rather than per-pipeline bookkeeping - but it's a subtle enough invariant (relying on state rather than notification counting) that a dedicated test for "navigate A (with loader) -> navigate B (no loader) before A's loader resolves, chain partially overlaps" would be valuable to lock in the behavior.🤖 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/Brouter.cs` around lines 2029 - 2051, Add a focused test covering chained superseded navigation during the pending-UI window: start navigation A with a loader, then navigate to loaderless B before A resolves, with partially overlapping route chains. Assert the Navigating fragment and departure notifications occur only as intended, and that final arrival behavior follows BrouterRouteContext.IsActive rather than per-pipeline notification counts. Use the existing navigation test harness and cover the departuresNotified/NotifyChainDepartures path.
🤖 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.
Nitpick comments:
In `@src/Brouter/Bit.Brouter/Brouter.cs`:
- Around line 2029-2051: Add a focused test covering chained superseded
navigation during the pending-UI window: start navigation A with a loader, then
navigate to loaderless B before A resolves, with partially overlapping route
chains. Assert the Navigating fragment and departure notifications occur only as
intended, and that final arrival behavior follows BrouterRouteContext.IsActive
rather than per-pipeline notification counts. Use the existing navigation test
harness and cover the departuresNotified/NotifyChainDepartures path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cb2afe75-dc44-4a83-8118-28e74ea3e885
📒 Files selected for processing (21)
src/Brouter/Bit.Brouter/Broute.cssrc/Brouter/Bit.Brouter/Brouter.cssrc/Brouter/Bit.Brouter/BrouterKeepAliveContext.cssrc/Brouter/Bit.Brouter/BrouterOutlet.cssrc/Brouter/Bit.Brouter/BrouterRouteActivation.cssrc/Brouter/Bit.Brouter/BrouterRouteBase.cssrc/Brouter/Bit.Brouter/BrouterRouteContext.cssrc/Brouter/Bit.Brouter/BrouterRouteDeactivation.cssrc/Brouter/Bit.Brouter/BrouterRouteDeactivationReason.cssrc/Brouter/Bit.Brouter/BrouterRouteRenavigation.cssrc/Brouter/Bit.Brouter/BrouterRouteRenderer.cssrc/Brouter/Bit.Brouter/IBrouterRoute.cssrc/Brouter/InteralDemos/Core/AppRouter.razorsrc/Brouter/InteralDemos/Core/Pages/StickyNotePage.razorsrc/Brouter/README.mdsrc/Brouter/Tests/Bit.Brouter.Tests/KeepAliveHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/KeepAliveLifecycle.razorsrc/Brouter/Tests/Bit.Brouter.Tests/KeepAliveTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/LifecyclePage.razorsrc/Brouter/Tests/Bit.Brouter.Tests/LifecycleProbe.razorsrc/Brouter/Tests/Bit.Brouter.Tests/RouteLifecycleTests.cs
💤 Files with no reviewable changes (1)
- src/Brouter/Bit.Brouter/BrouterKeepAliveContext.cs
Member
Author
|
@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/Bit.Brouter/BrouterRouteRenderer.cs`:
- Around line 102-194: Snapshot _keptEntries with ToArray() before each
lifecycle callback enumeration in NotifyDeparture, PrepareArrival,
NotifyTeardown, and FireArrival. Iterate the snapshot while invoking
FireDeactivated or FireArrival so reentrant ClearKeepAlive mutations cannot
invalidate the enumeration, preserving the existing callback behavior.
🪄 Autofix (Beta)
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
Run ID: e2736e39-b3e4-4831-9f0e-c3b044fe38e2
📒 Files selected for processing (26)
src/Brouter/Bit.Brouter/Broute.cssrc/Brouter/Bit.Brouter/Brouter.cssrc/Brouter/Bit.Brouter/BrouterKeepAliveContext.cssrc/Brouter/Bit.Brouter/BrouterOutlet.cssrc/Brouter/Bit.Brouter/BrouterRouteActivation.cssrc/Brouter/Bit.Brouter/BrouterRouteBase.cssrc/Brouter/Bit.Brouter/BrouterRouteContext.cssrc/Brouter/Bit.Brouter/BrouterRouteDeactivation.cssrc/Brouter/Bit.Brouter/BrouterRouteDeactivationReason.cssrc/Brouter/Bit.Brouter/BrouterRouteRenavigation.cssrc/Brouter/Bit.Brouter/BrouterRouteRenderer.cssrc/Brouter/Bit.Brouter/IBrouterRoute.cssrc/Brouter/InteralDemos/Core/AppRouter.razorsrc/Brouter/InteralDemos/Core/DemoState.cssrc/Brouter/InteralDemos/Core/Pages/HomePage.razorsrc/Brouter/InteralDemos/Core/Pages/LifecyclePage.razorsrc/Brouter/InteralDemos/Core/Pages/LifecyclePulse.razorsrc/Brouter/InteralDemos/Core/Pages/StickyNotePage.razorsrc/Brouter/InteralDemos/Core/Shared/Header.razorsrc/Brouter/README.mdsrc/Brouter/Tests/Bit.Brouter.Tests/KeepAliveHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/KeepAliveLifecycle.razorsrc/Brouter/Tests/Bit.Brouter.Tests/KeepAliveTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/LifecyclePage.razorsrc/Brouter/Tests/Bit.Brouter.Tests/LifecycleProbe.razorsrc/Brouter/Tests/Bit.Brouter.Tests/RouteLifecycleTests.cs
💤 Files with no reviewable changes (1)
- src/Brouter/Bit.Brouter/BrouterKeepAliveContext.cs
Member
Author
|
@coderabbitai full-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Brouter/Bit.Brouter/Brouter.cs (1)
1310-1349: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStale-navigation guard missing in
HandleAppNotFoundAsync.Every other
NotifyChainDeparturescall site in this file was hardened (per the earlier "Abort stale commits after synchronous lifecycle callbacks" review round) with atoken.IsCancellationRequested || version != _navVersioncheck immediately afterward, before any further commit-state mutation. This call site was missed: afterNotifyChainDepartures(...)at lines 1324-1328 (and again afterawait OnNotFound(location)at line 1334), there is no check before unconditionally overwriting_committedChain/_currentRouteDataand potentially callingNavigateInternal/StateHasChanged().Since a departure/
OnNotFoundcallback can run synchronous code that starts a brand-new navigation on the same dispatcher (InvokeAsyncruns inline when already on it), that nested navigation's freshly-committed_committedChain/_currentRouteDatawould be clobbered when control returns here. This method has no_navVersion/token of its own, so the exact guard used elsewhere doesn't transplant directly - it needs its own staleness check (e.g., comparingCurrentLocationagainst the capturedlocation).🐛 Proposed fix
NotifyChainDepartures( new BrouterNavigationContext(location, location, CancellationToken.None), _committedChain); + // A departure callback may have synchronously started a new navigation; abandon + // this stale continuation instead of clobbering its committed state. + if (string.Equals(CurrentLocation.FullUri, location.FullUri, StringComparison.Ordinal) is false) return; _committedChain = []; _currentRouteData = null; if (OnNotFound is not null) await OnNotFound(location); + if (string.Equals(CurrentLocation.FullUri, location.FullUri, StringComparison.Ordinal) is false) return; + if (string.IsNullOrEmpty(NotFoundUrl) is false && IsSamePath(location.Path, NotFoundUrl) is false)🤖 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/Brouter.cs` around lines 1310 - 1349, Add stale-navigation checks in HandleAppNotFoundAsync after NotifyChainDepartures and after awaiting OnNotFound, using the captured location versus the current navigation state. Return immediately when a nested navigation has changed the location, before mutating _committedChain or _currentRouteData and before calling NavigateInternal or StateHasChanged.
🤖 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/Brouter.cs`:
- Around line 1042-1084: The lifecycle flush in OnAfterRenderAsync must be
scoped to the render pass that staged each arrival, not only filtered by
_lifecycleNavGeneration. Add a render token or per-render queue when staging
entries in _pendingLifecycleFlush, and have the matching commit flush only its
own entries; prevent an older overlapping OnAfterRenderAsync from draining
arrivals staged by a newer commit while preserving stale-navigation filtering.
---
Outside diff comments:
In `@src/Brouter/Bit.Brouter/Brouter.cs`:
- Around line 1310-1349: Add stale-navigation checks in HandleAppNotFoundAsync
after NotifyChainDepartures and after awaiting OnNotFound, using the captured
location versus the current navigation state. Return immediately when a nested
navigation has changed the location, before mutating _committedChain or
_currentRouteData and before calling NavigateInternal or StateHasChanged.
🪄 Autofix (Beta)
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
Run ID: fc5060bf-650b-429e-a049-04484c5ddb1a
📒 Files selected for processing (28)
src/Brouter/Bit.Brouter/Broute.cssrc/Brouter/Bit.Brouter/Brouter.cssrc/Brouter/Bit.Brouter/BrouterKeepAliveContext.cssrc/Brouter/Bit.Brouter/BrouterOutlet.cssrc/Brouter/Bit.Brouter/BrouterRouteActivation.cssrc/Brouter/Bit.Brouter/BrouterRouteBase.cssrc/Brouter/Bit.Brouter/BrouterRouteContext.cssrc/Brouter/Bit.Brouter/BrouterRouteDeactivation.cssrc/Brouter/Bit.Brouter/BrouterRouteDeactivationReason.cssrc/Brouter/Bit.Brouter/BrouterRouteRenavigation.cssrc/Brouter/Bit.Brouter/BrouterRouteRenderer.cssrc/Brouter/Bit.Brouter/IBrouterRoute.cssrc/Brouter/InteralDemos/Core/AppRouter.razorsrc/Brouter/InteralDemos/Core/DemoState.cssrc/Brouter/InteralDemos/Core/Pages/HomePage.razorsrc/Brouter/InteralDemos/Core/Pages/LifecyclePage.razorsrc/Brouter/InteralDemos/Core/Pages/LifecyclePulse.razorsrc/Brouter/InteralDemos/Core/Pages/StickyNotePage.razorsrc/Brouter/InteralDemos/Core/Shared/Header.razorsrc/Brouter/InteralDemos/Core/Shared/Header.razor.csssrc/Brouter/InteralDemos/Core/wwwroot/app.csssrc/Brouter/README.mdsrc/Brouter/Tests/Bit.Brouter.Tests/KeepAliveHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/KeepAliveLifecycle.razorsrc/Brouter/Tests/Bit.Brouter.Tests/KeepAliveTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/LifecyclePage.razorsrc/Brouter/Tests/Bit.Brouter.Tests/LifecycleProbe.razorsrc/Brouter/Tests/Bit.Brouter.Tests/RouteLifecycleTests.cs
💤 Files with no reviewable changes (1)
- src/Brouter/Bit.Brouter/BrouterKeepAliveContext.cs
Member
Author
|
@coderabbitai full-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Brouter/Bit.Brouter/Broute.cs (1)
605-612: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSnapshot
Outlets.Valuesbefore invoking teardown callbacks, for consistency with the_keptfix.
ForgetChildsynchronously fires deactivation callbacks (NotifyEntryTeardown→FireDeactivated) for each outlet. If a handler's synchronous prefix causes another outlet under the same host to unregister (e.g. via component disposal during the same batch), this liveDictionary.Valuesenumeration would throw. The exact same class of hazard was just fixed for_keptelsewhere in this PR by snapshotting with.ToArray().♻️ Proposed fix
var outletHost = FindOutletHost(); if (outletHost is not null) { - foreach (var outlet in outletHost.Outlets.Values) + foreach (var outlet in outletHost.Outlets.Values.ToArray()) { outlet.ForgetChild(this); } }🤖 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/Broute.cs` around lines 605 - 612, Snapshot outletHost.Outlets.Values with ToArray() before iterating in the teardown block, then pass each snapshot outlet to ForgetChild(this). Preserve the existing null check and teardown behavior while preventing callbacks from invalidating the live dictionary enumeration.
🤖 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/Brouter.cs`:
- Around line 1327-1356: After NotifyChainDepartures in the not-found handling
flow, immediately check whether _lifecycleNavGeneration differs from the
captured generation and return if a new navigation started. Keep this check
before clearing _committedChain and _currentRouteData, while retaining the
existing post-OnNotFound check.
---
Nitpick comments:
In `@src/Brouter/Bit.Brouter/Broute.cs`:
- Around line 605-612: Snapshot outletHost.Outlets.Values with ToArray() before
iterating in the teardown block, then pass each snapshot outlet to
ForgetChild(this). Preserve the existing null check and teardown behavior while
preventing callbacks from invalidating the live dictionary enumeration.
🪄 Autofix (Beta)
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
Run ID: 3b1bb039-9396-42bb-a926-a6f22af9995d
📒 Files selected for processing (28)
src/Brouter/Bit.Brouter/Broute.cssrc/Brouter/Bit.Brouter/Brouter.cssrc/Brouter/Bit.Brouter/BrouterKeepAliveContext.cssrc/Brouter/Bit.Brouter/BrouterOutlet.cssrc/Brouter/Bit.Brouter/BrouterRouteActivation.cssrc/Brouter/Bit.Brouter/BrouterRouteBase.cssrc/Brouter/Bit.Brouter/BrouterRouteContext.cssrc/Brouter/Bit.Brouter/BrouterRouteDeactivation.cssrc/Brouter/Bit.Brouter/BrouterRouteDeactivationReason.cssrc/Brouter/Bit.Brouter/BrouterRouteRenavigation.cssrc/Brouter/Bit.Brouter/BrouterRouteRenderer.cssrc/Brouter/Bit.Brouter/IBrouterRoute.cssrc/Brouter/InteralDemos/Core/AppRouter.razorsrc/Brouter/InteralDemos/Core/DemoState.cssrc/Brouter/InteralDemos/Core/Pages/HomePage.razorsrc/Brouter/InteralDemos/Core/Pages/LifecyclePage.razorsrc/Brouter/InteralDemos/Core/Pages/LifecyclePulse.razorsrc/Brouter/InteralDemos/Core/Pages/StickyNotePage.razorsrc/Brouter/InteralDemos/Core/Shared/Header.razorsrc/Brouter/InteralDemos/Core/Shared/Header.razor.csssrc/Brouter/InteralDemos/Core/wwwroot/app.csssrc/Brouter/README.mdsrc/Brouter/Tests/Bit.Brouter.Tests/KeepAliveHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/KeepAliveLifecycle.razorsrc/Brouter/Tests/Bit.Brouter.Tests/KeepAliveTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/LifecyclePage.razorsrc/Brouter/Tests/Bit.Brouter.Tests/LifecycleProbe.razorsrc/Brouter/Tests/Bit.Brouter.Tests/RouteLifecycleTests.cs
💤 Files with no reviewable changes (1)
- src/Brouter/Bit.Brouter/BrouterKeepAliveContext.cs
Member
Author
|
@coderabbitai full-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Brouter/Bit.Brouter/BrouterRouteRenderer.cs (1)
69-90: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep-alive removals need a
Disposingcallback.
DropKeptContentclears hidden keep-alive entries and nulls_contextwithout firingFireDeactivated(BrouterRouteDeactivationReason.Disposing, ...), soOnDeactivatedAsynccleanup is skipped whenIBrouter.ClearKeepAlive()drops a route.- The same callback is missing when
RenderKeptEntriesevicts LRU entries.Thread
location/onErrorthrough the clear path and fireDisposingbefore removing each dropped context.🤖 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/BrouterRouteRenderer.cs` around lines 69 - 90, Update DropKeptContent and the clear path used by IBrouter.ClearKeepAlive to accept and propagate location and onError, then invoke FireDeactivated with BrouterRouteDeactivationReason.Disposing before removing each dropped keep-alive context. Apply the same disposal callback when RenderKeptEntries evicts LRU entries, while preserving the active route entry and existing singleton behavior.
🧹 Nitpick comments (2)
src/Brouter/InteralDemos/Core/Pages/NavigationLockPage.razor (1)
53-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd basic accessibility attributes to the confirmation dialog.
The modal overlay lacks
role="dialog",aria-modal="true", and focus management — keyboard users can Tab past the overlay to background elements. This is a demo, but the dialog is the central interaction of this page.♿ Suggested accessibility improvements
`@if` (_prompt is not null) { - <div style="position: fixed; inset: 0; background: rgba(0,0,0,.45); display: grid; place-items: center; z-index: 1000;"> - <div class="bb-card" style="max-width: 26rem; margin: 0;"> + <div role="dialog" aria-modal="true" aria-labelledby="lock-prompt-title" + style="position: fixed; inset: 0; background: rgba(0,0,0,.45); display: grid; place-items: center; z-index: 1000;"> + <div class="bb-card" style="max-width: 26rem; margin: 0;" `@onkeydown`="@(e => { if (e.Key == "Escape") AnswerPrompt(stay: true); })"> <h3 class="bb-card-title" id="lock-prompt-title"><span class="bb-dot bb-dot-warning"></span> Unsaved changes</h3>🤖 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/InteralDemos/Core/Pages/NavigationLockPage.razor` around lines 53 - 67, Update the confirmation modal rendered by the _prompt block in NavigationLockPage to include role="dialog" and aria-modal="true", and implement focus management so keyboard focus is moved into the dialog when it opens and cannot tab to background content while it remains active.src/Brouter/Tests/Bit.Brouter.Tests/RouteLifecycleTests.cs (1)
17-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
RenderAthelper across test classes.Identical to
KeepAliveTests.cs'sRenderAt(string url)and structurally the same asNavigationLockTests.cs's version (differing only by host type). Worth consolidating into a generic helper on the sharedBunitTestContextbase.♻️ Suggested consolidation
// In BunitTestContext (shared base) protected (IRenderedComponent<THost> Cut, IBrouter Brouter) RenderAt<THost>(string url) where THost : IComponent { var nav = Services.GetRequiredService<FakeNavigationManager>(); nav.NavigateTo(url); var cut = RenderComponent<THost>(); return (cut, Services.GetRequiredService<IBrouter>()); }Then each test class calls
RenderAt<KeepAliveHost>(url)/RenderAt<NavigationLockHost>(url), removing the three near-identical private copies.🤖 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/Tests/Bit.Brouter.Tests/RouteLifecycleTests.cs` around lines 17 - 23, Consolidate the duplicated RenderAt helpers from the test classes into a protected generic RenderAt<THost> method on the shared BunitTestContext, constrained to IComponent and preserving navigation, rendering, and IBrouter resolution. Remove the private helpers from KeepAliveTests, NavigationLockTests, and the current test class, and update callers to specify KeepAliveHost or NavigationLockHost explicitly.
🤖 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/BrouterOutlet.cs`:
- Around line 108-126: Fix the cleanup predicate in BrouterOutlet.ForgetChild so
_kept.RemoveAll compares each ChildEntry’s Route property with the route
parameter via ReferenceEquals, matching the earlier teardown checks. Ensure
retained entries belonging to the disposed route are actually removed while
preserving entries for other routes.
In `@src/Brouter/Tests/Bit.Brouter.Tests/KeepAliveLifecycle.razor`:
- Around line 1-8: Rename the lifecycle markup label `_activations` in the `<div
data-testid="lifecycle">` output to a non-overlapping label, and update the
corresponding assertions in KeepAliveTests and RouteLifecycleTests to use that
label. Preserve the existing counter values and all other lifecycle labels
unchanged.
---
Outside diff comments:
In `@src/Brouter/Bit.Brouter/BrouterRouteRenderer.cs`:
- Around line 69-90: Update DropKeptContent and the clear path used by
IBrouter.ClearKeepAlive to accept and propagate location and onError, then
invoke FireDeactivated with BrouterRouteDeactivationReason.Disposing before
removing each dropped keep-alive context. Apply the same disposal callback when
RenderKeptEntries evicts LRU entries, while preserving the active route entry
and existing singleton behavior.
---
Nitpick comments:
In `@src/Brouter/InteralDemos/Core/Pages/NavigationLockPage.razor`:
- Around line 53-67: Update the confirmation modal rendered by the _prompt block
in NavigationLockPage to include role="dialog" and aria-modal="true", and
implement focus management so keyboard focus is moved into the dialog when it
opens and cannot tab to background content while it remains active.
In `@src/Brouter/Tests/Bit.Brouter.Tests/RouteLifecycleTests.cs`:
- Around line 17-23: Consolidate the duplicated RenderAt helpers from the test
classes into a protected generic RenderAt<THost> method on the shared
BunitTestContext, constrained to IComponent and preserving navigation,
rendering, and IBrouter resolution. Remove the private helpers from
KeepAliveTests, NavigationLockTests, and the current test class, and update
callers to specify KeepAliveHost or NavigationLockHost explicitly.
🪄 Autofix (Beta)
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
Run ID: 0961fd26-7f75-4a42-8299-fa9bec3d3694
📒 Files selected for processing (35)
src/Brouter/Bit.Brouter/Broute.cssrc/Brouter/Bit.Brouter/Brouter.cssrc/Brouter/Bit.Brouter/BrouterKeepAliveContext.cssrc/Brouter/Bit.Brouter/BrouterOutlet.cssrc/Brouter/Bit.Brouter/BrouterRouteActivation.cssrc/Brouter/Bit.Brouter/BrouterRouteBase.cssrc/Brouter/Bit.Brouter/BrouterRouteContext.cssrc/Brouter/Bit.Brouter/BrouterRouteDeactivatingContext.cssrc/Brouter/Bit.Brouter/BrouterRouteDeactivation.cssrc/Brouter/Bit.Brouter/BrouterRouteDeactivationReason.cssrc/Brouter/Bit.Brouter/BrouterRouteRenavigatingContext.cssrc/Brouter/Bit.Brouter/BrouterRouteRenavigation.cssrc/Brouter/Bit.Brouter/BrouterRouteRenderer.cssrc/Brouter/Bit.Brouter/IBrouterRoute.cssrc/Brouter/InteralDemos/Core/AppRouter.razorsrc/Brouter/InteralDemos/Core/DemoState.cssrc/Brouter/InteralDemos/Core/Pages/HomePage.razorsrc/Brouter/InteralDemos/Core/Pages/LifecyclePage.razorsrc/Brouter/InteralDemos/Core/Pages/LifecyclePulse.razorsrc/Brouter/InteralDemos/Core/Pages/NavigationLockPage.razorsrc/Brouter/InteralDemos/Core/Pages/StickyNotePage.razorsrc/Brouter/InteralDemos/Core/Shared/Header.razorsrc/Brouter/InteralDemos/Core/Shared/Header.razor.csssrc/Brouter/InteralDemos/Core/wwwroot/app.csssrc/Brouter/README.mdsrc/Brouter/Tests/Bit.Brouter.Tests/KeepAliveHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/KeepAliveLifecycle.razorsrc/Brouter/Tests/Bit.Brouter.Tests/KeepAliveTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/LifecyclePage.razorsrc/Brouter/Tests/Bit.Brouter.Tests/LifecycleProbe.razorsrc/Brouter/Tests/Bit.Brouter.Tests/NavigationLockHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/NavigationLockProbe.razorsrc/Brouter/Tests/Bit.Brouter.Tests/NavigationLockState.cssrc/Brouter/Tests/Bit.Brouter.Tests/NavigationLockTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/RouteLifecycleTests.cs
💤 Files with no reviewable changes (1)
- src/Brouter/Bit.Brouter/BrouterKeepAliveContext.cs
Member
Author
|
@coderabbitai full-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/Brouter.cs`:
- Around line 2541-2544: Update the reason selection in the route deactivation
flow around FireDeactivatingAsync so a KeepAlive route whose outlet host is
leaving is reported as BrouterRouteDeactivationReason.Disposing rather than
Hidden. Use the existing hosting-layout/outlet context to distinguish retained
routes from kept children destroyed with their host, while preserving Hidden for
routes that remain alive.
In `@src/Brouter/Bit.Brouter/BrouterOutlet.cs`:
- Around line 160-172: Update the named-outlet rendering branch around
RenderChild so named views are wrapped with the same entry context via
WrapRouteContext as primary outlet children. Preserve the existing named-outlet
rendering behavior while ensuring BrouterRouteBase descendants receive lifecycle
and navigation-lock callbacks, and add a regression test covering named-outlet
lock registration.
- Around line 197-203: Update the transient-entry branch in the BrouterOutlet
navigation logic to preserve the current entry when willRemainMatched indicates
its route remains matched, including parameter/query changes. Only fire
Disposing and clear _current when the transient route is no longer matched;
retained entries must continue through the renavigation path so component state
is preserved.
In `@src/Brouter/Bit.Brouter/BrouterRouteRenderer.cs`:
- Around line 513-524: Update the route rendering flow around
ApplyTypedParameters and context.AutoRegisterDelegate so replacing direct
component rendering with error, content, or Found output removes the previously
registered page instance from the stable context. Ensure every
non-direct-component branch unregisters the matching capture before rendering
replacement output, and add a regression test covering error-content navigation
to verify stale lifecycle registrations cannot block later navigation.
In `@src/Brouter/InteralDemos/Core/Pages/NavigationLockPage.razor`:
- Around line 116-124: Update the navigation lock flow around the token.Register
callback and stay handling so token cancellation does not clear _draft. Track
whether the prompt completed through an explicit user decision, invoke cancel
only for an explicit stay choice, and assign _draft = _saved only when the user
explicitly chooses to leave; preserve the dirty state for superseding
navigation.
In `@src/Brouter/Tests/Bit.Brouter.Tests/NavigationLockTests.cs`:
- Around line 8-14: Update the navigation-lock test scenario around the Prompt
and Navigate calls to leave the first custom prompt unresolved, then start a
second navigation while it is pending. Make NavigationLockProbe observe
context.CancellationToken, assert the second navigation completes through
supersession, and verify the original prompt is not resolved.
🪄 Autofix (Beta)
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
Run ID: c20b0cd7-2582-4919-a538-283b326648e5
📒 Files selected for processing (36)
src/Brouter/Bit.Brouter/Broute.cssrc/Brouter/Bit.Brouter/Brouter.cssrc/Brouter/Bit.Brouter/BrouterKeepAliveContext.cssrc/Brouter/Bit.Brouter/BrouterOutlet.cssrc/Brouter/Bit.Brouter/BrouterRouteActivation.cssrc/Brouter/Bit.Brouter/BrouterRouteBase.cssrc/Brouter/Bit.Brouter/BrouterRouteContext.cssrc/Brouter/Bit.Brouter/BrouterRouteDeactivatingContext.cssrc/Brouter/Bit.Brouter/BrouterRouteDeactivation.cssrc/Brouter/Bit.Brouter/BrouterRouteDeactivationReason.cssrc/Brouter/Bit.Brouter/BrouterRouteRenavigatingContext.cssrc/Brouter/Bit.Brouter/BrouterRouteRenavigation.cssrc/Brouter/Bit.Brouter/BrouterRouteRenderer.cssrc/Brouter/Bit.Brouter/IBrouterRoute.cssrc/Brouter/InteralDemos/Core/AppRouter.razorsrc/Brouter/InteralDemos/Core/DemoState.cssrc/Brouter/InteralDemos/Core/Pages/HomePage.razorsrc/Brouter/InteralDemos/Core/Pages/LifecyclePage.razorsrc/Brouter/InteralDemos/Core/Pages/LifecyclePulse.razorsrc/Brouter/InteralDemos/Core/Pages/NavigationLockPage.razorsrc/Brouter/InteralDemos/Core/Pages/StickyNotePage.razorsrc/Brouter/InteralDemos/Core/Shared/Header.razorsrc/Brouter/InteralDemos/Core/Shared/Header.razor.csssrc/Brouter/InteralDemos/Core/wwwroot/app.csssrc/Brouter/README.mdsrc/Brouter/Tests/Bit.Brouter.Tests/BunitTestContext.cssrc/Brouter/Tests/Bit.Brouter.Tests/KeepAliveHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/KeepAliveLifecycle.razorsrc/Brouter/Tests/Bit.Brouter.Tests/KeepAliveTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/LifecyclePage.razorsrc/Brouter/Tests/Bit.Brouter.Tests/LifecycleProbe.razorsrc/Brouter/Tests/Bit.Brouter.Tests/NavigationLockHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/NavigationLockProbe.razorsrc/Brouter/Tests/Bit.Brouter.Tests/NavigationLockState.cssrc/Brouter/Tests/Bit.Brouter.Tests/NavigationLockTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/RouteLifecycleTests.cs
💤 Files with no reviewable changes (1)
- src/Brouter/Bit.Brouter/BrouterKeepAliveContext.cs
Member
Author
|
@coderabbitai full-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Brouter/Bit.Brouter/BrouterRouteDeactivatingContext.cs (1)
15-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a shared base for the two navigation-lock context types.
BrouterRouteDeactivatingContextandBrouterRouteRenavigatingContextduplicate From/To/NavigationType/CancellationToken/IsCancelled/Cancel/Redirect/HasDecision verbatim; onlyReasondiffers. Extracting a common base keeps the two in lockstep as the lock API evolves.♻️ Proposed shared base
+public abstract class BrouterRouteNavigationLockContext +{ + protected readonly BrouterNavigationContext Navigation; + protected BrouterRouteNavigationLockContext(BrouterNavigationContext navigation) => Navigation = navigation; + + public BrouterLocation From => Navigation.From; + public BrouterLocation To => Navigation.To; + public BrouterNavigationType NavigationType => Navigation.NavigationType; + public CancellationToken CancellationToken => Navigation.CancellationToken; + public bool IsCancelled => Navigation.IsCancelled; + public void Cancel() => Navigation.Cancel(); + public void Redirect(string url) => Navigation.Redirect(url); + internal bool HasDecision => Navigation.IsCancelled || Navigation.RedirectUrl is not null; +} -public sealed class BrouterRouteDeactivatingContext +public sealed class BrouterRouteDeactivatingContext : BrouterRouteNavigationLockContext { - private readonly BrouterNavigationContext _navigation; - internal BrouterRouteDeactivatingContext(BrouterNavigationContext navigation, BrouterRouteDeactivationReason reason) + : base(navigation) { - _navigation = navigation; Reason = reason; } - // ...delegated members removed, inherited from base public BrouterRouteDeactivationReason Reason { get; } }🤖 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/BrouterRouteDeactivatingContext.cs` around lines 15 - 71, Extract the duplicated navigation-lock members into a shared base context used by both BrouterRouteDeactivatingContext and BrouterRouteRenavigatingContext. Move the common navigation state and operations—From, To, NavigationType, CancellationToken, IsCancelled, Cancel, Redirect, and HasDecision—into the base, while keeping the deactivation-specific Reason property in BrouterRouteDeactivatingContext and preserving existing constructors and 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.
Nitpick comments:
In `@src/Brouter/Bit.Brouter/BrouterRouteDeactivatingContext.cs`:
- Around line 15-71: Extract the duplicated navigation-lock members into a
shared base context used by both BrouterRouteDeactivatingContext and
BrouterRouteRenavigatingContext. Move the common navigation state and
operations—From, To, NavigationType, CancellationToken, IsCancelled, Cancel,
Redirect, and HasDecision—into the base, while keeping the deactivation-specific
Reason property in BrouterRouteDeactivatingContext and preserving existing
constructors and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 31eaa4b3-aa83-4790-84ca-fed193276978
📒 Files selected for processing (51)
src/Brouter/Bit.Brouter/Broute.cssrc/Brouter/Bit.Brouter/Brouter.cssrc/Brouter/Bit.Brouter/BrouterKeepAliveContext.cssrc/Brouter/Bit.Brouter/BrouterLocation.cssrc/Brouter/Bit.Brouter/BrouterOutlet.cssrc/Brouter/Bit.Brouter/BrouterParameterAttribute.cssrc/Brouter/Bit.Brouter/BrouterQueryAttribute.cssrc/Brouter/Bit.Brouter/BrouterRouteActivation.cssrc/Brouter/Bit.Brouter/BrouterRouteBase.cssrc/Brouter/Bit.Brouter/BrouterRouteContext.cssrc/Brouter/Bit.Brouter/BrouterRouteDeactivatingContext.cssrc/Brouter/Bit.Brouter/BrouterRouteDeactivation.cssrc/Brouter/Bit.Brouter/BrouterRouteDeactivationReason.cssrc/Brouter/Bit.Brouter/BrouterRouteRenavigatingContext.cssrc/Brouter/Bit.Brouter/BrouterRouteRenavigation.cssrc/Brouter/Bit.Brouter/BrouterRouteRenderer.cssrc/Brouter/Bit.Brouter/IBrouterRoute.cssrc/Brouter/InteralDemos/Core/AppRouter.razorsrc/Brouter/InteralDemos/Core/DemoState.cssrc/Brouter/InteralDemos/Core/Pages/GalleryItemPage.razorsrc/Brouter/InteralDemos/Core/Pages/HomePage.razorsrc/Brouter/InteralDemos/Core/Pages/LifecyclePage.razorsrc/Brouter/InteralDemos/Core/Pages/LifecyclePulse.razorsrc/Brouter/InteralDemos/Core/Pages/NavigationLockPage.razorsrc/Brouter/InteralDemos/Core/Pages/NotebookPage.razorsrc/Brouter/InteralDemos/Core/Pages/PostPage.razorsrc/Brouter/InteralDemos/Core/Pages/ProfilePage.razorsrc/Brouter/InteralDemos/Core/Pages/StickyNotePage.razorsrc/Brouter/InteralDemos/Core/Shared/Header.razorsrc/Brouter/InteralDemos/Core/Shared/Header.razor.csssrc/Brouter/InteralDemos/Core/wwwroot/app.csssrc/Brouter/README.mdsrc/Brouter/Tests/Bit.Brouter.Tests/BindConsumer.razorsrc/Brouter/Tests/Bit.Brouter.Tests/BindHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/BunitTestContext.cssrc/Brouter/Tests/Bit.Brouter.Tests/ErrorContentHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/ErrorContentTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/ErrorLockPage.razorsrc/Brouter/Tests/Bit.Brouter.Tests/KeepAliveHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/KeepAliveLifecycle.razorsrc/Brouter/Tests/Bit.Brouter.Tests/KeepAliveTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/LifecyclePage.razorsrc/Brouter/Tests/Bit.Brouter.Tests/LifecycleProbe.razorsrc/Brouter/Tests/Bit.Brouter.Tests/NavigationLockHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/NavigationLockProbe.razorsrc/Brouter/Tests/Bit.Brouter.Tests/NavigationLockState.cssrc/Brouter/Tests/Bit.Brouter.Tests/NavigationLockTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/ParameterBindingTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/QueryBindingTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/QueryConsumer.razorsrc/Brouter/Tests/Bit.Brouter.Tests/RouteLifecycleTests.cs
💤 Files with no reviewable changes (1)
- src/Brouter/Bit.Brouter/BrouterKeepAliveContext.cs
Member
Author
|
@coderabbitai full-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Brouter/Bit.Brouter/BrouterOutlet.cs (1)
128-147: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
ClearKeepAlivedrops entries without firing theirDisposingdeactivation.
_kept.RemoveAll(k => ReferenceEquals(k, active) is false)(line 135) discards every non-active retained entry, but unlikeForgetChildandDispose- which both callNotifyEntryTeardownon entries before removing/disposing them - this method never firesDisposingfor the entries it drops. Consumers relying onOnDeactivated/lifecycle handlers to clean up (timers, subscriptions, etc.) on a kept route never get notified whenIBrouter.ClearKeepAlive()unmounts their subtree, breaking the "every teardown path fires Disposing" contract this PR otherwise establishes.🐛 Proposed fix
internal void ClearKeepAlive() { var active = _current is not null && _current.Route.Matched ? _current : null; + // Snapshot: teardown handlers run synchronously and can mutate _kept. + foreach (var k in _kept.ToArray()) + { + if (ReferenceEquals(k, active) is false) NotifyEntryTeardown(k); + } var removed = _kept.RemoveAll(k => ReferenceEquals(k, active) is false);🤖 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/BrouterOutlet.cs` around lines 128 - 147, Update ClearKeepAlive to call NotifyEntryTeardown for every non-active entry before removing it from _kept, matching the teardown behavior of ForgetChild and Dispose. Preserve the active matched entry, clear _current when it is no longer active, and retain the existing rerender 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/Brouter.cs`:
- Around line 2526-2548: Update the deactivation loop in the route lifecycle
dispatch to determine the reason for each collected lifecycle context rather
than once from the route node. Carry or derive retention metadata for every
context, preserving Hidden only for contexts whose outlet content remains
retained and sending Disposing to non-retained named views before calling
FireDeactivatingAsync.
- Around line 2629-2631: Update Brouter.cs at lines 2629-2631 to notify the
boundary node’s departure with an explicit forced Disposing reason. Update
BrouterRouteRenderer.cs at lines 117-126 to accept that reason, clear _context
when the active page is replaced, and preserve existing reason inference for
other departures.
In `@src/Brouter/Bit.Brouter/BrouterOutlet.cs`:
- Around line 176-207: Move the willRemainMatched early return in
NotifyDeparture so it applies before the keep-alive and transient/named-outlet
branches. Preserve the existing deactivation and cleanup behavior when
willRemainMatched is false, while skipping FireDeactivated and _current removal
for every route that remains matched.
In `@src/Brouter/InteralDemos/Core/Pages/NavigationLockPage.razor`:
- Around line 82-83: Bind cancellation callbacks and cleanup in the navigation
prompt flow to the specific _prompt instance they create, rather than the
mutable field alone. Capture the prompt TaskCompletionSource and use it in
cancellation, decision, and finally/cleanup paths; only clear or complete
_prompt when the current field still references that captured instance. Preserve
prompt B when prompt A is replaced before A finishes unwinding.
---
Outside diff comments:
In `@src/Brouter/Bit.Brouter/BrouterOutlet.cs`:
- Around line 128-147: Update ClearKeepAlive to call NotifyEntryTeardown for
every non-active entry before removing it from _kept, matching the teardown
behavior of ForgetChild and Dispose. Preserve the active matched entry, clear
_current when it is no longer active, and retain the existing rerender behavior.
🪄 Autofix (Beta)
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
Run ID: d7369b10-e694-4bc6-8791-4d3217ee61e9
📒 Files selected for processing (52)
src/Brouter/Bit.Brouter/Broute.cssrc/Brouter/Bit.Brouter/Brouter.cssrc/Brouter/Bit.Brouter/BrouterKeepAliveContext.cssrc/Brouter/Bit.Brouter/BrouterLocation.cssrc/Brouter/Bit.Brouter/BrouterOutlet.cssrc/Brouter/Bit.Brouter/BrouterParameterAttribute.cssrc/Brouter/Bit.Brouter/BrouterQueryAttribute.cssrc/Brouter/Bit.Brouter/BrouterRouteActivation.cssrc/Brouter/Bit.Brouter/BrouterRouteBase.cssrc/Brouter/Bit.Brouter/BrouterRouteContext.cssrc/Brouter/Bit.Brouter/BrouterRouteDeactivatingContext.cssrc/Brouter/Bit.Brouter/BrouterRouteDeactivation.cssrc/Brouter/Bit.Brouter/BrouterRouteDeactivationReason.cssrc/Brouter/Bit.Brouter/BrouterRouteNavigatingContext.cssrc/Brouter/Bit.Brouter/BrouterRouteRenavigatingContext.cssrc/Brouter/Bit.Brouter/BrouterRouteRenavigation.cssrc/Brouter/Bit.Brouter/BrouterRouteRenderer.cssrc/Brouter/Bit.Brouter/IBrouterRoute.cssrc/Brouter/InteralDemos/Core/AppRouter.razorsrc/Brouter/InteralDemos/Core/DemoState.cssrc/Brouter/InteralDemos/Core/Pages/GalleryItemPage.razorsrc/Brouter/InteralDemos/Core/Pages/HomePage.razorsrc/Brouter/InteralDemos/Core/Pages/LifecyclePage.razorsrc/Brouter/InteralDemos/Core/Pages/LifecyclePulse.razorsrc/Brouter/InteralDemos/Core/Pages/NavigationLockPage.razorsrc/Brouter/InteralDemos/Core/Pages/NotebookPage.razorsrc/Brouter/InteralDemos/Core/Pages/PostPage.razorsrc/Brouter/InteralDemos/Core/Pages/ProfilePage.razorsrc/Brouter/InteralDemos/Core/Pages/StickyNotePage.razorsrc/Brouter/InteralDemos/Core/Shared/Header.razorsrc/Brouter/InteralDemos/Core/Shared/Header.razor.csssrc/Brouter/InteralDemos/Core/wwwroot/app.csssrc/Brouter/README.mdsrc/Brouter/Tests/Bit.Brouter.Tests/BindConsumer.razorsrc/Brouter/Tests/Bit.Brouter.Tests/BindHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/BunitTestContext.cssrc/Brouter/Tests/Bit.Brouter.Tests/ErrorContentHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/ErrorContentTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/ErrorLockPage.razorsrc/Brouter/Tests/Bit.Brouter.Tests/KeepAliveHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/KeepAliveLifecycle.razorsrc/Brouter/Tests/Bit.Brouter.Tests/KeepAliveTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/LifecyclePage.razorsrc/Brouter/Tests/Bit.Brouter.Tests/LifecycleProbe.razorsrc/Brouter/Tests/Bit.Brouter.Tests/NavigationLockHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/NavigationLockProbe.razorsrc/Brouter/Tests/Bit.Brouter.Tests/NavigationLockState.cssrc/Brouter/Tests/Bit.Brouter.Tests/NavigationLockTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/ParameterBindingTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/QueryBindingTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/QueryConsumer.razorsrc/Brouter/Tests/Bit.Brouter.Tests/RouteLifecycleTests.cs
💤 Files with no reviewable changes (1)
- src/Brouter/Bit.Brouter/BrouterKeepAliveContext.cs
Member
Author
|
@coderabbitai full-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/Brouter/InteralDemos/Core/Pages/LifecyclePulse.razor (1)
26-42: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider capturing the context reference at registration time.
Disposeuses the currentRouteContextcascading parameter value to unregister, but if the cascade were ever cleared or swapped before disposal, the unregister would silently no-op (or target the wrong context). Capturing the reference at registration time would make the dispose path robust regardless of cascade lifecycle:🛡️ Optional defensive refactor
private bool _registered; +private BrouterRouteContext? _registeredContext; private int _activations; private int _renavigations; protected override void OnParametersSet() { if (_registered is false && RouteContext is not null) { + _registeredContext = RouteContext; RouteContext.Register(this); _registered = true; } } // ... in Dispose: public void Dispose() { - if (_registered) RouteContext?.Unregister(this); + if (_registered) _registeredContext?.Unregister(this); }The comment states the cascade is a stable, fixed instance, so this is purely defensive and can be deferred.
🤖 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/InteralDemos/Core/Pages/LifecyclePulse.razor` around lines 26 - 42, Optionally store the RouteContext instance used by Register in a dedicated field when registration succeeds, then update Dispose to unregister through that captured reference instead of the current cascading RouteContext value. Preserve the existing one-time registration behavior and null handling.
🤖 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/BrouterRouteContext.cs`:
- Line 25: Update BrouterRouteContext.Register and Unregister to locate handlers
by object identity using ReferenceEquals rather than List.Contains/Remove value
equality. Apply the same reference-based lookup when cleaning up
_autoRegistered, ensuring registration and removal affect only the exact route
instance supplied.
In `@src/Brouter/InteralDemos/Core/Pages/StickyNotePage.razor`:
- Around line 70-76: Update the Timer callback in StickyNotePage so the disposed
check, _visibleSeconds increment, and StateHasChanged call all execute inside a
single InvokeAsync dispatcher callback. Remove the off-dispatcher state mutation
and preserve the existing disposal guard and repaint behavior.
---
Nitpick comments:
In `@src/Brouter/InteralDemos/Core/Pages/LifecyclePulse.razor`:
- Around line 26-42: Optionally store the RouteContext instance used by Register
in a dedicated field when registration succeeds, then update Dispose to
unregister through that captured reference instead of the current cascading
RouteContext value. Preserve the existing one-time registration behavior and
null handling.
🪄 Autofix (Beta)
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
Run ID: 30e03f35-459e-4b5c-a824-2255c04daa9e
📒 Files selected for processing (52)
src/Brouter/Bit.Brouter/Broute.cssrc/Brouter/Bit.Brouter/Brouter.cssrc/Brouter/Bit.Brouter/BrouterKeepAliveContext.cssrc/Brouter/Bit.Brouter/BrouterLocation.cssrc/Brouter/Bit.Brouter/BrouterOutlet.cssrc/Brouter/Bit.Brouter/BrouterParameterAttribute.cssrc/Brouter/Bit.Brouter/BrouterQueryAttribute.cssrc/Brouter/Bit.Brouter/BrouterRouteActivation.cssrc/Brouter/Bit.Brouter/BrouterRouteBase.cssrc/Brouter/Bit.Brouter/BrouterRouteContext.cssrc/Brouter/Bit.Brouter/BrouterRouteDeactivatingContext.cssrc/Brouter/Bit.Brouter/BrouterRouteDeactivation.cssrc/Brouter/Bit.Brouter/BrouterRouteDeactivationReason.cssrc/Brouter/Bit.Brouter/BrouterRouteNavigatingContext.cssrc/Brouter/Bit.Brouter/BrouterRouteRenavigatingContext.cssrc/Brouter/Bit.Brouter/BrouterRouteRenavigation.cssrc/Brouter/Bit.Brouter/BrouterRouteRenderer.cssrc/Brouter/Bit.Brouter/IBrouterRoute.cssrc/Brouter/InteralDemos/Core/AppRouter.razorsrc/Brouter/InteralDemos/Core/DemoState.cssrc/Brouter/InteralDemos/Core/Pages/GalleryItemPage.razorsrc/Brouter/InteralDemos/Core/Pages/HomePage.razorsrc/Brouter/InteralDemos/Core/Pages/LifecyclePage.razorsrc/Brouter/InteralDemos/Core/Pages/LifecyclePulse.razorsrc/Brouter/InteralDemos/Core/Pages/NavigationLockPage.razorsrc/Brouter/InteralDemos/Core/Pages/NotebookPage.razorsrc/Brouter/InteralDemos/Core/Pages/PostPage.razorsrc/Brouter/InteralDemos/Core/Pages/ProfilePage.razorsrc/Brouter/InteralDemos/Core/Pages/StickyNotePage.razorsrc/Brouter/InteralDemos/Core/Shared/Header.razorsrc/Brouter/InteralDemos/Core/Shared/Header.razor.csssrc/Brouter/InteralDemos/Core/wwwroot/app.csssrc/Brouter/README.mdsrc/Brouter/Tests/Bit.Brouter.Tests/BindConsumer.razorsrc/Brouter/Tests/Bit.Brouter.Tests/BindHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/BunitTestContext.cssrc/Brouter/Tests/Bit.Brouter.Tests/ErrorContentHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/ErrorContentTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/ErrorLockPage.razorsrc/Brouter/Tests/Bit.Brouter.Tests/KeepAliveHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/KeepAliveLifecycle.razorsrc/Brouter/Tests/Bit.Brouter.Tests/KeepAliveTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/LifecyclePage.razorsrc/Brouter/Tests/Bit.Brouter.Tests/LifecycleProbe.razorsrc/Brouter/Tests/Bit.Brouter.Tests/NavigationLockHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/NavigationLockProbe.razorsrc/Brouter/Tests/Bit.Brouter.Tests/NavigationLockState.cssrc/Brouter/Tests/Bit.Brouter.Tests/NavigationLockTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/ParameterBindingTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/QueryBindingTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/QueryConsumer.razorsrc/Brouter/Tests/Bit.Brouter.Tests/RouteLifecycleTests.cs
💤 Files with no reviewable changes (1)
- src/Brouter/Bit.Brouter/BrouterKeepAliveContext.cs
| /// </remarks> | ||
| public sealed class BrouterRouteContext | ||
| { | ||
| private readonly List<IBrouterRoute> _handlers = []; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 'class .*IBrouterRoute|record .*IBrouterRoute|override\s+bool\s+Equals' src/BrouterRepository: bitfoundation/bitplatform
Length of output: 2088
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the context implementation and related route types.
sed -n '1,220p' src/Brouter/Bit.Brouter/BrouterRouteContext.cs
printf '\n---\n'
sed -n '1,220p' src/Brouter/Bit.Brouter/BrouterRouteBase.cs
printf '\n---\n'
rg -n -C3 'interface IBrouterRoute|class .*IBrouterRoute|record .*IBrouterRoute|override\s+bool\s+Equals|GetHashCode\(' src/Brouter/Bit.Brouter src/Brouter/Bit.Brouter.GeneratorsRepository: bitfoundation/bitplatform
Length of output: 21590
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map all IBrouterRoute implementers and any equality overrides in the Brouter area.
rg -n -C2 ':\s*IBrouterRoute\b|override\s+bool\s+Equals|record\s+\w+.*IBrouterRoute|record\s+\w+' src/Brouter
printf '\n--- IBrouterRoute ---\n'
sed -n '1,220p' src/Brouter/Bit.Brouter/IBrouterRoute.csRepository: bitfoundation/bitplatform
Length of output: 14868
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find every in-repo implementation/use of IBrouterRoute and any custom equality on those types.
rg -n -C2 'IBrouterRoute' src/Brouter --glob '*.cs'
printf '\n--- equality overrides near route types ---\n'
rg -n -C2 'override\s+bool\s+Equals|override\s+int\s+GetHashCode|record\s+(class|struct)?' src/Brouter --glob '*.cs'Repository: bitfoundation/bitplatform
Length of output: 33117
Use reference identity for lifecycle handlers src/Brouter/Bit.Brouter/BrouterRouteContext.cs:25,65-73,85-87
Register/Unregister currently go through List<T>.Contains/Remove, so a value-equality IBrouterRoute can suppress or unregister a different instance. Switch these paths, including _autoRegistered cleanup, to ReferenceEquals-based lookup.
🤖 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/BrouterRouteContext.cs` at line 25, Update
BrouterRouteContext.Register and Unregister to locate handlers by object
identity using ReferenceEquals rather than List.Contains/Remove value equality.
Apply the same reference-based lookup when cleaning up _autoRegistered, ensuring
registration and removal affect only the exact route instance supplied.
Comment on lines
70
to
76
| _timer = new System.Threading.Timer(_ => | ||
| { | ||
| // Timer.Dispose does not wait for in-flight callbacks, so guard against | ||
| // touching the component after disposal has started. | ||
| if (_disposed) return; | ||
| _visibleSeconds++; | ||
| _ = InvokeAsync(StateHasChanged); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Move the timer state update onto the renderer dispatcher.
System.Threading.Timer mutates _visibleSeconds off-dispatcher and may race with rendering or an in-flight deactivation. Dispatch the state check, increment, and repaint together.
Proposed fix
_timer = new System.Threading.Timer(_ =>
{
- // Timer.Dispose does not wait for in-flight callbacks, so guard against
- // touching the component after disposal has started.
- if (_disposed) return;
- _visibleSeconds++;
- _ = InvokeAsync(StateHasChanged);
+ _ = InvokeAsync(() =>
+ {
+ if (_disposed || IsActive is false) return;
+ _visibleSeconds++;
+ StateHasChanged();
+ });
}, null, dueTime: System.Threading.Timeout.Infinite, period: System.Threading.Timeout.Infinite);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| _timer = new System.Threading.Timer(_ => | |
| { | |
| // Timer.Dispose does not wait for in-flight callbacks, so guard against | |
| // touching the component after disposal has started. | |
| if (_disposed) return; | |
| _visibleSeconds++; | |
| _ = InvokeAsync(StateHasChanged); | |
| _timer = new System.Threading.Timer(_ => | |
| { | |
| _ = InvokeAsync(() => | |
| { | |
| if (_disposed || IsActive is false) return; | |
| _visibleSeconds++; | |
| StateHasChanged(); | |
| }); | |
| }, null, dueTime: System.Threading.Timeout.Infinite, period: System.Threading.Timeout.Infinite); |
🤖 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/InteralDemos/Core/Pages/StickyNotePage.razor` around lines 70 -
76, Update the Timer callback in StickyNotePage so the disposed check,
_visibleSeconds increment, and StateHasChanged call all execute inside a single
InvokeAsync dispatcher callback. Remove the off-dispatcher state mutation and
preserve the existing disposal guard and repaint behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #12642
Summary by CodeRabbit
IBrouterRoute/BrouterRouteBase, including activation/renavigation/deactivation payloads and keep-alive deactivation reasons.BrouterKeepAliveContext.Broute’sBindComponentParametersByName.