Freeplay alg#37
Conversation
📝 WalkthroughWalkthroughThe FreeplayScheduler class has been substantially expanded to support staff and admin assignment tracking, post-based scheduling, and multi-step camper allocation logic with gender-based grouping and conflict detection across multiple assignment fallback strategies. Changes
Sequence Diagram(s)sequenceDiagram
participant Scheduler as FreeplayScheduler
participant Staff as Staff/Admin Pool
participant Campers as Camper Queue
participant FirstStep as First Step<br/>(New Bunk)
participant SecondStep as Second Step<br/>(Existing Bunk)
participant ThirdStep as Third Step<br/>(Final Fallback)
Scheduler->>Staff: Filter available (daysOff)
Staff-->>Scheduler: Available staff/admins
Scheduler->>Campers: Split by gender
rect rgba(100, 150, 200, 0.5)
Note over Campers,ThirdStep: For each gender group
Campers->>FirstStep: Try to place camper<br/>with new staffer
FirstStep-->>Scheduler: Success? Return true
end
rect rgba(200, 150, 100, 0.5)
Note over Campers,ThirdStep: If first step fails
Campers->>SecondStep: Try to place camper<br/>with existing bunk
SecondStep-->>Scheduler: Success? Return true
end
rect rgba(150, 100, 200, 0.5)
Note over Campers,ThirdStep: If second step fails
Campers->>ThirdStep: Place camper<br/>with any staffer<br/>(no conflicts)
ThirdStep-->>Scheduler: Success? Return true
end
Scheduler->>Scheduler: Log unassigned campers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/features/scheduling/FreeplayScheduler.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/features/scheduling/FreeplayScheduler.ts (1)
src/types/sessionTypes.ts (5)
CamperAttendeeID(24-24)StaffAttendeeID(33-33)AdminAttendeeID(39-39)PostID(135-135)Freeplay(125-128)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_preview
eshavignesh123
left a comment
eshavignesh123
left a comment
There was a problem hiding this comment.
Hi Varun! This looks good for the most part! Just some minor edits
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/features/scheduling/FreeplayScheduler.ts (3)
115-122: Remove the commented-out line.The active implementation correctly places
assignedStaffbeforeassignedAdminas requested in feedback. Please remove the commented line 117 to keep the code clean.Apply this diff:
const allAssignedStaffers = [...this.assignedStaff, ...this.assignedAdmin]; - // const allAssignedStaffers = [...this.assignedStaff];
153-171: Consider adding buddy conflict check in the fallback logic.The fallback assignment doesn't verify whether the camper was previously paired with the staffer in
otherFreeplayBuddies, which could result in repeated pairings. Based on previous feedback, consider adding this check to further avoid buddy repetition.Apply this diff to add the conflict check:
if (!assigned) { for (const staffer of allAssignedStaffers) { if (staffer.gender !== "Female") continue; const alreadyAssigned = this.schedule.buddies[staffer.id] || []; + const prevBuddies = this.otherFreeplayBuddies[staffer.id] || []; + const hasConflict = prevBuddies.includes(camper.id); + + if (hasConflict) continue; + if (alreadyAssigned.length == 1) { const otherCamper = this.getCamperById(alreadyAssigned[0]) || camper; if (otherCamper.bunk == camper.bunk && otherCamper.id !== camper.id) { this.schedule.buddies[staffer.id].push(camper.id); assigned = true; break; } } } }Based on learnings
199-216: Consider adding buddy conflict check in the male fallback logic.Similar to the female fallback, this logic doesn't verify
otherFreeplayBuddies, which could result in repeated pairings. Consider adding the conflict check here as well.Apply this diff to add the conflict check:
if (!assigned) { for (const staffer of allAssignedStaffers) { const alreadyAssigned = this.schedule.buddies[staffer.id] || []; + const prevBuddies = this.otherFreeplayBuddies[staffer.id] || []; + const hasConflict = prevBuddies.includes(camper.id); + + if (hasConflict) continue; + if (alreadyAssigned.length == 1) { const otherCamper = this.getCamperById(alreadyAssigned[0]) || camper; if (otherCamper.bunk == camper.bunk && otherCamper.id !== camper.id) { this.schedule.buddies[staffer.id].push(camper.id); assigned = true; break; } } } }Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/features/scheduling/FreeplayScheduler.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/features/scheduling/FreeplayScheduler.ts (1)
src/types/sessionTypes.ts (4)
StaffAttendeeID(33-33)AdminAttendeeID(39-39)PostID(135-135)Freeplay(125-128)
🔇 Additional comments (12)
src/features/scheduling/FreeplayScheduler.ts (12)
1-1: LGTM!The addition of
PostIDto the imports is necessary for the newpostsfield and related functionality.
12-15: LGTM!The new tracking fields (
assignedStaff,assignedAdmin,posts) are correctly typed and necessary for the assignment logic.
20-21: LGTM!The constructor is correctly left blank as requested, and the comment helpfully explains the purpose of the
postsfield.
31-31: LGTM!The
withPostsmethod correctly follows the builder pattern and was requested in previous feedback.
33-35: LGTM!The helper methods correctly use
find()and will returnundefinedif no match is found, which is properly handled by callers.
64-71: LGTM!The filtering logic correctly excludes already-assigned staff and admins, addressing previous feedback. The use of filtered copies ensures the original arrays remain unchanged.
72-83: LGTM!The admin-required post assignment logic is correct and now properly records admins in
assignedAdmin(line 80), addressing the previous review feedback.
85-99: LGTM!The general post assignment correctly prioritizes admins over staff and properly tracks assignments in both
assignedAdminandassignedStaff.
100-102: LGTM!The method correctly returns
thisfor builder pattern chaining.
124-151: LGTM!The female camper assignment logic correctly breaks out of the inner loop after assignment (lines 143, 148), addressing previous feedback. The gender matching and buddy conflict checks are properly implemented.
217-219: LGTM!The method correctly returns
thisfor builder pattern chaining.
173-197: Add break statements after male camper assignment.After setting
assigned = trueand assigning the camper (lines 190, 194), the loop continues iterating, potentially assigning the same camper to multiple staffers. This is the same issue previously flagged for the female assignment loop.Apply this diff to add the missing breaks:
if (!hasConflict && alreadyAssigned.length == 0) { if (staffer.role === "STAFF") { if (staffer.bunk !== camper.bunk) { assigned = true; this.schedule.buddies[staffer.id] = [camper.id]; + break; } } else { assigned = true; this.schedule.buddies[staffer.id] = [camper.id]; + break; } }
nkanchinadam
left a comment
nkanchinadam
left a comment
There was a problem hiding this comment.
Overall, really good! Just left a couple comments noting on stuff that could be simplified/optimized, but other than that, I think it hits the major points that were needed for scheduling Freeplays.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/features/scheduling/FreeplayScheduler.ts (4)
48-51: Consider using Sets for efficient duplicate checking.The current implementation uses
includes()for duplicate checking, which has O(n) complexity for each check. As suggested in previous feedback, using Sets would provide O(1) lookups and improve performance when merging large buddy lists.Here's an optimized approach using Sets:
withOtherFreeplays(otherFreeplays: Freeplay[]): FreeplayScheduler { + // Use Sets for efficient deduplication + const buddySets: Map<number, Set<number>> = new Map(); + for (const freeplay of otherFreeplays) { for (const buddieIDStr in freeplay.buddies) { const buddieID = Number(buddieIDStr); - if (buddieID in this.otherFreeplayBuddies) { - - const attendees = freeplay.buddies[buddieID]; - - // add all attendees that don't already exist - for (const att of attendees) { - if (!this.otherFreeplayBuddies[buddieID].includes(att)) { - this.otherFreeplayBuddies[buddieID].push(att); - } - } - } else { - const attendees = freeplay.buddies[buddieID]; - this.otherFreeplayBuddies[buddieID] = attendees; + + if (!buddySets.has(buddieID)) { + buddySets.set(buddieID, new Set()); } + + const attendees = freeplay.buddies[buddieID]; + for (const att of attendees) { + buddySets.get(buddieID)!.add(att); + } } } + + // Convert Sets back to arrays + for (const [buddieID, attendeeSet] of buddySets) { + this.otherFreeplayBuddies[buddieID] = Array.from(attendeeSet); + } return this; }Based on learnings
117-119: Consider caching gender-split staff at class level.As suggested in previous feedback, maintaining gender-split staff as class-level variables would eliminate redundant filtering on every call to
assignCampers(). This would improve performance ifassignCampers()is called multiple times.Example approach:
// Add to class fields private femaleStaff: StaffAttendeeID[] = []; private femaleAdmins: AdminAttendeeID[] = []; // Update in withStaff/withAdmins withStaff(staff: StaffAttendeeID[]): FreeplayScheduler { this.staff = staff; this.femaleStaff = staff.filter(s => s.gender === "Female"); return this; } // Then use directly const allAssignedFemaleStaffers = [ ...this.assignedStaff.filter(s => this.femaleStaff.some(f => f.id === s.id)), ...this.assignedAdmin.filter(a => this.femaleAdmins.some(f => f.id === a.id)) ];Based on learnings
148-148: Add JSDoc comments for helper methods.The helper methods
assignToOpenStaffFirstStepandassignToOpenStaffSecondStepimplement complex assignment logic but lack documentation. Consider adding JSDoc comments to explain:
- The purpose of each step
- The buddy conflict checking mechanism
- The bunk matching constraints
- Return values and side effects
Example:
+ /** + * First assignment step: Attempts to assign a camper to a staffer who: + * - Has no campers assigned yet + * - Has no previous buddy conflict with this camper + * - Is not from the same bunk (for STAFF role only) + * + * @returns true if assignment was successful, false otherwise + */ assignToOpenStaffFirstStep(allAssignedStaffers: (StaffAttendeeID | AdminAttendeeID)[], camper: CamperAttendeeID) {
1-1: Remove unused importThe
Staffimport from@/types/personTypesis unused inFreeplayScheduler.ts. Remove it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/features/scheduling/FreeplayScheduler.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/features/scheduling/FreeplayScheduler.ts (1)
src/types/sessionTypes.ts (5)
StaffAttendeeID(33-33)AdminAttendeeID(39-39)PostID(135-135)Freeplay(125-128)CamperAttendeeID(24-24)
🔇 Additional comments (3)
src/features/scheduling/FreeplayScheduler.ts (3)
64-104: LGTM! Post assignment logic is correct.The implementation properly:
- Filters available staff/admins without mutating the original lists
- Prioritizes admins for admin-required posts (lines 75-85)
- Records all assignments in
assignedAdminandassignedStaff(lines 82, 94, 98)- Falls back to staff for non-admin posts when admins are exhausted
The critical issue from previous feedback about recording admin assignments has been correctly addressed.
161-161: Document the bunk exclusion logic for staff.The condition
(staffer.role === "STAFF" && staffer.bunk !== camper.bunk) || staffer.role !== "STAFF"prevents STAFF members from being assigned to campers from their own bunk, while allowing ADMINs to supervise any bunk. This business rule is not documented and may be unclear to future maintainers.Please verify this is the intended behavior and add a comment explaining why staff cannot supervise their own bunk during freeplay. For example:
if (!hasConflict && alreadyAssigned.length == 0) { + // Staff members cannot supervise their own bunk during freeplay + // Admins (who don't have assigned bunks) can supervise any campers if ( (staffer.role === "STAFF" && staffer.bunk !== camper.bunk) || staffer.role !== "STAFF" ) {
178-179: Verify the fallback logic for missing campers.Line 178 uses
|| camperas a fallback ifgetCamperByIdreturns undefined, but this would cause the same-bunk check to comparecamperwith itself. The subsequent checkotherCamper.id !== camper.idwould then always fail, silently skipping the assignment.If a camper ID is in
this.schedule.buddiesbut not found viagetCamperById, it indicates a data inconsistency that should probably be handled explicitly:- const otherCamper = this.getCamperById(alreadyAssigned[0]) || camper; + const otherCamper = this.getCamperById(alreadyAssigned[0]); + if (!otherCamper) continue; // Skip if camper not found if (otherCamper.bunk == camper.bunk && otherCamper.id !== camper.id) {
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: 5
🤖 Fix all issues with AI agents
In `@src/features/scheduling/generation/FreeplayScheduler.ts`:
- Around line 34-36: getPostByID currently finds posts by p.name which is wrong
for schedule keys — match PostID.id instead; update the getPostByID
implementation to compare the incoming id to the post's id field (e.g.,
this.posts.find(p => p.id === id)), ensure the parameter type aligns with
PostID.id, and leave posts/requiredAdmin checks intact so requiresAdmin-based
assignments work correctly.
- Around line 126-141: Change the mutable declarations for the loop-local
variable "assigned" to immutable "const" in FreeplayScheduler.ts: in the
femaleCampers loop (where you call
assignToOpenStaffFirstStep(allAssignedFemaleStaffers, camper)) and in the
maleCampers loop (where you call assignToOpenStaffFirstStep(allAssignedStaffers,
camper)); both "assigned" variables are never reassigned so replace "let
assigned" with "const assigned" to satisfy ESLint prefer-const while leaving the
fallback calls to assignToOpenStaffSecondStep unchanged.
- Around line 87-99: The loop in FreeplayScheduler that fills empty
this.schedule.posts currently falls back to assigning staff to any empty post,
which can assign STAFF into admin-only posts; update the assignment logic in the
post-filling loop inside FreeplayScheduler to check each post's requiresAdmin
flag (or equivalent metadata) before assigning from availableStaff: if the
post.requiresAdmin is true and availableAdmins is empty, skip assigning staff
(leave unassigned or handle via existing unfilled-post flow); ensure you still
assign admins when available (using availableAdmins.shift() and
this.assignedAdmin.push) and only use availableStaff for posts where
requiresAdmin is false.
- Line 1: The import of the Staff type in FreeplayScheduler.ts is unused and
causing an ESLint lint failure; remove the unused import line "import { Staff }
from "@/types/personTypes"; from the top of FreeplayScheduler.ts (or
alternatively use the Staff type where appropriate), ensuring no other
references to Staff remain so CI lint passes.
- Around line 45-56: The code mutates arrays from freeplay.buddies by assigning
references and then pushing into them; fix by cloning buddy arrays before
storing or extending them: when setting this.otherFreeplayBuddies[buddieID] in
the else branch assign a shallow copy of freeplay.buddies[buddieID] (e.g.,
create a new array from attendees) and in the branch where you extend the
existing this.otherFreeplayBuddies[buddieID] ensure you are comparing against
the cloned target and only push items into that cloned array (do not push into
the original freeplay.buddies array); reference symbols: FreeplayScheduler,
this.otherFreeplayBuddies, freeplay.buddies, buddieID, attendees.
| getCamperById = (id: number) => this.campers.find(c => c.id === id); | ||
|
|
||
| getPostByID = (id: string) => this.posts.find(p => p.name === id); |
There was a problem hiding this comment.
Post lookup should match PostID.id, not name.
Schedule keys are post IDs; using name risks missing requiresAdmin and mis-assigning staff.
🔧 Suggested fix
- getPostByID = (id: string) => this.posts.find(p => p.name === id);
+ getPostByID = (id: string) => this.posts.find(p => p.id === id);📝 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.
| getCamperById = (id: number) => this.campers.find(c => c.id === id); | |
| getPostByID = (id: string) => this.posts.find(p => p.name === id); | |
| getCamperById = (id: number) => this.campers.find(c => c.id === id); | |
| getPostByID = (id: string) => this.posts.find(p => p.id === id); |
🤖 Prompt for AI Agents
In `@src/features/scheduling/generation/FreeplayScheduler.ts` around lines 34 -
36, getPostByID currently finds posts by p.name which is wrong for schedule keys
— match PostID.id instead; update the getPostByID implementation to compare the
incoming id to the post's id field (e.g., this.posts.find(p => p.id === id)),
ensure the parameter type aligns with PostID.id, and leave posts/requiredAdmin
checks intact so requiresAdmin-based assignments work correctly.
| const attendees = freeplay.buddies[buddieID]; | ||
|
|
||
| // add all attendees that don't already exist | ||
| for (const att of attendees) { | ||
| if (!this.otherFreeplayBuddies[buddieID].includes(att)) { | ||
| this.otherFreeplayBuddies[buddieID].push(att); | ||
| } | ||
| } | ||
| } else { | ||
| const attendees = freeplay.buddies[buddieID]; | ||
| this.otherFreeplayBuddies[buddieID] = attendees; | ||
| } |
There was a problem hiding this comment.
Avoid mutating input freeplay buddy arrays.
Assigning and then pushing into arrays sourced from freeplay.buddies can mutate the input objects. Clone them first.
🧹 Suggested fix
- const attendees = freeplay.buddies[buddieID];
+ const attendees = [...freeplay.buddies[buddieID]];
// add all attendees that don't already exist
for (const att of attendees) {
if (!this.otherFreeplayBuddies[buddieID].includes(att)) {
this.otherFreeplayBuddies[buddieID].push(att);
}
}
} else {
- const attendees = freeplay.buddies[buddieID];
- this.otherFreeplayBuddies[buddieID] = attendees;
+ const attendees = [...freeplay.buddies[buddieID]];
+ this.otherFreeplayBuddies[buddieID] = attendees;
}📝 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.
| const attendees = freeplay.buddies[buddieID]; | |
| // add all attendees that don't already exist | |
| for (const att of attendees) { | |
| if (!this.otherFreeplayBuddies[buddieID].includes(att)) { | |
| this.otherFreeplayBuddies[buddieID].push(att); | |
| } | |
| } | |
| } else { | |
| const attendees = freeplay.buddies[buddieID]; | |
| this.otherFreeplayBuddies[buddieID] = attendees; | |
| } | |
| const attendees = [...freeplay.buddies[buddieID]]; | |
| // add all attendees that don't already exist | |
| for (const att of attendees) { | |
| if (!this.otherFreeplayBuddies[buddieID].includes(att)) { | |
| this.otherFreeplayBuddies[buddieID].push(att); | |
| } | |
| } | |
| } else { | |
| const attendees = [...freeplay.buddies[buddieID]]; | |
| this.otherFreeplayBuddies[buddieID] = attendees; | |
| } |
🤖 Prompt for AI Agents
In `@src/features/scheduling/generation/FreeplayScheduler.ts` around lines 45 -
56, The code mutates arrays from freeplay.buddies by assigning references and
then pushing into them; fix by cloning buddy arrays before storing or extending
them: when setting this.otherFreeplayBuddies[buddieID] in the else branch assign
a shallow copy of freeplay.buddies[buddieID] (e.g., create a new array from
attendees) and in the branch where you extend the existing
this.otherFreeplayBuddies[buddieID] ensure you are comparing against the cloned
target and only push items into that cloned array (do not push into the original
freeplay.buddies array); reference symbols: FreeplayScheduler,
this.otherFreeplayBuddies, freeplay.buddies, buddieID, attendees.
| // assigns all other roles (not Admin-specific) to admins first, then staff | ||
| for (const postID in this.schedule.posts) { | ||
| const assigned = this.schedule.posts[postID]; | ||
| if (assigned.length == 0) { | ||
| if (availableAdmins.length > 0) { | ||
| const adminID: AdminAttendeeID = availableAdmins.shift()!; | ||
| this.schedule.posts[postID] = [adminID.id]; | ||
| this.assignedAdmin.push(adminID); | ||
| } else if (availableStaff.length > 0) { | ||
| const staffID: StaffAttendeeID = availableStaff.shift()!; | ||
| this.schedule.posts[postID] = [staffID.id]; | ||
| this.assignedStaff.push(staffID); | ||
| } |
There was a problem hiding this comment.
Don’t assign STAFF to admin-only posts in the second pass.
The second loop assigns staff to any empty post, including requiresAdmin posts when admins are exhausted. That violates the admin-only constraint.
🛠️ Suggested fix
- for (const postID in this.schedule.posts) {
- const assigned = this.schedule.posts[postID];
- if (assigned.length == 0) {
- if (availableAdmins.length > 0) {
- const adminID: AdminAttendeeID = availableAdmins.shift()!;
- this.schedule.posts[postID] = [adminID.id];
- this.assignedAdmin.push(adminID);
- } else if (availableStaff.length > 0) {
- const staffID: StaffAttendeeID = availableStaff.shift()!;
- this.schedule.posts[postID] = [staffID.id];
- this.assignedStaff.push(staffID);
- }
- }
- }
+ for (const postID in this.schedule.posts) {
+ const assigned = this.schedule.posts[postID];
+ if (assigned.length == 0) {
+ const post = this.getPostByID(postID);
+ if (post?.requiresAdmin) {
+ if (availableAdmins.length > 0) {
+ const adminID: AdminAttendeeID = availableAdmins.shift()!;
+ this.schedule.posts[postID] = [adminID.id];
+ this.assignedAdmin.push(adminID);
+ }
+ continue; // keep empty if no admins left
+ }
+ if (availableAdmins.length > 0) {
+ const adminID: AdminAttendeeID = availableAdmins.shift()!;
+ this.schedule.posts[postID] = [adminID.id];
+ this.assignedAdmin.push(adminID);
+ } else if (availableStaff.length > 0) {
+ const staffID: StaffAttendeeID = availableStaff.shift()!;
+ this.schedule.posts[postID] = [staffID.id];
+ this.assignedStaff.push(staffID);
+ }
+ }
+ }🤖 Prompt for AI Agents
In `@src/features/scheduling/generation/FreeplayScheduler.ts` around lines 87 -
99, The loop in FreeplayScheduler that fills empty this.schedule.posts currently
falls back to assigning staff to any empty post, which can assign STAFF into
admin-only posts; update the assignment logic in the post-filling loop inside
FreeplayScheduler to check each post's requiresAdmin flag (or equivalent
metadata) before assigning from availableStaff: if the post.requiresAdmin is
true and availableAdmins is empty, skip assigning staff (leave unassigned or
handle via existing unfilled-post flow); ensure you still assign admins when
available (using availableAdmins.shift() and this.assignedAdmin.push) and only
use availableStaff for posts where requiresAdmin is false.
| for (const camper of femaleCampers) { | ||
| let assigned = this.assignToOpenStaffFirstStep(allAssignedFemaleStaffers, camper); | ||
|
|
||
| // Fallback: assign to any female staffer with another camper of the same bunk | ||
| if (!assigned) { | ||
| this.assignToOpenStaffSecondStep(allAssignedFemaleStaffers, camper); | ||
| } | ||
| } | ||
|
|
||
| // 3. Assign male campers | ||
| for (const camper of maleCampers) { | ||
| let assigned = this.assignToOpenStaffFirstStep(allAssignedStaffers, camper); | ||
|
|
||
| // Fallback: assign to any staffer with another camper of the same bunk | ||
| if (!assigned) { | ||
| this.assignToOpenStaffSecondStep(allAssignedStaffers, camper); |
There was a problem hiding this comment.
Use const for assigned to fix lint errors.
Both assigned variables are never reassigned; ESLint prefer-const fails CI.
🧹 Suggested fix
- let assigned = this.assignToOpenStaffFirstStep(allAssignedFemaleStaffers, camper);
+ const assigned = this.assignToOpenStaffFirstStep(allAssignedFemaleStaffers, camper);
...
- let assigned = this.assignToOpenStaffFirstStep(allAssignedStaffers, camper);
+ const assigned = this.assignToOpenStaffFirstStep(allAssignedStaffers, camper);📝 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.
| for (const camper of femaleCampers) { | |
| let assigned = this.assignToOpenStaffFirstStep(allAssignedFemaleStaffers, camper); | |
| // Fallback: assign to any female staffer with another camper of the same bunk | |
| if (!assigned) { | |
| this.assignToOpenStaffSecondStep(allAssignedFemaleStaffers, camper); | |
| } | |
| } | |
| // 3. Assign male campers | |
| for (const camper of maleCampers) { | |
| let assigned = this.assignToOpenStaffFirstStep(allAssignedStaffers, camper); | |
| // Fallback: assign to any staffer with another camper of the same bunk | |
| if (!assigned) { | |
| this.assignToOpenStaffSecondStep(allAssignedStaffers, camper); | |
| for (const camper of femaleCampers) { | |
| const assigned = this.assignToOpenStaffFirstStep(allAssignedFemaleStaffers, camper); | |
| // Fallback: assign to any female staffer with another camper of the same bunk | |
| if (!assigned) { | |
| this.assignToOpenStaffSecondStep(allAssignedFemaleStaffers, camper); | |
| } | |
| } | |
| // 3. Assign male campers | |
| for (const camper of maleCampers) { | |
| const assigned = this.assignToOpenStaffFirstStep(allAssignedStaffers, camper); | |
| // Fallback: assign to any staffer with another camper of the same bunk | |
| if (!assigned) { | |
| this.assignToOpenStaffSecondStep(allAssignedStaffers, camper); |
🧰 Tools
🪛 GitHub Actions: Lint & Build on PR
[error] 127-127: ESLint: 'assigned' is never reassigned. Use 'const' instead. (prefer-const)
[error] 137-137: ESLint: 'assigned' is never reassigned. Use 'const' instead. (prefer-const)
🤖 Prompt for AI Agents
In `@src/features/scheduling/generation/FreeplayScheduler.ts` around lines 126 -
141, Change the mutable declarations for the loop-local variable "assigned" to
immutable "const" in FreeplayScheduler.ts: in the femaleCampers loop (where you
call assignToOpenStaffFirstStep(allAssignedFemaleStaffers, camper)) and in the
maleCampers loop (where you call assignToOpenStaffFirstStep(allAssignedStaffers,
camper)); both "assigned" variables are never reassigned so replace "let
assigned" with "const assigned" to satisfy ESLint prefer-const while leaving the
fallback calls to assignToOpenStaffSecondStep unchanged.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/features/scheduling/test/generateEmulatorData.tsx`:
- Around line 1-6: The file has many unused imports, helper functions,
parameters and locals (e.g., imports CamperAttendeeID, StaffAttendeeID,
AdminAttendeeID, AgeGroup, SectionSchedule, BundleActivityWithAssignments,
SectionPreferences, BunkID, BunkJamboreeActivityWithAssignments,
NonBunkJamboreeActivityWithAssignments, Freeplay, PostID, SectionID, SessionID,
NightShiftID, ProgramAreaID, SchedulingSectionID, FreeplayID and helpers
generateNonoLists, generateYesyesLists, generateBunks,
generateBundleBlockSchedule, generateBunkJamboBlockSchedule,
generateNonBunkJamboBlockSchedule, generateBlockIDs, generatePrefs, and
locals/params blockID, counselors, nightShifts) — either remove the unused
imports/helpers/params, wire them into generateSession where appropriate, or add
focused eslint-disable-next-line comments only on the specific unused symbols to
silence CI; update imports at top to match what generateSession actually
references and delete or export unused helper functions if intended for later
use.
- Around line 537-548: The generatePosts function currently uses
faker.word.noun() for PostID.id which can produce duplicate IDs; update
generatePosts to guarantee uniqueness by generating IDs with a
collision-resistant generator (e.g., faker.datatype.uuid() or append a
sequential/index suffix) when creating each PostID, and ensure the returned
PostID objects (id, name, requiresAdmin) use that unique id so the schedule map
won't be overwritten; references: function generatePosts and the PostID objects
created inside it.
♻️ Duplicate comments (3)
src/features/scheduling/generation/FreeplayScheduler.ts (3)
38-38: UsePostID.idfor lookup.
getPostByIDshould compare againstid, notname, or admin-required posts may be missed.🛠️ Suggested fix
- getPostByID = (id: string) => this.posts.find(p => p.name === id); + getPostByID = (id: string) => this.posts.find(p => p.id === id);
47-58: Avoid mutating sourcefreeplay.buddiesarrays.Clone before storing/pushing to prevent side effects across schedules.
🧹 Suggested fix
- const attendees = freeplay.buddies[buddieID]; + const attendees = [...freeplay.buddies[buddieID]]; ... - const attendees = freeplay.buddies[buddieID]; - this.otherFreeplayBuddies[buddieID] = attendees; + const attendees = [...freeplay.buddies[buddieID]]; + this.otherFreeplayBuddies[buddieID] = attendees;
95-107: Prevent STAFF assignment to admin-only posts in pass two.Second loop can still place staff into
requiresAdminposts once admins are exhausted.🛠️ Suggested fix
- for (const postID in this.schedule.posts) { - const assigned = this.schedule.posts[postID]; - if (assigned.length == 0) { - if (availableAdmins.length > 0) { - const adminID: AdminAttendeeID = availableAdmins.shift()!; - this.schedule.posts[postID] = [adminID.id]; - this.assignedAdmin.push(adminID); - } else if (availableStaff.length > 0) { - const staffID: StaffAttendeeID = availableStaff.shift()!; - this.schedule.posts[postID] = [staffID.id]; - this.assignedStaff.push(staffID); - } - } - } + for (const postID in this.schedule.posts) { + const assigned = this.schedule.posts[postID]; + if (assigned.length == 0) { + const post = this.getPostByID(postID); + if (post?.requiresAdmin) { + if (availableAdmins.length > 0) { + const adminID: AdminAttendeeID = availableAdmins.shift()!; + this.schedule.posts[postID] = [adminID.id]; + this.assignedAdmin.push(adminID); + } + continue; // leave unfilled if no admins are available + } + if (availableAdmins.length > 0) { + const adminID: AdminAttendeeID = availableAdmins.shift()!; + this.schedule.posts[postID] = [adminID.id]; + this.assignedAdmin.push(adminID); + } else if (availableStaff.length > 0) { + const staffID: StaffAttendeeID = availableStaff.shift()!; + this.schedule.posts[postID] = [staffID.id]; + this.assignedStaff.push(staffID); + } + } + }
🧹 Nitpick comments (2)
src/app/LoginPage.tsx (1)
37-45: Gate test-data generation behind a dev-only flag.This adds a production-visible test utility on the login screen; consider hiding it behind
NODE_ENVor a feature flag.💡 Suggested guard
- const [error, setError] = useState<string>(""); + const [error, setError] = useState<string>(""); + const isDev = process.env.NODE_ENV !== "production"; ... - <button - onClick={generateTest} - className="flex flex-row justify-around items-center w-5/6 max-w-[344px] bg-white - mt-5 py-4 px-12 rounded-full shadow-[0_4px_4px_-1px_rgba(0,0,0,0.2)] font-lato text-xl text-gray-600" - > - <Image src={MicrosoftIcon.src} alt="Microsoft" width={32} height={32} /> - Generate Test Data - </button> + {isDev && ( + <button + onClick={generateTest} + className="flex flex-row justify-around items-center w-5/6 max-w-[344px] bg-white + mt-5 py-4 px-12 rounded-full shadow-[0_4px_4px_-1px_rgba(0,0,0,0.2)] font-lato text-xl text-gray-600" + > + <Image src={MicrosoftIcon.src} alt="Microsoft" width={32} height={32} /> + Generate Test Data + </button> + )}Also applies to: 81-88
src/features/scheduling/test/generateEmulatorData.tsx (1)
346-351: Remove duplicatealternatePeriodsOff["RH"]assignment.It’s set twice in a row, which is redundant.
♻️ Simplify duplicate assignment
- schedule.alternatePeriodsOff["RH"] = [] - - schedule.alternatePeriodsOff["RH"] = [] + schedule.alternatePeriodsOff["RH"] = []
| function generatePosts(totalPosts: number): PostID[] { | ||
| const posts: PostID[] = []; | ||
|
|
||
| for (let i = 0; i < totalPosts; i++) { | ||
| const name = faker.word.noun(); | ||
| const requiresAdmin = faker.datatype.boolean(); | ||
|
|
||
| posts.push({ | ||
| name, | ||
| requiresAdmin, | ||
| id: name | ||
| }); |
There was a problem hiding this comment.
Ensure generated post IDs are unique.
faker.word.noun() can collide, which can overwrite posts in the schedule map.
✅ Safer IDs
- const name = faker.word.noun();
+ const name = faker.word.noun();
+ const id = `${name}-${i}`;
...
- posts.push({
- name,
- requiresAdmin,
- id: name
- });
+ posts.push({
+ name,
+ requiresAdmin,
+ id
+ });📝 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.
| function generatePosts(totalPosts: number): PostID[] { | |
| const posts: PostID[] = []; | |
| for (let i = 0; i < totalPosts; i++) { | |
| const name = faker.word.noun(); | |
| const requiresAdmin = faker.datatype.boolean(); | |
| posts.push({ | |
| name, | |
| requiresAdmin, | |
| id: name | |
| }); | |
| function generatePosts(totalPosts: number): PostID[] { | |
| const posts: PostID[] = []; | |
| for (let i = 0; i < totalPosts; i++) { | |
| const name = faker.word.noun(); | |
| const requiresAdmin = faker.datatype.boolean(); | |
| const id = `${name}-${i}`; | |
| posts.push({ | |
| name, | |
| requiresAdmin, | |
| id | |
| }); |
🤖 Prompt for AI Agents
In `@src/features/scheduling/test/generateEmulatorData.tsx` around lines 537 -
548, The generatePosts function currently uses faker.word.noun() for PostID.id
which can produce duplicate IDs; update generatePosts to guarantee uniqueness by
generating IDs with a collision-resistant generator (e.g., faker.datatype.uuid()
or append a sequential/index suffix) when creating each PostID, and ensure the
returned PostID objects (id, name, requiresAdmin) use that unique id so the
schedule map won't be overwritten; references: function generatePosts and the
PostID objects created inside it.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/features/scheduling/generation/FreeplayScheduler.ts`:
- Around line 198-208: The code currently only handles staffers with
staffer.role === "STAFF" and accesses staffer.bunk, so AdminAttendeeID (role
"ADMIN") entries are silently ignored; either restrict the function parameter
type to StaffAttendeeID[] to match the implementation, or add explicit ADMIN
handling: update the loop in FreeplayScheduler to branch on staffer.role (e.g.,
if "STAFF" keep the bunk != camper.bunk check, if "ADMIN" skip the bunk check or
apply the intended admin logic), ensure any use of staffer.bunk is guarded or
typed, and keep schedule.buddies[staffer.id] initialization/assignment logic for
both branches.
🧹 Nitpick comments (2)
src/features/scheduling/generation/FreeplayScheduler.ts (2)
78-79: Consider removing debug console.log statements.These debug logs (
"Available Staff: ","Available Admins: ") are useful during development but should be removed or converted to a proper logging framework with configurable levels before production.
136-137: Remove debug console.log statements.Similar to lines 78-79, these debug logs should be removed or converted to proper logging for production.
| if (!hasConflict && alreadyAssigned.length == 0) { | ||
| if ( (staffer.role === "STAFF" && staffer.bunk !== camper.bunk)) { | ||
|
|
||
| if (!this.schedule.buddies[staffer.id]) { | ||
| this.schedule.buddies[staffer.id] = []; | ||
| } | ||
| this.schedule.buddies[staffer.id].push(camper.id); | ||
| assigned = true; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the FreeplayScheduler.ts file and extract the function signature
fd -e ts -n FreeplayScheduler && head -230 src/features/scheduling/generation/FreeplayScheduler.ts | tail -50Repository: Hack4Impact-UMD/camp-starfish
Length of output: 303
🏁 Script executed:
# Search for AdminAttendee interface definition
rg -n "interface AdminAttendee" -A 10Repository: Hack4Impact-UMD/camp-starfish
Length of output: 748
🏁 Script executed:
# Check if AdminAttendee has a bunk property
rg -n "bunk" --type=ts -A 2 -B 2 | head -80Repository: Hack4Impact-UMD/camp-starfish
Length of output: 4890
🏁 Script executed:
# Find all calls to assignToOpenStaffFirstStep to see what's passed
rg -n "assignToOpenStaffFirstStep" -B 3 -A 3Repository: Hack4Impact-UMD/camp-starfish
Length of output: 2151
🏁 Script executed:
# Check the commented-out line 128 context
sed -n '120,135p' src/features/scheduling/generation/FreeplayScheduler.tsRepository: Hack4Impact-UMD/camp-starfish
Length of output: 756
🏁 Script executed:
# Find AdminAttendee interface (not AdminAttendeeID)
rg -n "interface AdminAttendee[^I]" -A 15Repository: Hack4Impact-UMD/camp-starfish
Length of output: 55
🏁 Script executed:
# Check the specific lines 198-208 in FreeplayScheduler.ts to see the exact code
sed -n '198,208p' src/features/scheduling/generation/FreeplayScheduler.tsRepository: Hack4Impact-UMD/camp-starfish
Length of output: 444
🏁 Script executed:
# Check if AdminAttendee has role property and what values it can have
rg -n "role.*ADMIN" --type=ts -B 2 -A 2Repository: Hack4Impact-UMD/camp-starfish
Length of output: 3833
ADMINs are silently skipped due to role check.
The function signature accepts (StaffAttendeeID | AdminAttendeeID)[], but line 199 only processes staffers where staffer.role === "STAFF". AdminAttendees have role "ADMIN" and lack a bunk property entirely, so they would be silently ignored if ever passed to this function. The commented-out line 128 suggests admins were considered as input.
Either narrow the type signature to StaffAttendeeID[] to match the actual behavior, or add explicit handling for ADMINs if they should be supported.
🤖 Prompt for AI Agents
In `@src/features/scheduling/generation/FreeplayScheduler.ts` around lines 198 -
208, The code currently only handles staffers with staffer.role === "STAFF" and
accesses staffer.bunk, so AdminAttendeeID (role "ADMIN") entries are silently
ignored; either restrict the function parameter type to StaffAttendeeID[] to
match the implementation, or add explicit ADMIN handling: update the loop in
FreeplayScheduler to branch on staffer.role (e.g., if "STAFF" keep the bunk !=
camper.bunk check, if "ADMIN" skip the bunk check or apply the intended admin
logic), ensure any use of staffer.bunk is guarded or typed, and keep
schedule.buddies[staffer.id] initialization/assignment logic for both branches.
3 participants
Note that, for the constructor, I included a parameters for all necessary information (i.e. freeplayschedule, admins, staffers, campers, etc.)
I also included a variable that includes all PostID objects as a list. This is necessary for "requiresAdmin" lookup, since the only other information about posts is given through the schedule object, which only contains the postID as a string.
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.