Feature/session wide algorithm#36
Conversation
… out employee days off throughout a session
…3, and 14 and maxing out days off from days 2-6 and 7-12
…layout for both assigning nightshifts and days off for employees
… into test/testData
|
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:
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (3)
📝 WalkthroughWalkthroughImplements ChangesDays-Off Scheduling & Moment Type Migration
Navbar RBAC & Minor Cleanup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 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 (2)
src/features/sessionWideAlgorithm.ts(1 hunks)src/types/sessionTypes.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/features/sessionWideAlgorithm.ts (1)
src/types/sessionTypes.ts (2)
Session(4-15)StaffSessionAttendee(39-45)
eshavignesh123
left a comment
eshavignesh123
left a comment
There was a problem hiding this comment.
Hi Parth! Could you pull from main when you get a chance? We added a bunch of updates to sessionTypes, so refactoring your code based on that would be super helpful! We also added a new file, SessionScheduler.ts, to keep all our session-wide algorithms, so could you move your functions there as well? Thanks!
…, and used updated types to represent and store calculated days off and night shifts
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
src/features/scheduling/SessionScheduler.ts (1)
29-30: Use a named constant for milliseconds per day.The magic number
86400000(milliseconds in a day) reduces readability.Apply this diff:
+ const MS_PER_DAY = 86400000; const numDays = - Math.floor((endDate.getTime() - startDate.getTime()) / 86400000) + 1; + Math.floor((endDate.getTime() - startDate.getTime()) / MS_PER_DAY) + 1;
📜 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 (1)
src/features/scheduling/SessionScheduler.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/features/scheduling/SessionScheduler.ts (1)
src/types/sessionTypes.ts (3)
SessionID(11-11)StaffAttendeeID(35-35)NightShiftID(49-49)
🔇 Additional comments (2)
src/features/scheduling/SessionScheduler.ts (2)
70-76: Boundary condition: date comparison at half-point may be ambiguous.Lines 71 and 75 use
<=and>to split the schedule at the half-point. IfhalfPointDateexactly matches a day off, it's counted in the first half. Verify this is the intended behavior.
136-145: Major: Program counselor consecutive days off eligibility logic is unclear and requires clarification.The logic at lines 137-145 checks whether a program counselor has any two consecutive days off anywhere during the session. However, the intent is ambiguous:
- Currently, the code creates one shift assignment per bunk for the entire session (not per night), making the consecutive days off check questionable in context.
- The logic returns
eligible = falseif the employee has ANY consecutive days off during the session, but it's unclear why consecutive days off (versus any days off) affects session-wide night shift eligibility.- The boundary checks (
prev <= endDate) are incomplete—they don't verify the dates fall within the session'sstartDatetoendDaterange.Given the architectural issue mentioned in your review (single-shift-per-bunk for entire session), this eligibility logic needs clarification alongside the overall night shift structure. Consider:
- Should night shifts be assigned per-night rather than per-session?
- If per-night, should eligibility check whether the counselor has a day off immediately before or after that specific night?
- If per-session, what is the business rule for consecutive days off affecting overall eligibility?
| const firstHalf = dayIndex <= Math.floor(numDays / 2); | ||
| const halfPointDate = new Date(startDate); | ||
| halfPointDate.setDate(startDate.getDate() + Math.floor(numDays / 2)); | ||
|
|
||
| const hasDayOffInFirstHalf = randomEmployee.daysOff.some( | ||
| (d) => new Date(d).getTime() <= halfPointDate.getTime() | ||
| ); | ||
|
|
||
| const hasDayOffInSecondHalf = randomEmployee.daysOff.some( | ||
| (d) => new Date(d).getTime() > halfPointDate.getTime() | ||
| ); | ||
|
|
||
| if ( | ||
| (firstHalf && hasDayOffInFirstHalf) || | ||
| (!firstHalf && hasDayOffInSecondHalf) | ||
| ) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Half-schedule logic may not guarantee one day off per half.
The current logic prevents assigning two days off in the same half but doesn't ensure both halves receive a day off. An employee could get both days off in one half if they're selected twice before the half switches, or end up with only one day off total.
Consider revising the algorithm to:
- First assign one day off in the first half to each employee, then
- Assign the second day off in the second half to each employee.
This ensures the constraint is met deterministically rather than relying on random chance.
🤖 Prompt for AI Agents
In src/features/scheduling/SessionScheduler.ts around lines 66–83 the current
check only blocks assigning a second day-off in the same half but doesn’t
guarantee each employee gets exactly one day-off in each half; change the
algorithm to a deterministic two-phase assignment: first iterate employees and
assign one day-off inside the first half (pick a random available date within
first half that doesn’t conflict with existing constraints), mark it, then
iterate again and assign the second day-off inside the second half (pick a
random available date within second half); if no valid date exists in the target
half, implement a clear fallback (e.g., try nearest valid date in that half or
log/fail the employee assignment) to preserve constraints and avoid leaving
employees with zero or two days in the same half.
…es, and fixed bugs in the assign nights off function such as not assigning 4 employees per bunk a night off
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
src/features/scheduling/SessionScheduler.ts (6)
63-65: Random selection bug has been fixed.The previous critical issue where the code randomly selected from the full
employeesarray instead ofemployeeChoiceshas been corrected. The code now properly builds an array from the eligible employee set and selects randomly from it.
144-145: Typo has been corrected.The previous typo "progrma" has been fixed to
programCounselor.
159-159: Typos have been corrected.The previous typos "counsoler" and "choosen" have been fixed to "counselor" and "chosen".
74-91: Half-schedule logic still doesn't guarantee one day off per half.The algorithm prevents assigning two days off in the same half but doesn't ensure each employee receives exactly one day off in each half. An employee could end up with both days in one half (if selected before reaching that half's limit) or only one day total.
Consider a deterministic two-phase approach:
- First pass: Assign one day off in the first half to each employee
- Second pass: Assign one day off in the second half to each employee
This ensures the constraint is met reliably rather than depending on random selection order.
94-94: Input mutation creates side effects.The method directly mutates the
employeesparameter by pushing torandomEmployee.daysOff. This creates side effects that may surprise callers and complicates testing.Consider either:
- Clearly documenting this mutation behavior in a comment above the method, or
- Returning a map of employee IDs to assigned days off and letting the caller apply updates
111-168: Critical: Night shift assignment is still incomplete.The method creates only one
NightShiftIDobject per bunk for the entire session (lines 128-135) rather than creating shifts for each night. For a multi-day session, this leaves all nights except one without coverage.Expected behavior:
- Loop through each night of the session (use a date iterator from startDate to endDate)
- For each night, assign eligible counselors per bunk
- Rotate counselors across nights to distribute the workload
- Check day-off eligibility for the specific night being assigned
Current implementation would require substantial refactoring to assign shifts per night rather than per session.
🧹 Nitpick comments (3)
src/features/scheduling/SessionScheduler.ts (3)
3-8: Consider using Fisher-Yates for better performance.The current shuffle works correctly but has O(n log n) complexity due to sorting. For better performance with large employee lists, consider an in-place Fisher-Yates shuffle:
function shuffleArray<T>(arr: T[]): T[] { - return arr - .map(value => ({ value, sort: Math.random() })) - .sort((a, b) => a.sort - b.sort) - .map(({ value }) => value); + const shuffled = [...arr]; + for (let i = shuffled.length - 1; i > 0; i--) { + const j = Math.floor(Math.random() * (i + 1)); + [shuffled[i], shuffled[j]] = [shuffled[j], shuffled[i]]; + } + return shuffled; }
146-154: Clarify or simplify the date boundary checks in eligibility logic.The eligibility check verifies that a program counselor doesn't have adjacent days off, but the
prev <= endDateandnext <= endDateconditions (lines 152-153) appear redundant. Ifemployee.daysOffonly contains dates within the session range, checking whether computed adjacent dates fall withinendDateshouldn't be necessary for correctness.Consider simplifying to:
const eligible = !employee.daysOff.some((day) => { const dayDate = new Date(day); const prev = new Date(dayDate); prev.setDate(prev.getDate() - 1); const next = new Date(dayDate); next.setDate(next.getDate() + 1); - return prev <= endDate && employee.daysOff.includes(toISODate(prev)) || - next <= endDate && employee.daysOff.includes(toISODate(next)); + return employee.daysOff.includes(toISODate(prev)) || + employee.daysOff.includes(toISODate(next)); });If the boundary check serves a specific purpose, please add a comment explaining why.
120-120: Consider hoistingtoISODatehelper to reduce duplication.The
toISODatehelper is defined insideassignNightShifts, butassignDaysOff(line 42) uses inlinetoISOString().split("T")[0]for the same purpose. Hoisting this helper to the module level would improve consistency and reduce duplication.+function toISODate(date: Date): string { + return date.toISOString().split("T")[0]; +} + export class SessionScheduler {Then update line 42 in
assignDaysOff:- const dateStr = currentDate.toISOString().split("T")[0]; + const dateStr = toISODate(currentDate);
📜 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 (3)
package.json(1 hunks)src/features/scheduling/FreeplayScheduler.ts(1 hunks)src/features/scheduling/SessionScheduler.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/features/scheduling/SessionScheduler.ts (1)
src/types/sessionTypes.ts (3)
SessionID(11-11)StaffAttendeeID(35-35)NightShiftID(49-49)
🔇 Additional comments (2)
src/features/scheduling/FreeplayScheduler.ts (1)
26-27: LGTM!The lint directive appropriately suppresses warnings for the unused parameter in this stub method. The explanatory comment provides helpful context.
package.json (1)
25-25: Verify or remove unusedsharpdependency
No imports or invocations ofsharpwere found in the codebase; remove it from package.json if it isn’t actually used.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/types/sessionTypes.ts (1)
62-62: LGTM! Consider adding a JSDoc comment for clarity.The addition of the
hasDaysOffboolean field toCommonSectionis well-typed and appropriately named. This flag appears to control whether the scheduling algorithm should assign days off during this section.Consider adding a brief JSDoc comment to clarify when this flag should be set:
+ /** + * Indicates whether staff/admin should have days off assigned during this section. + */ hasDaysOff: boolean;
nkanchinadam
left a comment
nkanchinadam
left a comment
There was a problem hiding this comment.
Overall, a good structure of the functions and got overall logic down. Main things to change are:
- Use moment library instead of Date objects
- Don't make assumptions about the session length when assigning Days Off
- Make sure to assign all Staff & Admins
- Use helper functions to abstract out the logic & make it more readable
|
|
||
| const employeeChoices = new Set(employees); | ||
|
|
||
| const startDate = new Date(session.startDate); |
There was a problem hiding this comment.
instead of using the built-in JS Date class, which is annoying & difficult to work with, import the moment library and do all datetime calculations with that
| const currentDate = new Date(startDate); | ||
|
|
||
| for (let dayIndex = 1; dayIndex <= numDays; dayIndex++) { | ||
| const dateStr = currentDate.toISOString().split("T")[0]; |
There was a problem hiding this comment.
you could probably extract this logic into a helper function to make it more readable
| const dateStr = currentDate.toISOString().split("T")[0]; | ||
|
|
||
| // Skip restricted days (first, 13th, and 14th) | ||
| if (dayIndex === 1 || dayIndex === 13 || dayIndex === 14) { |
There was a problem hiding this comment.
Don't hardcode the days that shouldn't have anyone with a Day Off since they could change if Camp Starfish changes the length of the session they do, which they do throughout the year with shorter sessions as well as experimenting with different sessinon lengths. I realize that there's no real way to tell if a day should have anyone scheduled for a Day Off on it, so I just pushed a hasDaysOff field to the Section type in sessionTypes.ts.
| } | ||
|
|
||
| // Move to next day | ||
| currentDate.setDate(currentDate.getDate() + 1); |
There was a problem hiding this comment.
Since you're doing a for loop, you can just calculate the current date from the index of the loop instead of keeping track of it separately
| if (randomEmployee.daysOff.includes(dateStr)) continue; | ||
|
|
||
| // Checks if employee has a day off in the first half of schedule | ||
| const firstHalf = dayIndex <= Math.floor(numDays / 2); |
There was a problem hiding this comment.
Similar to earlier comment about not hardcoding sessions, the only rule about Days Off is that each staff & admin legally has to have 1 Day Off every 7 days of camp, so divide up the timeframe into weeks & round down for partial weeks and then assign Days Off for each week. It would probably also be helpful to create a helper function that handles a single week to simplify the logic.
|
|
||
| assignDaysOff(): SessionScheduler { return this; } | ||
| assignDaysOff(session: SessionID, employees: StaffAttendeeID[]): SessionScheduler { | ||
| const MAX_STAFF_OFF_PER_DAY = 2; |
There was a problem hiding this comment.
Unsure where the number 2 came from, but every staff & admin member needs to have a Day Off every week, so you can calculate the number off each day by doing (numStaff + numAdmin) / numDays
|
|
||
| // Keep track of attempts to avoid infinite loops | ||
| let attempts = 0; | ||
| const maxAttempts = numEmployees * 2; |
There was a problem hiding this comment.
I don't really get how assigning the staff & admin could fail, so modify the logic to ensure that everyone gets assigned since we don't the user to try generating a schedule only to encounter failures randomly
| ) { | ||
| attempts++; | ||
|
|
||
| const choices = Array.from(employeeChoices); |
There was a problem hiding this comment.
When choosing employees to assign, assign Program Counselors first and then everyone else since Program Counselors can't have a Day Off on a day where they're supposed to lead an activity. Try to assign them to Jamboree days since Program Counselor duties only apply to Bundles.
Additionally, try to ensure that of the people who have a Day Off on a given day, they're all from different bunks, so no bunk has too few counselors for the night.
| const shift: NightShiftID = { | ||
| id: `night-${bunkNumber}-${session.id}`, | ||
| sessionId: session.id, | ||
| [bunkNumber]: { |
There was a problem hiding this comment.
NightShiftID is a type intended for 1 night of and would contain the info for each bunk that night. [bunkId: number]: ... means that the type is a map where you can have multiple keys (the bunk numbers), so only create 1 of these objects per night and store the data in that single object.
|
|
||
| for (const employee of selected) { | ||
| // employee is a program counselor, check if they have day off before or after | ||
| if (employee.programCounselor) { |
There was a problem hiding this comment.
This condition doesn't only apply to Program Counselors, it applies to all staff. The only thing related to Program Counselors in these functions is assigning their Days Off to be during Jamborees.
…r being assigned off on jamboree days, assigning more evenly throughout bunks, and assigning a night shift object per night rather than per bunk
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/features/scheduling/SessionScheduler.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/features/scheduling/SessionScheduler.ts (1)
src/types/sessionTypes.ts (5)
SectionID(55-55)StaffAttendeeID(35-35)AdminAttendeeID(41-41)SessionID(11-11)NightShiftID(49-49)
… into test/testData
…mp-starfish into test/testData
nkanchinadam
force-pushed
the
main
branch
2 times, most recently
from
a884ab9 to
974f469
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/features/scheduling/generation/SessionScheduler.ts`:
- Around line 55-69: In isJamboreeDay (private method in SessionScheduler)
remove the unused local variable dateStr that is declared as const dateStr =
this.toISODate(date); so that the function no longer defines an unused variable
and the linter error is resolved; simply delete that line and ensure the rest of
isJamboreeDay (including references to date, section.startDate, section.endDate
and section.type) remains unchanged.
- Around line 1-3: The import list in SessionScheduler.ts includes an unused
type NightShift which causes an ESLint unused import error; remove NightShift
from the import statement that currently reads "import { StaffAttendeeID,
AdminAttendeeID, NightShiftID, SessionID, SectionID, NightShift } from
"@/types/sessionTypes"; so update that import to exclude NightShift (leaving
StaffAttendeeID, AdminAttendeeID, NightShiftID, SessionID, SectionID) and run
lint/TS build to confirm there are no remaining references to the NightShift
symbol.
♻️ Duplicate comments (2)
src/features/scheduling/generation/SessionScheduler.ts (2)
30-38: Include the session end date ingetDateRange.The loop currently excludes
endDate, which drops the final day (and yields an empty range for single-day sessions). Also flipslettoconstto satisfy lint.✅ Proposed fix
- let fromDate = startDate - let toDate = endDate - let diff = toDate.diff(fromDate, "day") - let range = [] - for (let i = 0; i < diff; i++) { + const fromDate = startDate + const toDate = endDate + const diff = toDate.diff(fromDate, "day") + const range = [] + for (let i = 0; i <= diff; i++) { range.push(moment(startDate).add(i, "day")) }
204-227: Reset the day-off pool per week (not per session).
employeesNeedingDayOffandMAX_STAFF_OFF_PER_DAYare global for the whole session, so multi‑week sessions give each person only one day off total. The week windows also overlap becausecurrDate.add(6, "day")advances only 6 days. Recompute the pool and cap per week and advance by 7 days.✅ Proposed fix
- const NUM_WEEKS = Math.ceil(end.diff(start, "week", true)); - const NUM_DAYS = (end.diff(start, "day")) + 1 - const MAX_STAFF_OFF_PER_DAY = Math.ceil(employees.length / NUM_DAYS); - const employeesNeedingDayOff = new Set(employees); - - let currDate = start; // Need a future implementation for this date range to only include dates that allow for a day off. const sessionDateRange: Set<Moment> = new Set(this.getDateRange(start, end)) - - for (let i = 0; i < NUM_WEEKS; i++) { - if (currDate.clone().add(1, "week").isBefore(end)) { - this.assignDaysOffForWeek(currDate, currDate.clone().add(6, "day"), sessionDateRange, employees, employeesNeedingDayOff, MAX_STAFF_OFF_PER_DAY) - currDate.add(6, "day") - } - else { - this.assignDaysOffForWeek(currDate, end, sessionDateRange, employees, employeesNeedingDayOff, MAX_STAFF_OFF_PER_DAY) - currDate = end - } - - } + let weekStart = start.clone() + while (weekStart.isSameOrBefore(end)) { + const weekEnd = moment.min(weekStart.clone().add(6, "day"), end) + const daysInWeek = weekEnd.diff(weekStart, "day") + 1 + const maxStaffOffPerDay = Math.ceil(employees.length / daysInWeek) + const employeesNeedingDayOff = new Set(employees) + + this.assignDaysOffForWeek( + weekStart, + weekEnd, + sessionDateRange, + employees, + employeesNeedingDayOff, + maxStaffOffPerDay + ) + weekStart.add(7, "day") + }
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Fix all issues with AI agents
In `@src/app/LoginPage.tsx`:
- Around line 37-45: The catch in generateTestData incorrectly sets a sign-in
message; change it to handle errors from generateSession by catching the error
(catch (err)) and calling setError with a message like "An error occurred while
generating test data. Please try again." and optionally log the actual error via
console.error(err) to aid debugging; update references in generateTestData and
ensure setError usage is consistent.
- Around line 81-88: Remove or hide the debug "Generate Test" button from the
production LoginPage: locate the button using the generateTestData function and
MicrosoftIcon in LoginPage.tsx and either delete it or wrap its JSX in a
condition that only renders in development (e.g., check process.env.NODE_ENV ===
'development'); if you keep it for dev use, replace the MicrosoftIcon with a
neutral dev icon and modify generateTestData to provide user-visible feedback
(toast or inline message) instead of only console.log, and ensure this
functionality is moved to an admin/dev-only route if it needs to exist
long-term.
In `@src/features/scheduling/generation/SessionScheduler.ts`:
- Line 125: The variable remainingWF in SessionScheduler.ts (declared as const
remainingWF = shuffledWF.filter(e => !selectedWFWindow1.includes(e) &&
!selectedWFWindow2.includes(e));) is unused; remove this declaration to fix the
pipeline by deleting the remainingWF assignment (or if its intent was to be used
later, instead integrate it where needed—e.g., replace any placeholder logic
that should reference remainingWF with this filtered array). Ensure references
to shuffledWF, selectedWFWindow1 and selectedWFWindow2 remain unchanged.
- Around line 114-116: The local variable otherEmployees created by
employees.filter(...) in SessionScheduler (the expression "const otherEmployees
= employees.filter(e => !('programCounselor' in e && e.programCounselor))") is
unused; remove that declaration or rename it to _otherEmployees if you intend to
keep it for future use to silence the linter/pipeline. Edit the
SessionScheduler.ts occurrence to either delete the const otherEmployees = ...
line or change its name to _otherEmployees so the variable is no longer flagged
as unused.
- Around line 138-146: The loop mutates the shared maxStaffOffPerDay causing it
to stay Infinity for subsequent employees; inside the window-1 and window-2
loops in SessionScheduler.ts, compute a per-employee local limit (e.g., let
perEmployeeMax = (dates.length === 1) ? Infinity : maxStaffOffPerDay) and pass
that perEmployeeMax into assignOneDayOff instead of mutating the outer
maxStaffOffPerDay; ensure you do the same pattern in the window2 loop so the
global cap isn't permanently changed.
- Around line 1-7: Remove the erroneous/unused imports in SessionScheduler.ts:
delete the faulty imports of da (from "@faker-js/faker/."), w (from
"@faker-js/faker/dist/airline-CLphikKp"), em (from "@mantine/core") and max
(from "moment"), and also remove the unused type NightShift from the import list
(keeping only used symbols like StaffAttendeeID, AdminAttendeeID, NightShiftID,
SessionID, SectionID, Moment, and moment); ensure no references to the removed
symbols remain elsewhere in SessionScheduler.ts and run the type-check/lint to
confirm pipeline errors are resolved.
In `@src/features/scheduling/test/generateEmulatorData.tsx`:
- Around line 495-513: The function parameter blockID is unused; either remove
it from the signature or rename it to _blockID to silence the unused-parameter
error — update generateBunkJamboActivities accordingly and apply the same change
to generateNonBunkJamboActivities so their signatures remain consistent with
callers/types.
- Around line 1-6: The import statement in generateEmulatorData.tsx includes
unused symbols causing pipeline failures; remove Freeplay and
SchedulingSectionID from the named import list (the import that currently lists
CamperAttendeeID, StaffAttendeeID, AdminAttendeeID, AgeGroup, SectionSchedule,
BundleActivityWithAssignments, SectionPreferences, BunkID,
BunkJamboreeActivityWithAssignments, NonBunkJamboreeActivityWithAssignments,
Freeplay, PostID, SectionID, SessionID, NightShiftID, ProgramAreaID,
SchedulingSectionID, FreeplayID) so only used types remain imported.
- Line 739: The local variable campers assigned from
generateCampers(TOTAL_CAMPERS, TOTAL_BUNKS) is unused; either remove the
declaration or rename it to _campers (prefix underscore) to silence the
unused-variable warning, or if generateCampers is being removed altogether,
delete this line; update the area around the generateCampers call in
generateEmulatorData.tsx so no unused symbol remains.
- Around line 349-352: The code sets schedule.alternatePeriodsOff["RH"] twice in
generateEmulatorData (variable schedule of type SectionSchedule<'BUNK-JAMBO'>);
remove the duplicate assignment so alternatePeriodsOff["RH"] is only initialized
once (leave a single schedule.alternatePeriodsOff["RH"] = [] and delete the
redundant repeated line).
- Around line 130-192: The function generateNonoLists is defined but never used,
causing linting and pipeline issues. To fix this, either remove the
generateNonoLists function if it is not needed, or export it to make it
available externally, or prefix its name with an underscore (e.g.,
_generateNonoLists) if you intend to keep it for future use. Apply the same fix
to other unused functions in the file such as generateYesyesLists and
generateBunks.
🧹 Nitpick comments (3)
src/features/scheduling/test/generateEmulatorData.tsx (2)
608-731: Remove large blocks of commented-out code.Over 120 lines of commented-out code should be removed. Version control preserves history if these implementations are needed later. Keeping them makes the file harder to maintain and navigate.
If these functions are planned for near-term use, consider keeping them but marking with
// TODO:comments explaining the plan, or moving to a separate file.
1-12: Consider renaming file from.tsxto.ts.This file contains no JSX/React components - it's pure TypeScript logic. Using
.tsxextension is misleading and may cause unnecessary JSX parsing overhead.src/features/scheduling/generation/SessionScheduler.ts (1)
186-186: Useconstwith clone pattern for clarity.ESLint flags this because
currDateis never reassigned. Whilemoment.add()mutates in place (solettechnically works), usingconstwith explicit reassignment is clearer:Proposed fix
- let currDate = startDate; + let currDate = startDate.clone(); // Create a night bunk object for each night while (currDate.isSameOrBefore(endDate)) { // ... shift logic ... this.nightShifts.push(shift); - currDate.add(1, "day"); + currDate = currDate.clone().add(1, "day"); }Or simply suppress the lint warning with a comment if mutation is intentional.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/features/scheduling/generation/SessionScheduler.ts`:
- Around line 73-81: The current logic in SessionScheduler.ts skips bunk checks
when maxStaffOffPerDay === Infinity, which can let multiple employees from the
same bunk take the same day off; change the condition around the bunk check so
the bunkConflict check (using 'bunk' in employee, allEmployees, employee.bunk,
and employee.daysOff.includes(date)) always runs regardless of
maxStaffOffPerDay, and if a bunkConflict is detected either skip assigning that
day-off or implement a fallback assignment path that searches for an alternative
employee/date rather than bypassing the constraint entirely; ensure the code in
the loop that currently contains "if ('bunk' in employee && maxStaffOffPerDay
!== Infinity) { ... }" is refactored to always evaluate bunk constraints and
handle failures gracefully.
- Line 187: The variable currDate in SessionScheduler.ts is never reassigned so
change its declaration from let to const to satisfy the prefer-const rule; keep
using currDate with moment.add() (which mutates the moment instance) so no other
code changes are needed—update the declaration where currDate is defined (near
startDate usage) to const currDate.
♻️ Duplicate comments (2)
src/features/scheduling/generation/SessionScheduler.ts (2)
137-140: Use a local variable instead of mutatingmaxStaffOffPerDay.While the recalculation at line 131 resets the value each iteration, the mutation pattern is still error-prone. Using a local variable as suggested in a previous review would be cleaner.
Proposed fix
for (const employee of employees) { - maxStaffOffPerDay = Math.ceil(employees.length / window1.length); const dates = 'programCounselor' in employee && jamboreeWindow1.has(employee) ? window1.filter(d => this.isJamboreeISO(d, this.sections)) : window1; - if (dates.length === 1){ - // set maxStaffOffPerDay to Infinity - maxStaffOffPerDay = Infinity; - }; + const effectiveMax = dates.length === 1 + ? Infinity + : Math.ceil(employees.length / window1.length); - if (!this.assignOneDayOff(employee, dates, dayCounts, employees, maxStaffOffPerDay)) { + if (!this.assignOneDayOff(employee, dates, dayCounts, employees, effectiveMax)) { console.warn(`Failed to assign window-1 day off for ${employee.id}`); } }Apply the same pattern to the window2 loop (lines 148-164).
1-7: Remove unused and erroneous imports to fix pipeline failures.The pipeline is still failing due to unused imports:
NightShift,da,w,em, andmax. These appear to be accidental auto-imports and must be removed.Proposed fix
-import { StaffAttendeeID, AdminAttendeeID, NightShiftID, SessionID, SectionID, NightShift } from "@/types/sessionTypes"; -import { da } from "@faker-js/faker/."; -import { w } from "@faker-js/faker/dist/airline-CLphikKp"; -import { em } from "@mantine/core"; -import { max } from "moment"; -import { Moment } from "moment"; -import moment from "moment"; +import { StaffAttendeeID, AdminAttendeeID, NightShiftID, SessionID, SectionID } from "@/types/sessionTypes"; +import moment, { Moment } from "moment";
🧹 Nitpick comments (3)
src/features/scheduling/generation/SessionScheduler.ts (3)
100-102: Hardcoded scheduling windows reduce flexibility.The windows are hardcoded to days 2-6 and 7-12 relative to session start, with a comment noting this is temporary. Consider deriving these windows dynamically from the session's actual duration to handle sessions of varying lengths.
203-207: Extract magic number for night shift staff count.The value
4for the number of employees selected per bunk per night is unexplained. Consider extracting this as a named constant for clarity and easier configuration.Proposed fix
+const NIGHT_SHIFT_STAFF_PER_BUNK = 4; + export class SessionScheduler { // ... existing code ... const shuffled = this.shuffleArray(employeesInBunk); - const selected = shuffled.slice(0, 4); + const selected = shuffled.slice(0, NIGHT_SHIFT_STAFF_PER_BUNK);
96-171: Note: Method mutates employee objects directly.
assignDaysOffmodifiesemployee.daysOffarrays in place rather than returning new data. This is a side-effect pattern that callers should be aware of. Consider documenting this behavior or returning a map of assignments for a more functional approach.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/features/scheduling/generation/SessionScheduler.ts`:
- Around line 200-227: The on‑duty split logic under‑allocates because it uses
"length + 1 < PER_SPLIT" and `selected` is never reassigned; change `selected`
to a const array and fix the condition to compare the current count directly
(e.g., use "shift[bunkNumber].counselorsOnDuty.length < PER_SPLIT") or
explicitly enforce a cap (introduce a MAX_ON_DUTY constant if the intent is to
"pick 4 employees") before pushing into shift[bunkNumber].counselorsOnDuty;
ensure fallback pushes go to shift[bunkNumber].nightBunkDuty, and keep all other
checks (daysOff via this.toISO(currDate), prevDate, nextDate) the same while
referencing employeesInBunk, PER_SPLIT, selected, and the shift[...] arrays to
locate the change.
♻️ Duplicate comments (7)
src/features/scheduling/generation/SessionScheduler.ts (2)
1-1: Remove unusedNightShiftimport to unblock lint.
NightShiftisn’t referenced in this file and fails the lint/build pipeline.🧹 Proposed fix
-import { StaffAttendeeID, AdminAttendeeID, NightShiftID, SessionID, SectionID, NightShift } from "@/types/sessionTypes"; +import { StaffAttendeeID, AdminAttendeeID, NightShiftID, SessionID, SectionID } from "@/types/sessionTypes";
68-77: Always enforce bunk conflicts, even when caps are lifted.When
maxStaffOffPerDayisInfinity, the bunk constraint is skipped, allowing same‑bunk counselors to take the same day off.🔒 Proposed fix
- if ('bunk' in employee && maxStaffOffPerDay !== Infinity) { + if ('bunk' in employee) { const bunkConflict = allEmployees.some(e => 'bunk' in e && e.bunk === employee.bunk && e.daysOff.includes(date) ); if (bunkConflict) continue; }src/features/scheduling/test/generateEmulatorData.tsx (5)
1-6: Remove unusedFreeplayandSchedulingSectionIDimports.Both are unused and currently fail lint.
🧹 Proposed fix
-import { CamperAttendeeID, StaffAttendeeID, AdminAttendeeID, AgeGroup, SectionSchedule, BundleActivityWithAssignments, - SectionPreferences, BunkID, BunkJamboreeActivityWithAssignments,NonBunkJamboreeActivityWithAssignments, Freeplay, - PostID, SectionID, SessionID, NightShiftID, - ProgramAreaID, - SchedulingSectionID, - FreeplayID} from "@/types/sessionTypes"; +import { CamperAttendeeID, StaffAttendeeID, AdminAttendeeID, AgeGroup, SectionSchedule, BundleActivityWithAssignments, + SectionPreferences, BunkID, BunkJamboreeActivityWithAssignments, NonBunkJamboreeActivityWithAssignments, + PostID, SectionID, SessionID, NightShiftID, + ProgramAreaID, + FreeplayID } from "@/types/sessionTypes";
130-261: Unused list/bunk helpers trip lint.
generateNonoLists,generateYesyesLists, andgenerateBunksare never called (only referenced in commented code). Remove, export, or prefix with_to satisfy lint.
317-593: Unused schedule helpers + duplicatealternatePeriodsOffinit.
generateBundleBlockSchedule,generateBunkJamboBlockSchedule,generateNonBunkJamboBlockSchedule,generateFreeplayBlockSchedule,generateBlockIDs,generatePosts, andgeneratePrefsare unused and fail lint. Also,schedule.alternatePeriodsOff["RH"]is set twice ingenerateBunkJamboBlockSchedule.🧹 Proposed fix (duplicate init)
- schedule.alternatePeriodsOff["RH"] = [] - - schedule.alternatePeriodsOff["RH"] = [] + schedule.alternatePeriodsOff["RH"] = []
483-521: Silence unusedblockIDparameters.
blockIDisn’t used in either jamboree activity generator and fails lint.🧹 Proposed fix
-function generateBunkJamboActivities(totalActivities: number, blockID: string): BunkJamboreeActivityWithAssignments[] { +function generateBunkJamboActivities(totalActivities: number, _blockID: string): BunkJamboreeActivityWithAssignments[] { @@ -function generateNonBunkJamboActivities(totalActivities: number, blockID: string): NonBunkJamboreeActivityWithAssignments[] { +function generateNonBunkJamboActivities(totalActivities: number, _blockID: string): NonBunkJamboreeActivityWithAssignments[] {
727-729: Remove unusedcampersvariable ingenerateSession.
campersis unused and fails lint.🧹 Proposed fix
- const campers: CamperAttendeeID[] = generateCampers(TOTAL_CAMPERS, TOTAL_BUNKS);
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/features/scheduling/generation/SessionScheduler.ts`:
- Around line 95-159: The fixed windows (window1/window2 created via
getDateStringsInRange) can extend past the session end causing out‑of‑range
dates and window2.length === 0 which leads to division by zero and Infinity for
maxStaffOffPerDay; clamp each window to the session end (e.g., compute end-bound
when calling getDateStringsInRange or trim the returned arrays against the
session end/last valid date) and skip a pass when a window is empty (if
window1.length===0 or window2.length===0, do not run the corresponding
for-loop), and also guard before computing maxStaffOffPerDay to avoid dividing
by zero (only compute Math.ceil(employees.length / window.length) when
window.length>0); apply the same guards to the jamboree filtering that uses
isJamboreeISO and to calls to assignOneDayOff so you never pass empty date
arrays.
♻️ Duplicate comments (2)
src/features/scheduling/generation/SessionScheduler.ts (2)
62-76: Always enforce the bunk constraint—even when using an Infinity cap.Right now the bunk check is skipped when
maxStaffOffPerDay === Infinity, which can schedule multiple counselors from the same bunk off on the same day and break coverage. This is the same risk previously flagged and still present.✅ Proposed fix
- if ('bunk' in employee && maxStaffOffPerDay !== Infinity) { + if ('bunk' in employee) { const bunkConflict = allEmployees.some(e => 'bunk' in e && e.bunk === employee.bunk && e.daysOff.includes(date) ); if (bunkConflict) continue; }
201-224: Fix on‑duty split off‑by‑one.
length + 1 < PER_SPLITunder‑allocates on‑duty staff. For example, whenPER_SPLITis 1, no one can ever be placed on duty. This was previously flagged and is still present.✅ Proposed fix
- if (!hasAdjacentDayOff && shift[bunkNumber].counselorsOnDuty.length + 1 < PER_SPLIT ) { + if (!hasAdjacentDayOff && shift[bunkNumber].counselorsOnDuty.length < PER_SPLIT) { shift[bunkNumber].counselorsOnDuty.push(employee.id); selected.push(employee); }
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/Navbar.tsx`:
- Around line 68-70: The Link component pointing to "/profile" contains only the
MdAccountCircle icon with no accessible text or label. Add an aria-label
attribute to the Link element to provide a descriptive accessible name for
screen readers, such as "Profile" or "Go to profile", so users with assistive
technologies can understand the link's purpose.
- Around line 57-59: The Text component's className attribute on line 57 uses
the Tailwind class `font-Lato` with an uppercase L, which differs from the
correct lowercase usage `font-lato` elsewhere in the component (line 52). Since
Tailwind class names are case-sensitive, this causes the font style to not be
applied correctly. Change the className from `font-Lato` to `font-lato` in the
Text component to ensure consistency and proper styling.
In `@src/components/NightScheduleTable.tsx`:
- Around line 171-173: The bunks array at line 171-173 is being built using
map() which creates sequential indices (0..n), but the code at lines 207-211
reads this array using actual bunk numbers as indices (e.g.,
row.bunks[bunkNum]). This mismatch causes undefined values when bunk numbers are
not sequential. Instead of using map() to create an array with sequential
indices, restructure the bunks construction to use actual bunk numbers as keys,
so that when accessed at lines 207-211 by bunkNum, the correct staff member is
retrieved for each bunk column.
In `@src/data/firestore/freeplays.ts`:
- Around line 3-8: Update import statements to use the `@/`* alias for all
internal src/ imports to comply with project conventions. In
src/data/firestore/freeplays.ts (lines 3-8), replace all relative imports
starting with ./ (such as ./types/documents, ./types/collections,
./types/queries, and ./firestoreClientOperations) with their `@/data/firestore/`
equivalents (for example, `@/data/firestore/types/documents`). Apply the same
transformation to src/data/firestore/nightSchedules.ts (lines 3-19), converting
all relative imports of types and firestoreClientOperations to use the
`@/data/firestore/` path alias instead.
In `@src/features/scheduling/generation/generateDaysOffSchedule.ts`:
- Line 5: Replace `moment` with `dayjs` across all four affected files to
consolidate on the standard date library and reduce bundle size. In
`src/features/scheduling/generation/generateDaysOffSchedule.ts` (line 5),
replace the `Moment` import with a `Dayjs` import and update the
`daysOffInSession` parameter type in `GenerateDaysOffScheduleRequest` to use
`Dayjs` instead of `Moment`. In `src/data/firestore/freeplays.ts` (line 7),
replace the `Moment` import with `Dayjs` and update the
`moment(snapshot.ref.id).startOf('day')` call to use `dayjs()` instead, also
updating any parameter types that use `Moment` to use `Dayjs`. In
`src/data/firestore/nightSchedules.ts` (line 18), perform the same replacements:
swap the `Moment` import for `Dayjs`, convert
`moment(snapshot.ref.id).startOf('day')` to use `dayjs()`, and update parameter
types from `Moment` to `Dayjs`. In `src/components/NightScheduleTable.tsx` (line
18), replace the `Moment` type import with `Dayjs` and convert any `moment()`
function calls to `dayjs()`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1bd717be-2745-4774-b15a-a0cbddcf7763
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
functions/src/features/accountManagement.tssrc/app/LoginPage.tsxsrc/components/Navbar.tsxsrc/components/NightScheduleTable.tsxsrc/data/firestore/freeplays.tssrc/data/firestore/nightSchedules.tssrc/features/scheduling/generation/SessionScheduler.tssrc/features/scheduling/generation/generateDaysOffSchedule.tssrc/features/scheduling/generation/generateFreeplaySchedule.ts
💤 Files with no reviewable changes (1)
- src/features/scheduling/generation/SessionScheduler.ts
✅ Files skipped from review due to trivial changes (3)
- src/features/scheduling/generation/generateFreeplaySchedule.ts
- functions/src/features/accountManagement.ts
- src/app/LoginPage.tsx
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/Navbar.tsx`:
- Around line 68-70: The Link component pointing to "/profile" contains only the
MdAccountCircle icon with no accessible text or label. Add an aria-label
attribute to the Link element to provide a descriptive accessible name for
screen readers, such as "Profile" or "Go to profile", so users with assistive
technologies can understand the link's purpose.
- Around line 57-59: The Text component's className attribute on line 57 uses
the Tailwind class `font-Lato` with an uppercase L, which differs from the
correct lowercase usage `font-lato` elsewhere in the component (line 52). Since
Tailwind class names are case-sensitive, this causes the font style to not be
applied correctly. Change the className from `font-Lato` to `font-lato` in the
Text component to ensure consistency and proper styling.
In `@src/components/NightScheduleTable.tsx`:
- Around line 171-173: The bunks array at line 171-173 is being built using
map() which creates sequential indices (0..n), but the code at lines 207-211
reads this array using actual bunk numbers as indices (e.g.,
row.bunks[bunkNum]). This mismatch causes undefined values when bunk numbers are
not sequential. Instead of using map() to create an array with sequential
indices, restructure the bunks construction to use actual bunk numbers as keys,
so that when accessed at lines 207-211 by bunkNum, the correct staff member is
retrieved for each bunk column.
In `@src/data/firestore/freeplays.ts`:
- Around line 3-8: Update import statements to use the `@/`* alias for all
internal src/ imports to comply with project conventions. In
src/data/firestore/freeplays.ts (lines 3-8), replace all relative imports
starting with ./ (such as ./types/documents, ./types/collections,
./types/queries, and ./firestoreClientOperations) with their `@/data/firestore/`
equivalents (for example, `@/data/firestore/types/documents`). Apply the same
transformation to src/data/firestore/nightSchedules.ts (lines 3-19), converting
all relative imports of types and firestoreClientOperations to use the
`@/data/firestore/` path alias instead.
In `@src/features/scheduling/generation/generateDaysOffSchedule.ts`:
- Line 5: Replace `moment` with `dayjs` across all four affected files to
consolidate on the standard date library and reduce bundle size. In
`src/features/scheduling/generation/generateDaysOffSchedule.ts` (line 5),
replace the `Moment` import with a `Dayjs` import and update the
`daysOffInSession` parameter type in `GenerateDaysOffScheduleRequest` to use
`Dayjs` instead of `Moment`. In `src/data/firestore/freeplays.ts` (line 7),
replace the `Moment` import with `Dayjs` and update the
`moment(snapshot.ref.id).startOf('day')` call to use `dayjs()` instead, also
updating any parameter types that use `Moment` to use `Dayjs`. In
`src/data/firestore/nightSchedules.ts` (line 18), perform the same replacements:
swap the `Moment` import for `Dayjs`, convert
`moment(snapshot.ref.id).startOf('day')` to use `dayjs()`, and update parameter
types from `Moment` to `Dayjs`. In `src/components/NightScheduleTable.tsx` (line
18), replace the `Moment` type import with `Dayjs` and convert any `moment()`
function calls to `dayjs()`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1bd717be-2745-4774-b15a-a0cbddcf7763
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
functions/src/features/accountManagement.tssrc/app/LoginPage.tsxsrc/components/Navbar.tsxsrc/components/NightScheduleTable.tsxsrc/data/firestore/freeplays.tssrc/data/firestore/nightSchedules.tssrc/features/scheduling/generation/SessionScheduler.tssrc/features/scheduling/generation/generateDaysOffSchedule.tssrc/features/scheduling/generation/generateFreeplaySchedule.ts
💤 Files with no reviewable changes (1)
- src/features/scheduling/generation/SessionScheduler.ts
✅ Files skipped from review due to trivial changes (3)
- src/features/scheduling/generation/generateFreeplaySchedule.ts
- functions/src/features/accountManagement.ts
- src/app/LoginPage.tsx
🛑 Comments failed to post (5)
src/components/Navbar.tsx (2)
57-59:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix Tailwind font class casing for navbar text.
Line 57 uses
font-Lato, but elsewhere in this component the class isfont-lato(Line 52). Tailwind class names are case-sensitive, so this likely drops the intended font style.Suggested fix
- <Text className="cursor-pointer font-Lato font-bold"> + <Text className="cursor-pointer font-lato font-bold"> {item.name} </Text>📝 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.<Text className="cursor-pointer font-lato font-bold"> {item.name} </Text>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Navbar.tsx` around lines 57 - 59, The Text component's className attribute on line 57 uses the Tailwind class `font-Lato` with an uppercase L, which differs from the correct lowercase usage `font-lato` elsewhere in the component (line 52). Since Tailwind class names are case-sensitive, this causes the font style to not be applied correctly. Change the className from `font-Lato` to `font-lato` in the Text component to ensure consistency and proper styling.
68-70:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd an accessible name to the icon-only profile link.
Line 69 renders only an icon inside the link, so screen readers may announce an unlabeled link.
Suggested fix
- <Link href="/profile"> + <Link href="/profile" aria-label="Profile"> <MdAccountCircle size={50} color="gray" /> </Link>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Navbar.tsx` around lines 68 - 70, The Link component pointing to "/profile" contains only the MdAccountCircle icon with no accessible text or label. Add an aria-label attribute to the Link element to provide a descriptive accessible name for screen readers, such as "Profile" or "Go to profile", so users with assistive technologies can understand the link's purpose.src/components/NightScheduleTable.tsx (1)
171-173:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winBunk-to-staff mapping is indexed incorrectly.
At Line 171,
bunksis built withmap()(array indices 0..n), but at Line 207 it is read by actual bunk numbers (row.bunks[bunkNum]). This misaligns staff with bunk columns and can returnundefinedfor valid bunks.Suggested fix
- const row: NightScheduleTableRow = { + const row: NightScheduleTableRow = { date: nightShift.date, position: position, - bunks: bunkNums.map((bunkNum) => - getStaffForPosition(nightShift, bunkNum, position), - ), + bunks: Object.fromEntries( + bunkNums.map((bunkNum) => [ + bunkNum, + getStaffForPosition(nightShift, bunkNum, position), + ]), + ) as Record<number, StaffAttendee[]>, };Also applies to: 207-211
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/NightScheduleTable.tsx` around lines 171 - 173, The bunks array at line 171-173 is being built using map() which creates sequential indices (0..n), but the code at lines 207-211 reads this array using actual bunk numbers as indices (e.g., row.bunks[bunkNum]). This mismatch causes undefined values when bunk numbers are not sequential. Instead of using map() to create an array with sequential indices, restructure the bunks construction to use actual bunk numbers as keys, so that when accessed at lines 207-211 by bunkNum, the correct staff member is retrieved for each bunk column.src/data/firestore/freeplays.ts (1)
3-8:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
@/*alias for internalsrc/imports in Firestore modules.Both files use relative imports for modules that live under
src/, which breaks the project’s import convention and risks lint/config drift.
src/data/firestore/freeplays.ts#L3-L8: convert./types/*and./firestoreClientOperationsimports to@/data/firestore/....src/data/firestore/nightSchedules.ts#L3-L19: convert./types/*and./firestoreClientOperationsimports to@/data/firestore/....As per coding guidelines, "Use the
@/*alias for all imports undersrc/... TypeScript configuration uses the@/*path alias... All imports must use this alias."📍 Affects 2 files
src/data/firestore/freeplays.ts#L3-L8(this comment)src/data/firestore/nightSchedules.ts#L3-L19🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/data/firestore/freeplays.ts` around lines 3 - 8, Update import statements to use the `@/`* alias for all internal src/ imports to comply with project conventions. In src/data/firestore/freeplays.ts (lines 3-8), replace all relative imports starting with ./ (such as ./types/documents, ./types/collections, ./types/queries, and ./firestoreClientOperations) with their `@/data/firestore/` equivalents (for example, `@/data/firestore/types/documents`). Apply the same transformation to src/data/firestore/nightSchedules.ts (lines 3-19), converting all relative imports of types and firestoreClientOperations to use the `@/data/firestore/` path alias instead.Source: Coding guidelines
src/features/scheduling/generation/generateDaysOffSchedule.ts (1)
5-5:
⚠️ Potential issue | 🟠 Major🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Verify date-library consistency in src/ rg -n --type=ts --type=tsx 'from ["'\'']moment["'\'']|\\bmoment\\(' src rg -n --type=ts --type=tsx 'from ["'\'']dayjs["'\'']|\\bdayjs\\(' srcRepository: Hack4Impact-UMD/camp-starfish
Length of output: 299
🏁 Script executed:
#!/bin/bash # Search for moment imports echo "=== Moment imports ===" rg -n "from ['\"]moment['\"]" src --type ts --type tsx echo -e "\n=== Dayjs imports ===" rg -n "from ['\"]dayjs['\"]" src --type ts --type tsx echo -e "\n=== Direct moment() calls ===" rg -n "\bmoment\(" src --type ts --type tsx | head -20 echo -e "\n=== Direct dayjs() calls ===" rg -n "\bdayjs\(" src --type ts --type tsx | head -20Repository: Hack4Impact-UMD/camp-starfish
Length of output: 310
🏁 Script executed:
#!/bin/bash # Search without file type filters echo "=== Moment imports in src ===" rg "from ['\"]moment['\"]" src 2>/dev/null || echo "No matches" echo -e "\n=== Dayjs imports in src ===" rg "from ['\"]dayjs['\"]" src 2>/dev/null || echo "No matches" echo -e "\n=== Check the 4 specific files ===" echo "File 1: src/features/scheduling/generation/generateDaysOffSchedule.ts" head -20 src/features/scheduling/generation/generateDaysOffSchedule.ts echo -e "\nFile 2: src/data/firestore/freeplays.ts" head -30 src/data/firestore/freeplays.ts echo -e "\nFile 3: src/data/firestore/nightSchedules.ts" head -30 src/data/firestore/nightSchedules.ts echo -e "\nFile 4: src/components/NightScheduleTable.tsx" head -30 src/components/NightScheduleTable.tsxRepository: Hack4Impact-UMD/camp-starfish
Length of output: 8537
Replace Moment usage with dayjs across the scheduling path.
This PR adds
momentusage to the scheduling generation and data modules, which conflicts with the codebase's targeted date-library standard. Per coding guidelines, the codebase should "Consolidate ondayjsfor date manipulation instead of using bothdayjsandmoment. Removemomentdependencies to reduce bundle size."
src/features/scheduling/generation/generateDaysOffSchedule.ts: ReplaceMomenttype withDayjsfor thedaysOffInSessionparameter inGenerateDaysOffScheduleRequest.src/data/firestore/freeplays.ts: Replacemoment(snapshot.ref.id).startOf('day')andMomentparameter types withdayjsequivalents.src/data/firestore/nightSchedules.ts: Replacemoment(snapshot.ref.id).startOf('day')andMomentparameter types withdayjsequivalents.src/components/NightScheduleTable.tsx: ReplaceMomenttype imports and anymoment()calls withdayjs.📍 Affects 4 files
src/features/scheduling/generation/generateDaysOffSchedule.ts#L5-L5(this comment)src/data/firestore/freeplays.ts#L7-L7src/data/firestore/nightSchedules.ts#L18-L18src/components/NightScheduleTable.tsx#L18-L18🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/scheduling/generation/generateDaysOffSchedule.ts` at line 5, Replace `moment` with `dayjs` across all four affected files to consolidate on the standard date library and reduce bundle size. In `src/features/scheduling/generation/generateDaysOffSchedule.ts` (line 5), replace the `Moment` import with a `Dayjs` import and update the `daysOffInSession` parameter type in `GenerateDaysOffScheduleRequest` to use `Dayjs` instead of `Moment`. In `src/data/firestore/freeplays.ts` (line 7), replace the `Moment` import with `Dayjs` and update the `moment(snapshot.ref.id).startOf('day')` call to use `dayjs()` instead, also updating any parameter types that use `Moment` to use `Dayjs`. In `src/data/firestore/nightSchedules.ts` (line 18), perform the same replacements: swap the `Moment` import for `Dayjs`, convert `moment(snapshot.ref.id).startOf('day')` to use `dayjs()`, and update parameter types from `Moment` to `Dayjs`. In `src/components/NightScheduleTable.tsx` (line 18), replace the `Moment` type import with `Dayjs` and convert any `moment()` function calls to `dayjs()`.Source: Coding guidelines
3 participants
Created the initial scheduling algorithm to assign days off and nights off for employees, and added extra fields to the StaffSessionAttendee type to store nights off and days off.
Summary by CodeRabbit
New Features
Refactor
Style