fix(navbar-menu): menu element cannot be found in the DOM after navigation #96
Conversation
WalkthroughThe pull request introduces several modifications primarily to the Changes
Possibly related PRs
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
desmondinho
temporarily deployed
to
Production
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
src/LumexUI/Components/Navbar/NavbarContext.cs (2)
Line range hint
1-14: LGTM! Good architectural improvements.The simplified
NavbarContextclass now follows better architectural principles:
- Removes state management responsibility, adhering to Single Responsibility Principle
- Allows components to manage their own state, improving lifecycle handling
- Simplifies the navigation scenario by reducing shared state
These changes should effectively address the issue of menu not being found in DOM after navigation.
10-13: Consider adding null check and disposal.While the implementation is functionally correct, consider adding:
- Null check to prevent accidental unregistration
- Disposal of the previous menu if a new one is registered
public void RegisterMenu( LumexNavbarMenu menu ) { + if (Menu is IDisposable disposable) + { + disposable.Dispose(); + } + ArgumentNullException.ThrowIfNull(menu); Menu = menu; }src/LumexUI/Components/Navbar/LumexNavbarMenuToggle.razor.cs (1)
48-51: Consider adding error handling for null Menu.The Toggle method silently fails if Menu is null. Consider adding logging or user feedback in this case.
private void Toggle() { - Context.Menu?.Toggle(); + if (Context.Menu is null) + { + Console.WriteLine("Warning: Menu is not initialized"); + return; + } + Context.Menu.Toggle(); }src/LumexUI/Components/Navbar/LumexNavbarMenu.razor.cs (3)
18-18: Good architectural improvement moving away from JS interop!The shift from
IJSRuntimetoNavigationManagerand implementingIDisposablerepresents a more maintainable approach by:
- Reducing JS interop dependencies
- Properly managing event subscriptions
- Following Blazor's recommended patterns for navigation handling
Also applies to: 27-27
29-29: Add XML documentation for the Expanded property.Consider adding XML documentation to explain the property's purpose and usage:
+ /// <summary> + /// Gets or sets whether the menu is currently expanded. + /// </summary> internal bool Expanded { get; private set; }
57-60: Add XML documentation for the Toggle method.Consider adding XML documentation to explain the method's purpose:
+ /// <summary> + /// Toggles the expanded state of the menu and triggers a UI refresh. + /// </summary> internal void Toggle()src/LumexUI/Styles/Navbar.cs (2)
144-166: Consider using an enum for the slot parameter.The
slotparameter being a string makes the code prone to runtime errors. Consider using an enum to make it type-safe.-private static ElementClass GetBlurredStyles(bool blurred, string slot) +private enum NavbarSlot { Base, Menu } +private static ElementClass GetBlurredStyles(bool blurred, NavbarSlot slot) { return blurred switch { false => ElementClass.Empty() - .Add("bg-background", when: slot is nameof(_base)) - .Add("bg-background", when: slot is nameof(_menu)), + .Add("bg-background", when: slot is NavbarSlot.Base or NavbarSlot.Menu),
152-160: Document the Chrome-specific fix inline.The Stack Overflow link might become unavailable. Consider documenting the fix's purpose directly in the code.
- // https://stackoverflow.com/questions/60997948/backdrop-filter-not-working-for-nested-elements-in-chrome + // Chrome requires a pseudo-element for nested backdrop-filter to work correctly. + // The pseudo-element creates a new stacking context, preventing the blur effect from being clipped.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
docs/LumexUI.Docs.Client/Components/Header.razor(1 hunks)src/LumexUI/Components/Navbar/LumexNavbarMenu.razor(1 hunks)src/LumexUI/Components/Navbar/LumexNavbarMenu.razor.cs(3 hunks)src/LumexUI/Components/Navbar/LumexNavbarMenuToggle.razor(1 hunks)src/LumexUI/Components/Navbar/LumexNavbarMenuToggle.razor.cs(2 hunks)src/LumexUI/Components/Navbar/NavbarContext.cs(1 hunks)src/LumexUI/Styles/Navbar.cs(2 hunks)
🔇 Additional comments (15)
src/LumexUI/Components/Navbar/LumexNavbarMenu.razor (1)
7-8: LGTM! The state management change addresses the navigation issue.
The switch from @Context.IsMenuExpanded to @Expanded improves component-level state management, which should fix the menu element not being found in DOM after navigation.
Let's verify the implementation of state management and cleanup:
#!/bin/bash
# Description: Verify the implementation of IDisposable and navigation handling
# Test 1: Check if IDisposable is properly implemented
echo "Checking IDisposable implementation..."
ast-grep --pattern 'public void Dispose() {
$$$
_navigationManager.LocationChanged -= HandleLocationChanged;
$$$
}'
# Test 2: Verify navigation event subscription
echo "Checking navigation event subscription..."
ast-grep --pattern 'protected override void OnInitialized() {
$$$
_navigationManager.LocationChanged += HandleLocationChanged;
$$$
}'
# Test 3: Look for any direct DOM manipulation that might interfere
echo "Checking for direct DOM manipulation..."
rg -l "IJSRuntime|JSRuntime" --type cssrc/LumexUI/Components/Navbar/LumexNavbarMenuToggle.razor (2)
8-8: Verify menu state persistence during navigation
The null-conditional operator adds safety but might mask underlying initialization issues. Ensure that Context.Menu is properly initialized before navigation occurs.
#!/bin/bash
# Search for Menu initialization in the Context class
ast-grep --pattern 'class NavbarContext {
$$$
Menu = $_;
$$$
}'
# Search for potential null checks or error handling
rg "Context\.Menu\?" -A 310-10: Consider documenting the Toggle method implementation
The click handler has been moved from Context to a local Toggle method. Please ensure the Toggle method implementation includes proper error handling and state management.
#!/bin/bash
# Search for the Toggle method implementation
ast-grep --pattern 'class LumexNavbarMenuToggle {
$$$
Toggle($$$) {
$$$
}
$$$
}'src/LumexUI/Components/Navbar/LumexNavbarMenuToggle.razor.cs (4)
9-9: LGTM! Proper implementation of event handling infrastructure.
The addition of the Routing namespace and IDisposable interface implementation follows best practices for components that need to handle navigation events and clean up subscriptions.
Also applies to: 17-17
21-22: LGTM! Proper dependency injection setup.
The NavigationManager is correctly injected as a private service with appropriate null-safety handling.
53-56: LGTM! Proper resource cleanup implementation.
The Dispose method correctly unsubscribes from the LocationChanged event, preventing memory leaks.
41-46: Verify Context initialization timing.
While the null check for Context is present, we should ensure it's always initialized before the LocationChanged event fires. Consider moving the event subscription after the Context null check to prevent any race conditions.
#!/bin/bash
# Search for any other components that might affect Context initialization
ast-grep --pattern 'class $_ : NavbarContext {
$$$
}'docs/LumexUI.Docs.Client/Components/Header.razor (2)
38-40: Verify padding changes don't cause layout shifts.
The added padding classes look good for spacing, but since this PR addresses a navigation-related issue, please verify that these padding adjustments don't contribute to any layout shifts during navigation transitions.
#!/bin/bash
# Description: Check if NavMenu has any navigation-related layout styles or classes
# that might conflict with the new padding
# Look for any layout-related styles in NavMenu
rg -A 5 'class=' docs/LumexUI.Docs.Client/Components/NavMenu.razor38-40: Verify NavMenu's navigation handling.
Since the menu element was reported to disappear from DOM after navigation, and this component contains the NavMenu, please ensure that the NavMenu component properly handles navigation events and maintains its state.
#!/bin/bash
# Description: Check NavMenu's implementation for navigation-related code
# Look for navigation manager usage or navigation event handlers
ast-grep --pattern 'class NavMenu { $$$ NavigationManager $$$ }'
# Look for IDisposable implementation which might be needed for cleanup
ast-grep --pattern 'class NavMenu : IDisposable { $$$ }'src/LumexUI/Components/Navbar/LumexNavbarMenu.razor.cs (3)
67-69: LGTM! Proper resource cleanup implemented.
The Dispose method correctly unsubscribes from the LocationChanged event, preventing memory leaks.
53-55: Verify the RegisterMenu method exists in NavbarContext.
The initialization looks correct, but let's verify the existence and signature of the RegisterMenu method.
#!/bin/bash
# Description: Verify the RegisterMenu method exists in NavbarContext
# Expected: Find the method declaration in NavbarContext
ast-grep --pattern 'class NavbarContext {
$$$
RegisterMenu($_) {
$$$
}
$$$
}'63-64: Consider handling same-page navigation differently.
The current implementation toggles the menu on every location change, including same-page navigation. Consider if this is the desired behavior.
#!/bin/bash
# Description: Check if there are any same-page navigation scenarios in the codebase
# Expected: Find potential same-page navigation implementations that might be affected
rg -g '*.razor' -g '*.cs' 'NavigateTo.*fragment|#.*href'src/LumexUI/Styles/Navbar.cs (3)
174-174: LGTM!
The change correctly integrates the new GetBlurredStyles method for base navbar styling.
229-229: LGTM!
The change correctly integrates the new GetBlurredStyles method for menu styling.
144-166: Verify alignment with PR objective.
The styling changes look good but don't seem to directly address the PR's objective of fixing the menu element's DOM presence after navigation. Please verify if these changes are part of a larger fix or if additional changes are needed.
#!/bin/bash
# Description: Search for related navigation handling code in navbar components
# Test: Look for navigation-related code in navbar components
rg -t cs "navigation|router|route" --glob "src/LumexUI/**/*Navbar*.cs"
# Test: Look for DOM-related code in navbar components
rg -t cs "DOM|Document|Element" --glob "src/LumexUI/**/*Navbar*.cs"Also applies to: 174-174, 229-229
Summary by CodeRabbit
Release Notes
New Features
LumexNavbarMenuto improve visual appearance.LumexNavbarMenuandLumexNavbarMenuTogglecomponents, allowing for better responsiveness to location changes.Bug Fixes
Refactor
Navbarstruct for better clarity and maintainability.Chores