fix: Automatic collapse via pathname#3199
Conversation
revogabe
requested review from
chronark,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@revogabe is attempting to deploy a commit to the Unkey Team on Vercel. A member of the Team first needs to authorize it. |
📝 Walkthrough""" WalkthroughThe changes refactor the logic in the Changes
Assessment against linked issues
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
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. 🪧 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 (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![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
🧹 Nitpick comments (1)
apps/dashboard/components/navigation/sidebar/app-sidebar/components/nav-items/nested-nav-item.tsx (1)
165-169: Simplified active state logic.The component now correctly uses the item's active state directly, making the code more readable and maintainable by removing conditional logic complexity.
The comment on line 165 is now slightly misaligned with the implementation since we're no longer excluding child activity from affecting the parent's highlight. Consider updating the comment to match the current behavior if this is the intended design.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/dashboard/components/navigation/sidebar/app-sidebar/components/nav-items/nested-nav-item.tsx(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/dashboard/components/navigation/sidebar/app-sidebar/components/nav-items/nested-nav-item.tsx (2)
apps/dashboard/components/ui/sidebar.tsx (2)
SidebarMenuItem(596-596)SidebarMenuButton(595-595)apps/dashboard/components/navigation/sidebar/app-sidebar/components/nav-items/utils.ts (1)
getButtonStyles(3-11)
🔇 Additional comments (4)
apps/dashboard/components/navigation/sidebar/app-sidebar/components/nav-items/nested-nav-item.tsx (4)
11-12: Good addition of pathname and lifecycle hooks.The addition of
usePathnameanduseLayoutEffecthooks is well-aligned with the PR objective to control sidebar collapse based on the current path.
32-35: Improved state initialization based on pathname.Setting the initial state of
isOpenbased on the pathname creates a direct connection between the navigation state and the URL, which solves the mentioned issue in PR #3183.
37-44: Good implementation of automatic sidebar state control.The
useLayoutEffecthook is an excellent choice here as it ensures the UI updates synchronously before browser paint, preventing flickering. The logic properly tracks both direct matches and active children to determine when a navigation item should be open.Using
useLayoutEffectinstead ofuseEffectwas a good decision for UI changes that could cause layout shifts.
158-158: Properly connected defaultOpen with component state.Setting
defaultOpento match the currentisOpenstate ensures consistent behavior during initial render and subsequent updates.
perkinsjr
left a comment
perkinsjr
left a comment
There was a problem hiding this comment.
This produces a really bad user experience.
You can see it in the video and also when using the application. The side nav bounces around because you are now collapsing everything when a path changes, so it's hard to click on the right thing.
The original issue only needs to do two things:
- When I switch APIs, the sidebar doesn't update and still shows the old API expanded. Instead it should collapse the old one and expand the new one
- When I navigate to /apis the sidebar collapses entirely
Instead it should collapse the api itself, but keep the list of apis expanded
This is the only sidebar changes we should see.
|
Sorry but I didn't understand, isn't that exactly what's happening? 1- this was asked: "Instead it should collapse the old one and expand the new one" 2- this was asked: "Instead it should collapse the api itself, but keep the list of apis expanded" And these two things are happening now in this PR. I didn't understand what was done wrong. Something that I can clearly see that could be improved to improve the UX is to improve the layout shift to expand the collapse |
|
As you can see in the video. Every collapsing field collapses on navigation when you navigate around the application. The only part of the side bar that needs collapsing is the API or Ratelimits sections and it only needs to partially collapse for example I am on I navigate to settings, it should collapse the sub nav and keep the APIs open with the list of APIs. If I am on the settings section and navigate to APIs it shouldn't collapse the settings unless the user collapses it themselves. It makes the experience for a user horrible if you click through items, everything shifts around making navigation hard. Also if you collapse a field because you don't need it open this implementation reopens the collapse navigation which is also bad for the user. Screen.Recording.2025-04-28.at.1.05.38.PM.mov |
|
I got it, it makes sense |
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
apps/dashboard/components/navigation/sidebar/app-sidebar/components/nav-items/nested-nav-item.tsx (1)
114-127: 🛠️ Refactor suggestion
⚠️ Potential issueStale closure when resetting
subPendingleads to inconsistent spinnersInside
setTimeout, you copy the capturedsubPendingobject; if state has changed meanwhile, the reset may drop newer keys. Use functional updates instead:- setTimeout(() => { - const resetPending = { ...subPending }; - resetPending[subItem.label as string] = false; - setSubPending(resetPending); - }, 300); + setTimeout(() => { + setSubPending((prev) => ({ + ...prev, + [subItem.label as string]: false, + })); + }, 300);This guarantees you don’t overwrite concurrent pending states.
🧹 Nitpick comments (2)
apps/dashboard/components/navigation/navbar.tsx (1)
4-4: Replace<a>withnext/link>LGTM, but considerprefetch/scrollflagsMigrating to
next/linkimproves client-side routing and removes full-page reloads.
Optional follow-ups you may want to consider:
prefetch={false}(ortrue) – disable/enable auto-prefetching if the breadcrumb list is large.scroll={false}– prevents the browser from scrolling to top on route change, which can feel more natural for in-page breadcrumb navigation.No blocking issues – just optimisation suggestions.
Also applies to: 116-129
apps/dashboard/components/navigation/sidebar/app-sidebar/components/nav-items/nested-nav-item.tsx (1)
165-167: Opening state source of truth split between parent & sub-items
open={isSubItem ? isChildrenOpen : isOpen}means two separate booleans can control visibility of the sameCollapsible. Keeping them in sync is error-prone. Consider derivingopenfrom a single piece of state (e.g.,isOpenfor all levels) or lifting the state up to avoid bifurcation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/dashboard/components/navigation/navbar.tsx(3 hunks)apps/dashboard/components/navigation/sidebar/app-sidebar/components/nav-items/nested-nav-item.tsx(4 hunks)
...dashboard/components/navigation/sidebar/app-sidebar/components/nav-items/nested-nav-item.tsx
Outdated
Show resolved
Hide resolved
...dashboard/components/navigation/sidebar/app-sidebar/components/nav-items/nested-nav-item.tsx
Show resolved
Hide resolved
...dashboard/components/navigation/sidebar/app-sidebar/components/nav-items/nested-nav-item.tsx
Show resolved
Hide resolved
|
I believe I now better understood what was requested. I made a few adjustments:
I’ve attached a video showing the current behavior after these latest commits. Recording.2025-04-28.200236.mp4 |
perkinsjr
changed the title
3 participants
What does this PR do?
Adjusted the sidebar collapse so that it automatically closes and opens children by path name, making it easier to recognize and improving state management.
Recording.2025-04-25.225719.mp4
Fixes #3183
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit