Make sidebar nav groups with landing pages collapsible and fix mobile drawer closing on expand#786
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughNavigationDocs.jsx refactors navigation component behavior to support mobile-aware group expansion, href-based header navigation, independent chevron control, and explicit nested link rendering. Two integration guide files (Azure AD and Zitadel) are removed. ChangesNavigation Component & Documentation Cleanup
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/components/NavigationDocs.jsx (1)
974-989: 💤 Low valueConsider guarding
router.pushto avoid redundant navigation.Line 977 calls
router.push(group.href)unconditionally, even whenrouter.pathname === group.href. This triggers a redundant navigation when clicking the header of the group you're already on.Consider wrapping it in a conditional:
🔧 Proposed optimization
onClick={() => { if (group.href) { if (!isOpen) setIsOpen(true) - router.push(group.href) + if (router.pathname !== group.href) { + router.push(group.href) + } setActiveHighlight() return }🤖 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/NavigationDocs.jsx` around lines 974 - 989, The onClick handler calls router.push(group.href) unconditionally which can trigger redundant navigation; update the handler in the NavigationDocs component to check the current route before pushing (e.g., compare router.pathname or router.asPath to group.href) and only call router.push(group.href) when they differ, preserving the existing setIsOpen and setActiveHighlight logic and all other conditions (references: router.push, router.pathname/router.asPath, group.href, isOpen, setIsOpen, setActiveHighlight, isActiveGroup, isInsideMobileNavigation, group.links).
🤖 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/NavigationDocs.jsx`:
- Around line 1058-1074: The key for the nested NavigationGroup is unstable
because it includes isOpen (key={link.title + isOpen}), causing remounts and
state loss; change the key to a stable identifier (for example use link.href or
link.id or link.title alone) so NavigationGroup retains its identity across
parent toggles—update the JSX that renders <NavigationGroup ... key={...} /> to
remove isOpen from the key and use a permanent unique property on link instead.
- Around line 954-961: The component initializes isOpen with useState but never
updates it on route changes, so groups containing the active route can remain
collapsed; add a useEffect (import useEffect from React) that depends on
[router.pathname, isActiveGroup] and calls setIsOpen(true) when isActiveGroup
becomes true (you can leave collapse behaviour unchanged so it only auto-opens
groups that contain the current route). Locate the isActiveGroup computation and
the isOpen/setIsOpen state (symbols: isActiveGroup, isOpen, setIsOpen,
findActiveGroupIndex, router.pathname) and implement the effect there to
synchronize expansion with navigation.
---
Nitpick comments:
In `@src/components/NavigationDocs.jsx`:
- Around line 974-989: The onClick handler calls router.push(group.href)
unconditionally which can trigger redundant navigation; update the handler in
the NavigationDocs component to check the current route before pushing (e.g.,
compare router.pathname or router.asPath to group.href) and only call
router.push(group.href) when they differ, preserving the existing setIsOpen and
setActiveHighlight logic and all other conditions (references: router.push,
router.pathname/router.asPath, group.href, isOpen, setIsOpen,
setActiveHighlight, isActiveGroup, isInsideMobileNavigation, group.links).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c4018db8-2ae1-47fc-b592-138a3ae86aee
📒 Files selected for processing (3)
docs/integrations/identity-providers/self-hosted/azure-ad.mddocs/integrations/identity-providers/self-hosted/zitadel.mdsrc/components/NavigationDocs.jsx
💤 Files with no reviewable changes (2)
- docs/integrations/identity-providers/self-hosted/zitadel.md
- docs/integrations/identity-providers/self-hosted/azure-ad.md
SunsetDrifter
approved these changes
Jun 12, 2026
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
2 participants
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
Sidebar items that have both a landing page and child links previously rendered as plain links with their sub pages always expanded, and the
isOpen: falsewas ignored. These items now render as collapsible groups and clicking the title navigates to the landing page. This also fixes a mobile bug where tapping a chevron to expand a group closed the entire nav drawer.Changes
docsNavigationwith alinksarray now render asNavigationGroupinstead of an always-expandedNavLink, so groups with their ownhrefare collapsible.href, the title click navigates to the landing page (opening the group if closed) and on click it toggles collapse without navigating.isOpen: falsein the navigation data is now respected,useIsInsideMobileNavigation), which previously firedrouteChangeStartand closed the drawer.docs/integrations/identity-providers/self-hosted/, two orphaned Docusaurus-era files.Summary by CodeRabbit
Documentation
User Interface