refactor: replace router.push with next/link in SidebarItem#562
refactor: replace router.push with next/link in SidebarItem#562miyaji255 wants to merge 2 commits intonetbirdio:mainfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughRefactors Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
This pull request refactors the SidebarItem component to replace imperative navigation using router.push with Next.js's declarative Link component, addressing performance and accessibility concerns identified in issue #561.
Changes:
- Replaced manual routing with Next.js
Linkcomponent for better prefetching and SPA-like navigation - Refactored rendering logic to conditionally render
Linkorbuttonbased on whether the item is collapsible - Improved click handler to prevent default behavior for active items and handle edge cases
Comments suppressed due to low confidence (1)
src/components/SidebarItem.tsx:176
- There's a structural issue with the Collapsible component usage. All sidebar items are wrapped in
Collapsible.RootandCollapsible.Trigger, even whencollapsible={false}. For non-collapsible items (the majority), this adds unnecessary DOM structure and potentially unwanted behavior. TheCollapsible.TriggerwithasChildwill delegate its trigger behavior to the child element (Link or button), which means non-collapsible items will have collapsible trigger behavior even though there's no content to collapse. Consider conditionally rendering the Collapsible wrapper only whencollapsible={true}:
if (collapsible) {
return (
<Collapsible.Root open={open} onOpenChange={setOpen}>
<Collapsible.Trigger asChild>
<li>...</li>
</Collapsible.Trigger>
<Collapsible.Content>{children}</Collapsible.Content>
</Collapsible.Root>
);
} else {
return <li>...</li>;
} return (
<Collapsible.Root open={open} onOpenChange={setOpen}>
<Collapsible.Trigger asChild>
<li className={"px-3 cursor-pointer list-none"}>
{href && !collapsible ? (
<Link
href={href}
target={target}
onClick={handleClick}
>
{content}
</Link>
) : (
<button
type="button"
className={"w-full"}
onClick={handleClick}
disabled={href === ""}
>
{content}
</button>
)}
</li>
</Collapsible.Trigger>
{collapsible && <Collapsible.Content>{children}</Collapsible.Content>}
</Collapsible.Root>
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/SidebarItem.tsx (1)
151-176:⚠️ Potential issue | 🟠 Major
Collapsible.Triggeraddsaria-expandedto non-collapsible link items.Since
Collapsible.Root/Collapsible.Trigger asChildalways wraps the<li>, non-collapsible<Link>items receivearia-expandedanddata-stateattributes from Radix. Screen readers will announce these links as expandable toggles, which undermines the accessibility goal of this PR.Consider conditionally wrapping with Collapsible only when
collapsibleis true:♻️ Sketch
- return ( - <Collapsible.Root open={open} onOpenChange={setOpen}> - <Collapsible.Trigger asChild> - <li className={"px-3 cursor-pointer list-none"}> - {href && !collapsible ? ( - <Link - href={href} - target={target} - onClick={handleClick} - > - {content} - </Link> - ) : ( - <button - type="button" - className={"w-full"} - onClick={handleClick} - disabled={href === ""} - > - {content} - </button> - )} - </li> - </Collapsible.Trigger> - {collapsible && <Collapsible.Content>{children}</Collapsible.Content>} - </Collapsible.Root> - ); + if (collapsible) { + return ( + <Collapsible.Root open={open} onOpenChange={setOpen}> + <Collapsible.Trigger asChild> + <li className={"px-3 cursor-pointer list-none"}> + <button + type="button" + className={"w-full"} + onClick={handleClick} + disabled={href === ""} + > + {content} + </button> + </li> + </Collapsible.Trigger> + <Collapsible.Content>{children}</Collapsible.Content> + </Collapsible.Root> + ); + } + + return ( + <li className={"px-3 cursor-pointer list-none"}> + {href ? ( + <Link href={href} target={target} onClick={handleClick}> + {content} + </Link> + ) : ( + <button + type="button" + className={"w-full"} + onClick={handleClick} + disabled + > + {content} + </button> + )} + </li> + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/SidebarItem.tsx` around lines 151 - 176, The current implementation wraps every item in Collapsible.Root/Collapsible.Trigger causing Radix props like aria-expanded/data-state to be applied to non-collapsible links; update SidebarItem so it only uses Collapsible.Root, Collapsible.Trigger, and Collapsible.Content when the prop collapsible is true—if collapsible is false render the <li> with the <Link> or <button> directly (preserve existing handleClick, href, target, disabled logic and content) to avoid passing Radix attributes to non-collapsible items; keep the children rendering inside Collapsible.Content only in the collapsible branch.
🧹 Nitpick comments (1)
src/components/SidebarItem.tsx (1)
84-93: Consider usingcnconsistently instead of mixingclassNamesandcn.Lines 86, 106 use
classNames()while lines 115, 129, 139 usecn(). SincecnwrapstwMerge(clsx(...))and correctly resolves Tailwind class conflicts, it's the safer choice for all class merging in this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/SidebarItem.tsx` around lines 84 - 93, The component builds its JSX class strings using classNames in the const content block but uses cn elsewhere; replace classNames(...) with cn(...) so all class merging uses the twMerge+clsx wrapper (e.g., change the classNames call inside the const content JSX to cn) and ensure the existing cn import is used (remove classNames import if now unused) so Tailwind conflicts are consistently resolved across SidebarItem (refer to the const content and other JSX class attribute usages).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/SidebarItem.tsx`:
- Around line 155-162: The Link JSX in SidebarItem renders external links with
target but doesn't add rel="noopener noreferrer"; update the Link element
(inside the SidebarItem component where href, target, handleClick and content
are used) to include a rel attribute when target === "_blank" (e.g., set rel to
"noopener noreferrer" only for blank targets) so opened tabs cannot access
window.opener while preserving current behavior for other targets.
- Around line 59-70: The isActive computation is duplicated in handleClick; move
the useMemo that computes isActive (the memo using exactPathMatch, path and
href) above handleClick and reuse that memoized isActive inside handleClick
instead of recomputing; change any loose equality (==) to strict equality (===)
in the memo and where used (e.g., comparisons of path and href), and simplify
the collapsible guard to rely on collapsible alone (e.g., if (collapsible)
return;) since collapsible items always render a button and Collapsible.Trigger
handles toggling—update handleClick to call e.preventDefault() when the memoized
isActive is true.
---
Outside diff comments:
In `@src/components/SidebarItem.tsx`:
- Around line 151-176: The current implementation wraps every item in
Collapsible.Root/Collapsible.Trigger causing Radix props like
aria-expanded/data-state to be applied to non-collapsible links; update
SidebarItem so it only uses Collapsible.Root, Collapsible.Trigger, and
Collapsible.Content when the prop collapsible is true—if collapsible is false
render the <li> with the <Link> or <button> directly (preserve existing
handleClick, href, target, disabled logic and content) to avoid passing Radix
attributes to non-collapsible items; keep the children rendering inside
Collapsible.Content only in the collapsible branch.
---
Nitpick comments:
In `@src/components/SidebarItem.tsx`:
- Around line 84-93: The component builds its JSX class strings using classNames
in the const content block but uses cn elsewhere; replace classNames(...) with
cn(...) so all class merging uses the twMerge+clsx wrapper (e.g., change the
classNames call inside the const content JSX to cn) and ensure the existing cn
import is used (remove classNames import if now unused) so Tailwind conflicts
are consistently resolved across SidebarItem (refer to the const content and
other JSX class attribute usages).
miyaji255
force-pushed
the
refactor/sidebar-navigation-link
branch
from
92a51b9 to
13ec058
Compare
|
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/SidebarItem.tsx`:
- Around line 148-170: The button in itemContent is being disabled whenever href
=== "" which prevents clicks from reaching Collapsible.Trigger and breaks
expand/collapse for items where collapsible === true; update the disabled prop
on the <button> (in the SidebarItem component where itemContent and handleClick
are defined) to only disable when the item is non-collapsible and has no href
(i.e., disabled should be true only when !collapsible && href === ""); this
preserves pointer events for collapsible items while still disabling
non-interactive, non-collapsible buttons.
---
Duplicate comments:
In `@src/components/SidebarItem.tsx`:
- Around line 151-158: Confirm that the Next.js Link component sets rel when
target is "_blank": keep the Link usage as written (Link with href={href},
target={target}, rel={target === "_blank" ? "noopener noreferrer" : undefined},
onClick={handleClick}) to ensure external links get rel="noopener noreferrer";
no code change required, but ensure this exact conditional remains on the Link
definition (refer to Link, href, target, rel, handleClick, content).
This pull request refactors the
SidebarItemcomponent to improve navigation handling and accessibility. The most significant changes include replacing custom navigation logic with Next.js'sLinkcomponent, refining click handling, and improving the structure for collapsible sidebar items.Navigation and click handling improvements:
router.pushwith Next.js'sLinkcomponent for navigation, ensuring better accessibility and client-side routing. (src/components/SidebarItem.tsx) [1] [2]handleClickfunction to prevent navigation when the item is already active and to handle special cases for collapsible items and external links. (src/components/SidebarItem.tsx)Component structure and rendering:
<Link>or<button>based on props, improving semantics and accessibility. (src/components/SidebarItem.tsx) [1] [2]Related: #561
Summary by CodeRabbit
Refactor
Style
Bug Fixes