Open
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR reorganizes event and coaching management by introducing URL-driven filtering via Changes
Sequence DiagramsequenceDiagram
participant User as User
participant EventsPage as EventsPage Component
participant URL as URL State (nuqs)
participant API as API Client
participant EventListTable as EventListTable
participant Database as Database
User->>EventsPage: Open /events or /coaching
EventsPage->>URL: Initialize filter state (date range, mode)
User->>EventsPage: Adjust filters (name, activist, date, type)
EventsPage->>URL: Update URL with new filter state
EventsPage->>API: getEventList(filter params)
API->>Database: Query events with filters
Database-->>API: Event list response
API-->>EventsPage: EventListItem[]
EventsPage->>EventListTable: Pass events, mode, onDelete handler
EventListTable-->>User: Render sortable/expandable table or cards
User->>EventListTable: Click sort, expand row, or edit/delete
EventListTable->>User: Show expandable details or navigate
User->>EventListTable: Trigger delete action
EventListTable->>EventsPage: onDelete callback
EventsPage->>API: deleteEvent(eventId)
API->>Database: Delete event
Database-->>API: Confirm deletion
API-->>EventsPage: Success response
EventsPage->>API: Refetch event list (cache invalidation)
API->>Database: Query updated events
Database-->>API: Updated event list
API-->>EventListTable: Refresh data
EventListTable-->>User: Display updated list
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
Comment |
jakehobbs
commented
Mar 2, 2026
| ) | ||
| const [showFilters, setShowFilters] = useState(false) | ||
|
|
||
| // Dirty = committed filters differ from defaults (controls Reset button) |
Member
Author
There was a problem hiding this comment.
claude wrote a bunch of comments again. not sure if necessary
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend-v2/src/app/`(authed)/events/[id]/loading.tsx:
- Line 7: The loading header currently uses the ambiguous title "Attendance";
update the JSX heading in the loading.tsx component (the <h1 className="text-3xl
font-bold"> element) to an edit-event-specific label such as "Editing event" or
"Editing event…" so the UI clearly reflects the event edit route during
transitions; keep the existing className and structure but replace the inner
text accordingly.
In `@frontend-v2/src/app/`(authed)/events/event-list-table.tsx:
- Around line 95-102: The icon-only delete Button (component named Button
rendering <Trash2 /> with onClick calling onDelete(row.original)) lacks an
accessible name; update both instances in event-list-table.tsx to provide an
explicit accessible label (e.g., add aria-label or aria-labelledby and/or a
title) that describes the action and the target item (for example "Delete event:
{row.original.title}" or similar derived from row.original), ensuring screen
readers announce the purpose; keep the visual icon-only appearance (do not add
visible text) and ensure the prop is applied to the same Button elements that
call onDelete.
- Around line 366-368: The attendee list uses key={name} in the
event.attendees.map which can collide for duplicate names; change the key to a
truly unique value (e.g., include the event id and the index or an attendee id
if available). Update the map callback (event.attendees.map((name) => ...)) to
accept the index or use an attendee object and set key to a composite like
`${event.id}-${index}-${name}` or attendee.id so each <li> has a stable, unique
key.
In `@frontend-v2/src/app/`(authed)/events/events-page.tsx:
- Around line 194-201: handleFilter is currently committing empty string date
values (formStart/formEnd) into setUrlParams which results in invalid
event_date_start/event_date_end being sent to getEventList; update the date
parameters so that when formStart or formEnd are empty strings they become null
— e.g. replace the current checks with logic that treats '' as null and only
sets start/end when the value is non-empty and different from
defaultParams.event_date_start/event_date_end (or use a small helper like
normalizeDateParam(value, default) to return null for '' or default), ensuring
setUrlParams receives null for cleared dates rather than ''.
- Around line 49-60: ActivistFilterInput currently calls useActivistRegistry()
directly which causes the registry hook to remount and re-sync whenever
ActivistFilterInput is mounted/unmounted via the showFilters toggle; lift the
hook out of the input component into the parent (e.g., the EventsPage component
that owns showFilters), call useActivistRegistry() there once, and pass the
resulting registry (and any relevant registry methods) into ActivistFilterInput
as props; update ActivistFilterInput to accept a registry prop instead of
calling useActivistRegistry() itself (apply the same lift to the other filter
components noted in the comment).
In `@frontend-v2/src/app/`(authed)/events/new/loading.tsx:
- Line 7: Update the heading text in the loading component so it matches the
/events/new route: in frontend-v2/src/app/(authed)/events/new/loading.tsx
replace the h1 content currently reading "Attendance" with "New event" (preserve
the existing className and structure in the default Loading component or
whichever function returns that JSX).
In `@frontend-v2/src/components/nav.tsx`:
- Around line 105-114: Replace the simple equality check page === item.page in
AdbNav.vue with the TSX active-state logic: compute siblingExactMatch by
checking item.items.some(sibling => sibling !== innerItem &&
sibling.href.startsWith('/v2') && pathname === sibling.href.substring(3)), then
compute isActive using navPath !== null && (pathname === navPath ||
(!siblingExactMatch && pathname.startsWith(navPath + '/'))); use that isActive
value for the active class/flag. Ensure you reference the same symbols used in
the TSX code: siblingExactMatch, innerItem, item.items, pathname, navPath, and
isActive.
In `@frontend-v2/src/lib/api.ts`:
- Around line 406-411: The deleteEvent method sends a mutating POST to
API_PATH.EVENT_DELETE without the X-CSRF-Token header; update deleteEvent to
include the same CSRF header used by other write methods (e.g., add headers: {
"X-CSRF-Token": this.csrfToken || this.getCsrfToken() } or whatever CSRF
accessor your class uses) when calling this.client.post(API_PATH.EVENT_DELETE, {
body, headers }) so the request matches the application's CSRF-protected write
paths.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend-v2/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (28)
frontend-v2/package.jsonfrontend-v2/src/app/(authed)/coaching/[id]/loading.tsxfrontend-v2/src/app/(authed)/coaching/[id]/page.tsxfrontend-v2/src/app/(authed)/coaching/coaching-list-page.tsxfrontend-v2/src/app/(authed)/coaching/loading.tsxfrontend-v2/src/app/(authed)/coaching/new/loading.tsxfrontend-v2/src/app/(authed)/coaching/new/page.tsxfrontend-v2/src/app/(authed)/coaching/page.tsxfrontend-v2/src/app/(authed)/events/[id]/loading.tsxfrontend-v2/src/app/(authed)/events/[id]/page.tsxfrontend-v2/src/app/(authed)/events/activist-registry.tsfrontend-v2/src/app/(authed)/events/activist-storage.tsfrontend-v2/src/app/(authed)/events/attendee-input-field.tsxfrontend-v2/src/app/(authed)/events/event-form.tsxfrontend-v2/src/app/(authed)/events/event-list-table.tsxfrontend-v2/src/app/(authed)/events/events-page.tsxfrontend-v2/src/app/(authed)/events/loading.tsxfrontend-v2/src/app/(authed)/events/new/loading.tsxfrontend-v2/src/app/(authed)/events/new/page.tsxfrontend-v2/src/app/(authed)/events/page.tsxfrontend-v2/src/app/(authed)/events/useActivistRegistry.tsfrontend-v2/src/app/(authed)/interest/generator/generator-form.tsxfrontend-v2/src/app/(authed)/users/user-table.tsxfrontend-v2/src/app/providers.tsxfrontend-v2/src/components/nav.tsxfrontend-v2/src/components/ui/date-picker.tsxfrontend-v2/src/lib/api.tsshared/nav.json
jakehobbs
commented
Mar 2, 2026
| setSelectedIndex(-1) | ||
| } | ||
|
|
||
| // Handles ArrowUp, ArrowDown, and Escape. Callers handle Enter/Tab themselves. |
Member
Author
There was a problem hiding this comment.
lol this is kinda funny
jakehobbs
commented
Mar 2, 2026
| @@ -0,0 +1,371 @@ | |||
| 'use client' | |||
Member
Author
There was a problem hiding this comment.
no prefetching on this page b/c the way we do the default date filter is a little weird. might need to make changes on backend later to enable this.
Collaborator
There was a problem hiding this comment.
might be nice to add this as a code comment
alexsapps
requested changes
Mar 2, 2026
Collaborator
alexsapps
left a comment
alexsapps
left a comment
There was a problem hiding this comment.
comments round 1 part 1. didn't read event table and events page yet but looks good so far.
Collaborator
There was a problem hiding this comment.
should the directory name be plural, "coachings"?
| <CalendarIcon className="mr-2 h-4 w-4 shrink-0" /> | ||
| <span className="truncate"> | ||
| {value ? format(value, 'PP') : placeholder} | ||
| </span> |
Collaborator
There was a problem hiding this comment.
wanna document/explain this change in the components/README?
| .array(z.string()) | ||
| .nullable() | ||
| .transform((v) => v ?? []), | ||
| attendee_emails: z |
Collaborator
There was a problem hiding this comment.
this should maybe be a separate change but i wanna change it so attendance takers don't get access to anyone's email just by adding their name to attendance.
| export type EventListMode = 'events' | 'connections' | ||
|
|
||
| const EVENT_TYPES: { value: EventType; label: string }[] = [ | ||
| { value: 'noConnections', label: 'All' }, |
Collaborator
There was a problem hiding this comment.
why is this "noConnections"? maybe add a comment or rename it to "All"?
Comment on lines
+103
to
+115
| const navPath = innerItem.href.startsWith('/v2') | ||
| ? innerItem.href.substring(3) | ||
| : null | ||
| const siblingExactMatch = item.items.some( | ||
| (sibling) => | ||
| sibling !== innerItem && | ||
| sibling.href.startsWith('/v2') && | ||
| pathname === sibling.href.substring(3), | ||
| ) | ||
| const isActive = | ||
| navPath !== null && | ||
| (pathname === navPath || | ||
| (!siblingExactMatch && pathname.startsWith(navPath + '/'))) |
Collaborator
There was a problem hiding this comment.
could this logic be moved above to keep layout separate from navigation logic?
maybe like
let childrenItems = null;
if (isExpanded) {
childrenItems = item.items.map(item => {navPath:..., siblingExactMatch:..., isActive:...});
}
then later
return ... {childrenItems && <div> ... {childrenItems.map(...)} }
| <NuqsAdapter> | ||
| <QueryClientProvider client={queryClient}> | ||
| {children} | ||
| <ReactQueryDevtools |
Collaborator
There was a problem hiding this comment.
yay. hope it doesn't cover up nextjs on the bottom left.
| label, | ||
| className, | ||
| }: { | ||
| registry: ReturnType<typeof useActivistRegistry>['registry'] |
Collaborator
There was a problem hiding this comment.
this could just be registry: ActivistRegistry ?
| ] | ||
|
|
||
| // Activist autocomplete input backed by the activist registry | ||
| function ActivistFilterInput({ |
Collaborator
There was a problem hiding this comment.
big enough to be its own file imo
alexsapps
reviewed
Mar 3, 2026
Collaborator
alexsapps
left a comment
alexsapps
left a comment
There was a problem hiding this comment.
round 1 pt 2, got more to look at still
| className?: string | ||
| }) { | ||
| const { suggestions, selectedIndex, onInputChange, onSelect, onKeyDown } = | ||
| useSuggestions((v) => registry.getSuggestions(v)) |
Collaborator
There was a problem hiding this comment.
this gets me pretty confused, calling a hook that returns 2 outputs and 3 callbacks with names that overlap with this function's callbacks, and some of the outputs forwarded to while others are used directly.
how about a component that wraps all of these concerns into one place?
#315
alternatively i was thinking about monitoring key presses in this component and dispatching to methods on with a ref, but codex told me that's not idiomatic so i didn't try it, and anyway it might not solve the problem as well as a dedicated component.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
UI/Style