Apply Brouter improvements (#12559)#12560
Open
msynk wants to merge 21 commits into
Open
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
WalkthroughThis PR renames the route component to ChangesBroute routing, constraints, discovery, and navigation
Estimated code review effort: 5 (Critical) | ~120 minutes Demo app, docs, tests, generators, and benchmarks
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant NavigationManager
participant Brouter
participant Broute
participant BrouterService
participant BitBrouterJS
NavigationManager->>Brouter: OnLocationChanging(to)
Brouter->>Broute: InvokeGuardsAsync(ctx)
alt cancel or redirect
Broute-->>Brouter: ctx.Cancel() / ctx.Redirect(url)
Brouter->>Brouter: NavigateInternal(prevented target)
else approve
Broute-->>Brouter: guard completes
end
Brouter->>Brouter: ProcessNavigationAsync(decisionAlreadyMade, navType)
Brouter->>BrouterService: ApplyNavigationEffectsAsync / SaveScrollPositionAsync
BrouterService->>BitBrouterJS: applyNavigationEffects(...) / saveScrollPosition(...)
Poem
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/Brouter/Bit.Brouter/wwwroot/BitBrouter.js (1)
134-137: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider bounding
scrollPositionsgrowth.Every distinct URL leaves a permanent entry, and
persistScrollPositionswrites the whole map into a single storage slot on each save. Over a long-lived session (or query-param-heavy URLs like search/pagination) this grows unbounded and can hit the Web Storage quota; the quota error is swallowed inpersistScrollPositions, so persistence silently stops. A small cap (e.g. LRU eviction of oldest keys) keeps it bounded without changing behavior for typical apps.🤖 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/wwwroot/BitBrouter.js` around lines 134 - 137, The scroll position cache in `scrollPositions` is unbounded, so each new URL can permanently add entries and eventually overflow Web Storage. Update the save path around `scrollPositions.set(...)` and `persistScrollPositions()` to enforce a small cap on stored keys, evicting the oldest entries first (LRU/oldest-first is fine) before persisting. Keep the change localized to the scroll persistence logic in `BitBrouter.js` so typical `currentScroll()` behavior remains unchanged.src/Brouter/Bit.Brouter/BrouterService.cs (1)
368-427: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated catch blocks across the four JS-interop call sites.
ApplyNavigationEffectsAsync,SaveScrollPositionAsync,DisposeAsync, andGoAsyncall repeat the same four-exception swallow list. Consider extracting a smallSafeJsCallAsynchelper to centralize this pattern and reduce duplication.🤖 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/BrouterService.cs` around lines 368 - 427, The JS interop error handling is duplicated across ApplyNavigationEffectsAsync, SaveScrollPositionAsync, DisposeAsync, and GoAsync. Extract the repeated try/catch swallow pattern into a small shared helper such as SafeJsCallAsync in BrouterService, and have each of those call sites wrap their module invocation through it so the same four exception types are handled in one place.
🤖 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/Broute.cs`:
- Around line 122-125: Update the stale comment in Broute so it matches the
current constraint resolution flow: the parser in Broute.ParseTemplate should be
described as using the DI-container-scoped Brouter.Options.Constraints plus
built-in constraints only. Remove the reference to a “process-wide registry”
since the global static registry has been removed, and keep the comment aligned
with BrouterConstraintRegistry and BrouterTemplateParser.ParseTemplate.
In `@src/Brouter/Bit.Brouter/BroutePrerenderState.cs`:
- Around line 64-84: TryRestore currently only guards the JSON deserialization
path, but the Type.GetType lookup can still throw for stale or unloadable
persisted type names and should fall back to rerunning the loader instead of
crashing. Update BroutePrerenderState.TryRestore to catch type-resolution
failures around the Type.GetType(state.TypeName, throwOnError: false) step and
return false on those failures, using the same fallback behavior already used
for a missing type or JsonException.
In `@src/Brouter/Bit.Brouter/Brouter.cs`:
- Line 279: The `_navigating` flag in `Brouter` is never driven, so the
`Navigating` pending UI in `BuildRenderTree` never appears. Update
`ProcessNavigationAsync` (especially its loader loop) to set `_navigating` to
true when navigation starts and back to false when it completes or aborts, and
ensure state changes trigger a rerender so the `Navigating` fragment is
shown/hidden correctly. Keep the logic aligned with the existing `_navigating`
field and `Navigating` parameter behavior.
In `@src/Brouter/Bit.Brouter/BrouterRouteRenderer.cs`:
- Around line 99-100: The ApplyTypedParameters call in BrouterRouteRenderer can
leave reused component instances with stale route-parameter values when a
parameter disappears from the current template. Update the parameter application
logic around ApplyTypedParameters so that route parameters not present in the
current _route.TemplateParameterNames are explicitly emitted as default/cleared
frames instead of being skipped, ensuring reused components do not retain values
from a previous route.
In `@src/Brouter/README.md`:
- Around line 254-256: Clarify the navigation precedence wording in the README
so it explicitly states that when a fragment target resolves, it scrolls and
takes focus and no further focus handling occurs. Update the precedence sentence
around the navigation behavior description to make it clear that
FocusOnNavigateSelector only applies when no fragment target was resolved, so
the rules do not imply both focus targets run on the same navigation.
---
Nitpick comments:
In `@src/Brouter/Bit.Brouter/BrouterService.cs`:
- Around line 368-427: The JS interop error handling is duplicated across
ApplyNavigationEffectsAsync, SaveScrollPositionAsync, DisposeAsync, and GoAsync.
Extract the repeated try/catch swallow pattern into a small shared helper such
as SafeJsCallAsync in BrouterService, and have each of those call sites wrap
their module invocation through it so the same four exception types are handled
in one place.
In `@src/Brouter/Bit.Brouter/wwwroot/BitBrouter.js`:
- Around line 134-137: The scroll position cache in `scrollPositions` is
unbounded, so each new URL can permanently add entries and eventually overflow
Web Storage. Update the save path around `scrollPositions.set(...)` and
`persistScrollPositions()` to enforce a small cap on stored keys, evicting the
oldest entries first (LRU/oldest-first is fine) before persisting. Keep the
change localized to the scroll persistence logic in `BitBrouter.js` so typical
`currentScroll()` behavior remains unchanged.
🪄 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: b9b04859-9f89-402b-bc03-dd74ed9d7c93
📒 Files selected for processing (37)
src/Brouter/Bit.Brouter/Broute.cssrc/Brouter/Bit.Brouter/BroutePrerenderState.cssrc/Brouter/Bit.Brouter/BrouteScanner.cssrc/Brouter/Bit.Brouter/Brouter.cssrc/Brouter/Bit.Brouter/BrouterConstraintRegistry.cssrc/Brouter/Bit.Brouter/BrouterConstraints.cssrc/Brouter/Bit.Brouter/BrouterLink.cssrc/Brouter/Bit.Brouter/BrouterNavigationContext.cssrc/Brouter/Bit.Brouter/BrouterNavigationType.cssrc/Brouter/Bit.Brouter/BrouterOptions.cssrc/Brouter/Bit.Brouter/BrouterOutlet.cssrc/Brouter/Bit.Brouter/BrouterRouteConstraint.cssrc/Brouter/Bit.Brouter/BrouterRouteRenderer.cssrc/Brouter/Bit.Brouter/BrouterScrollPositionStorage.cssrc/Brouter/Bit.Brouter/BrouterService.cssrc/Brouter/Bit.Brouter/BrouterTemplateParser.cssrc/Brouter/Bit.Brouter/BrouterTemplateSegment.cssrc/Brouter/Bit.Brouter/wwwroot/BitBrouter.jssrc/Brouter/InteralDemos/Core/AppRouter.razorsrc/Brouter/InteralDemos/Core/Pages/DiscoveredPage.razorsrc/Brouter/InteralDemos/Core/Shared/Header.razorsrc/Brouter/README.mdsrc/Brouter/Tests/Bit.Brouter.Tests/BrouterLinkTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/BrouterTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/ConstraintHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/ConstraintIntegrationTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/ConstraintRegistryTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/DiscoveredPage.razorsrc/Brouter/Tests/Bit.Brouter.Tests/DiscoveryAndPersistenceTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/DiscoveryHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/GuardAndLoaderTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/NavigationEffectsHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/NavigationEffectsTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/NavigationTypeHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/NavigationTypeTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/PreventiveGuardHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/SampleDto.cs
💤 Files with no reviewable changes (1)
- src/Brouter/Bit.Brouter/BrouterConstraints.cs
✅ Files skipped from review due to trivial changes (5)
- src/Brouter/Tests/Bit.Brouter.Tests/DiscoveredPage.razor
- src/Brouter/Tests/Bit.Brouter.Tests/PreventiveGuardHost.razor
- src/Brouter/InteralDemos/Core/Shared/Header.razor
- src/Brouter/Bit.Brouter/BrouterNavigationType.cs
- src/Brouter/Tests/Bit.Brouter.Tests/BrouterLinkTests.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Brouter/InteralDemos/Core/AppRouter.razor
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/Brouter/Tests/Bit.Brouter.Tests/GuardAndLoaderTests.cs (1)
46-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest doesn't actually assert concurrent execution.
The test only asserts render completion within 3s; per the inline comment this is meant to prove overlap indirectly (a sequential implementation would time out). Consider directly asserting on an events/timestamp collection (similar to the sequential test above) to make the concurrency guarantee explicit and resistant to a slow-but-still-sequential regression that happens to finish under 3s.
🤖 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/GuardAndLoaderTests.cs` around lines 46 - 58, The ParallelLoaders_runs_chain_loaders_concurrently test is only checking that rendering finishes, which does not explicitly prove overlap. Update the test in GuardAndLoaderTests to assert concurrent execution directly, using the same event/timestamp collection approach as the sequential loader test and referencing ParallelLoadersHost so you can verify both loaders started before either completed. Keep the final render assertion if needed, but make the concurrency guarantee explicit by checking the recorded ordering/timing rather than relying on a timeout-based proxy.src/Brouter/Bit.Brouter/BrouterService.cs (1)
115-131: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider logging swallowed JS interop failures.
SafeJsCallAsyncsilently swallowsJSDisconnectedException,JSException,InvalidOperationException, andTaskCanceledExceptionacross all interop call sites (module import, scroll/focus effects,history.go, module dispose). This is reasonable for expected transient conditions (disconnected circuit, pre-render), but a genuine misconfiguration (e.g., wrong content path breaking the module import) would also be swallowed with zero diagnostics, making it hard to notice broken scroll/focus behavior in production.♻️ Optional: add debug-level logging on swallowed exceptions
- private static async ValueTask SafeJsCallAsync(Func<ValueTask> call) + private static async ValueTask SafeJsCallAsync(Func<ValueTask> call, ILogger? logger = null) { try { await call(); } - catch (JSDisconnectedException) { /* circuit disconnected mid-call */ } - catch (JSException) { /* JS interop failure (e.g. non-browser host) */ } - catch (InvalidOperationException) { /* JS interop unavailable during pre-render */ } - catch (TaskCanceledException) { /* component disposed mid-call */ } + catch (JSDisconnectedException ex) { logger?.LogDebug(ex, "Circuit disconnected mid-call."); } + catch (JSException ex) { logger?.LogDebug(ex, "JS interop failure."); } + catch (InvalidOperationException ex) { logger?.LogDebug(ex, "JS interop unavailable during pre-render."); } + catch (TaskCanceledException ex) { logger?.LogDebug(ex, "Component disposed mid-call."); } }🤖 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/BrouterService.cs` around lines 115 - 131, SafeJsCallAsync currently swallows JS interop exceptions with no diagnostics, which can hide real failures at call sites like GoAsync and the other interop uses in BrouterService. Update SafeJsCallAsync to log swallowed exceptions at debug level (or equivalent) with enough context before suppressing them, while still preserving the existing non-fatal behavior for JSDisconnectedException, JSException, InvalidOperationException, and TaskCanceledException. Keep the centralized handling in SafeJsCallAsync so all interop paths get the same logging 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/Bit.Brouter.csproj`:
- Around line 84-92: The BuildJavaScript target is not configuration-aware
because its Inputs/Outputs only track the TypeScript files and
wwwroot/bit-brouter.js, so a Debug build can incorrectly satisfy up-to-date
checks for a later Release build. Update the target in Bit.Brouter.csproj so the
incremental build key includes $(Configuration) or separate the Release
minify/move path into configuration-specific outputs, ensuring BuildJavaScript
reruns when switching between Debug and Release. Use the existing
BuildJavaScript target, the esbuild minify step, and the Move action as the
symbols to adjust.
In `@src/Brouter/Bit.Brouter/BrouterRelativeUrl.cs`:
- Around line 18-24: `IsRelative` in `BrouterRelativeUrl` does not treat
dot-prefixed refs with a مباشرة appended query or hash as relative, so `Resolve`
can send them through base-relative navigation instead of preserving the
intended target. Update `IsRelative(string url)` to recognize forms like ".?..."
and "..?..."/".#..."/"..#..." as relative alongside the existing "."/"./" and
".."/"../" cases, keeping the logic aligned with `Resolve`’s
query/hash-preservation behavior.
---
Nitpick comments:
In `@src/Brouter/Bit.Brouter/BrouterService.cs`:
- Around line 115-131: SafeJsCallAsync currently swallows JS interop exceptions
with no diagnostics, which can hide real failures at call sites like GoAsync and
the other interop uses in BrouterService. Update SafeJsCallAsync to log
swallowed exceptions at debug level (or equivalent) with enough context before
suppressing them, while still preserving the existing non-fatal behavior for
JSDisconnectedException, JSException, InvalidOperationException, and
TaskCanceledException. Keep the centralized handling in SafeJsCallAsync so all
interop paths get the same logging behavior.
In `@src/Brouter/Tests/Bit.Brouter.Tests/GuardAndLoaderTests.cs`:
- Around line 46-58: The ParallelLoaders_runs_chain_loaders_concurrently test is
only checking that rendering finishes, which does not explicitly prove overlap.
Update the test in GuardAndLoaderTests to assert concurrent execution directly,
using the same event/timestamp collection approach as the sequential loader test
and referencing ParallelLoadersHost so you can verify both loaders started
before either completed. Keep the final render assertion if needed, but make the
concurrency guarantee explicit by checking the recorded ordering/timing rather
than relying on a timeout-based proxy.
🪄 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: afea28b0-16f9-4a7e-b0b2-7c84c19ab68f
⛔ Files ignored due to path filters (1)
src/Brouter/Bit.Brouter/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (43)
.gitignoresrc/Brouter/Bit.Brouter.slnxsrc/Brouter/Bit.Brouter/Bit.Brouter.csprojsrc/Brouter/Bit.Brouter/Broute.cssrc/Brouter/Bit.Brouter/BroutePrerenderState.cssrc/Brouter/Bit.Brouter/Brouter.cssrc/Brouter/Bit.Brouter/BrouterLink.cssrc/Brouter/Bit.Brouter/BrouterNavigationContext.cssrc/Brouter/Bit.Brouter/BrouterRelativeUrl.cssrc/Brouter/Bit.Brouter/BrouterRouteData.cssrc/Brouter/Bit.Brouter/BrouterRouteRenderer.cssrc/Brouter/Bit.Brouter/BrouterService.cssrc/Brouter/Bit.Brouter/IBrouter.cssrc/Brouter/Bit.Brouter/Scripts/bit-brouter.tssrc/Brouter/Bit.Brouter/package.jsonsrc/Brouter/Bit.Brouter/tsconfig.jsonsrc/Brouter/README.mdsrc/Brouter/Tests/Bit.Brouter.Benchmarks/BenchmarkComponents.cssrc/Brouter/Tests/Bit.Brouter.Benchmarks/Bit.Brouter.Benchmarks.csprojsrc/Brouter/Tests/Bit.Brouter.Benchmarks/Harness.cssrc/Brouter/Tests/Bit.Brouter.Benchmarks/Program.cssrc/Brouter/Tests/Bit.Brouter.Benchmarks/README.mdsrc/Brouter/Tests/Bit.Brouter.Tests/AmbiguousRouteTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/AmbiguousRoutesHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/BrouterLinkTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/DiscoveryOverrideHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/GuardAndLoaderTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/MultiReplaceLinkHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/NavigationEffectsTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/NestedIndexHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/ParallelLoadersHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/RelativeLinkHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/RelativeNavigationTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/RelativeRedirectHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/RenderInvalidationHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/RenderInvalidationTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/ResolveUrlTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/RouteDataCascadeTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/SequentialLoadersHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/SwapRouteHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/TypedDataHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/TypedValueReader.razorsrc/Brouter/Tests/Bit.Brouter.Tests/ValueReader.razor
✅ Files skipped from review due to trivial changes (3)
- src/Brouter/Bit.Brouter/tsconfig.json
- src/Brouter/Bit.Brouter.slnx
- src/Brouter/Bit.Brouter/IBrouter.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Brouter/Bit.Brouter/BroutePrerenderState.cs
- src/Brouter/Tests/Bit.Brouter.Tests/NavigationEffectsTests.cs
- src/Brouter/Bit.Brouter/Broute.cs
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
src/Brouter/Bit.Brouter/BrouterView.cs (1)
41-59: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueStale registration if the cascading
Routeinstance itself changes.The unregister/re-register logic only handles a changed
Name, not a changedRoute(e.g., if the component is reparented to a differentBrouteinstance across renders, which can happen with dynamic outlet content). In that case the oldRoutekeeps the stale fragment registered under_registeredNamesince only the newRoutereceivesSetNamedView. This is an edge case unlikely to occur in typical usage whereBrouterViewis statically nested inside a fixedBroute.Optional defensive fix
protected override void OnParametersSet() { if (Route is null) throw new InvalidOperationException( "A BrouterView must be declared inside a Broute (its ChildContent), whose parent layout hosts the named outlet."); if (string.IsNullOrEmpty(Name)) throw new InvalidOperationException( "BrouterView requires a non-empty Name. The route's main content already renders in the primary (unnamed) outlet via Content/Component."); - // Re-register every parameter pass: a host re-render produces a fresh fragment instance - // that the outlet must pick up, and a changed Name must vacate the old slot. - if (_registeredName is not null && string.Equals(_registeredName, Name, StringComparison.Ordinal) is false) - { - Route.SetNamedView(_registeredName, null); - } + // Re-register every parameter pass: a host re-render produces a fresh fragment instance + // that the outlet must pick up, and a changed Name or Route must vacate the old slot. + if (_registeredName is not null && (_registeredRoute != Route || string.Equals(_registeredName, Name, StringComparison.Ordinal) is false)) + { + _registeredRoute?.SetNamedView(_registeredName, null); + } Route.SetNamedView(Name, ChildContent); _registeredName = Name; + _registeredRoute = Route; }🤖 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/BrouterView.cs` around lines 41 - 59, The OnParametersSet logic in BrouterView only clears the previous registration when Name changes, so a new cascading Route instance can leave the old Route with a stale named view. Update the re-registration flow to detect when Route itself changes, unregister the previously stored _registeredName from the old Route before switching, then register ChildContent on the current Route and refresh _registeredName. Keep the fix localized to BrouterView.OnParametersSet and the _registeredName bookkeeping.src/Brouter/Tests/Bit.Brouter.Tests/RevalidateTests.cs (1)
55-83: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssertions run before the fire-and-forget invoke completes.
Both
Revalidate_with_no_matched_chain_is_a_noop(Lines 56-67) andRoutes_without_loaders_revalidate_as_a_noop(Lines 69-83) callcut.InvokeAsync(() => brouter.RevalidateAsync().AsTask())without awaiting the returned task or wrapping the assertion incut.WaitForAssertion, unlike every other test in this class. IfRevalidateAsyncthrows or behaves asynchronously, these tests can pass trivially regardless of actual behavior, and any exception would be silently swallowed as an unobserved task fault.🧪 Proposed fix
- var brouter = Services.GetRequiredService<IBrouter>(); - cut.InvokeAsync(() => brouter.RevalidateAsync().AsTask()); - - Assert.AreEqual(0, cut.Instance.ParentLoaderRuns); - Assert.AreEqual(0, cut.Instance.ChildLoaderRuns); + var brouter = Services.GetRequiredService<IBrouter>(); + await cut.InvokeAsync(() => brouter.RevalidateAsync().AsTask()); + + Assert.AreEqual(0, cut.Instance.ParentLoaderRuns); + Assert.AreEqual(0, cut.Instance.ChildLoaderRuns);🤖 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/RevalidateTests.cs` around lines 55 - 83, The two no-op revalidation tests are asserting before `IBrouter.RevalidateAsync` finishes because `cut.InvokeAsync(...)` is fire-and-forget. Update `Revalidate_with_no_matched_chain_is_a_noop` and `Routes_without_loaders_revalidate_as_a_noop` to await the `InvokeAsync` task or verify results via `cut.WaitForAssertion`, matching the async pattern used elsewhere in `RevalidateTests`. Ensure the assertions on `ParentLoaderRuns`, `ChildLoaderRuns`, and the rendered `other` element happen only after `RevalidateAsync` completes so failures are observed.
🤖 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.Generators/BrouterRoutesGenerator.cs`:
- Around line 51-69: The route generator currently deduplicates templates in
Emit but still passes wildcard routes through, and EmitMethod does not handle
SegmentKind.Wildcard. Update BrouterRoutesGenerator so wildcard templates
emitted by RouteTemplateParser are either skipped during Emit or surfaced as a
diagnostic before generation, and make sure EmitMethod/related switch logic has
an explicit path for SegmentKind.Wildcard to avoid producing incomplete URL
builders.
- Around line 34-69: Duplicate route templates are still resolved by whichever
entry is seen first in BrouterRoutesGenerator.Emit, which silently drops
conflicting explicit names. Update the dedup logic in Emit to detect when the
same Template appears with conflicting route metadata (especially different
non-null Name values) and report a diagnostic through the generator context
instead of choosing a winner arbitrarily; keep the existing stable sort/output
path unchanged.
In `@src/Brouter/Bit.Brouter/BrouterAwait.cs`:
- Around line 41-68: The async render path in BrouterAwait’s ObserveAsync can
resume after the component has started disposing, so guard the final
InvokeAsync(StateHasChanged) call with a disposal check. Add a _disposed flag or
cancellation token to BrouterAwait, set it during disposal, and ensure
ObserveAsync exits without scheduling a render once disposal has begun; keep the
existing ReferenceEquals(_observed, task) check as part of the guard.
In `@src/Brouter/README.md`:
- Around line 320-342: Clarify the caching behavior in the README section around
preloading and loader caching: “No StaleTime anywhere → no caching” applies only
to the normal loader cache, while preloaded results are cached separately and
may still be reused during navigation via PreloadAsync and BrouterLinkPreload
with PreloadStaleTime. Update the wording in the StaleTime/preloading
explanation to distinguish the loader cache from the preload cache and avoid
implying that all caching is disabled when StaleTime is absent.
In
`@src/Brouter/Tests/Bit.Brouter.Generators.Tests/Bit.Brouter.Generators.Tests.csproj`:
- Around line 1-27: The test project is missing the shared build import/target
setup, so CI cannot resolve InstallNodejsDependencies. Update
Bit.Brouter.Generators.Tests.csproj to match the generator project’s build
integration by importing the shared Bit.Build.props/Bit.Build.targets pattern
and ensuring the InstallNodejsDependencies target is available. Use the existing
Bit.Brouter.Generators.csproj and its build-related symbols as the reference for
where the common build files and target wiring should be added.
In `@src/Brouter/Tests/Bit.Brouter.Tests/NavigateAsyncTests.cs`:
- Around line 20-129: The test methods in NavigateAsyncTests are reading the
navigation ValueTask before the cut.InvokeAsync dispatch has completed, so
navigation can still be the default value and produce a bogus outcome. Update
the affected tests to be async Task methods, await cut.InvokeAsync when
assigning navigation from BrouterNavigation.NavigateAsync, and then call Await
only after the dispatch has finished so the real ValueTask is observed.
---
Nitpick comments:
In `@src/Brouter/Bit.Brouter/BrouterView.cs`:
- Around line 41-59: The OnParametersSet logic in BrouterView only clears the
previous registration when Name changes, so a new cascading Route instance can
leave the old Route with a stale named view. Update the re-registration flow to
detect when Route itself changes, unregister the previously stored
_registeredName from the old Route before switching, then register ChildContent
on the current Route and refresh _registeredName. Keep the fix localized to
BrouterView.OnParametersSet and the _registeredName bookkeeping.
In `@src/Brouter/Tests/Bit.Brouter.Tests/RevalidateTests.cs`:
- Around line 55-83: The two no-op revalidation tests are asserting before
`IBrouter.RevalidateAsync` finishes because `cut.InvokeAsync(...)` is
fire-and-forget. Update `Revalidate_with_no_matched_chain_is_a_noop` and
`Routes_without_loaders_revalidate_as_a_noop` to await the `InvokeAsync` task or
verify results via `cut.WaitForAssertion`, matching the async pattern used
elsewhere in `RevalidateTests`. Ensure the assertions on `ParentLoaderRuns`,
`ChildLoaderRuns`, and the rendered `other` element happen only after
`RevalidateAsync` completes so failures are observed.
🪄 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: 931608d6-38f7-480f-9527-d182d4ca6dd0
📒 Files selected for processing (75)
src/Brouter/Bit.Brouter.Generators/Bit.Brouter.Generators.csprojsrc/Brouter/Bit.Brouter.Generators/BrouterRoutesGenerator.cssrc/Brouter/Bit.Brouter.Generators/IsExternalInit.cssrc/Brouter/Bit.Brouter.Generators/RazorRouteScanner.cssrc/Brouter/Bit.Brouter.Generators/RouteModel.cssrc/Brouter/Bit.Brouter.Generators/RouteTemplateParser.cssrc/Brouter/Bit.Brouter.slnxsrc/Brouter/Bit.Brouter/Broute.cssrc/Brouter/Bit.Brouter/BroutePrerenderState.cssrc/Brouter/Bit.Brouter/Brouter.cssrc/Brouter/Bit.Brouter/BrouterAwait.cssrc/Brouter/Bit.Brouter/BrouterErrorContext.cssrc/Brouter/Bit.Brouter/BrouterLink.cssrc/Brouter/Bit.Brouter/BrouterLinkPreload.cssrc/Brouter/Bit.Brouter/BrouterLoaderCache.cssrc/Brouter/Bit.Brouter/BrouterLocation.cssrc/Brouter/Bit.Brouter/BrouterNavigationContext.cssrc/Brouter/Bit.Brouter/BrouterNavigationOutcome.cssrc/Brouter/Bit.Brouter/BrouterOptions.cssrc/Brouter/Bit.Brouter/BrouterOutlet.cssrc/Brouter/Bit.Brouter/BrouterQueryBuilder.cssrc/Brouter/Bit.Brouter/BrouterRouteRenderer.cssrc/Brouter/Bit.Brouter/BrouterService.cssrc/Brouter/Bit.Brouter/BrouterStaleReloadMode.cssrc/Brouter/Bit.Brouter/BrouterView.cssrc/Brouter/Bit.Brouter/IBrouter.cssrc/Brouter/Bit.Brouter/Scripts/bit-brouter.tssrc/Brouter/InteralDemos/Core/AppRouter.razorsrc/Brouter/InteralDemos/Core/Bit.Brouter.Demos.Core.csprojsrc/Brouter/InteralDemos/Core/DemoState.cssrc/Brouter/InteralDemos/Core/Extensions/IServiceCollectionExtensions.cssrc/Brouter/InteralDemos/Core/Pages/DataPage.razorsrc/Brouter/InteralDemos/Core/Pages/DeferredPage.razorsrc/Brouter/InteralDemos/Core/Pages/EditorPage.razorsrc/Brouter/InteralDemos/Core/Pages/HistoryStatePage.razorsrc/Brouter/InteralDemos/Core/Pages/HomePage.razorsrc/Brouter/InteralDemos/Core/Pages/OutcomesPage.razorsrc/Brouter/InteralDemos/Core/Pages/StickyNotePage.razorsrc/Brouter/InteralDemos/Core/Pages/UnstablePage.razorsrc/Brouter/InteralDemos/Core/Shared/Header.razorsrc/Brouter/README.mdsrc/Brouter/Tests/Bit.Brouter.Generators.Tests/Bit.Brouter.Generators.Tests.csprojsrc/Brouter/Tests/Bit.Brouter.Generators.Tests/BrouterRoutesGeneratorTests.cssrc/Brouter/Tests/Bit.Brouter.Generators.Tests/GeneratorTestHarness.cssrc/Brouter/Tests/Bit.Brouter.Tests/CacheHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/CachedView.razorsrc/Brouter/Tests/Bit.Brouter.Tests/ChildDataView.razorsrc/Brouter/Tests/Bit.Brouter.Tests/DeferredDataTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/DeferredHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/DiscoveryAndPersistenceTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/ErrorContentHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/ErrorContentTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/GroupRouteHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/GroupRouteTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/HistoryStateHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/HistoryStateTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/KeepAliveHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/KeepAliveTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/LeaveGuardHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/LeaveGuardTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/LoaderCacheTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/NamedOutletHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/NamedOutletTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/NavigateAsyncHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/NavigateAsyncTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/NotFoundInteropHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/NotFoundInteropTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/OnNavigateAsyncTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/OnNavigateHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/QueryBuilderTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/RevalidateHost.razorsrc/Brouter/Tests/Bit.Brouter.Tests/RevalidateTests.cssrc/Brouter/Tests/Bit.Brouter.Tests/SampleDto.cssrc/Brouter/Tests/Bit.Brouter.Tests/StatefulCounter.razorsrc/Brouter/Tests/Bit.Brouter.Tests/ViewTransitionTests.cs
✅ Files skipped from review due to trivial changes (4)
- src/Brouter/Bit.Brouter/BrouterLinkPreload.cs
- src/Brouter/Tests/Bit.Brouter.Tests/KeepAliveHost.razor
- src/Brouter/InteralDemos/Core/DemoState.cs
- src/Brouter/Bit.Brouter.slnx
🚧 Files skipped from review as they are similar to previous changes (6)
- src/Brouter/Bit.Brouter/BroutePrerenderState.cs
- src/Brouter/Bit.Brouter/BrouterNavigationContext.cs
- src/Brouter/Bit.Brouter/Broute.cs
- src/Brouter/Bit.Brouter/BrouterLink.cs
- src/Brouter/Bit.Brouter/BrouterRouteRenderer.cs
- src/Brouter/Bit.Brouter/BrouterService.cs
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedReview finished.
|
This comment was marked as resolved.
This comment was marked as resolved.
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 #12559
beforeunloaddialog, always-on or toggled at runtime.NavigateAsynctells you if it succeeded, was cancelled, redirected, not found, failed, or superseded.ErrorContentrenders failures in place, with retry.NavigationManager.NotFound()support and real HTTP 404s under static SSR.StaleTimemakes Back/Forward instant; stale data renders now, refreshes in background.<BrouterAwait>streams slow data in after the page reveals.BrouterView.Bit.Brouter.Generators— compile-time-safe URL builders generated from your route declarations.Plus: 4 pre-existing bugs fixed, 77 tests added (205 total), a demo for every feature, full README coverage.
Summary by CodeRabbit
New Features
<Broute>route component, including improved route/query parameter binding (by name), container-scoped custom constraints, named outlets, keep-alive behavior, and discovery-based@pagematching.Bug Fixes
Documentation
<Broute>.