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.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRedesigns the navbar: new SCSS classes (.navLink, .navLinkDefault, .navLinkActive), replaces action Buttons with an internal NavLink component that uses usePathname for active state, changes admin redirect from /admin/home to /admin/schedule, and removes the admin home page file. Changes
Sequence Diagram(s)(omitted — changes are primarily component/styling replacements and a small routing redirect; no new multi-component sequential flow needing visualization) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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
| </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/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.
| <Group gap={30}> | ||
| <NavLink href="/admin/agencies">View Agencies</NavLink> | ||
| <NavLink href="/admin/invite">Invite</NavLink> | ||
| <NavLink href="/admin/rider-logs">Rider Logs</NavLink> |
There was a problem hiding this comment.
the invite tab should bring up the invite modal. this only exists on the http://localhost:3000/admin/home button right now. i also thought we were going to remove admin/home?
remove unused functions |
tanvimahal
force-pushed
the
SANC-55-Admin-Dashboard-Home-Page-header
branch
from
0f0c6bc to
7a66ca3
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)
35-61: 🛠️ Refactor suggestion | 🟠 MajorRemove unused
navbarText(),homeLink()functions andsectionvariable.These functions are defined but never called anywhere in the component. Additionally,
homeLink()references the stale/admin/homepath which is being removed per the PR description. Thesectionvariable (line 37) is only used bynavbarText(), making it dead code as well.🧹 Proposed removal of dead code
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 (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/_components/common/navbar.tsx` around lines 35 - 61, Remove the dead code: delete the section constant and the navbarText() and homeLink() functions from the component (they're unused and homeLink() still returns the stale `/admin/home` path). Search for references to navbarText, homeLink, and section to confirm none remain; if other code expects a home path, replace usages with the new routing logic elsewhere rather than restoring homeLink. Keep view and agencyName as-is if they are used elsewhere.
🧹 Nitpick comments (1)
src/app/_components/common/navbar.module.scss (1)
11-14: Duplicatefont-weight: 400declaration for.navLinkDefault.
font-weight: 400is declared twice for.navLinkDefault(lines 12 and 30). Additionally, thetransitionproperty at line 13 (color 0.2s ease) is overwritten by line 20 (background-color 0.2s ease), so the color transition won't apply.Consider consolidating into a single block or combining transitions:
♻️ Proposed consolidation
.navLinkDefault { font-weight: 400; - transition: color 0.2s ease; + transition: color 0.2s ease, background-color 0.2s ease; } .navLinkDefault, .navLinkActive { font-size: 0.75rem; cursor: pointer; - transition: background-color 0.2s ease; padding: 0.5rem 0.75rem; border-radius: 4px; } .navLinkDefault:hover { background-color: var(--color-light-grey); } -.navLinkDefault { - font-weight: 400; -} - .navLinkActive { font-weight: 600; + transition: background-color 0.2s ease; }Also applies to: 29-31
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/_components/common/navbar.module.scss` around lines 11 - 14, The .navLinkDefault rules contain duplicated font-weight and conflicting transition declarations; remove the duplicate font-weight and consolidate the transition declarations for .navLinkDefault so both color and background-color animate (e.g., use a single .navLinkDefault rule that keeps font-weight: 400 and a combined transition like "transition: color 0.2s ease, background-color 0.2s ease"). Update any other duplicate blocks (the one around lines 29-31) to use the same consolidated rule so there’s only one .navLinkDefault definition.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/app/_components/common/navbar.tsx`:
- Around line 35-61: Remove the dead code: delete the section constant and the
navbarText() and homeLink() functions from the component (they're unused and
homeLink() still returns the stale `/admin/home` path). Search for references to
navbarText, homeLink, and section to confirm none remain; if other code expects
a home path, replace usages with the new routing logic elsewhere rather than
restoring homeLink. Keep view and agencyName as-is if they are used elsewhere.
---
Nitpick comments:
In `@src/app/_components/common/navbar.module.scss`:
- Around line 11-14: The .navLinkDefault rules contain duplicated font-weight
and conflicting transition declarations; remove the duplicate font-weight and
consolidate the transition declarations for .navLinkDefault so both color and
background-color animate (e.g., use a single .navLinkDefault rule that keeps
font-weight: 400 and a combined transition like "transition: color 0.2s ease,
background-color 0.2s ease"). Update any other duplicate blocks (the one around
lines 29-31) to use the same consolidated rule so there’s only one
.navLinkDefault definition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 698cbd51-ad22-4f1a-be11-20c9dd4052da
📒 Files selected for processing (3)
src/app/_components/common/navbar.module.scsssrc/app/_components/common/navbar.tsxsrc/server/api/routers/organizations.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/server/api/routers/organizations.ts
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
New Features
Refactor