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.
src/components/SidebarItem.tsx
Outdated
| const isActive = href | ||
| ? exactPathMatch | ||
| ? path == href | ||
| : path.includes(href) | ||
| : false; | ||
| if (collapsible && href) return; | ||
| if (collapsible && mobileNavOpen) return; | ||
| if (collapsible && open) return; | ||
| if (preventRedirect) return; | ||
| if (target == "_blank") return window.open(href, "_blank"); | ||
|
|
||
| if (collapsible && (href || mobileNavOpen || open)) return; | ||
|
|
||
| if (isActive) { | ||
| return e.preventDefault(); |
There was a problem hiding this comment.
There's duplicate logic for determining if an item is active. The handleClick function calculates isActive locally (lines 60-64), while there's also a memoized isActive computed at lines 77-80. The local calculation in handleClick doesn't account for collapsible items (which should never be active according to line 78), potentially causing inconsistent behavior. Use the memoized isActive value instead of recalculating it in handleClick to ensure consistency.
| useEffect(() => { | ||
| if (hasActiveChild && !open) { | ||
| setOpen(true); | ||
| } | ||
| }, [hasActiveChild]); |
There was a problem hiding this comment.
The useEffect dependency array is incomplete. The effect depends on open (read at line 52) but doesn't include it in the dependency array at line 55. This could lead to stale closure issues where the effect doesn't see updated open values. Either add open to the dependency array, or refactor the condition to avoid the dependency: if (hasActiveChild) { setOpen(true); } (which will run every time hasActiveChild changes, ensuring the collapsible opens when a child becomes active).
src/components/SidebarItem.tsx
Outdated
| if (preventRedirect) return; | ||
| if (target == "_blank") return window.open(href, "_blank"); | ||
|
|
||
| if (collapsible && (href || mobileNavOpen || open)) return; |
There was a problem hiding this comment.
The consolidated logic in this line changes behavior compared to the original implementation. In the original code, when an item had both collapsible=true and href, the function would return immediately, preventing navigation. The new condition collapsible && (href || mobileNavOpen || open) still returns early when collapsible=true and href exists, but the logic grouping with || could be confusing. Consider separating the conditions for clarity:
if (collapsible && href) return;
if (collapsible && mobileNavOpen) return;
if (collapsible && open) return;
This makes the intent clearer: collapsible items with hrefs shouldn't navigate, collapsible items shouldn't close when mobile nav is open, and already-open collapsible items shouldn't toggle closed on click.
| if (target === "_blank") return; | ||
|
|
There was a problem hiding this comment.
The external link handling is incomplete. The original code used window.open(href, "_blank") for external links, which opened them in a new tab/window. Now, external links are handled by the Link component with target={target}, but the handleClick function returns early for target === "_blank", which prevents default link behavior from being properly handled. The Link component will correctly handle target="_blank" on its own, so the early return here is correct. However, this creates a subtle behavior change: the original window.open was imperative and controlled, while the new implementation relies on standard link behavior. This is actually an improvement for accessibility, but the mobile navigation toggle (line 74) won't execute for external links anymore. Consider whether mobile nav should close when clicking external links.
| if (target === "_blank") return; |
src/components/SidebarItem.tsx
Outdated
| "flex w-full items-center shrink-0 ", | ||
| href == "" ? "disabled pointer-events-none" : "", | ||
| )} | ||
| {href && !collapsible ? ( |
There was a problem hiding this comment.
The condition for rendering Link vs button should also check if href is not an empty string. Currently, when href="" (the default), the condition href && !collapsible will be falsy (correct), but the logic would be clearer and more defensive if it explicitly checked href && href !== "" && !collapsible. The disabled attribute on the button at line 168 handles empty hrefs, but explicit validation in the conditional improves code clarity.
| {href && !collapsible ? ( | |
| {href && href !== "" && !collapsible ? ( |
src/components/SidebarItem.tsx
Outdated
| <Link | ||
| href={href} | ||
| target={target} | ||
| onClick={handleClick} | ||
| > | ||
| {content} | ||
| </Link> |
There was a problem hiding this comment.
The Link component with target="_blank" should include rel="noopener noreferrer" for security. While Next.js Link automatically handles this for external URLs in newer versions, explicitly adding it ensures protection against tabnabbing attacks across all versions. Note that while <a> tags in the codebase consistently use this pattern (e.g., src/components/ExternalLinkText.tsx:23-24), Next.js Link components in the codebase are inconsistent. Consider adding rel="noopener noreferrer" when target="_blank" to align with the security pattern used for HTML anchor tags.
![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).
2 participants
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