Open
Conversation
📝 WalkthroughWalkthroughThe pull request replaces the schedule page's timeline-based UI with a table-based component. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/components/shadcn/ui/table.tsx (1)
84-97:⚠️ Potential issue | 🟠 MajorRemove default padding from TableCell without updating dependent components breaks existing tables.
Removing
p-4fromTableCellis a breaking change affecting all tables using this component. WhileScheduleTablewas updated with explicitclassName="p-2", the tables inEventDataTable,UserDataTable, andNavItemsManagerwere not updated and now lack cell padding. Either keep the default padding and override where needed, or update all affected tables to specify explicit padding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/shadcn/ui/table.tsx` around lines 84 - 97, The TableCell component was changed to remove the default padding (previously "p-4"), which breaks tables that weren't updated; either restore the original default padding in TableCell (add "p-4" back into the cn(...) call in the TableCell forwardRef) or update every dependent table component (e.g., ScheduleTable, EventDataTable, UserDataTable, NavItemsManager) to explicitly pass a padding class (like className="p-2" or "p-4") when rendering TableCell so existing tables keep their expected spacing.
🧹 Nitpick comments (3)
apps/web/src/components/schedule/ScheduleTable.tsx (3)
84-96: Stale comment and unnecessary option.
- Line 84: The comment
// Needs timezone propappears outdated since the component uses the module-leveluserTimeZone.- Line 94:
useAdditionalDayOfYearTokensis only needed forD/DDtokens; the format"hh:mm a"doesn't use these.Proposed cleanup
-// Needs timezone prop type eventRowProps = { event: Event; }; export function EventRow({ event }: eventRowProps) { const startTimeFormatted = formatInTimeZone( event.startTime, userTimeZone, "hh:mm a", - { - useAdditionalDayOfYearTokens: true, - }, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/schedule/ScheduleTable.tsx` around lines 84 - 96, Remove the stale comment and the unnecessary option: delete the "// Needs timezone prop" comment above the eventRowProps/EventRow definitions since EventRow already uses the module-level userTimeZone, and remove the useAdditionalDayOfYearTokens option passed to formatInTimeZone in EventRow (the format "hh:mm a" doesn't require that token). Keep the call to formatInTimeZone with event.startTime and userTimeZone unchanged otherwise.
106-108: Redundantkeyprop.The
keyis already provided when renderingEventRowat line 70. This innerkeyonTableRowis unnecessary.Proposed fix
return ( <TableRow - key={event.id} className="flex flex-col justify-around bg-transparent px-4 pb-1 odd:bg-white/5" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/schedule/ScheduleTable.tsx` around lines 106 - 108, Remove the redundant key prop from the inner TableRow element: the unique key is already assigned to EventRow where the list is mapped, so delete the key={event.id} on TableRow (the JSX element named TableRow in ScheduleTable.tsx) and keep the existing key on EventRow to avoid duplicate/unused keys.
46-50: Fragile date formatting with hardcoded substring.Trimming the last 6 characters assumes a specific locale format. Consider using a format pattern that produces the desired output directly, such as
"EEEE, MMMM d"for "Friday, March 20".Proposed fix
function eventDateString(arr: Event[], timezone: string) { - const date = formatInTimeZone(arr[0].startTime, timezone, "PPPP"); - const retString = date.substring(0, date.length - 6); - return retString; + return formatInTimeZone(arr[0].startTime, timezone, "EEEE, MMMM d"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/schedule/ScheduleTable.tsx` around lines 46 - 50, The eventDateString function uses a fragile substring to trim the formatted date; replace this by calling formatInTimeZone with an explicit format string (e.g., "EEEE, MMMM d") so the output is directly "Friday, March 20" and not dependent on locale length; update eventDateString to formatInTimeZone(arr[0].startTime, timezone, "EEEE, MMMM d") and also add a small guard to handle empty arr to avoid runtime errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/components/schedule/ScheduleTable.tsx`:
- Line 103: The code gets color via const color = (c.eventTypes as
Record<string, string>)[event.type] which can be undefined for unknown
event.type; change this to use a fallback (same approach used in
EventDetails.tsx) so color defaults to a safe value (e.g., an empty string or a
default color key) when event.type is not present in c.eventTypes; update the
reference in ScheduleTable.tsx where color is used (the badge border rendering)
to rely on this fallback to ensure the badge always renders.
- Around line 58-77: The mapped fragment uses a shorthand <> but the key is
placed on the inner TableBody which causes a React "missing key on fragment"
warning; change the outer fragment to an explicit React.Fragment and move the
key to that fragment (i.e., replace <> with React.Fragment key={dateID}) so the
key is on the outermost element returned from the
Array.from(splitByDay(schedule).entries()).map callback (referencing splitByDay,
dateID, TableBody, and EventRow to locate the code).
In `@apps/web/src/components/shared/NavBarLinksGrouper.tsx`:
- Line 15: Remove the trailing whitespace after the closing brace in
NavBarLinksGrouper.tsx (the extra space following the '}' in the component/file)
so Prettier passes; simply delete the trailing spaces or run Prettier/format on
the file (e.g., target the NavBarLinksGrouper component/definition) and commit
the formatted file.
---
Outside diff comments:
In `@apps/web/src/components/shadcn/ui/table.tsx`:
- Around line 84-97: The TableCell component was changed to remove the default
padding (previously "p-4"), which breaks tables that weren't updated; either
restore the original default padding in TableCell (add "p-4" back into the
cn(...) call in the TableCell forwardRef) or update every dependent table
component (e.g., ScheduleTable, EventDataTable, UserDataTable, NavItemsManager)
to explicitly pass a padding class (like className="p-2" or "p-4") when
rendering TableCell so existing tables keep their expected spacing.
---
Nitpick comments:
In `@apps/web/src/components/schedule/ScheduleTable.tsx`:
- Around line 84-96: Remove the stale comment and the unnecessary option: delete
the "// Needs timezone prop" comment above the eventRowProps/EventRow
definitions since EventRow already uses the module-level userTimeZone, and
remove the useAdditionalDayOfYearTokens option passed to formatInTimeZone in
EventRow (the format "hh:mm a" doesn't require that token). Keep the call to
formatInTimeZone with event.startTime and userTimeZone unchanged otherwise.
- Around line 106-108: Remove the redundant key prop from the inner TableRow
element: the unique key is already assigned to EventRow where the list is
mapped, so delete the key={event.id} on TableRow (the JSX element named TableRow
in ScheduleTable.tsx) and keep the existing key on EventRow to avoid
duplicate/unused keys.
- Around line 46-50: The eventDateString function uses a fragile substring to
trim the formatted date; replace this by calling formatInTimeZone with an
explicit format string (e.g., "EEEE, MMMM d") so the output is directly "Friday,
March 20" and not dependent on locale length; update eventDateString to
formatInTimeZone(arr[0].startTime, timezone, "EEEE, MMMM d") and also add a
small guard to handle empty arr to avoid runtime errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ac00d3e6-1c47-4768-8362-6c7a00cd671b
📒 Files selected for processing (4)
apps/web/src/app/schedule/page.tsxapps/web/src/components/schedule/ScheduleTable.tsxapps/web/src/components/shadcn/ui/table.tsxapps/web/src/components/shared/NavBarLinksGrouper.tsx
Comment on lines
+58
to
+77
| {Array.from(splitByDay(schedule).entries()).map( | ||
| ([dateID, arr]): ReactNode => ( | ||
| <> | ||
| <TableBody key={dateID} className="border"> | ||
| <TableHeader className="flex justify-start"> | ||
| <span className="m-1 p-4 text-center text-xl font-bold lg:text-4xl"> | ||
| <p>{`${eventDateString(arr, userTimeZone)}`}</p> | ||
| </span> | ||
| </TableHeader> | ||
| {arr.map( | ||
| (event): ReactNode => ( | ||
| <EventRow | ||
| key={event.id} | ||
| event={event} | ||
| /> | ||
| ), | ||
| )} | ||
| </TableBody> | ||
| </> | ||
| ), |
There was a problem hiding this comment.
Missing key on Fragment causes React warning.
When mapping, the key should be on the outermost element. The fragment <> is outermost, but the key is on TableBody inside. Use React.Fragment with an explicit key instead.
Proposed fix
+import React, { type ReactNode } from "react";
-import { ReactNode } from "react"; {Array.from(splitByDay(schedule).entries()).map(
([dateID, arr]): ReactNode => (
- <>
- <TableBody key={dateID} className="border">
+ <React.Fragment key={dateID}>
+ <TableBody className="border">
<TableHeader className="flex justify-start">
<span className="m-1 p-4 text-center text-xl font-bold lg:text-4xl">
<p>{`${eventDateString(arr, userTimeZone)}`}</p>
</span>
</TableHeader>
{arr.map(
(event): ReactNode => (
<EventRow
key={event.id}
event={event}
/>
),
)}
</TableBody>
- </>
+ </React.Fragment>
),
)}📝 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.
Suggested change
| {Array.from(splitByDay(schedule).entries()).map( | |
| ([dateID, arr]): ReactNode => ( | |
| <> | |
| <TableBody key={dateID} className="border"> | |
| <TableHeader className="flex justify-start"> | |
| <span className="m-1 p-4 text-center text-xl font-bold lg:text-4xl"> | |
| <p>{`${eventDateString(arr, userTimeZone)}`}</p> | |
| </span> | |
| </TableHeader> | |
| {arr.map( | |
| (event): ReactNode => ( | |
| <EventRow | |
| key={event.id} | |
| event={event} | |
| /> | |
| ), | |
| )} | |
| </TableBody> | |
| </> | |
| ), | |
| {Array.from(splitByDay(schedule).entries()).map( | |
| ([dateID, arr]): ReactNode => ( | |
| <React.Fragment key={dateID}> | |
| <TableBody className="border"> | |
| <TableHeader className="flex justify-start"> | |
| <span className="m-1 p-4 text-center text-xl font-bold lg:text-4xl"> | |
| <p>{`${eventDateString(arr, userTimeZone)}`}</p> | |
| </span> | |
| </TableHeader> | |
| {arr.map( | |
| (event): ReactNode => ( | |
| <EventRow | |
| key={event.id} | |
| event={event} | |
| /> | |
| ), | |
| )} | |
| </TableBody> | |
| </React.Fragment> | |
| ), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/schedule/ScheduleTable.tsx` around lines 58 - 77, The
mapped fragment uses a shorthand <> but the key is placed on the inner TableBody
which causes a React "missing key on fragment" warning; change the outer
fragment to an explicit React.Fragment and move the key to that fragment (i.e.,
replace <> with React.Fragment key={dateID}) so the key is on the outermost
element returned from the Array.from(splitByDay(schedule).entries()).map
callback (referencing splitByDay, dateID, TableBody, and EventRow to locate the
code).
| userTimeZone, | ||
| "h:mm a", | ||
| ); | ||
| const color = (c.eventTypes as Record<string, string>)[event.type]; |
There was a problem hiding this comment.
Add fallback for unknown event types.
If event.type is not a key in c.eventTypes, color will be undefined and the badge border won't render. Add a fallback like EventDetails.tsx does (see apps/web/src/components/events/admin/EventDetails.tsx:17-18).
Proposed fix
- const color = (c.eventTypes as Record<string, string>)[event.type];
+ const color = (c.eventTypes as Record<string, string>)[event.type] || c.eventTypes.Other;📝 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.
Suggested change
| const color = (c.eventTypes as Record<string, string>)[event.type]; | |
| const color = (c.eventTypes as Record<string, string>)[event.type] || c.eventTypes.Other; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/schedule/ScheduleTable.tsx` at line 103, The code
gets color via const color = (c.eventTypes as Record<string,
string>)[event.type] which can be undefined for unknown event.type; change this
to use a fallback (same approach used in EventDetails.tsx) so color defaults to
a safe value (e.g., an empty string or a default color key) when event.type is
not present in c.eventTypes; update the reference in ScheduleTable.tsx where
color is used (the badge border rendering) to rely on this fallback to ensure
the badge always renders.
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix trailing whitespace causing Prettier CI failure.
Line 15 contains extra trailing whitespace after the closing brace. Please remove it (or run Prettier on the file) so Prettier --check passes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/shared/NavBarLinksGrouper.tsx` at line 15, Remove the
trailing whitespace after the closing brace in NavBarLinksGrouper.tsx (the extra
space following the '}' in the component/file) so Prettier passes; simply delete
the trailing spaces or run Prettier/format on the file (e.g., target the
NavBarLinksGrouper component/definition) and commit the formatted file.
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
1 participant
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.
Why
What
Satisfies
HK-215
Summary by CodeRabbit
Release Notes
New Features
Style