SANC-55: admin dashboard home page header #48
Conversation
tanvimahal
requested review from
JustinTan-1,
Lujarios,
Yemyam,
burtonjong,
jason-duong4509,
promatty,
themaxboucher and
wesleylui
as code owners
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR refactors the navbar component by introducing styled navigation links with dynamic active state detection using Next.js path detection, replacing button-based admin navigation with NavLink items, and updates admin dashboard redirect endpoints from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/app/_components/common/navbar.tsx`:
- Around line 44-48: Navbar contains NavLink entries that point to non-existent
admin routes (/admin/agencies, /admin/invite, /admin/vehicle-logs) which cause
404s; either implement the missing pages or update the NavLink instances in
navbar.tsx: remove or replace the NavLink href="/admin/agencies" and
href="/admin/invite" with correct existing routes (or add new pages matching
those hrefs), and change href="/admin/vehicle-logs" to the existing route (e.g.,
href="/admin/driver-logs" or another correct path); update the NavLink elements’
text accordingly so links and targets match actual route components.
🧹 Nitpick comments (3)
src/app/_components/common/navbar.module.scss (1)
11-14: Duplicate style declaration for.navLinkDefault.The
font-weight: 400property is declared twice for.navLinkDefault(lines 12 and 30). Additionally, thetransition: color 0.2s easeon line 13 is overwritten bytransition: background-color 0.2s easeon line 20 (since CSStransitionshorthand replaces, not merges). Consider consolidating:♻️ Suggested consolidation
.navLinkDefault, .navLinkActive { font-size: 0.75rem; cursor: pointer; - transition: background-color 0.2s ease; + transition: background-color 0.2s ease, color 0.2s ease; padding: 0.5rem 0.75rem; border-radius: 4px; } .navLinkDefault:hover { background-color: `#f3f4f6`; } -.navLinkDefault { - font-weight: 400; -} - .navLinkActive { font-weight: 600; }Also applies to: 29-31
src/app/_components/common/navbar.tsx (2)
36-36: SCSS.navbarclass is imported but not used; inline styles duplicate it.The SCSS module defines
.navbarwithpadding: 1rem 2rem; border-bottom: 1px solid#e5e7eb;but line 36 uses inline styles and the"border-bottom"className (likely a global/Tailwind class). Consider using the imported SCSS class for consistency:♻️ Suggested change
- <Group justify="space-between" className="border-bottom" style={{ padding: "1rem 2rem" }}> + <Group justify="space-between" className={styles.navbar}>
23-32: ConsiderstartsWithfor nested route highlighting.The current exact match (
pathname === href) won't mark a link as active when navigating to child routes (e.g.,/admin/schedule/123won't highlight "View Schedule"). If nested routes exist or are planned, consider:const isActive = pathname === href || pathname.startsWith(`${href}/`);This is optional depending on your route structure.
| <NavLink href="/admin/agencies">View Agencies</NavLink> | ||
| <NavLink href="/admin/invite">Invite</NavLink> | ||
| <NavLink href="/admin/rider-logs">Rider Logs</NavLink> | ||
| <NavLink href="/admin/vehicle-logs">Vehicle Log</NavLink> | ||
| <NavLink href="/admin/schedule">View Schedule</NavLink> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for admin route pages/directories
echo "=== Checking admin app directory structure ==="
if [ -d "src/app/admin" ]; then
echo "Admin directory found. Contents:"
ls -la src/app/admin 2>/dev/null || echo "Cannot list admin directory"
echo ""
# Check for specific route directories
routes=("agencies" "invite" "rider-logs" "vehicle-logs" "schedule")
echo "=== Checking for expected route directories/pages ==="
for route in "${routes[@]}"; do
if [ -d "src/app/admin/$route" ]; then
echo "✓ $route: Directory found"
# Check if it has a page.tsx or page.ts
if [ -f "src/app/admin/$route/page.tsx" ] || [ -f "src/app/admin/$route/page.ts" ]; then
echo " └─ Has page component"
fi
elif [ -f "src/app/admin/$route.tsx" ] || [ -f "src/app/admin/$route.ts" ]; then
echo "✓ $route: File found"
else
echo "✗ $route: NOT FOUND"
fi
done
else
echo "Admin directory not found at src/app/admin"
fiRepository: Code-the-Change-YYC/salvationarmy
Length of output: 809
Remove or create the missing admin routes.
The navbar links to /admin/agencies, /admin/invite, and /admin/vehicle-logs, but these pages do not exist in the codebase. Clicking these links will result in 404 errors. Either create these missing route pages or remove the broken links and replace with the routes that do exist (driver-logs, rider-logs, schedule).
🤖 Prompt for AI Agents
In `@src/app/_components/common/navbar.tsx` around lines 44 - 48, Navbar contains
NavLink entries that point to non-existent admin routes (/admin/agencies,
/admin/invite, /admin/vehicle-logs) which cause 404s; either implement the
missing pages or update the NavLink instances in navbar.tsx: remove or replace
the NavLink href="/admin/agencies" and href="/admin/invite" with correct
existing routes (or add new pages matching those hrefs), and change
href="/admin/vehicle-logs" to the existing route (e.g.,
href="/admin/driver-logs" or another correct path); update the NavLink elements’
text accordingly so links and targets match actual route components.
There was a problem hiding this comment.
instead of admin/vehicle-logs the endpoint is admin/driver-logs
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the driver-logs route exists and check vehicle-logs
echo "=== Checking for driver-logs route ==="
if [ -d "src/app/admin/driver-logs" ]; then
echo "✓ driver-logs directory exists"
ls -la src/app/admin/driver-logs
elif [ -f "src/app/admin/driver-logs.tsx" ] || [ -f "src/app/admin/driver-logs.ts" ]; then
echo "✓ driver-logs file exists"
fi
echo ""
echo "=== Checking for vehicle-logs route ==="
if [ -d "src/app/admin/vehicle-logs" ]; then
echo "✓ vehicle-logs directory exists"
ls -la src/app/admin/vehicle-logs
elif [ -f "src/app/admin/vehicle-logs.tsx" ] || [ -f "src/app/admin/vehicle-logs.ts" ]; then
echo "✓ vehicle-logs file exists"
else
echo "✗ vehicle-logs does NOT exist"
fi
echo ""
echo "=== Current navbar.tsx NavLink for vehicle/driver logs ==="
rg -n "vehicle-logs|driver-logs" src/app/_components/common/navbar.tsx
Length of output: 437
@promatty, thank you for the clarification! You're right—the navbar should link to /admin/driver-logs, not /admin/vehicle-logs.
Line 47 currently has:
<NavLink href="/admin/vehicle-logs">Vehicle Log</NavLink>This should be updated to:
<NavLink href="/admin/driver-logs">Vehicle Log</NavLink>The /admin/driver-logs route exists in the codebase, so this will fix the 404 for that link. However, /admin/agencies and /admin/invite still need to be addressed (either created or removed from the navbar).
🧬 Code Graph Analysis Results
[src/app/_components/common/navbar.tsx] (lines 1- end)
"use client";
import { Group, Text } from "@mantine/core";
import Link from "next/link";
import { usePathname } from "next/navigation";
import Bell from "@/assets/icons/bell";
import Home from "@/assets/icons/home";
import styles from "./navbar.module.scss";
import Profile from "./profile/profile";
type NavbarView = "admin" | "agency" | "driver";
interface NavbarProps {
view: NavbarView;
agencyName?: string; //only used in Agency View
}
interface NavLinkProps {
href: string;
children: React.ReactNode;
}
function NavLink({ href, children }: NavLinkProps) {
const pathname = usePathname();
const isActive = pathname === href;
return (
<Link href={href} className={styles.navLink}>
<span className={isActive ? styles.navLinkActive : styles.navLinkDefault}>{children}</span>
</Link>
);
}
export default function Navbar({ view, agencyName }: NavbarProps) {
return (
<Group justify="space-between" className="border-bottom" style={{ padding: "1rem 2rem" }}>
<Group>
<Home />
<Text>{view === "agency" ? `${agencyName ?? "[Agency name]"} Home` : ""}</Text>
</Group>
{view === "admin" && (
<Group gap={30}>
<NavLink href="/admin/agencies">View Agencies</NavLink>
<NavLink href="/admin/invite">Invite</NavLink>
<NavLink href="/admin/rider-logs">Rider Logs</NavLink>
<NavLink href="/admin/vehicle-logs">Vehicle Log</NavLink>
<NavLink href="/admin/schedule">View Schedule</NavLink>
<Profile />
</Group>
)}
{view === "agency" && (
<Group gap={30}>
<Bell />
<Profile />
</Group>
)}
{view === "driver" && (
<Group>
<Profile />
</Group>
)}
</Group>
);
}
✏️ Learnings added
Learnt from: promatty
Repo: Code-the-Change-YYC/salvationarmy PR: 48
File: src/app/_components/common/navbar.tsx:44-48
Timestamp: 2026-01-24T06:06:38.145Z
Learning: In the Salvation Army project, the correct admin route for vehicle/driver logs is `/admin/driver-logs`, not `/admin/vehicle-logs`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
promatty
left a comment
promatty
left a comment
There was a problem hiding this comment.
also, in app/page.tsx can we redirect to the schedule page instead of admin/home
| @@ -0,0 +1,35 @@ | |||
| .navbar { | |||
| padding: 1rem 2rem; | |||
| border-bottom: 1px solid #e5e7eb; | |||
There was a problem hiding this comment.
i always forget, is it possible to import these from the global styles file instead of hardcoding colors?
There was a problem hiding this comment.
sorry I didn't realize the global ones were usable now, i changed it
| } | ||
|
|
||
| .navLinkDefault:hover { | ||
| background-color: #f3f4f6; |
| </Link> | ||
| <Group gap={30}> | ||
| <NavLink href="/admin/agencies">View Agencies</NavLink> | ||
| <NavLink href="/admin/invite">Invite</NavLink> |
There was a problem hiding this comment.
can we make this open the invite modal we currently have, instead of waiting for sanc-64
| <NavLink href="/admin/agencies">View Agencies</NavLink> | ||
| <NavLink href="/admin/invite">Invite</NavLink> | ||
| <NavLink href="/admin/rider-logs">Rider Logs</NavLink> | ||
| <NavLink href="/admin/vehicle-logs">Vehicle Log</NavLink> |
| <NavLink href="/admin/agencies">View Agencies</NavLink> | ||
| <NavLink href="/admin/invite">Invite</NavLink> | ||
| <NavLink href="/admin/rider-logs">Rider Logs</NavLink> | ||
| <NavLink href="/admin/vehicle-logs">Vehicle Log</NavLink> | ||
| <NavLink href="/admin/schedule">View Schedule</NavLink> |
There was a problem hiding this comment.
instead of admin/vehicle-logs the endpoint is admin/driver-logs
|
@tanvimahal merge conflicts |
|
@tanvimahal bump |
tanvimahal
force-pushed
the
SANC-55-Admin-Dashboard-Home-Page-header
branch
from
f46177d to
0f0c6bc
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/_components/common/navbar.tsx (1)
39-61: 🛠️ Refactor suggestion | 🟠 MajorRemove dead code:
navbarText()andhomeLink()are unused.Neither function is called in the returned JSX. Additionally,
homeLink()still references the stale/admin/homepath. These should be cleaned up.Proposed fix
export default function Navbar({ view, agencyName }: NavbarProps) { - const pathname = usePathname(); - - const section = pathname.split("/")[2] ?? "home"; - - const navbarText = () => { - switch (view) { - case "admin": - return `Admin ${section}`; - case "agency": - return `${agencyName ?? "[Agency name]"} ${section}`; - case "driver": - return `Driver ${section}`; - default: - return ""; - } - }; - - const homeLink = () => { - switch (view) { - case "admin": - return `/admin/home`; - case "agency": - return `/agency/home`; - case "driver": - return `/driver/home`; - } - }; - return (
🧹 Nitpick comments (1)
src/app/_components/common/navbar.tsx (1)
18-32: NavLink component looks good overall.Clean implementation with active state detection. One minor optimization: each
NavLinkinstance independently callsusePathname(). Since the parentNavbaralready calls it (line 35), consider passingpathnameas a prop to avoid redundant hook calls across all NavLink instances.
3 participants
invite tab on the navbar needs to be changed from a link to the dropdown modal from SANC-64
Summary by CodeRabbit
Style
Refactor