restyled components used react-pdf-tailwind and react-pdf-table - #125
restyled components used react-pdf-tailwind and react-pdf-table#125atungcod wants to merge 16 commits into
Conversation
|
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:
📝 WalkthroughWalkthroughAdds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/features/scheduling/exporting/CamperPreferencesSheet.tsx (1)
2-45: Remove unusedStyleSheetimport and dead StyleSheet block (fixes lint).
StyleSheetis no longer used since the old styles are commented out, which causes the eslintno-unused-varserror and breaks the pipeline. You can safely delete both theStyleSheetimport and the commentedStyleSheet.createblock.-import { Document, Page, Text, View, StyleSheet } from "@react-pdf/renderer"; +import { Document, Page, Text, View } from "@react-pdf/renderer"; - -/*const styles = StyleSheet.create({ - page: { padding: 40, fontSize: 12, fontFamily: "Helvetica" }, - ... - activityDesc: { fontSize: 11 }, -}); */src/features/scheduling/exporting/EmployeeGrid.tsx (1)
59-62: Unsafe cast ofgetFreeplayAssignmentIdresult may throw at runtimeThe function
getFreeplayAssignmentIdreturnsnumber[] | number | string | null, but at line 61 it is blindly cast tonumber[]without a type guard. The code at lines 60–61:const fpBuddyIds = getFreeplayAssignmentId(freeplay, employee.id); const fpBuddies = fpBuddyIds ? (fpBuddyIds as number[]).map(id => campers.find(c => c.id === id)) : [];will throw at runtime if
getFreeplayAssignmentIdreturns astring(post ID) or singlenumber(staff ID), since.map()does not exist on those types.Guard on the runtime type instead of casting:
- const fpBuddyIds = getFreeplayAssignmentId(freeplay, employee.id); - const fpBuddies = fpBuddyIds ? (fpBuddyIds as number[]).map(id => campers.find(c => c.id === id)) : []; + const fpAssignment = getFreeplayAssignmentId(freeplay, employee.id); + const fpBuddyIds = Array.isArray(fpAssignment) ? fpAssignment : []; + const fpBuddies = fpBuddyIds.map(id => campers.find(c => c.id === id));If posts (string IDs) or other cases need different handling, add explicit branches for those cases.
🧹 Nitpick comments (12)
src/features/scheduling/exporting/CamperPreferencesSheet.tsx (1)
10-18: Tailwind-based PDF styling and section props look consistent; minor cleanup only.The
createTwconfig and usage on<Page>, header rows, and activity text all look good and keep Helvetica as the base font. The newsectionType,sectionName, and conditionalageGroupprops are wired correctly into the header and BUNDLE-only activity filtering. The inner<View style={tw("")}>can be dropped since it carries no classes, but that’s purely cosmetic.Also applies to: 62-90
src/features/scheduling/ProgramAreaGrid.tsx (1)
59-72: Confirm that showing only the first activity per area/block is intended.
renderActivityTextintentionally picksactivities[0]and ignores any additional activities in that area/block. If prior behavior showed all activities, this is a functional change that may hide data in the PDF; if that’s desired for readability, all good, otherwise consider joining names or adding a multi-line rendering.src/features/scheduling 2/generation/FreeplayScheduler.ts (1)
3-44: Fluent builder surface forFreeplaySchedulerlooks consistent; methods are ready for future logic.The default state and
with*chainable methods give a clear, composable API aligned with the other schedulers introduced in the PR. The stubbedwithOtherFreeplays,assignPosts, andassignCampersare fine placeholders for now; just ensure callers don’t assume they perform real assignments until the logic is filled in.src/features/scheduling 2/generation/BunkJamboreeScheduler.ts (1)
3-30:BunkJamboreeSchedulerAPI matches the other schedulers and is ready for implementation.The defaults and fluent
with*/forBlocksmethods provide a clean configuration surface. The threeassign*methods are currently no-ops, which is fine as long as they’re treated as TODOs and not relied on for real allocations yet.src/features/scheduling 2/generation/schedulingUtils.ts (1)
10-47: Type guards and ID helpers look solid; consider aliasinggetFreeplayAssignmentId’s union.The bundle/jamboree, assignment-shape, and attendee-ID helpers are idiomatic and match the shapes in
sessionTypes.getFreeplayAssignmentId’s logic for posts vs buddy mappings is clear; you may want to export a named union type (e.g.,type FreeplayAssignmentRef = string | number | number[] | null) so callers can share and re-use the return type instead of repeating the literal union.src/features/scheduling 2/generation/BundleScheduler.ts (1)
17-56: Builder API is clear, but allassign*methods are currently no‑opsThe fluent setters/read‑only configuration are fine, but every
assign*method (assignProgramCounselors,assignPeriodsOff,assignOcpChats,assignSwimBlocks,assignCampers,assignStaff,assignAdmins) just returnsthiswithout touchingscheduleor any of the collections. That’s OK as a scaffold, but if these are already being wired into the scheduling flow, callers may think work is happening when it is not.Consider either:
- Implementing the first slice of logic, or
- Making it explicit (via TODOs/JSDoc) that these are placeholders and must not be relied on yet.
src/features/scheduling 2/exporting/CamperGrid.tsx (3)
16-141: Trim or reuse unused style definitionsSeveral style rules (
page,cell,bold,table,headerCell,bunkCell,dataCell,twoColumnPage,leftColumn,rightColumn) are currently unused. Consider removing them or wiring them into the layout to keep this component focused and easier to maintain.
155-163: Avoid mutatingcampersprops and recomputing blocks per row
- Line 155:
campers.sort(...)mutates props in place; safer to sort a shallow copy:const sortedCampers = [...campers].sort(...)and iterate that.- Line 183:
const blocksArray = Object.values(schedule.blocks);is recomputed for every camper; compute once outside thecampers.mapto avoid unnecessary work.- campers.sort((a, b) => { + const sortedCampers = [...campers].sort((a, b) => { @@ - {campers.map((camper) => { - const blocksArray = Object.values(schedule.blocks); + {sortedCampers.map((camper) => { + const blocksArray = Object.values(schedule.blocks);Also applies to: 182-219
173-177: Consider stabilizing block column order
Object.keys(schedule.blocks)relies on object insertion order. If blocks can be created/edited dynamically, you may want a stable sort (e.g., numeric or lexicographic) so column order doesn’t jump unexpectedly in exported PDFs.- {Object.keys(schedule.blocks).map((blockId) => ( + {Object.keys(schedule.blocks).sort().map((blockId) => (src/features/scheduling 2/generation/SessionScheduler.ts (1)
4-24: Duplicate SessionScheduler implementation in two locationsThis
SessionSchedulermirrors the existing one undersrc/features/scheduling/generation/SessionScheduler.ts. Maintaining two copies risks divergence.Consider exporting a single shared implementation (e.g., move to a common module and re-export) rather than duplicating the class.
src/features/scheduling 2/exporting/BlockRatiosGrid.tsx (1)
128-141: Optional: Avoid recomputingisIndividualAssignmentsand add basic safety on attendee lookupWithin each row you call
isIndividualAssignments(activity.assignments)multiple times and assumeattendeesById[id]always exists. For robustness and small perf win:- const camperOrBunkIds = isIndividualAssignments(activity.assignments) ? activity.assignments.camperIds : activity.assignments.bunkNums; - const employeeIds = isIndividualAssignments(activity.assignments) ? [...activity.assignments.staffIds, ...activity.assignments.adminIds] : activity.assignments.adminIds; + const individual = isIndividualAssignments(activity.assignments); + const camperOrBunkIds = individual + ? activity.assignments.camperIds + : activity.assignments.bunkNums; + const employeeIds = individual + ? [...activity.assignments.staffIds, ...activity.assignments.adminIds] + : activity.assignments.adminIds; @@ - {camperOrBunkIds.map(camperOrBunkId => <Text key={camperOrBunkId}>{isIndividualAssignments(activity.assignments) ? getFullName(attendeesById[camperOrBunkId]) : `Bunk ${camperOrBunkId}`}</Text>)} + {camperOrBunkIds.map(camperOrBunkId => ( + <Text key={camperOrBunkId}> + {individual + ? (attendeesById[camperOrBunkId] ? getFullName(attendeesById[camperOrBunkId]) : "") + : `Bunk ${camperOrBunkId}`} + </Text> + ))}src/features/scheduling 2/generation/NonBunkJamboreeScheduler.ts (1)
9-41: Clarify stubbed scheduling methods or add TODOs
assignPeriodsOff,assignCampers, andassignCounselorsare no-ops returningthis. That’s fine as scaffolding, but it’s easy to forget they’re unimplemented.Add brief comments/TODOs indicating intended future behavior, or throw if called before implementation, so misuse is more obvious during development:
- assignPeriodsOff(): NonBunkJamboreeScheduler { return this; } + // TODO: Implement NON-BUNK-JAMBO staff/admin period-off assignment. + assignPeriodsOff(): NonBunkJamboreeScheduler { return this; } @@ - assignCampers(): NonBunkJamboreeScheduler { return this; } + // TODO: Implement camper assignment for NON-BUNK-JAMBO blocks. + assignCampers(): NonBunkJamboreeScheduler { return this; } @@ - assignCounselors(): NonBunkJamboreeScheduler { return this; } + // TODO: Implement counselor assignment for NON-BUNK-JAMBO blocks. + assignCounselors(): NonBunkJamboreeScheduler { return this; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (18)
package.json(2 hunks)src/components/SmallDirectoryBlock.tsx(1 hunks)src/components/demo/page.tsx(1 hunks)src/features/scheduling 2/exporting/BlockRatiosGrid.tsx(1 hunks)src/features/scheduling 2/exporting/CamperGrid.tsx(1 hunks)src/features/scheduling 2/exporting/CamperPreferencesSheet.tsx(1 hunks)src/features/scheduling 2/exporting/EmployeeGrid.tsx(1 hunks)src/features/scheduling 2/generation/BundleScheduler.ts(1 hunks)src/features/scheduling 2/generation/BunkJamboreeScheduler.ts(1 hunks)src/features/scheduling 2/generation/FreeplayScheduler.ts(1 hunks)src/features/scheduling 2/generation/NonBunkJamboreeScheduler.ts(1 hunks)src/features/scheduling 2/generation/SessionScheduler.ts(1 hunks)src/features/scheduling 2/generation/schedulingUtils.ts(1 hunks)src/features/scheduling/ProgramAreaGrid.tsx(2 hunks)src/features/scheduling/exporting/BlockRatiosGrid.tsx(5 hunks)src/features/scheduling/exporting/CamperGrid.tsx(3 hunks)src/features/scheduling/exporting/CamperPreferencesSheet.tsx(4 hunks)src/features/scheduling/exporting/EmployeeGrid.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (14)
src/features/scheduling 2/exporting/EmployeeGrid.tsx (3)
src/types/sessionTypes.ts (5)
SchedulingSectionType(52-52)SectionScheduleID(76-76)Freeplay(132-135)CamperAttendeeID(26-26)AdminAttendeeID(41-41)src/features/scheduling 2/generation/schedulingUtils.ts (3)
getFreeplayAssignmentId(18-33)isIndividualAssignments(35-37)isBundleActivity(10-12)src/utils/personUtils.ts (1)
getFullName(3-5)
src/components/SmallDirectoryBlock.tsx (1)
src/types/personTypes.ts (4)
CamperID(27-27)StaffID(50-50)AdminID(52-52)UserRole(18-18)
src/features/scheduling 2/exporting/CamperPreferencesSheet.tsx (2)
src/types/sessionTypes.ts (4)
SchedulingSectionType(52-52)SectionSchedule(72-75)AgeGroup(144-144)BundleActivity(105-108)src/features/scheduling 2/generation/schedulingUtils.ts (1)
isBundleActivity(10-12)
src/features/scheduling 2/exporting/BlockRatiosGrid.tsx (3)
src/types/sessionTypes.ts (5)
SchedulingSectionType(52-52)SectionSchedule(72-75)CamperAttendeeID(26-26)StaffAttendeeID(35-35)AdminAttendeeID(41-41)src/features/scheduling 2/generation/schedulingUtils.ts (3)
getAttendeesById(43-47)isBundleActivity(10-12)isIndividualAssignments(35-37)src/utils/personUtils.ts (1)
getFullName(3-5)
src/features/scheduling 2/generation/BundleScheduler.ts (2)
src/types/sessionTypes.ts (5)
SectionSchedule(72-75)CamperAttendeeID(26-26)StaffAttendeeID(35-35)AdminAttendeeID(41-41)SectionPreferences(150-152)src/features/scheduling/generation/BundleScheduler.ts (1)
BundleScheduler(3-57)
src/features/scheduling/exporting/CamperPreferencesSheet.tsx (1)
src/features/scheduling/generation/schedulingUtils.ts (1)
isBundleActivity(10-12)
src/features/scheduling/exporting/BlockRatiosGrid.tsx (1)
src/features/scheduling/generation/schedulingUtils.ts (2)
isBundleActivity(10-12)isIndividualAssignments(35-37)
src/features/scheduling/exporting/CamperGrid.tsx (1)
src/features/scheduling/generation/schedulingUtils.ts (3)
getFreeplayAssignmentId(18-33)isIndividualAssignments(35-37)isBundleActivity(10-12)
src/features/scheduling 2/generation/SessionScheduler.ts (2)
src/types/sessionTypes.ts (5)
SessionID(11-11)SectionID(55-55)StaffAttendeeID(35-35)AdminAttendeeID(41-41)NightShiftID(49-49)src/features/scheduling/generation/SessionScheduler.ts (1)
SessionScheduler(4-25)
src/features/scheduling 2/generation/BunkJamboreeScheduler.ts (1)
src/types/sessionTypes.ts (4)
SectionSchedule(72-75)BunkID(130-130)AdminAttendeeID(41-41)SectionPreferences(150-152)
src/features/scheduling 2/generation/NonBunkJamboreeScheduler.ts (1)
src/types/sessionTypes.ts (5)
SectionSchedule(72-75)CamperAttendeeID(26-26)StaffAttendeeID(35-35)AdminAttendeeID(41-41)SectionPreferences(150-152)
src/features/scheduling/ProgramAreaGrid.tsx (1)
src/types/sessionTypes.ts (2)
BundleActivity(105-108)ProgramAreaID(98-98)
src/features/scheduling 2/exporting/CamperGrid.tsx (3)
src/types/sessionTypes.ts (3)
SchedulingSectionType(52-52)SectionScheduleID(76-76)Freeplay(132-135)src/utils/personUtils.ts (1)
getFullName(3-5)src/features/scheduling 2/generation/schedulingUtils.ts (3)
getFreeplayAssignmentId(18-33)isIndividualAssignments(35-37)isBundleActivity(10-12)
src/features/scheduling 2/generation/schedulingUtils.ts (1)
src/types/sessionTypes.ts (6)
AttendeeID(14-14)BundleActivity(105-108)JamboreeActivity(100-103)Freeplay(132-135)IndividualAssignments(114-118)BunkAssignments(120-123)
🪛 GitHub Actions: Lint & Build on PR
src/components/SmallDirectoryBlock.tsx
[error] 23-23: Step 'npm run lint' failed. ESLint: 'CamperAttendee' is defined but never used. (no-unused-vars)
src/features/scheduling/exporting/CamperPreferencesSheet.tsx
[error] 2-2: Step 'npm run lint' failed. ESLint: 'StyleSheet' is defined but never used. (no-unused-vars)
src/features/scheduling/exporting/BlockRatiosGrid.tsx
[error] 2-2: Step 'npm run lint' failed. ESLint: 'StyleSheet' is defined but never used. (no-unused-vars)
src/features/scheduling/exporting/EmployeeGrid.tsx
[error] 9-9: Step 'npm run lint' failed. ESLint: 'View' is defined but never used. (no-unused-vars)
src/features/scheduling/exporting/CamperGrid.tsx
[error] 9-9: Step 'npm run lint' failed. ESLint: 'View' is defined but never used. (no-unused-vars)
[error] 170-170: Step 'npm run lint' failed. ESLint: 'staff' is assigned a value but never used. (no-unused-vars)
src/features/scheduling/ProgramAreaGrid.tsx
[error] 9-9: Step 'npm run lint' failed. ESLint: 'TH' is defined but never used. (no-unused-vars)
🔇 Additional comments (9)
src/components/demo/page.tsx (1)
1-11: LGTM!The demo page component is clean and straightforward. The use of Mantine's Container for layout is appropriate.
src/components/SmallDirectoryBlock.tsx (4)
25-34: LGTM!Type definitions are clear and well-structured. The optional
bunkproperty onCamperWithBunkappropriately extendsCamperID.
47-60: LGTM!The filtering logic correctly handles both role-based and name-based filtering with case-insensitive search. The slicing logic appropriately respects the
showAllflag.
121-125: LGTM!The conditional rendering of bunk numbers uses proper type guards (
person.role === 'CAMPER' && 'bunk' in person) to safely access the optionalbunkproperty.
141-158: LGTM!The view more/less button logic correctly shows the button only when there are more items than
initialVisibleCount, and the icon appropriately changes based on theshowAllstate.package.json (1)
16-16: Dependencies verified—no issues found.All three newly added packages exist at the specified versions, are currently at their latest releases, and have no known security vulnerabilities. The dependency versions are valid and safe to use.
src/features/scheduling 2/generation/schedulingUtils.ts (1)
3-8: Double-check conflict rule for non-camper roles (requires both nono and yesyes).For campers you flag a conflict if any
nonoListid appears, but for staff/admins you currently require thatotherAttendeeIdscontain at least onenonoListid and at least oneyesyesListid. That’s a pretty specific condition; if the intent was “any no-no present is a conflict” for all roles, the non-camper branch should likely only checknonoList.return attendee.nonoList.some((id) => otherAttendeeIds.includes(id));src/features/scheduling 2/exporting/CamperPreferencesSheet.tsx (1)
38-95: CamperPreferencesSheet generic and filtering logic look solidThe generic props, conditional
ageGrouptyping, and thesectionType === "BUNDLE"filter all align with the session types andisBundleActivityguard. Rendering of titles, blocks, and descriptions is clear and matches the data model.src/features/scheduling 2/exporting/BlockRatiosGrid.tsx (1)
14-72: No changes required — CSS shorthand borders are valid in@react-pdf/renderer@react-pdf/renderer supports both the CSS-like shorthand "border" (e.g. "1pt solid black") and the explicit properties borderWidth/borderStyle/borderColor. The code's use of
border: '1pt solid black'and individual border properties (e.g.,borderTop: '1pt solid black') is correct and does not need refactoring.Likely an incorrect or invalid review comment.
| const handleViewMore = () => { | ||
| if (showAll) { | ||
| setShowAll(false); | ||
| setVisibleCount(initialVisibleCount); | ||
| } else { | ||
| setVisibleCount((prev) => Math.min(prev + loadMoreCount, filteredCount)); | ||
| if (visibleCount + loadMoreCount >= filteredCount) { | ||
| setShowAll(true); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
Fix stale state reference in view more handler.
Line 68 reads visibleCount after it was updated in line 67, but state updates in React are asynchronous. The condition evaluates against the stale (old) value, causing the "Show all" toggle to activate one step late.
Apply this diff to use the computed new value:
const handleViewMore = () => {
if (showAll) {
setShowAll(false);
setVisibleCount(initialVisibleCount);
} else {
- setVisibleCount((prev) => Math.min(prev + loadMoreCount, filteredCount));
- if (visibleCount + loadMoreCount >= filteredCount) {
+ const newCount = Math.min(visibleCount + loadMoreCount, filteredCount);
+ setVisibleCount(newCount);
+ if (newCount >= filteredCount) {
setShowAll(true);
}
}
};🤖 Prompt for AI Agents
In src/components/SmallDirectoryBlock.tsx around lines 62 to 72, the
handleViewMore reads visibleCount immediately after calling setVisibleCount so
it uses a stale value; compute the new visible count first and use that to
update state and decide whether to setShowAll. Concretely: when expanding,
compute newCount = Math.min(visibleCount + loadMoreCount, filteredCount) (or use
the functional updater form to derive newCount), call setVisibleCount(newCount),
then call setShowAll(newCount >= filteredCount); keep the collapse branch
unchanged.
| const blockIds = Object.keys(schedule.blocks).sort() | ||
| const blocks = blockIds.map(blockId => schedule.blocks[blockId]); | ||
|
|
||
| const attendeesById = getAttendeesById([...campers, ...staff, ...admins]); | ||
|
|
||
| // Find the maximum number of activities across all blocks | ||
| const maxActivities = Math.max(...blocks.map(block => block.activities.length)); | ||
|
|
There was a problem hiding this comment.
Handle empty schedule.blocks to avoid Math.max(...[]) crash
If schedule.blocks is empty:
- Line 81:
Math.max(...blocks.map(...))becomesMath.max()with no arguments, yielding-Infinity. - Line 95:
Array.from({ length: maxActivities })then throws due to invalid (negative, non-finite) length.
Add a guard to early-return or skip the activity rows when there are no blocks:
- const maxActivities = Math.max(...blocks.map(block => block.activities.length));
+ const maxActivities = blocks.length
+ ? Math.max(...blocks.map(block => block.activities.length))
+ : 0;
@@
- {/* Render activities row by row across all blocks */}
- {Array.from({ length: maxActivities }).map((_, activityIndex) => {
+ {/* Render activities row by row across all blocks */}
+ {maxActivities > 0 && Array.from({ length: maxActivities }).map((_, activityIndex) => {Also applies to: 95-101
🤖 Prompt for AI Agents
In src/features/scheduling 2/exporting/BlockRatiosGrid.tsx around lines 75-82
and 95-101, the code calls Math.max over blocks.map(...) which crashes when
schedule.blocks is empty; add a guard to handle empty blocks by either
early-returning a sensible JSX (null or a “no blocks” placeholder) or by
defaulting maxActivities to 0 (e.g., compute maxActivities only if blocks.length
> 0, otherwise 0) and skip rendering the activity rows when maxActivities === 0
so Array.from({ length: maxActivities }) is never called with an invalid length.
| {employees.map((employee) => { | ||
| const fpBuddyIds = getFreeplayAssignmentId(freeplay, employee.id); | ||
| const fpBuddies = fpBuddyIds ? (fpBuddyIds as number[]).map(id => campers.find(c => c.id === id)) : []; | ||
|
|
||
| let apoText: string = '-'; | ||
| for (const period of Object.keys(schedule.alternatePeriodsOff)) { | ||
| if (schedule.alternatePeriodsOff[period].includes(employee.id)) { | ||
| apoText = period; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| return ( | ||
| <View key={employee.id} style={styles.row}> | ||
| {/* Name column - Use dataCell style */} | ||
| <View style={styles.dataCell}> | ||
| <Text> | ||
| {employee.name.firstName} {employee.name.lastName[0]}. | ||
| </Text> | ||
| </View> | ||
|
|
||
| {/* Block assignments - Use dataCell style */} | ||
| {Object.entries(schedule.blocks).map(([blockId, block]) => { | ||
| let activityText; | ||
| if (block.periodsOff.includes(employee.id)) { | ||
| activityText = "OFF"; | ||
| } else if (employee.role === "STAFF") { | ||
| const activity = block.activities.find((act) => | ||
| isIndividualAssignments(act.assignments) | ||
| ? act.assignments.staffIds.includes(employee.id) | ||
| : act.assignments.bunkNums.includes(employee.bunk) | ||
| ); | ||
| activityText = activity | ||
| ? isBundleActivity(activity) | ||
| ? activity.programArea.id | ||
| : activity.name | ||
| : "-"; | ||
| } else { | ||
| const activity = block.activities.find((act) => | ||
| act.assignments.adminIds.includes(employee.id) | ||
| ); | ||
| activityText = activity | ||
| ? isBundleActivity(activity) | ||
| ? activity.programArea.id | ||
| : activity.name | ||
| : "-"; | ||
| } | ||
|
|
||
| return ( | ||
| <View key={blockId} style={styles.cell}> | ||
| <Text>{activityText}</Text> | ||
| </View> | ||
| ); | ||
| })} | ||
|
|
||
| <View style={styles.dataCell}> | ||
| <Text> | ||
| {apoText} | ||
| </Text> | ||
| </View> | ||
|
|
||
| {/* Freeplay assignment - Use dataCell style */} | ||
| <View style={styles.dataCell}> | ||
| <Text> | ||
| {fpBuddies.map(fpBuddy => fpBuddy ? getFullName(fpBuddy) : "").join(", ")} | ||
| </Text> | ||
| </View> |
There was a problem hiding this comment.
Fix getFreeplayAssignmentId handling to avoid runtime crash and support all return types
getFreeplayAssignmentId can return number[] | number | string | null, but the code assumes it’s always a number[]:
const fpBuddyIds = getFreeplayAssignmentId(freeplay, employee.id);
const fpBuddies = fpBuddyIds ? (fpBuddyIds as number[]).map(...) : [];For employees assigned via freeplay.posts, this will be a string postId, so calling .map on it will throw at runtime. Similarly, a bare number would also explode.
Refactor to narrow on the actual type before mapping, e.g.:
const assignment = getFreeplayAssignmentId(freeplay, employee.id);
let fpBuddies: CamperAttendeeID[] = [];
let fpPostText = "";
if (Array.isArray(assignment)) {
fpBuddies = assignment
.map(id => campers.find(c => c.id === id))
.filter((c): c is CamperAttendeeID => !!c);
} else if (typeof assignment === "string") {
fpPostText = assignment; // or whatever label you want for admin/staff posts
}
// ...
<Text>
{fpBuddies.length
? fpBuddies.map(b => getFullName(b)).join(", ")
: fpPostText || "-"}
</Text>This preserves the buddy-name behavior while safely handling string/number/null cases.
🤖 Prompt for AI Agents
In src/features/scheduling 2/exporting/EmployeeGrid.tsx around lines 171-237,
the code assumes getFreeplayAssignmentId always returns number[] and calls .map,
which crashes for string/number/null returns; change to narrow by type: if
Array.isArray(result) map each id to campers.find(...) and filter out undefined,
if typeof result === "number" treat as a single id and find that camper, if
typeof result === "string" treat as a post label (keep as text), and if
null/undefined show "-" ; then render either the joined buddy full-names (if
any) or the post text or "-" accordingly.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/features/scheduling/exporting/BlockRatiosGrid.tsx (1)
92-99: GuardMath.maxagainst emptyschedule.blocksto prevent runtime error.When
blocksis empty,Math.max(...[])returns-Infinity, causingArray.from({ length: -Infinity })at line 106 to throw a TypeError. Apply the suggested guard:const maxActivities = blocks.length ? Math.max(...blocks.map((block) => block.activities.length)) : 0;Additionally, three Tailwind class strings are missing spaces:
- Line 138:
justify-centerp-[5px]→justify-center p-[5px]- Line 139:
justify-centerborder→justify-center border- Line 159:
justify-centertext-[6px]→justify-center text-[6px]Also add defensive null checks at lines 174 and 187 before dereferencing
attendeeandemployee, as they may be undefined from theattendeesByIdlookup.src/features/scheduling/exporting/CamperPreferencesSheet.tsx (1)
20-45: Remove commented-out code.The old StyleSheet-based styles are no longer needed after the migration to react-pdf-tailwind. Version control preserves the history if you need to reference it later.
Apply this diff to remove the commented code:
-const tw = createTw({ - theme: { - fontFamily: { - sans: ["Helvetica"], - }, - }, -}); - -/*const styles = StyleSheet.create({ - page: { padding: 40, fontSize: 12, fontFamily: "Helvetica" }, - title: { - fontSize: 18, - textAlign: "center", - marginBottom: 16, - fontWeight: "bold", - }, - nameRow: { - flexDirection: "row", - justifyContent: "space-between", - marginBottom: 10, - }, - block: { marginTop: 12 }, - blockTitle: { fontSize: 14, fontWeight: "bold", marginBottom: 4 }, - activity: { - marginLeft: 10, - marginBottom: 6, - display: "flex", - flexDirection: "row", - justifyContent: "space-between", - }, - activityInfo: {}, - activityDesc: { fontSize: 11 }, -}); */ +const tw = createTw({ + theme: { + fontFamily: { + sans: ["Helvetica"], + }, + }, +});
♻️ Duplicate comments (3)
src/features/scheduling/exporting/BlockRatiosGrid.tsx (1)
103-139: Normalize table header structure and fix Tailwind typos.
Header: use a
TRwithTHcells rather thanTHwrappingTRs.
Current markup:<TH style={tw("text-center items-center justify-center flex-row min-h-[20px]")}> {blockIds.map(... => ( <TR ...>...</TR> ))} </TH>is unusual for table semantics and for
@ag-media/react-pdf-table. Prefer a single header row:<TR style={tw("flex-row min-h-[20px]")}> {blockIds.map((blockId) => ( <TH key={blockId} style={tw( "flex-1 border border-black bg-gray-light p-[5px] text-center items-center justify-center font-bold text-[8px]" )} > <Text>BLOCK {blockId}</Text> </TH> ))} </TR>Fix Tailwind spacing typos so utilities apply correctly.
Activity header empty cell:
- if (!activity) return <TD key={
header-${blockId}-${activityIndex}} style={[tw("flex-1 border border-black text-center items-center justify-centerp-[5px] text-[6px]"), { backgroundColor: rowBgColor, textAlign: 'center' }]} />
- if (!activity) return <TD key={
header-${blockId}-${activityIndex}} style={[tw("flex-1 border border-black text-center items-center justify-center p-[5px] text-[6px]"), { backgroundColor: rowBgColor, textAlign: 'center' }]} />- Activity header populated cell: ```diff
- style={[tw("flex-1 text-center items-center justify-centerborder border-black p-[5px] text-[6px]"), { backgroundColor: rowBgColor, textAlign: 'center' }]}
- style={[tw("flex-1 text-center items-center justify-center border border-black p-[5px] text-[6px]"), { backgroundColor: rowBgColor, textAlign: 'center' }]}
- Empty data row staff/admin cell: ```diff
- style={[tw("flex-1 border border-black p-[3px] text-center items-center justify-centertext-[6px]"), { backgroundColor: rowBgColor, textAlign: 'center' }]}
- style={[tw("flex-1 border border-black p-[3px] text-center items-center justify-center text-[6px]"), { backgroundColor: rowBgColor, textAlign: 'center' }]}
These fixes restore the intended padding, borders, and centering.
src/features/scheduling/ProgramAreaGrid.tsx (1)
119-126: Fix Tailwind class typo in non-empty area cells.The non-empty branch uses
min-h-[13px]text-center(missing space), so Tailwind treats it as an invalid utility and doesn’t applytext-center.- ? tw("bg-white p-[2px] min-h-[13px] text-center border border-black w-[80px]") - : tw("bg-white p-[2px] min-h-[13px]text-center border border-black w-[80px]") + ? tw("bg-white p-[2px] min-h-[13px] text-center border border-black w-[80px]") + : tw("bg-white p-[2px] min-h-[13px] text-center border border-black w-[80px]")This makes empty and non-empty cells consistently centered.
src/features/scheduling/exporting/CamperGrid.tsx (1)
192-207: Label the bunk column header for clarity.The first header cell (for the bunk column) is empty, so the bunk numbers in the first data column have no label. Adding a simple “BUNK” label improves readability:
- <TR style={tw("bg-black")}> - <TH style={[tw("text-center text-[7px] font-bold text-white p-[1px]"), { width: "30px" }]}> - </TH> + <TR style={tw("bg-black")}> + <TH style={[tw("text-center text-[7px] font-bold text-white p-[1px]"), { width: "30px" }]}> + <Text>BUNK</Text> + </TH>
🧹 Nitpick comments (5)
src/features/scheduling/ProgramAreaGrid.tsx (1)
59-71: Confirm intent to show only the first activity per block.
renderActivityTextnow renders onlyactivities[0]. If blocks can have multiple activities per area, the rest are silently dropped. If that’s intentional (e.g., you only care about the primary activity), all good; otherwise, consider mapping and stacking all activities in the cell.src/features/scheduling/exporting/BlockRatiosGrid.tsx (1)
154-199: Defensively handle missing attendees inattendeesById.Both camper and staff/admin rendering assume
attendeesById[id]always exists:const attendee = attendeesById[camperOrBunkId]; ... const employee = attendeesById[employeeId];If schedule data or attendees arrays ever get out of sync, this will throw on
attendee.name/employee.name. A small guard keeps the PDF resilient:const attendee = attendeesById[camperOrBunkId]; if (!attendee) { return null; // or a fallback <Text key={...}>Unknown</Text> }and similarly for
employee. That way, bad data results in a blank or placeholder instead of a crash.src/features/scheduling/exporting/CamperGrid.tsx (1)
172-180: Avoid mutating thecampersprop when sorting.
campers.sort(...)mutates the array passed in via props, which is generally discouraged in React. Prefer sorting a copy:- campers.sort((a, b) => { + const sortedCampers = [...campers].sort((a, b) => { @@ - }); + });and then iterate over
sortedCampersin your JSX. This keeps props immutable and avoids surprises ifcampersis reused elsewhere.src/features/scheduling/exporting/CamperPreferencesSheet.tsx (2)
62-62: Consider using a consistent styling approach.The
Pagecomponent mixestw()utility styling with an inline style object. SinceHelveticais already configured as thesansfont in the theme (line 15), consider usingtw("font-sans")instead of the inlinefontFamilyproperty for consistency.If react-pdf-tailwind supports
font-sansat the Page level, apply this diff:- <Page size="A4" style={[tw("p-[40px] text-[12px]"), { fontFamily: "Helvetica" }]}> + <Page size="A4" style={tw("p-[40px] text-[12px] font-sans")}>
84-84: Remove unnecessary empty style.The
Viewcomponent on line 84 has an empty style stringtw("")which serves no purpose and can be removed.Apply this diff:
- <View style={tw("")}> + <View>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/components/SmallDirectoryBlock.tsx(1 hunks)src/features/scheduling/ProgramAreaGrid.tsx(2 hunks)src/features/scheduling/exporting/BlockRatiosGrid.tsx(5 hunks)src/features/scheduling/exporting/CamperGrid.tsx(4 hunks)src/features/scheduling/exporting/CamperPreferencesSheet.tsx(4 hunks)src/features/scheduling/exporting/EmployeeGrid.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/SmallDirectoryBlock.tsx
- src/features/scheduling/exporting/EmployeeGrid.tsx
🧰 Additional context used
🧬 Code graph analysis (4)
src/features/scheduling/ProgramAreaGrid.tsx (1)
src/types/sessionTypes.ts (2)
BundleActivity(105-108)ProgramAreaID(98-98)
src/features/scheduling/exporting/CamperPreferencesSheet.tsx (1)
src/features/scheduling/generation/schedulingUtils.ts (1)
isBundleActivity(10-12)
src/features/scheduling/exporting/BlockRatiosGrid.tsx (1)
src/features/scheduling/generation/schedulingUtils.ts (2)
isBundleActivity(10-12)isIndividualAssignments(35-37)
src/features/scheduling/exporting/CamperGrid.tsx (1)
src/features/scheduling/generation/schedulingUtils.ts (3)
getFreeplayAssignmentId(18-33)isIndividualAssignments(35-37)isBundleActivity(10-12)
🔇 Additional comments (1)
src/features/scheduling/exporting/CamperPreferencesSheet.tsx (1)
10-18: No compatibility issues found — all Tailwind classes are literal strings and use basic utilities supported by react-pdf-tailwind.All classes in the component (
p-[40px],text-[12px],flex-row,mb-[10px], etc.) are passed as literal strings totw(), so Tailwind will generate them at build time. The utilities used are basic CSS properties (padding, margin, text sizing, flex layout, font-weight) that map directly to react-pdf's supported styling—no browser-only features or unsupported modifiers. The explicitflex-rowusage is appropriate given that react-pdf (via Yoga) defaults flex-direction to column.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/features/scheduling/exporting/EmployeeGrid.tsx (1)
45-70: Remove<TR>wrapper from header—it violates@ag-media/react-pdf-tableAPI and causes misalignmentPer the library's documented structure and the previous review comment,
<TH>must directly contain<TD>elements without a<TR>wrapper. The current nesting (<TH><TR><TD>) causes the header row to misalign with data rows and defeats the stated PR objective.Apply this diff to remove the
<TR>wrapper:<TH style={tw("mb-[8px] border border-black")}> - <TR style={tw("flex-row bg-black")}> <TD> <Text style={tw("flex-1 text-center text-[8px] font-bold text-white border border-black p-[1px] bg-black justify-center items-center")}> NAME </Text> </TD> {Object.keys(schedule.blocks).map((blockId) => ( <TD key={blockId}> <Text style={tw("flex-1 text-center text-[8px] font-bold text-white border border-black p-[1px] bg-black justify-center items-center")}> {blockId} </Text> </TD> ))} <TD> <Text style={tw("flex-1 text-center text-[8px] font-bold text-white border border-black p-[1px] bg-black justify-center items-center")}> APO </Text> </TD> <TD> <Text style={tw("flex-1 text-center text-[8px] font-bold text-white border border-black p-[1px] bg-black justify-center items-center")}> AM/PM FP </Text> </TD> - </TR> </TH>Based on the previous review comment and library documentation.
🧹 Nitpick comments (2)
src/features/scheduling/ProgramAreaGrid.tsx (2)
85-85: Remove or clarify the alignment comment.The comment suggests a workaround was attempted. If the alignment issue has been resolved, remove this comment. Otherwise, clarify the current status or open a tracking issue.
142-142: Remove redundant inline fontFamily style.The
fontFamily: "Helvetica"inline style is redundant since Helvetica is already configured as the default font in the theme (lines 13-14). The tw() utility should apply it automatically.- <Page size="A4" style={[tw("p-[20px] text-[9px]"), { fontFamily: "Helvetica" }]}> + <Page size="A4" style={tw("p-[20px] text-[9px]")}>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/features/scheduling/ProgramAreaGrid.tsx(2 hunks)src/features/scheduling/exporting/EmployeeGrid.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/features/scheduling/ProgramAreaGrid.tsx (1)
src/types/sessionTypes.ts (2)
BundleActivity(105-108)ProgramAreaID(98-98)
src/features/scheduling/exporting/EmployeeGrid.tsx (1)
src/utils/personUtils.ts (1)
getFullName(3-5)
🔇 Additional comments (6)
src/features/scheduling/exporting/EmployeeGrid.tsx (3)
9-9: LGTM: Unused import removedThe
Viewimport has been correctly removed, addressing the previous ESLint error.
85-139: LGTM: Data rows correctly structured and styledThe data row structure (
<TR>directly containing<TD>elements) follows the@ag-media/react-pdf-tableconventions. The centering classes (text-center,justify-center,items-center) properly address the PR objective of centering text within table cells. The assignment logic for activities, APO, and freeplay buddies is sound.
42-44: LGTM: Page and table setup is appropriateThe Page and Table configuration with Tailwind-based styling and dynamic title generation based on employee role is well-structured.
src/features/scheduling/ProgramAreaGrid.tsx (3)
11-26: LGTM!The Tailwind configuration is well-structured with appropriate font family settings for PDF rendering and custom color definitions.
8-9: Imports are correctly configured and library versions are compatible.The react-pdf-tailwind v3.0.0 and @ag-media/react-pdf-table v2.0.3 packages are both designed for and compatible with @react-pdf/renderer v4.3.1. The imports on lines 8-9 are correct with no unused dependencies.
59-72: Confirm the behavioral change to display only the first activity aligns with requirements.The function now renders only the first activity from each filtered set, as indicated by the explicit comment. However, this behavioral change is undocumented and not mentioned in the PR description, which focuses on alignment and centering. Since the cell can contain multiple activities (filtered by
programArea.id), verify with your requirements that displaying only the first activity is the intended behavior and doesn't result in unintended data loss.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/features/scheduling/ProgramAreaGrid.tsx (3)
11-26: Remove unused color definitions.
gray-50andgray-700are defined in the theme but never used in this component. Consider removing them to keep the configuration lean.extend: { colors: { - 'gray-50': '#f8f9fa', 'gray-400': '#b8b8b8', - 'gray-700': '#495057', 'gray-900': '#333', }, },
39-47: ReplaceflatMapwithforEachfor side-effect-only iteration.
flatMapis intended for transformation and its return value is discarded here. UsingforEachis more appropriate and clearer for side-effect operations.const programAreaMap: Record<string, ProgramAreaID> = {}; - Object.values(schedule.blocks).flatMap((block) => - block.activities.forEach( - (a) => (programAreaMap[a.programArea.id] = a.programArea) - ) - ); + Object.values(schedule.blocks).forEach((block) => + block.activities.forEach((a) => { + programAreaMap[a.programArea.id] = a.programArea; + }) + );
85-86: Address or remove the TODO comment about header alignment.The comment indicates a known alignment issue that was worked around by using rows instead of a dedicated header. Consider documenting whether this workaround fully resolves the issue or if further investigation is needed.
Would you like me to open an issue to track investigating proper header alignment with
@ag-media/react-pdf-table?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/features/scheduling/ProgramAreaGrid.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/features/scheduling/ProgramAreaGrid.tsx (1)
src/types/sessionTypes.ts (2)
BundleActivity(105-108)ProgramAreaID(98-98)
🔇 Additional comments (3)
src/features/scheduling/ProgramAreaGrid.tsx (3)
1-9: LGTM!Imports are clean and all imported entities are used. The previously flagged unused
THimport has been correctly removed.
59-72: LGTM!The function correctly renders only the first activity as intended, with appropriate null handling for empty arrays.
133-144: LGTM!The document structure is clean with proper page sizing and layout. The inline
fontFamilystyle on the Page component ensures consistent font rendering, which is a reasonable approach for PDF generation.
nkanchinadam
left a comment
nkanchinadam
left a comment
There was a problem hiding this comment.
Overall, it looks good. I left a comments on how to streamline some of the code and to use the Tailwind config that we already defined.
nkanchinadam
left a comment
nkanchinadam
left a comment
There was a problem hiding this comment.
Overall, it looks good. I left a comments on how to streamline some of the code and to use the Tailwind config that we already defined.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/features/scheduling/ProgramAreaGrid.tsx (1)
42-55: First-activity rendering is intentional; theas BundleActivity[]cast is likely unnecessary.Limiting each cell to the first activity matches the comment and PR description. Given
scheduleis alreadySectionSchedule<"BUNDLE">,block.activitiesshould already be a bundle-activity array, soconst activities = block.activities as BundleActivity[];is probably redundant and drops the richer intersection type if you ever needassignmentsagain. You can let inference handle this.Also applies to: 85-86
src/features/scheduling/exporting/BlockRatiosGrid.tsx (1)
80-88: Fixtw()typos, guardmaxActivities, and re-check header/data column alignment.There are a few concrete issues in this section:
maxActivities = Math.max(...blocks.map(...))will return-Infinityifschedule.blocksis ever empty. Guard it with a length check:- const maxActivities = Math.max(...blocks.map(block => block.activities.length)); + const maxActivities = blocks.length + ? Math.max(...blocks.map((block) => block.activities.length)) + : 0;
- Several
tw()strings are missing spaces, so utilities don’t apply:- tw("flex-1 border border-black text-center items-center justify-centerp-[5px] text-[6px]") + tw("flex-1 border border-black text-center items-center justify-center p-[5px] text-[6px]") - tw("flex-1 text-center items-center justify-centerborder border-black p-[5px] text-[6px]") + tw("flex-1 text-center items-center justify-center border border-black p-[5px] text-[6px]") - tw("flex-1 border border-black p-[3px] text-center items-center justify-centertext-[6px]") + tw("flex-1 border border-black p-[3px] text-center items-center justify-center text-[6px]")Without these fixes, padding/centering will be off in exactly the cells you’re trying to standardize.
- With
@ag-media/react-pdf-table,THis intended as the header row whose direct children are cells. HereTHwraps multipleTRs, and each activity row renders twoTDs per block (campers + staff) while the activity-header row only renders oneTDper block. That mismatch in column counts can cause header cells not to line up cleanly with their data columns. It’s worth restructuring so the header row has the same number of logical columns as the data rows, following the library’sTH/TR/TDconventions.Also applies to: 90-101, 110-123, 130-139
src/features/scheduling/exporting/EmployeeGrid.tsx (1)
35-60: Header alignment is fixed; you can simplifyTH/TRnesting.The header now has one cell per column (NAME, each block, APO, AM/PM FP), so it lines up with the data rows. To better match
@ag-media/react-pdf-table’s pattern, you can drop the inner<TR>and render the<TD>s directly inside<TH>(sinceTHis already the header row), e.g.:- <TH style={tw("mb-[8px] border border-black")}> - <TR style={tw("flex-row bg-black")}> - <TD> - <Text ...>NAME</Text> - </TD> - {Object.keys(schedule.blocks).map((blockId) => ( - <TD key={blockId}> - <Text ...>{blockId}</Text> - </TD> - ))} - <TD> - <Text ...>APO</Text> - </TD> - <TD> - <Text ...>AM/PM FP</Text> - </TD> - </TR> - </TH> + <TH style={tw("mb-[8px] border border-black flex-row bg-black")}> + <TD> + <Text ...>NAME</Text> + </TD> + {Object.keys(schedule.blocks).map((blockId) => ( + <TD key={blockId}> + <Text ...>{blockId}</Text> + </TD> + ))} + <TD> + <Text ...>APO</Text> + </TD> + <TD> + <Text ...>AM/PM FP</Text> + </TD> + </TH>This removes one level of nesting and follows the library’s expected header structure.
src/features/scheduling/exporting/CamperGrid.tsx (1)
175-197: Fix invalid width tokens intw()and label the bunk header column.Two concrete issues in the header:
tw("... width: 30px")/tw("... width: 35px")won’t produce width styles—Tailwind-style parsers won’t understand rawwidth:tokens. These header cells will fall back to default widths, which won’t match the data cells below.The first header cell is still empty, so the bunk column is unlabeled.
You can also align header and data widths by sharing the same values, e.g.:
- <TR style={tw("bg-black")}> - <TH style={tw("text-center text-[7px] font-bold text-white p-[1px] width: 30px")}> - </TH> - <TH style={tw("text-center text-[7px] font-bold text-white p-[1px] width: 35px" )}> - <Text>NAME</Text> - </TH> + <TR style={tw("bg-black")}> + <TH style={[tw("text-center text-[7px] font-bold text-white p-[1px]"), { width: 10 }]}> + <Text>BUNK</Text> + </TH> + <TH style={[tw("text-center text-[7px] font-bold text-white p-[1px]"), { width: 50 }]}> + <Text>NAME</Text> + </TH> {blockIds.map((blockId) => ( <TH key={blockId} - style={[tw("text-center text-[7px] font-bold text-white p-[1px]"), { width: "30px" }]} + style={[tw("text-center text-[7px] font-bold text-white p-[1px]"), { width: 20 }]} > <Text>{blockId}</Text> </TH> ))} - <TH style={[tw("text-center text-[7px] font-bold text-white p-[1px]"), { width: "25px" }]}> + <TH style={[tw("text-center text-[7px] font-bold text-white p-[1px]"), { width: 20 }]}> <Text>+</Text> </TH> - <TH style={[tw("text-center text-[7px] font-bold text-white p-[1px]"), { width: "25px" }]}> + <TH style={[tw("text-center text-[7px] font-bold text-white p-[1px]"), { width: 20 }]}> <Text>FP</Text> </TH> </TR>These widths now match the corresponding
TDwidths below, which should keep the header row aligned with the data columns.
🧹 Nitpick comments (9)
src/utils/reactPdfTailwind.ts (1)
1-28: Centralizedtwhelper looks good; consider reusing more of the base theme.This setup is clean and correctly pulls
extend.colorsfromtailwindConfig. If you later rely on more Tailwind tokens in PDFs, consider spreadingtailwindConfig.themeintotheme(and then layering your overrides) to avoid the PDF theme drifting from the main Tailwind config, but it’s not required right now.src/app/demo/program-area-grid/page.tsx (1)
13-131: Demo page is fine; you can hoist mock data out of the component.The mock schedule/program areas work well for this viewer. If this page ever re-renders frequently, consider moving
programAreas,createActivity, andmockScheduleoutside the component so they’re constructed once, and double‑check the literals stay in sync withProgramAreaID/BundleActivityif those interfaces gain required fields.src/features/scheduling/ProgramAreaGrid.tsx (1)
21-40: Simplify program area collection for readability.Using
Object.values(schedule.blocks).flatMap(...forEach...)only for side effects is a bit hard to read. A simple nestedfor...oforforEachoverschedule.blocksandblock.activitieswould avoid allocating the intermediate array and make the intent (“collect unique program areas”) clearer without changing behavior.src/features/scheduling/exporting/BlockRatiosGrid.tsx (2)
20-78: Consider removing the commented-out StyleSheet block.The old StyleSheet definition is fully commented and no longer used. Dropping it would make this file easier to scan and keep the focus on the new
Table+twlayout.
143-191: Defensively handle missing attendees fromattendeesById.Both the camper/bunk and staff/admin sections assume
attendeesById[id]is always defined and immediately readnameandrole. If the schedule or attendee arrays ever drift (e.g., stale IDs), this will throw during PDF generation. A small guard, e.g. skipping entries when!attendeebefore callingformatNameShort, would make this more robust without changing the happy path.src/features/scheduling/exporting/CamperPreferencesSheet.tsx (1)
12-37: Remove the commented-out StyleSheet for clarity.The legacy StyleSheet block is fully commented and superseded by
twstyles. Removing it will declutter the file and reduce the chance of someone editing dead styles by mistake.src/features/scheduling/exporting/EmployeeGrid.tsx (1)
63-65: Avoid casting Freeplay assignments tonumber[]before mapping.
getFreeplayAssignmentId(freeplay, employee.id)can return a string postId, a numeric staffId, an array of camperIds, ornull. Casting that union tonumber[]will silently mis-handle the non-array cases. Safer is to branch on array-ness:- const fpBuddyIds = getFreeplayAssignmentId(freeplay, employee.id); - const fpBuddies = fpBuddyIds ? (fpBuddyIds as number[]).map(id => campers.find(c => c.id === id)) : []; + const fpAssignment = getFreeplayAssignmentId(freeplay, employee.id); + const fpBuddies = Array.isArray(fpAssignment) + ? fpAssignment + .map((id) => campers.find((c) => c.id === id)) + .filter((c): c is CamperAttendeeID => !!c) + : [];This keeps the existing behavior for “buddy” assignments while avoiding incorrect casts for postId/staffId returns.
Also applies to: 124-127
src/features/scheduling/exporting/CamperGrid.tsx (2)
18-143: Clean up the commented-out StyleSheet.The old StyleSheet block is fully commented and replaced by the
Table+twlayout. Removing it will simplify the file and avoid confusion about which styling path is authoritative.
199-267: Optionally, share block/width constants to reduce duplication and order assumptions.You’re relying on
Object.keys(schedule.blocks)for headers andObject.values(schedule.blocks)forblocksArray. JS currently preserves insertion order so these stay in sync, but deriving a singleconst blockEntries = Object.entries(schedule.blocks)and mapping it for both headers and data would make the coupling explicit. Likewise, the various hard-coded widths (10/50/20/20/20) could be pulled into named constants or Tailwind width utilities so you only need to change them in one place if the layout needs to be tweaked.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/app/demo/program-area-grid/page.tsx(1 hunks)src/features/scheduling/ProgramAreaGrid.tsx(2 hunks)src/features/scheduling/exporting/BlockRatiosGrid.tsx(5 hunks)src/features/scheduling/exporting/CamperGrid.tsx(4 hunks)src/features/scheduling/exporting/CamperPreferencesSheet.tsx(4 hunks)src/features/scheduling/exporting/EmployeeGrid.tsx(4 hunks)src/utils/reactPdfTailwind.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/features/scheduling/exporting/CamperPreferencesSheet.tsx (2)
src/utils/reactPdfTailwind.ts (1)
tw(8-28)src/features/scheduling/generation/schedulingUtils.ts (1)
isBundleActivity(10-12)
src/features/scheduling/exporting/BlockRatiosGrid.tsx (2)
src/utils/reactPdfTailwind.ts (1)
tw(8-28)src/features/scheduling/generation/schedulingUtils.ts (2)
isBundleActivity(10-12)isIndividualAssignments(35-37)
src/features/scheduling/exporting/CamperGrid.tsx (2)
src/utils/reactPdfTailwind.ts (1)
tw(8-28)src/features/scheduling/generation/schedulingUtils.ts (3)
getFreeplayAssignmentId(18-33)isIndividualAssignments(35-37)isBundleActivity(10-12)
src/features/scheduling/exporting/EmployeeGrid.tsx (2)
src/utils/reactPdfTailwind.ts (1)
tw(8-28)src/utils/personUtils.ts (1)
getFullName(3-5)
src/features/scheduling/ProgramAreaGrid.tsx (2)
src/types/sessionTypes.ts (2)
BundleActivity(105-108)ProgramAreaID(98-98)src/utils/reactPdfTailwind.ts (1)
tw(8-28)
🔇 Additional comments (2)
src/features/scheduling/ProgramAreaGrid.tsx (1)
67-80: Table structure now satisfies the alignment and centering goals.Header and body rows both use
TRwith matchingTDshapes and consistent widths/min-heights, and thetext-centerstyles on both header labels and cell content ensure horizontal centering. This should fix the header/body misalignment and oversized-row issues.Also applies to: 90-107, 118-125
src/features/scheduling/exporting/CamperPreferencesSheet.tsx (1)
54-62: Activity filtering and new layout look solid.The block-level
activitiescomputation correctly narrows bundle blocks byageGroupwhen provided, and the newtw-based Page/row styles keep the sheet compact while preserving readability. No functional issues here from my side.Also applies to: 64-92
a884ab9 to
974f469
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In @src/features/scheduling/exporting/CamperGrid.tsx:
- Around line 74-91: The code misses handling when getFreeplayAssignmentId
returns number[] for buddy cases; add an Array.isArray(fpAssignment) branch
after the typeof checks that iterates each staffId in fpAssignment, looks up
matching postId(s) from freeplay.posts (same logic used for the number case),
collects unique postIds, and sets fpActivityCode to either a sensible display
string (e.g., comma-separated unique postIds) or "-" if none found; update
references to fpAssignment, fpActivityCode, and freeplay.posts accordingly.
In @src/features/scheduling/exporting/EmployeeGrid.tsx:
- Around line 42-83: The table header currently nests a TR inside TH in
EmployeeGrid.tsx (TH contains a TR that contains TDs), which violates
@ag-media/react-pdf-table expectations; refactor the header so TH directly
contains the TD elements (move the NAME, mapped blockId TDs, APO and AM/PM FP
TDs out of the TR and into TH), remove the TR wrapper from inside TH, and ensure
any styling applied to the former TR is moved to the TDs or to TH as appropriate
so header alignment is preserved.
In @src/features/scheduling/exporting/ProgramAreaGrid.tsx:
- Line 118: In ProgramAreaGrid's JSX where the Text element renders
"{sectionName} Program Area Grid", fix the prop typo by replacing the incorrect
prop name "stylstyle" with "style" on the Text component so the styling is
applied and the runtime error is resolved; verify the Text component accepts a
style prop if it's a custom component (component name: Text, JSX containing
{sectionName} Program Area Grid).
- Line 2: The import line in ProgramAreaGrid.tsx includes unused symbols
Document and Page from "@react-pdf/renderer" which trigger lint errors; remove
Document and Page from the import so only the used symbols (e.g., Text, View)
are imported, or delete the entire import if none are used, then run the linter
to confirm the warning is resolved.
- Line 85: The explicit type assertion on block.activities should be removed to
preserve the narrowed generic typing from SectionSchedule<"BUNDLE">; replace the
cast "as BundleActivity[]" by using the already-typed value (e.g., change the
assignment in ProgramAreaGrid where const activities = block.activities as
BundleActivity[] to just use block.activities) so you don't lose extra
properties like assignments and keep the proper type information for downstream
code.
- Around line 42-55: renderActivityText in ProgramAreaGrid currently returns
only the first activity; update it to indicate when additional activities are
present by appending a concise count (e.g., "First Activity Name (Age) (+2
more)") so the PDF cell still fits but the user knows items are omitted. Locate
the renderActivityText function that takes BundleActivity[] and modify its
returned Text (using tw styling) to check activities.length>1 and append `
(+{activities.length - 1} more)` after the first activity string; keep the
existing styling/size constraints and ensure no line breaks are introduced so
the cell layout remains stable.
🧹 Nitpick comments (2)
src/features/scheduling/exporting/CamperGrid.tsx (1)
48-142: Consider using Tailwind width utilities for consistency.Throughout the table, width is specified using object literals like
{ width: "30px" }. Tailwind supports width utilities (e.g.,w-[30px]) that could be included in thetw()strings for more consistent styling.Example:
// Current style={[tw("text-center text-[7px] font-bold text-white p-[1px]"), { width: "30px" }]} // Alternative style={tw("text-center text-[7px] font-bold text-white p-[1px] w-[30px]")}Based on past review comments suggesting to use Tailwind built-in styles where available.
src/features/scheduling/exporting/ProgramAreaGrid.tsx (1)
69-80: UseTHinstead ofTRfor the header row to follow@ag-media/react-pdf-tableconventions.The library provides a dedicated
TH(TableHeader) component for header rows. The recommended pattern is<TH><TD>…</TD></TH>for headers and<TR><TD>…</TD></TR>for data rows. Replace the header<TR>at line 69 with<TH>for consistency with the library's API.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.jsonsrc/features/scheduling/exporting/CamperGrid.tsxsrc/features/scheduling/exporting/EmployeeGrid.tsxsrc/features/scheduling/exporting/ProgramAreaGrid.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧬 Code graph analysis (3)
src/features/scheduling/exporting/EmployeeGrid.tsx (3)
src/utils/reactPdfTailwind.ts (1)
tw(8-28)src/features/scheduling/generation/schedulingUtils.ts (3)
getFreeplayAssignmentId(18-33)isIndividualAssignments(35-37)isBundleActivity(10-12)src/utils/personUtils.ts (1)
getFullName(3-5)
src/features/scheduling/exporting/CamperGrid.tsx (2)
src/utils/reactPdfTailwind.ts (1)
tw(8-28)src/features/scheduling/generation/schedulingUtils.ts (3)
getFreeplayAssignmentId(18-33)isIndividualAssignments(35-37)isBundleActivity(10-12)
src/features/scheduling/exporting/ProgramAreaGrid.tsx (2)
src/types/sessionTypes.ts (2)
BundleActivity(104-107)ProgramAreaID(97-97)src/utils/reactPdfTailwind.ts (1)
tw(8-28)
🪛 GitHub Actions: Lint & Build on PR
src/features/scheduling/exporting/ProgramAreaGrid.tsx
[error] 2-10: ESLint: 'Document' is defined but never used. (no-unused-vars)
[error] 2-20: ESLint: 'Page' is defined but never used. (no-unused-vars)
🔇 Additional comments (2)
src/features/scheduling/exporting/EmployeeGrid.tsx (1)
101-174: Data row structure looks good.The employee row rendering correctly uses
TRwithTDelements for each column, properly handles OFF periods, and uses type guards to safely determine activity assignments.src/features/scheduling/exporting/CamperGrid.tsx (1)
93-141: Camper data row rendering looks good.The logic correctly:
- Formats camper names consistently
- Uses type guards to safely determine activity assignments
- Handles both individual and bunk-based assignments
- Gracefully displays empty strings for missing activities
|
The various PDF components, when populated with data, sometimes overflow the page they're on and get cut off, making them unreadable for users. Before we merge this PR, we should do the following:
|
3 participants
-for table components: the header row is slightly off from rows below it (check HR tags)
-text in cells need to be centered
-adjust TR tag styles to include a smaller height property across all cells
Summary by CodeRabbit
Release Notes