Skip to content

refactor: replace router.push with next/link in SidebarItem #562

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
miyaji255 wants to merge 2 commits into
base: main
Choose a base branch
from
Open

refactor: replace router.push with next/link in SidebarItem #562

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
13ec058
refactor: replace router.push with next/link in SidebarItem
miyaji255 Feb 17, 2026
1ab877b
fix: allow clickable collapsible items with no href
miyaji255 Feb 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
207 changes: 116 additions & 91 deletions src/components/SidebarItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@

import * as Collapsible from "@radix-ui/react-collapsible";
import { cn } from "@utils/helpers";
import classNames from "classnames";
import { ChevronDownIcon, ChevronUpIcon, DotIcon } from "lucide-react";
import { usePathname, useRouter } from "next/navigation";
import { usePathname } from "next/navigation";
import Link from "next/link";
import React, { useEffect, useMemo } from "react";
import { useApplicationContext } from "@/contexts/ApplicationProvider";

Expand Down Expand Up @@ -52,106 +52,131 @@ export default function SidebarItem({
setOpen(true);
}
}, [hasActiveChild]);
const router = useRouter();
const { mobileNavOpen, toggleMobileNav, isNavigationCollapsed } =
useApplicationContext();

const handleClick = () => {
const preventRedirect = href
const isActive = useMemo(() => {
if (collapsible) return false;
return href
? exactPathMatch
? path == href
? 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");
}, [path, href, exactPathMatch, collapsible]);

const handleClick = (e: React.MouseEvent) => {
if (collapsible) return;

if (isActive) {
return e.preventDefault();
}

if (target === "_blank") return;

Comment on lines +74 to +75
Copy link

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
if (target === "_blank") return;

Copilot uses AI. Check for mistakes.
if (mobileNavOpen) toggleMobileNav();
router.push(href);
};

const isActive = useMemo(() => {
if (collapsible) return false;
return href ? (exactPathMatch ? path == href : path.includes(href)) : false;
}, [path, href, exactPathMatch, collapsible]);

if (!visible) return;

return (
<Collapsible.Root open={open} onOpenChange={setOpen}>
<Collapsible.Trigger asChild>
<li className={"px-3 cursor-pointer list-none"}>
<button
className={classNames(
"rounded-lg text-[.87rem] w-full relative font-normal",
className,
isChild
? "pl-7 pr-2 py-[.45rem] mt-1 mb-0.5"
: "py-[.45rem] px-3",
isActive
? "text-gray-900 bg-gray-200 dark:text-white dark:bg-nb-gray-900"
: "text-gray-600 hover:bg-gray-200 dark:text-nb-gray-400 dark:hover:bg-nb-gray-900/50",
)}
onClick={handleClick}
data-cy={"left-navigation-item"}
>
{isChild && isNavigationCollapsed && !mobileNavOpen && (
<div
className={
"absolute left-0 top-0 w-full h-full flex items-center justify-center group-hover/navigation:hidden text-[10px]"
}
>
<DotIcon size={14} className={"shrink-0"} />
</div>
)}
<div
className={classNames(
"flex w-full items-center shrink-0 ",
href == "" ? "disabled pointer-events-none" : "",
const content = (
<div
className={cn(
"rounded-lg text-[.87rem] w-full relative font-normal",
className,
isChild ? "pl-7 pr-2 py-[.45rem] mt-1 mb-0.5" : "py-[.45rem] px-3",
isActive
? "text-gray-900 bg-gray-200 dark:text-white dark:bg-nb-gray-900"
: "text-gray-600 hover:bg-gray-200 dark:text-nb-gray-400 dark:hover:bg-nb-gray-900/50",
)}
data-cy={"left-navigation-item"}
>
{isChild && isNavigationCollapsed && !mobileNavOpen && (
<div
className={
"absolute left-0 top-0 w-full h-full flex items-center justify-center group-hover/navigation:hidden text-[10px]"
}
>
<DotIcon size={14} className={"shrink-0"} />
</div>
)}
<div
className={cn(
"flex w-full items-center shrink-0 ",
href === "" ? "disabled pointer-events-none" : "",
)}
>
<span className="peer/icon" data-active={isActive} />
{icon}

<span
className={cn(
"px-3 whitespace-nowrap flex-1 w-full text-left",
labelClassName,
isNavigationCollapsed &&
!mobileNavOpen &&
"opacity-0 group-hover/navigation:opacity-100",
)}
>
{label}
</span>
{collapsible &&
(open ? (
<ChevronUpIcon
size={18}
className={cn(
"shrink-0",
isNavigationCollapsed &&
!mobileNavOpen &&
"opacity-0 group-hover/navigation:opacity-100",
)}
/>
) : (
<ChevronDownIcon
size={18}
className={cn(
"shrink-0",
isNavigationCollapsed &&
!mobileNavOpen &&
"opacity-0 group-hover/navigation:opacity-100",
)}
>
<span className="peer/icon" data-active={isActive} />
{icon}

<span
className={cn(
"px-3 whitespace-nowrap flex-1 w-full text-left",
labelClassName,
isNavigationCollapsed &&
!mobileNavOpen &&
"opacity-0 group-hover/navigation:opacity-100",
)}
>
{label}
</span>
{collapsible &&
(open ? (
<ChevronUpIcon
size={18}
className={cn(
"shrink-0",
isNavigationCollapsed &&
!mobileNavOpen &&
"opacity-0 group-hover/navigation:opacity-100",
)}
/>
) : (
<ChevronDownIcon
size={18}
className={cn(
"shrink-0",
isNavigationCollapsed &&
!mobileNavOpen &&
"opacity-0 group-hover/navigation:opacity-100",
)}
/>
))}
</div>
</button>
</li>
</Collapsible.Trigger>
{collapsible && <Collapsible.Content>{children}</Collapsible.Content>}
</Collapsible.Root>
/>
))}
</div>
</div>
);

const itemContent = (
<li className={"px-3 cursor-pointer list-none"}>
{href && !collapsible ? (
<Link
href={href}
target={target}
rel={target === "_blank" ? "noopener noreferrer" : undefined}
onClick={handleClick}
>
{content}
</Link>
) : (
<button
type="button"
className={"w-full"}
onClick={handleClick}
disabled={!collapsible && href === ""}
>
{content}
</button>
)}
</li>
);

if (collapsible) {
return (
<Collapsible.Root open={open} onOpenChange={setOpen}>
<Collapsible.Trigger asChild>{itemContent}</Collapsible.Trigger>
<Collapsible.Content>{children}</Collapsible.Content>
</Collapsible.Root>
);
}

return itemContent;
}