Update site navigation#982
Merged
Merged
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:
📝 WalkthroughWalkthroughReplaces the previous flyout/mobile-menu navbar with a typed, data-driven mega menu system. New internal components handle animated pane transitions, desktop triggers, mobile collapsible groups, and library browsing. CSS view-transition animations and panel visibility rules are added with dark-mode and reduced-motion support. Library navbar titles are simplified to remove version props, and workshop layout overrides are updated to remove sidebar width offsets. ChangesMega Menu Navigation Overhaul
Sequence DiagramsequenceDiagram
participant User
participant DesktopNavTrigger
participant Navbar
participant MegaMenuContentTransition
participant MegaMenuContent
User->>DesktopNavTrigger: pointerenter / focus
DesktopNavTrigger->>Navbar: onOpen(menuKey, direction)
Navbar->>Navbar: cancelCloseTimer, setActiveMenuKey, recalcLayout
Navbar->>MegaMenuContentTransition: activeMenuKey, direction, layout
MegaMenuContentTransition->>MegaMenuContent: render pane (view-transition animates)
MegaMenuContent->>MegaMenuContent: switch libraries vs general group
User->>DesktopNavTrigger: pointerleave
DesktopNavTrigger->>Navbar: scheduleClose(CLOSE_DELAY_MS)
Navbar->>Navbar: closeTimer fires → setActiveMenuKey(null)
MegaMenuContentTransition->>MegaMenuContent: pane animation → hidden
User->>Navbar: Escape keydown / outside pointerdown
Navbar->>Navbar: clearTimer, setActiveMenuKey(null), closeMobileDrawer
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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/routes/-library-landing-route.tsx (1)
116-121: ⚡ Quick winRemove the stale required
versionprop fromLibraryNavbarTitleprops.Line 120 still requires
version: string, but this component no longer reads or renders version data. Keeping it required preserves an unnecessary contract and forces avoidable prop plumbing.Proposed diff
export function LibraryNavbarTitle({ libraryId, }: { libraryId: LandingLibraryId - version: string }) {🤖 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/routes/-library-landing-route.tsx` around lines 116 - 121, The LibraryNavbarTitle function component has a stale version property in its props type definition that is no longer used by the component. Remove the version: string line from the props type destructuring in the LibraryNavbarTitle function signature, keeping only the libraryId property which is actually being used.
🤖 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/components/Navbar.tsx`:
- Around line 1022-1032: When activeKey becomes null in the useLayoutEffect
hook, the panes remain mounted and focusable for MEGA_MENU_PANEL_CLOSE_MS even
though they are visually hidden via CSS, creating an accessibility issue. Fix
this by either calling setPanes([]) synchronously when activeKey becomes null
(removing the setTimeout delay) or immediately mark the closing pane as
non-focusable/inert before the timeout begins. This same issue appears at two
locations in the file around lines 1022-1032 and lines 1090-1096, so apply the
same fix to both useLayoutEffect hooks that handle pane closure.
- Around line 981-1000: The MobileMenuGroup component currently wraps the entire
group header in a CollapsibleTrigger, which makes the group label a disclosure
button rather than a direct link to group.to. To fix this, restructure the
CollapsibleTrigger to separate the navigation link from the expansion
affordance. Make the group label itself a direct link using group.to, and move
the collapsible trigger functionality (the arrow icon or expand indicator) to a
separate clickable element that controls the disclosure without triggering
navigation. This way users can directly navigate to Libraries on mobile while
still having the ability to expand the menu group.
- Around line 924-945: The Link and button trigger elements in the Navbar
component are declaring aria-haspopup="menu" and aria-expanded attributes, but
the popup content is regular navigation content that doesn't follow menu widget
patterns with proper focus management. Remove both aria-haspopup="menu" and
aria-expanded attributes from the Link and button elements since the popup
doesn't implement true menu semantics and focus continues in normal document
order.
In `@src/styles/app.css`:
- Line 145: The background property value `currentColor` is using incorrect
casing according to Stylelint's value-keyword-case rule. Change `currentColor`
to `currentcolor` (all lowercase) in the background property to normalize the
keyword casing and resolve the lint violation.
---
Nitpick comments:
In `@src/routes/-library-landing-route.tsx`:
- Around line 116-121: The LibraryNavbarTitle function component has a stale
version property in its props type definition that is no longer used by the
component. Remove the version: string line from the props type destructuring in
the LibraryNavbarTitle function signature, keeping only the libraryId property
which is actually being used.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 72d59333-7132-498a-b1d6-723b4e2606ec
📒 Files selected for processing (4)
src/components/Navbar.tsxsrc/routes/-library-landing-route.tsxsrc/routes/workshops.tsxsrc/styles/app.css
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/components/Collapsible.tsx`:
- Around line 94-98: The onClick handler in the Collapsible component calls
toggle() before the consumer's onClick callback, preventing consumers from
canceling the toggle via event.preventDefault(). Reorder the logic in the
onClick handler to invoke the consumer's onClick callback first, then check if
event.defaultPrevented is true before calling toggle(). This allows consumers to
prevent toggling when needed by calling preventDefault() in their onClick
handler.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 98aa73ba-3ee2-461d-95dd-e9e7cd86173c
📒 Files selected for processing (21)
src/components/Collapsible.tsxsrc/components/Navbar.tsxsrc/routes/-library-landing-route.tsxsrc/routes/ai.$version.index.tsxsrc/routes/cli.$version.index.tsxsrc/routes/config.$version.index.tsxsrc/routes/db.$version.index.tsxsrc/routes/devtools.$version.index.tsxsrc/routes/form.$version.index.tsxsrc/routes/hotkeys.$version.index.tsxsrc/routes/intent.$version.index.tsxsrc/routes/pacer.$version.index.tsxsrc/routes/query.$version.index.tsxsrc/routes/ranger.$version.index.tsxsrc/routes/router.$version.index.tsxsrc/routes/start.$version.index.tsxsrc/routes/store.$version.index.tsxsrc/routes/table.$version.index.tsxsrc/routes/virtual.$version.index.tsxsrc/routes/workflow.$version.index.tsxsrc/styles/app.css
💤 Files with no reviewable changes (1)
- src/routes/-library-landing-route.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/styles/app.css
- src/components/Navbar.tsx
Comment on lines
94
to
98
| onClick={(e) => { | ||
| e.stopPropagation() | ||
| toggle() | ||
| onClick?.(e) | ||
| }} |
There was a problem hiding this comment.
Honor consumer click cancellation before toggling.
On Line 96, toggle() runs before the consumer onClick (Line 97). With the new button-props API, consumers cannot prevent toggling via event.preventDefault(), which makes the trigger hard to compose in controlled flows.
Suggested fix
onClick={(e) => {
e.stopPropagation()
- toggle()
- onClick?.(e)
+ onClick?.(e)
+ if (!e.defaultPrevented) {
+ toggle()
+ }
}}📝 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
| onClick={(e) => { | |
| e.stopPropagation() | |
| toggle() | |
| onClick?.(e) | |
| }} | |
| onClick={(e) => { | |
| e.stopPropagation() | |
| onClick?.(e) | |
| if (!e.defaultPrevented) { | |
| toggle() | |
| } | |
| }} |
🤖 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/components/Collapsible.tsx` around lines 94 - 98, The onClick handler in
the Collapsible component calls toggle() before the consumer's onClick callback,
preventing consumers from canceling the toggle via event.preventDefault().
Reorder the logic in the onClick handler to invoke the consumer's onClick
callback first, then check if event.defaultPrevented is true before calling
toggle(). This allows consumers to prevent toggling when needed by calling
preventDefault() in their onClick handler.
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.
Summary
Validation
Note: pnpm test passes with existing lint warnings in shop/admin utility files unrelated to this navigation change.
Summary by CodeRabbit