fix multi active nav links #9717
Conversation
WalkthroughThe pull request introduces a new Changes
Assessment against linked issues
Poem
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 (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![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 (3)
src/components/ui/sidebar/ActiveLinks.tsx (2)
16-17: Fix typo in variable name.
isExactCollisonis missing an extra "i". Rename to maintain consistency and clarify meaning.- const isExactCollison = links.some((link) => link.url === pathname); + const isExactCollision = links.some((link) => link.url === pathname);
18-20: Fix another typo in variable name.
isNestedCollisonis also missing an extra "i" in "collision."- const isNestedCollison = links.some( + const isNestedCollision = links.some(src/components/ui/sidebar/nav-main.tsx (1)
35-35: Pass minimal props to avoid confusion.
Currently, passing the entirelinksarray may be more than the component needs if it only checks collisions among sibling routes. If only certain links need to be considered, you could pass a filtered subset to improve clarity and performance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/ui/sidebar/ActiveLinks.tsx(1 hunks)src/components/ui/sidebar/nav-main.tsx(2 hunks)
🔇 Additional comments (6)
src/components/ui/sidebar/ActiveLinks.tsx (5)
1-2: Ensure type references from "raviger/dist/Link" remain stable.
Currently, usingActiveLinkPropsfrom"raviger/dist/Link"may break if the library’s internal structure changes in future updates. Consider exporting and re-exporting official types fromravigerdirectly if they exist, to avoid referencing library internals.
4-10: Validate defaultlinksusage.
Thelinks = []default assignment is good for preventing runtime errors. However, confirm that this behavior aligns with the usage context—especially iflinksmust be guaranteed not to be empty.
13-15: Confirm logic for determining nested match.
The conditionpathname?.startsWith(href) && pathname.length > href.lengthflags routes as nested only when the pathname is strictly longer thanhref. This may skip routes that matchhrefplus a slash (e.g.,/examplevs/example/). Ensure this logic correctly handles trailing slashes or alternative route patterns.
24-29: Double-check collision logic precedence.
When bothisNestedMatchand collisions are true, the link is assigned no active class. Confirm if the intended UX is to treat all collisions as higher priority than nested matches, even if the current route is otherwise a valid nested match.
30-37: General structure is cohesive.
Returning aLinkwith combined classes works well, and spreading additional props is a clean approach for extensibility.src/components/ui/sidebar/nav-main.tsx (1)
11-11: Good import update for the new ActiveLink logic.
Switching from a directravigerimport to the localActiveLinksis appropriate for the updated functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/ui/sidebar/NavLink.tsx (2)
17-19: Potential missed accessibility considerations on click.Attaching
onClickto a wrapping<div>could cause unexpected behavior if the user clicks within the<div>but not on the actual link. Consider moving the click handler directly onto the<Link>element to ensure consistent user interactions and accessibility.<div> - <div onClick={handleClick}> <Link href={href} className={`${className} ${computedClassName}`} {...props} + onClick={handleClick} /> - </div> </div>
21-21: Use strict equality for robust type checking.Using
===instead of==ensures no unintended type coercions, helping avoid subtle bugs.- const computedClassName = activeLink == name ? activeClass : ""; + const computedClassName = activeLink === name ? activeClass : "";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/ui/sidebar/NavLink.tsx(1 hunks)src/components/ui/sidebar/nav-main.tsx(4 hunks)
🔇 Additional comments (6)
src/components/ui/sidebar/NavLink.tsx (3)
1-2: Imports look correct and concise.Using
LinkfromravigerandActiveLinkPropsshows consistent usage with the existing navigation utilities.
4-16: Prop definitions are clear and strongly typed.The addition of
activeLink,setActive, andnamewithin the type intersection aligns well with your custom active link logic. Good job on explicitly defining types.
23-31: Return structure is straightforward and readable.The wrapped
Linkcomponent is rendered clearly, aligned with the rest of the code style. No issues observed.src/components/ui/sidebar/nav-main.tsx (3)
24-29: Stateful approach for active link is well-structured.You’ve introduced
activeLinkto manage the currently active navigation item. The simpleuseStateusage andhandleSetActivefunction addresses the multi-active problem effectively.
43-55: Good usage of the new NavLink component with active state handling.You’ve removed the
exactActiveClassin favor ofactiveLinklogic and consolidated the responsibilities for setting the active state. This addresses the multi-active link problem comprehensively.
43-49: Consider verifying consistent naming conventions in your link objects.Each link object uses
nameandurlkeys. Ensure all references (likelink.url,link.name) remain consistent with any potential future expansions (e.g.,slugorpath). If the naming scheme changes, quickly reflect it in your typed definitions to maintain clarity.✅ Verification successful
Let me try a different approach to verify the naming conventions in link objects.
Let me try one more search to find the type definitions for these link objects.
Based on the results from our verification attempts, I can now provide a final response:
Link object properties are consistently used across the codebase
From the search results, we can see that the link objects consistently use
nameandurlproperties throughout the codebase. All references to these properties (link.nameandlink.url) are uniform in both the nav-main.tsx and ActiveLinks.tsx components. There's also a consistent pattern wherelink.nameis used for display text and tooltips, whilelink.urlis used for navigation purposes. The only additional property observed islink.icon, which is optionally used for rendering icons.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Look for occurrences of `links` usage across the entire codebase to confirm consistent naming. rg -A 5 $'links\\s*[:=]\\s*{\\s*name:'Length of output: 159
Script:
#!/bin/bash # Search for link object definitions and usages rg -A 5 "link\.(url|name|path|slug)" # Also search for any type definitions related to links ast-grep --pattern 'type $_ = { $$$name: $_,$$$url: $_,$$$}' ast-grep --pattern 'interface $_ { $$$name: $_,$$$url: $_,$$$}'Length of output: 2640
Script:
#!/bin/bash # Search for type definitions and interfaces containing 'link' in their name ast-grep --pattern 'type $_Link$_ = { $$$}' ast-grep --pattern 'interface $_Link$_ { $$$}' # Also search for any array definitions of links rg "const\s+\w*links\s*=\s*\[" -A 5Length of output: 132
|
@rithviknishad according to me its handling all the edge cases now. Can you please review once? |
There was a problem hiding this comment.
this approach doesn't work for multiple reasons
-
if you go to a specific page directly by pasting it's link in the browser, this shows facility tab regardless of what page it is currently in.
-
if you somehow let's say reach facility page through users profile, or patient details or anywhere else, the active tab doesn't automatically change to that.
so the collision approach was better i guess.. i thought tab should not change once upon inter-navigation |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
ActiveLinkcomponent for enhanced sidebar navigation.NavLinkcomponent for improved active link management.Refactor