fix: change navbar links to shadcn navigation menu components #365
Conversation
WalkthroughReplaced desktop navbar links with NavigationMenu components and adjusted click handling. Updated UI package NavigationMenu components’ forwardRef types from ElementRef to ComponentRef. Changed core app dev script port from 3000 to 3001. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant NV as NavigationMenu (Navbar)
participant ST as Store/UI State
U->>NV: Click "Browse" (NavigationMenuTrigger)
NV->>ST: closeAddToCart()
NV->>ST: toggleOpenMegaMenu()
U->>NV: Click "Become an author" (NavigationMenuLink)
NV->>U: Navigate to /become-auther
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
✨ Finishing Touches
🧪 Generate unit tests
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 |
![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
🧹 Nitpick comments (1)
apps/core/app/(landing)/_components/desktop-navbar/navbar-links.tsx (1)
28-31: Trigger without Content: consider a simple button for “Browse.”
NavigationMenuTriggeris intended to toggleNavigationMenuContent. Since none is rendered here, a button with the trigger styles is simpler and avoids odd ARIA/state expectations.Apply:
import { NavigationMenu, NavigationMenuItem, NavigationMenuLink, NavigationMenuList, - NavigationMenuTrigger, + NavigationMenuTrigger, + navigationMenuTriggerStyle, } from "@repo/ui/components/atoms/navigation-menu"; @@ - <NavigationMenuTrigger onClick={onClick}> - <Typography variant="label/sm">Browse</Typography> - </NavigationMenuTrigger> + <button + type="button" + onClick={onClick} + className={`${navigationMenuTriggerStyle()} group`} + > + <Typography variant="label/sm">Browse</Typography> + </button>Alternative: keep
NavigationMenuTriggerbut renderasChildwith a button.Also applies to: 3-9
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/core/app/(landing)/_components/desktop-navbar/navbar-links.tsx(2 hunks)apps/core/package.json(1 hunks)packages/ui/src/components/atoms/navigation-menu.tsx(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/core/app/(landing)/_components/desktop-navbar/navbar-links.tsx (2)
packages/ui/src/components/atoms/navigation-menu.tsx (5)
NavigationMenu(121-121)NavigationMenuList(122-122)NavigationMenuItem(123-123)NavigationMenuTrigger(125-125)NavigationMenuLink(126-126)packages/ui/src/components/atoms/typography.tsx (1)
Typography(135-135)
🔇 Additional comments (1)
apps/core/package.json (1)
6-6: No lingering 3000 references found – ranrg -nC2 '\b3000\b|localhost:3000' --glob '!**/node_modules/**' .with no matches.
| <NavigationMenuLink | ||
| href="/become-auther" | ||
| aria-label="become an author" | ||
| > | ||
| <Typography variant="label/sm">Become an author</Typography> | ||
| </NavigationMenuLink> | ||
| </NavigationMenuItem> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Typo and client-side navigation regression on “Become an author” link.
- href typo: "auther" → likely "author".
- Using Radix NavigationMenuLink directly renders an , losing Next.js client-side routing/prefetch.
Fix by using asChild and wrapping Next.js Link.
Apply:
- <NavigationMenuItem>
- <NavigationMenuLink
- href="/become-auther"
- aria-label="become an author"
- >
- <Typography variant="label/sm">Become an author</Typography>
- </NavigationMenuLink>
- </NavigationMenuItem>
+ <NavigationMenuItem>
+ <NavigationMenuLink asChild>
+ <Link href="/become-author">
+ <Typography variant="label/sm">Become an author</Typography>
+ </Link>
+ </NavigationMenuLink>
+ </NavigationMenuItem>If the canonical route is different (e.g., /become-an-author), adjust accordingly.
🏁 Script executed:
#!/bin/bash
# Locate the navbar-links.tsx file in the repository
fd navbar-links.tsxLength of output: 67
🏁 Script executed:
#!/bin/bash
# Inspect navbar-links.tsx for typo and navigation usage
FILE=apps/core/app/\(landing\)/_components/desktop-navbar/navbar-links.tsx
echo "=== Imports ==="
sed -n '1,20p' "$FILE"
echo
echo "=== Usage Snippet (lines 20-80) ==="
sed -n '20,80p' "$FILE"
echo
echo "=== Checking for typo in href ==="
grep -R "become-auther" -n "$FILE" || echo "No instances found"
echo
echo "=== Checking NavigationMenuLink usage ==="
grep -R "NavigationMenuLink" -n "$FILE"
echo
echo "=== Checking Next.js Link import ==="
grep -R "from 'next/link'" -n "$FILE" || echo "No import of Next.js Link"Length of output: 1576
Fix “Become an author” link: correct typo and enable client-side routing
- Change href from
/become-autherto/become-author(or your canonical route). - Wrap Next.js
<Link>usingNavigationMenuLink asChildto preserve prefetch and client-side navigation.
- <NavigationMenuItem>
- <NavigationMenuLink
- href="/become-auther"
- aria-label="become an author"
- >
- <Typography variant="label/sm">Become an author</Typography>
- </NavigationMenuLink>
- </NavigationMenuItem>
+ <NavigationMenuItem>
+ <NavigationMenuLink asChild>
+ <Link href="/become-author" aria-label="become an author">
+ <Typography variant="label/sm">Become an author</Typography>
+ </Link>
+ </NavigationMenuLink>
+ </NavigationMenuItem>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <NavigationMenuLink | |
| href="/become-auther" | |
| aria-label="become an author" | |
| > | |
| <Typography variant="label/sm">Become an author</Typography> | |
| </NavigationMenuLink> | |
| </NavigationMenuItem> | |
| <NavigationMenuItem> | |
| <NavigationMenuLink asChild> | |
| <Link href="/become-author" aria-label="become an author"> | |
| <Typography variant="label/sm">Become an author</Typography> | |
| </Link> | |
| </NavigationMenuLink> | |
| </NavigationMenuItem> |
🤖 Prompt for AI Agents
In apps/core/app/(landing)/_components/desktop-navbar/navbar-links.tsx around
lines 33 to 39, the link href and routing are wrong: change
href="/become-auther" to href="/become-author" (or your canonical route) and
enable Next.js client-side navigation by using NavigationMenuLink with asChild
and nesting a Next.js Link component inside it (ensure Link is imported from
'next/link'); keep the Typography element inside the Link and preserve
aria-label (fix spelling if needed) so prefetching and SPA navigation work.
| React.ComponentRef<typeof NavigationMenuPrimitive.Root>, | ||
| React.ComponentPropsWithoutRef<typeof NavigationMenuPrimitive.Root> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ref type regression: use React.ElementRef, not React.ComponentRef, for Radix primitives.
ComponentRef resolves to class component instances and is never for function components/primitives, breaking ref typings. ElementRef is the correct utility for Radix/shadcn components.
Apply:
- React.ComponentRef<typeof NavigationMenuPrimitive.Root>,
+ React.ElementRef<typeof NavigationMenuPrimitive.Root>,
@@
- React.ComponentRef<typeof NavigationMenuPrimitive.List>,
+ React.ElementRef<typeof NavigationMenuPrimitive.List>,
@@
- React.ComponentRef<typeof NavigationMenuPrimitive.Trigger>,
+ React.ElementRef<typeof NavigationMenuPrimitive.Trigger>,
@@
- React.ComponentRef<typeof NavigationMenuPrimitive.Content>,
+ React.ElementRef<typeof NavigationMenuPrimitive.Content>,
@@
- React.ComponentRef<typeof NavigationMenuPrimitive.Viewport>,
+ React.ElementRef<typeof NavigationMenuPrimitive.Viewport>,
@@
- React.ComponentRef<typeof NavigationMenuPrimitive.Indicator>,
+ React.ElementRef<typeof NavigationMenuPrimitive.Indicator>,Run a quick TS check to ensure refs are typed correctly:
Expect: no matches.
Also applies to: 28-29, 49-51, 67-69, 84-86, 102-104
🏁 Script executed:
#!/bin/bash
rg -nP 'forwardRef<\s*React\.ComponentRef<' -g 'packages/ui/**.tsx' -SLength of output: 148
🏁 Script executed:
#!/bin/bash
# Search for any usage of React.ComponentRef across all .tsx files
rg -nP 'React\.ComponentRef<' -t tsx -n
# Specifically look for forwardRef generics using React.ComponentRef
rg -nP 'forwardRef<\s*React\.ComponentRef<' -t tsx -n -C3Length of output: 55
🏁 Script executed:
#!/bin/bash
# Search for any usage of React.ComponentRef in .tsx files by glob pattern
rg -nP 'React\.ComponentRef<' -g '*.tsx' -n
# Search for forwardRef generics explicitly using React.ComponentRef
rg -nP 'forwardRef<\s*React\.ComponentRef<' -g '*.tsx' -n -C3Length of output: 705
Replace React.ComponentRef with React.ElementRef for Radix primitives
In packages/ui/src/components/atoms/navigation-menu.tsx at lines 10, 28, 49, 67, 84 and 102, swap all React.ComponentRef<typeof NavigationMenuPrimitive.> to React.ElementRef<typeof NavigationMenuPrimitive.> to ensure correct ref typings for function components.
🤖 Prompt for AI Agents
In packages/ui/src/components/atoms/navigation-menu.tsx around lines 10, 28, 49,
67, 84 and 102, the types use React.ComponentRef<typeof
NavigationMenuPrimitive.*> which is incorrect for Radix function components;
replace each React.ComponentRef<typeof NavigationMenuPrimitive.*> with
React.ElementRef<typeof NavigationMenuPrimitive.*> to ensure ref typing matches
function components (update all occurrences at the listed line ranges).
Description
Changes
Checks
Summary by CodeRabbit