Sanc 57 large view agencies pages #38
Conversation
Yemyam
requested review from
JustinTan-1,
Lujarios,
burtonjong,
jason-duong4509,
promatty,
tanvimahal,
themaxboucher and
wesleylui
as code owners
📝 WalkthroughWalkthroughAdds an admin "Agencies" page and navbar link, server endpoint to fetch organizations with members, database relation exports for organizations/members, a mail SVG icon, and SCSS for the agencies admin UI. Changes
Sequence DiagramsequenceDiagram
autonumber
actor User
participant Navbar as Navbar component
participant Browser as AgenciesPage (client)
participant TRPC as TRPC client
participant API as organizationRouter.getAllWithMembers
participant DB as Database
User->>Navbar: Click "View Agencies"
Navbar->>Browser: Navigate to /admin/agencies
Browser->>TRPC: request getAllWithMembers()
TRPC->>API: invoke procedure
API->>DB: query organizations with members & users
DB-->>API: return nested orgs + members
API-->>TRPC: return data
TRPC-->>Browser: deliver organizations payload
Browser->>Browser: set selectedAgencyId, render tabs & table
Browser-->>User: display agencies and members
alt User exports CSV
User->>Browser: Click "Export to CSV"
Browser->>Browser: generate CSV from selected agency & members
Browser-->>User: prompt file download
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
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: 4
🧹 Nitpick comments (4)
src/assets/icons/mail.tsx (1)
1-32: LGTM with a minor consistency note.The component follows the existing icon patterns with proper accessibility attributes. Consider using
#434343for the stroke color to match other icons (e.g.,edit.tsx,call.tsx) for visual consistency across the icon set.src/app/admin/agencies/page.tsx (1)
26-32: Consider memoizing theformatDatehelper.Defining
formatDateinside the component recreates it on every render. Since it has no dependencies on component state, extract it outside the component or wrap withuseCallback.🔎 Proposed fix - extract outside component
+// Format date helper - outside component +const formatDate = (date: Date) => { + return new Intl.DateTimeFormat("en-US", { + month: "long", + day: "numeric", + year: "numeric", + }).format(new Date(date)); +}; export default function AgenciesPage() { // ... - // Format date helper - const formatDate = (date: Date) => { - return new Intl.DateTimeFormat("en-US", { - month: "long", - day: "numeric", - year: "numeric", - }).format(new Date(date)); - };src/app/admin/agencies/agencies.module.scss (2)
17-26: Movetransitionto the base selector for smooth hover-out animation.Currently the transition is defined inside
:hover, which means the transition only applies when hovering in, not when hovering out. Moving it to the base.agencyTabselector ensures a smooth animation in both directions.🔎 Proposed fix
.agencyTab { border-radius: 24px; padding: 0.5rem 1.5rem; font-weight: 500; + transition: transform 0.2s ease; &:hover { transform: translateY(-2px); - transition: transform 0.2s ease; } }
43-44: Empty selector can be removed.
.userInfoSectionis empty and provides no styling. Consider removing it unless it's a placeholder for future styles.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/app/_components/common/navbar.tsxsrc/app/admin/agencies/agencies.module.scsssrc/app/admin/agencies/page.tsxsrc/assets/icons/mail.tsxsrc/server/api/routers/organizations.tssrc/server/db/auth-schema.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-23T05:44:35.017Z
Learnt from: tanvimahal
Repo: Code-the-Change-YYC/salvationarmy PR: 25
File: src/server/api/routers/organizations.ts:38-38
Timestamp: 2025-12-23T05:44:35.017Z
Learning: In this codebase's TRPC protectedProcedure context, ctx.session has a nested structure containing both a session object and a user object. To access activeOrganizationId, use ctx.session.session.activeOrganizationId (not ctx.session.activeOrganizationId).
Applied to files:
src/server/api/routers/organizations.ts
🧬 Code graph analysis (5)
src/server/api/routers/organizations.ts (2)
src/server/api/trpc.ts (1)
protectedProcedure(122-132)src/lib/auth.ts (1)
session(94-112)
src/server/db/auth-schema.ts (2)
src/server/db/booking-schema.ts (1)
agency(39-50)src/lib/auth.ts (1)
session(94-112)
src/assets/icons/mail.tsx (6)
src/assets/icons/bell.tsx (1)
props(4-35)src/assets/icons/edit.tsx (1)
props(4-78)src/assets/icons/user.tsx (1)
props(4-48)src/assets/icons/call.tsx (1)
props(4-52)src/assets/icons/calendar.tsx (1)
props(4-108)src/assets/icons/location.tsx (1)
props(4-110)
src/app/_components/common/navbar.tsx (1)
src/app/_components/common/button/Button.tsx (1)
Button(41-80)
src/app/admin/agencies/page.tsx (2)
src/trpc/react.tsx (1)
api(25-25)src/app/_components/common/button/Button.tsx (1)
Button(41-80)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci
🔇 Additional comments (2)
src/app/_components/common/navbar.tsx (1)
33-35: LGTM!The new "View Agencies" navigation item follows the existing pattern used for other admin navigation links, maintaining consistency with the codebase.
src/server/db/auth-schema.ts (1)
106-125: LGTM!The relation definitions are correctly structured:
userRelationsincludesmembershipsfor user → member (one-to-many)organizationRelationslinks organization → members (one-to-many)memberRelationsproperly defines the inverse relations with field/reference mappingsThis enables the nested
withqueries used ingetAllWithMembers.
promatty
left a comment
promatty
left a comment
There was a problem hiding this comment.
can we also get a vertical line separating the columns of the table
| month: "long", | ||
| day: "numeric", | ||
| year: "numeric", | ||
| }).format(new Date(date)); |
There was a problem hiding this comment.
i feel like creating a new Date object is causing slight timezone conversion issues. For example, my creation date for an org is "2025-12-23 02:31:17.386" in supabase, but the date joined says: "Date Joined December 22, 2025". if creating a new date isnt the issue, we must still figure out where this mismatch in conversion is.
admin view:
supabase item:
There was a problem hiding this comment.
I think the date was being formatted from utc to our local time. I updated formatDate to use the utc timezone
There was a problem hiding this comment.
@Yemyam ohh i see, its a supabase timezone issue. lets actually keep it as local time in the format function then. but still, is there an issue where we are double converting this date? since it comes in as a Date type already, but we make it a new Date() as well?
src/app/admin/agencies/page.tsx
Outdated
| ); | ||
| } | ||
|
|
||
| const handleExportToCSV = () => { |
There was a problem hiding this comment.
the export to csv functionality isnt apart of your ticket. it will have to be a separate, dynamic component which adjusts to all the different tables itll be used in. please remove the implementation here
|
can you also delete this line with the "hello {name}" as part of your pr lol salvationarmy/src/app/admin/layout.tsx Line 14 in 1646fe3 |
Yemyam
force-pushed
the
SANC-57-LARGE-view-agencies-pages
branch
from
6ec405b to
7ad1739
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/assets/icons/mail.tsx (1)
15-28: Consider usingcurrentColorfor stroke to enable color customization.The hardcoded
#424242stroke color limits reusability. UsingcurrentColorwould allow the icon to inherit the text color from its parent, making it easier to style in different contexts.🔎 Proposed fix
<path d="M4 4H20C21.1 4 22 4.9 22 6V18C22 19.1 21.1 20 20 20H4C2.9 20 2 19.1 2 18V6C2 4.9 2.9 4 4 4Z" - stroke="#424242" + stroke="currentColor" strokeWidth={2} strokeLinecap="round" strokeLinejoin="round" /> <path d="M22 6L12 13L2 6" - stroke="#424242" + stroke="currentColor" strokeWidth={2} strokeLinecap="round" strokeLinejoin="round" />src/app/admin/agencies/page.tsx (1)
27-35: Consider extractingformatDateoutside the component.Defining
formatDateinside the component recreates it on every render. Moving it outside improves performance slightly and clarifies it has no dependency on component state.🔎 Proposed fix
+// Format date helper +const formatDate = (date: Date) => { + return new Intl.DateTimeFormat("en-US", { + month: "long", + day: "numeric", + year: "numeric", + timeZone: "UTC", + }).format(new Date(date)); +}; + export default function AgenciesPage() { const { data: organizations, isLoading } = api.organization.getAllWithMembers.useQuery(); const [selectedAgencyId, setSelectedAgencyId] = useState<string | null>(null); // Set the first agency as selected when data loads const selectedAgency = organizations?.find((org) => org.id === selectedAgencyId) ?? organizations?.[0]; // Update selected ID when organizations load useEffect(() => { if (organizations && organizations.length > 0 && !selectedAgencyId) { setSelectedAgencyId(organizations[0]?.id ?? null); } }, [organizations, selectedAgencyId]); - // Format date helper - const formatDate = (date: Date) => { - return new Intl.DateTimeFormat("en-US", { - month: "long", - day: "numeric", - year: "numeric", - timeZone: "UTC", - }).format(new Date(date)); - };src/app/admin/agencies/agencies.module.scss (2)
17-26: Movetransitionoutside of&:hoverto enable smooth hover-out animation.The transition is defined inside the
:hoverpseudo-class, so the transform animates on hover-in but snaps back instantly on hover-out.🔎 Proposed fix
.agencyTab { border-radius: 24px; padding: 0.5rem 1.5rem; font-weight: 500; + transition: transform 0.2s ease; &:hover { transform: translateY(-2px); - transition: transform 0.2s ease; } }
43-44: Empty.userInfoSectionclass can be removed or populated.This class is currently empty. If no styles are planned, consider removing it to avoid dead code.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/app/_components/common/navbar.tsxsrc/app/admin/agencies/agencies.module.scsssrc/app/admin/agencies/page.tsxsrc/app/admin/layout.tsxsrc/assets/icons/mail.tsxsrc/server/api/routers/organizations.tssrc/server/db/auth-schema.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/server/db/auth-schema.ts
- src/app/_components/common/navbar.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-23T05:44:37.925Z
Learnt from: tanvimahal
Repo: Code-the-Change-YYC/salvationarmy PR: 25
File: src/server/api/routers/organizations.ts:38-38
Timestamp: 2025-12-23T05:44:37.925Z
Learning: In this codebase's TRPC protectedProcedure context, ctx.session has a nested structure containing both a session object and a user object. To access activeOrganizationId, use ctx.session.session.activeOrganizationId (not ctx.session.activeOrganizationId).
Applied to files:
src/server/api/routers/organizations.ts
🧬 Code graph analysis (2)
src/server/api/routers/organizations.ts (1)
src/server/api/trpc.ts (1)
adminProcedure(134-146)
src/app/admin/agencies/page.tsx (2)
src/trpc/react.tsx (1)
api(25-25)src/server/db/auth-schema.ts (1)
member(80-90)
🔇 Additional comments (4)
src/app/admin/layout.tsx (1)
7-15: LGTM!The greeting removal has been addressed as requested. The layout now cleanly wraps children with the admin navbar and HydrateClient.
src/server/api/routers/organizations.ts (1)
28-47: LGTM!The endpoint now correctly uses
adminProcedureto restrict access to admin users, addressing the previous review concern about protecting member PII. The implementation follows the established pattern in this router.src/app/admin/agencies/page.tsx (2)
21-25: LGTM!The
setSelectedAgencyIdcall has been correctly moved to auseEffecthook, addressing the previous review concern about calling setState during render.
45-54: LGTM!Good addition of the empty state message when no organizations exist, addressing the previous review feedback.
| <Button | ||
| variant="outline" | ||
| color="dark" | ||
| leftSection={<Grid />} | ||
| className={styles.exportButton} | ||
| > | ||
| Export to CSV File | ||
| </Button> |
There was a problem hiding this comment.
Export button has no functionality - consider removing or disabling it.
The "Export to CSV File" button is rendered but has no onClick handler. Per the earlier review comment that CSV export should be a separate, dynamic component, this button should either be removed or disabled with a tooltip indicating it's not yet implemented.
🔎 Option 1: Remove the button
<div className={styles.header}>
<Title order={2}>View all Agencies</Title>
- <Button
- variant="outline"
- color="dark"
- leftSection={<Grid />}
- className={styles.exportButton}
- >
- Export to CSV File
- </Button>
</div>🔎 Option 2: Disable with indication
<Button
variant="outline"
color="dark"
leftSection={<Grid />}
className={styles.exportButton}
+ disabled
+ title="Coming soon"
>
Export to CSV File
</Button>📝 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.
| <Button | |
| variant="outline" | |
| color="dark" | |
| leftSection={<Grid />} | |
| className={styles.exportButton} | |
| > | |
| Export to CSV File | |
| </Button> | |
| <Button | |
| variant="outline" | |
| color="dark" | |
| leftSection={<Grid />} | |
| className={styles.exportButton} | |
| disabled | |
| title="Coming soon" | |
| > | |
| Export to CSV File | |
| </Button> |
🤖 Prompt for AI Agents
In src/app/admin/agencies/page.tsx around lines 60 to 67, the "Export to CSV
File" Button is rendered without any onClick behavior; either remove the Button
or disable it with an explanatory tooltip until the CSV export component is
implemented. To fix: delete the Button JSX if you choose removal, or set the
Button to disabled and wrap it with a Tooltip (or add a title attribute) that
clearly indicates "CSV export not yet implemented" so users know why it’s
inactive.
There was a problem hiding this comment.
@promatty, understood! I'll leave the button as is. ✓
View Agencies Page
Adds admin page to view all organizations and their members.
Features
Changes
Frontend:
/admin/agenciesBackend:
getAllWithMembersquery to fetch organizations with membersSummary by CodeRabbit
New Features
Style / UI
✏️ Tip: You can customize this high-level summary in your review settings.