Skip to content

Bunk Jamboree Algorithm Implemented#44

Open
saharshmaloo wants to merge 8 commits into
mainfrom
feature/bunk-jamboree
Open

Bunk Jamboree Algorithm Implemented#44
saharshmaloo wants to merge 8 commits into
mainfrom
feature/bunk-jamboree

Conversation

@saharshmaloo
Copy link
Copy Markdown
Collaborator

@saharshmaloo saharshmaloo commented Oct 5, 2025

  • Wrote functions to do the following
    • Assign Admins randomly to each bunk
      • Each bunk should have at least 1 admin assigned to it
      • Check for personal conflicts when assigning Admins
    • Assign Bunks based on prefs to activities
      • Randomize order of bunks assigned

Summary by CodeRabbit

  • New Features

    • Enhanced scheduling: staff and camper-aware allocations, preference- and relationship-aware placement, balanced periods off, admin assignment, and improved bunk-to-activity placement.
    • Added a "Generate Test" button on Login to produce sample session data.
  • Tests / Tools

    • New test data generator exposing utilities to create synthetic sessions, attendees, bunks, preferences, and sample schedules.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Oct 5, 2025

Warning

Rate limit exceeded

@eshavignesh123 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 5 minutes and 1 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📝 Walkthrough

Walkthrough

Adds staff/camper fields and builder methods to BunkJamboreeScheduler, implements period-off allocation, admin assignment with conflict checks, and bunk-to-activity assignment; adds a large test-data generator and a "Generate Test" button on the login page.

Changes

Cohort / File(s) Summary
Scheduler core
src/features/scheduling/generation/BunkJamboreeScheduler.ts
Added public fields staff, campers, sectionID; builder methods withStaff(), withCampers(), withSectionID(); implemented assignPeriodsOff(), assignAdmin(), assignBunksToActivities() plus internal allocation logic, conflict checks, per-block capacity handling, yes/none relationship handling, and fallback allocation. Review focus: complex allocation logic, randomness/tiebreakers, and conflict/preference correctness.
Test data / Emulator
src/features/scheduling/test/generateEmulatorData.tsx
New comprehensive emulator: generates campers, staff, admins, bunks, blocks, activities, preferences, and sample BunkJamboreeScheduler; exports generateStaff, generateAdmins, generateBunkJamboreeSchedule, generateSession. Review focus: data realism, type correctness, and external libs (faker/moment) usage.
Dev UI trigger
src/app/LoginPage.tsx
Added generateTestData button and import of generateSession to manually produce test data from the login UI. Review focus: UI placement, invocation safety, and error handling.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant Scheduler as BunkJamboreeScheduler
    participant Block
    participant Activity
    participant Attendee

    Client->>Scheduler: withStaff(...).withCampers(...).withSectionID(...)
    Client->>Scheduler: assignPeriodsOff()
    Scheduler->>Attendee: evaluate day-off eligibility & yes/none links
    Scheduler->>Block: allocate periods off (per-block capacity & distribution)
    Client->>Scheduler: assignAdmin()
    loop per block/activity
        Scheduler->>Activity: check capacity & conflict rules
        Activity-->>Scheduler: accept/reject admin
    end
    Client->>Scheduler: assignBunksToActivities()
    loop per block
        Scheduler->>Activity: rank bunks by preferences & capacity
        Activity-->>Scheduler: assign bunks
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

Suggested reviewers

  • nkanchinadam

Poem

🐰 I hopped through prefs and lists so wide,
I nudged each bunk to find its stride,
Gave staff a break, then matched each game,
With carrots, chaos, and a tiny name—
Schedules bloom now—hip hop, hooray!

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: implementing the Bunk Jamboree scheduling algorithm with assignment logic for admins and activities.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 05e4d63 and ba57d21.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (2)
  • src/features/scheduling/BunkJamboreeScheduler.ts (2 hunks)
  • src/types/sessionTypes.ts (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/features/scheduling/BunkJamboreeScheduler.ts (1)
src/types/sessionTypes.ts (8)
  • SectionSchedule (71-74)
  • BunkID (125-125)
  • AdminAttendeeID (41-41)
  • StaffAttendeeID (35-35)
  • CamperAttendeeID (26-26)
  • SectionPreferences (145-147)
  • Block (84-87)
  • BlockPreferences (141-143)
🔇 Additional comments (1)
src/types/sessionTypes.ts (1)

116-118: Array casing for bunk assignments looks correct

Aligns with the new scheduler flow that pushes multiple bunk IDs per activity; no issues spotted.

Comment on lines +201 to +264
private assignBunksToActivitiesHelper(
block: Block<"BUNK-JAMBO">,
preferences: BlockPreferences
): void {
// Get all available bunk IDs
const availableBunks = this.bunks.map(bunk => bunk.id);

// Create a map to track how many bunks are assigned to each activity
const activityBunkCounts = new Map<string, number>();
block.activities.forEach(activity => {
activityBunkCounts.set(activity.name, activity.assignments.bunk.length);
});

// Shuffle bunks for fairness in assignment order
const shuffledBunks = [...availableBunks].sort(() => Math.random() - 0.5);

// Assign each bunk to exactly one activity
shuffledBunks.forEach(bunkId => {
// Find the minimum number of bunks assigned to any activity
const minBunkCount = Math.min(...Array.from(activityBunkCounts.values()));

// Find all activities with the minimum number of bunks
const activitiesWithMinBunks = block.activities.filter(activity =>
activityBunkCounts.get(activity.name) === minBunkCount
);

// If there's only one activity with minimum bunks, assign to it
if (activitiesWithMinBunks.length === 1) {
const activity = activitiesWithMinBunks[0];
activity.assignments.bunk.push(bunkId);
activityBunkCounts.set(activity.name, activityBunkCounts.get(activity.name)! + 1);
return;
}

// Multiple activities with minimum bunks - use preferences to break ties
const activityPreferences = activitiesWithMinBunks.map(activity => ({
activity,
preference: preferences[bunkId]?.[activity.name] || 0
}));

// Find the maximum preference score
const maxPreference = Math.max(...activityPreferences.map(ap => ap.preference));

// Filter to activities with the maximum preference
const activitiesWithMaxPreference = activityPreferences
.filter(ap => ap.preference === maxPreference)
.map(ap => ap.activity);

// If there's only one activity with max preference, assign to it
if (activitiesWithMaxPreference.length === 1) {
const activity = activitiesWithMaxPreference[0];
activity.assignments.bunk.push(bunkId);
activityBunkCounts.set(activity.name, activityBunkCounts.get(activity.name)! + 1);
return;
}

// Multiple activities with same max preference - pick randomly
const randomActivity = activitiesWithMaxPreference[
Math.floor(Math.random() * activitiesWithMaxPreference.length)
];
randomActivity.assignments.bunk.push(bunkId);
activityBunkCounts.set(randomActivity.name, activityBunkCounts.get(randomActivity.name)! + 1);
});
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Guard against empty activities before assigning bunks

If a block has no activities, activitiesWithMinBunks stays empty and randomActivity becomes undefined, so the next line throws (Cannot read properties of undefined). This can happen whenever we call the helper on a block that is still being built out. Add an early return (or continue) when block.activities.length === 0 so we don’t crash the scheduler.

Apply this diff:

   private assignBunksToActivitiesHelper(
     block: Block<"BUNK-JAMBO">, 
     preferences: BlockPreferences
   ): void {
+    if (block.activities.length === 0) {
+      return;
+    }
     // Get all available bunk IDs
     const availableBunks = this.bunks.map(bunk => bunk.id);
🤖 Prompt for AI Agents
In src/features/scheduling/BunkJamboreeScheduler.ts around lines 201 to 264, the
helper assumes block.activities is non-empty and will throw when
activitiesWithMinBunks is empty (or randomActivity becomes undefined); guard by
returning early when block.activities.length === 0 (or skip assignment) before
computing min counts and shuffling/iterating bunks, ensuring no assignments or
map lookups occur for an empty activities array.

Copy link
Copy Markdown
Collaborator

@eshavignesh123 eshavignesh123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Saharsh! Looks good for the most part, but just some edits have to be made. Also, make sure to test your algorithms with the test data.

// Add relationships to other admins
admin.yesyesList.forEach(relatedId => {
// Check if the related ID is an admin
if (this.admins.some(a => a.id === relatedId)) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of repeatedly checking if relatedID is a staff or admin member as you're parsing through both the admin yesyeslist and the staff yesyeslist, at the beginning of this method, maybe try creating a lookup map for all people.

Ex: const allStaff = new Map<number, { id: number; role: 'STAFF' | 'ADMIN'; yesyesList: number[] }>();

And then go through the admins list and the staff list and add it to the map. Then, when you're checking relatedIDs, you can just quickly reference this map and see their role.

const assignedPeriodsOff = new Set<number>();

// Initialize all blocks if they don't exist
this.blocksToAssign.forEach(blockId => {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed

});

// Check if any campers in this activity are in the admin's nono list
const hasConflict = admin.nonoList.some(nonoId =>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In schedulingUtils.ts, there's a method called doesConflictExist(...), you can use that.

);

// Find all compatible activities with the minimum number of admins
const activitiesWithMinAdmins = compatibleActivities.filter(activity =>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just store the first one that has minAdmins. No need to choose activity randomly.

Copy link
Copy Markdown
Collaborator

@eshavignesh123 eshavignesh123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Saharsh! The assignPeriodsOff() method has to be fixed.

const relationships = this.getYesYesRelationships();

// Get all available staff and admin IDs
const allAttendees = [
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure that when assigning staff periods off, no more than one staff member from the same bunk is off during the same period.

Also, make sure when you're assigning admins off, there's at least the same number of admins available as number of activities. The rest can be on their period off and you can alternate which admin gets the period off between blocks.

@nkanchinadam nkanchinadam force-pushed the main branch 2 times, most recently from a884ab9 to 974f469 Compare December 2, 2025 05:43
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In `@src/features/scheduling/generation/BunkJamboreeScheduler.ts`:
- Around line 7-8: The class BunkJamboreeScheduler contains a duplicate member
declaration "bunks: BunkID[] = [];" — remove the redundant declaration so only a
single "bunks" field remains (keep the initializer if needed) to resolve the
TypeScript duplicate member error; locate the duplicate declarations inside the
BunkJamboreeScheduler class and delete one.
- Around line 34-86: The assignPeriodsOff() method must validate two constraints
before pushing an attendee (or their related yes-yes people) into
this.schedule.blocks[blockId].periodsOff: (1) For staff entries (from
this.staff), check the candidate block's periodsOff does not already contain any
staff from the same bunk (use each staff's bunk property) and skip/choose
another block if it would violate the same-bunk rule; (2) For admins (from
this.admins), ensure that after assigning the admin the number of remaining
available admins for that block (total admins minus admins already in periodsOff
for that block) is still >= this.schedule.blocks[blockId].activities.length; if
not, do not assign that admin to that block (try another block or skip). Update
assignPeriodsOff to perform these checks both for the initially chosen
randomAttendee and for each relatedPerson from this.getYesYesRelationships(),
using findBlockWithLeastBreaks (or iterate candidate blocks in
this.blocksToAssign) to select a valid block, and only add ids to
this.schedule.blocks[blockId].periodsOff and assignedPeriodsOff when the checks
pass.
♻️ Duplicate comments (1)
src/features/scheduling/generation/BunkJamboreeScheduler.ts (1)

202-265: Guard against empty activities before assigning bunks.

If block.activities is empty, activitiesWithMinBunks and activitiesWithMaxPreference will be empty arrays, causing randomActivity to be undefined. Accessing randomActivity.assignments.bunk.push(bunkId) will throw a TypeError.

🐛 Proposed fix
 private assignBunksToActivitiesHelper(
   block: Block<"BUNK-JAMBO">, 
   preferences: BlockPreferences
 ): void {
+   if (block.activities.length === 0) {
+     return;
+   }
+   
   // Get all available bunk IDs
   const availableBunks = this.bunks.map(bunk => bunk.id);
🧹 Nitpick comments (5)
src/features/scheduling/generation/BunkJamboreeScheduler.ts (5)

1-2: Unused import and manual conflict check.

doesConflictExist is imported but never used. The conflict check at lines 148-150 manually iterates through admin.nonoList instead of using this utility function.

Consider either removing the unused import or using the utility:

♻️ Suggested refactor using the utility
-        // Check if any campers in this activity are in the admin's nono list
-        const hasConflict = admin.nonoList.some(nonoId => 
-          camperIdsInActivity.includes(nonoId)
-        );
-        
-        return !hasConflict;
+        // Check if any campers in this activity are in the admin's nono list
+        return !doesConflictExist(admin.nonoList, camperIdsInActivity);

91-104: Consider guarding against empty blocksToAssign.

If this.blocksToAssign is empty, this.blocksToAssign[0] returns undefined, causing the method to return undefined despite the string return type. While this is a private method called after initialization, adding a guard improves robustness.

♻️ Optional defensive guard
 private findBlockWithLeastBreaks(): string {
+   if (this.blocksToAssign.length === 0) {
+     throw new Error("No blocks available for assignment");
+   }
    let minBreaks = Infinity;
    let blockWithMinBreaks = this.blocksToAssign[0];

168-185: Consider simplifying activity selection.

Per previous review feedback, when multiple activities have the same minimum admin count, you can simply pick the first one instead of randomly selecting. This simplifies the logic:

♻️ Simplified selection
      // Find all compatible activities with the minimum number of admins
      const activitiesWithMinAdmins = compatibleActivities.filter(activity => 
        activityAdminCounts.get(activity.name) === minAdminCount
      );
      
-     // If there's only one activity with minimum admins, assign to it
-     if (activitiesWithMinAdmins.length === 1) {
-       const activity = activitiesWithMinAdmins[0];
-       activity.assignments.adminIds.push(adminId);
-       activityAdminCounts.set(activity.name, activityAdminCounts.get(activity.name)! + 1);
-       return;
-     }
-     
-     // Multiple activities with same minimum admin count - pick randomly
-     const randomActivity = activitiesWithMinAdmins[
-       Math.floor(Math.random() * activitiesWithMinAdmins.length)
-     ];
-     randomActivity.assignments.adminIds.push(adminId);
-     activityAdminCounts.set(randomActivity.name, activityAdminCounts.get(randomActivity.name)! + 1);
+     // Assign to the first activity with minimum admins
+     const activity = activitiesWithMinAdmins[0];
+     activity.assignments.adminIds.push(adminId);
+     activityAdminCounts.set(activity.name, activityAdminCounts.get(activity.name)! + 1);

189-200: Silent skip when preferences are missing.

The method silently skips blocks that don't have preferences defined (line 194). If preferences are optional, consider providing a fallback or logging a warning when a block is skipped.

if (block && blockPreferences) {
  this.assignBunksToActivitiesHelper(block, blockPreferences);
} else if (block && !blockPreferences) {
  // Consider: assign bunks without preference tie-breaking, or log warning
}

272-344: Optimize with lookup maps and prevent duplicate relationships.

Two concerns:

  1. Performance: Repeated this.admins.some() and this.staff.some() calls are O(n) each. Per previous review feedback, create lookup maps upfront:

  2. Duplicate entries: If both A and B list each other in their yesyesList, the bidirectional logic adds the relationship twice.

♻️ Suggested refactor with lookup maps and deduplication
 getYesYesRelationships(): Map<number, { id: number; role: 'STAFF' | 'ADMIN' }[]> {
   const relationships = new Map<number, { id: number; role: 'STAFF' | 'ADMIN' }[]>();
+  
+  // Create lookup maps for O(1) role checking
+  const adminIds = new Set(this.admins.map(a => a.id));
+  const staffIds = new Set(this.staff.map(s => s.id));
+  
+  // Track added pairs to prevent duplicates
+  const addedPairs = new Set<string>();
+  
+  const addRelationship = (fromId: number, fromRole: 'STAFF' | 'ADMIN', toId: number, toRole: 'STAFF' | 'ADMIN') => {
+    const pairKey = [Math.min(fromId, toId), Math.max(fromId, toId)].join('-');
+    if (addedPairs.has(pairKey)) return;
+    addedPairs.add(pairKey);
+    
+    if (!relationships.has(fromId)) relationships.set(fromId, []);
+    if (!relationships.has(toId)) relationships.set(toId, []);
+    relationships.get(fromId)!.push({ id: toId, role: toRole });
+    relationships.get(toId)!.push({ id: fromId, role: fromRole });
+  };
   
   // Process admin relationships
   this.admins.forEach(admin => {
     if (admin.yesyesList && admin.yesyesList.length > 0) {
-      // Initialize the array for this admin if it doesn't exist
-      if (!relationships.has(admin.id)) {
-        relationships.set(admin.id, []);
-      }
-      
       admin.yesyesList.forEach(relatedId => {
-        // Check if the related ID is an admin
-        if (this.admins.some(a => a.id === relatedId)) {
-          // ... existing logic
+        if (adminIds.has(relatedId)) {
+          addRelationship(admin.id, 'ADMIN', relatedId, 'ADMIN');
+        } else if (staffIds.has(relatedId)) {
+          addRelationship(admin.id, 'ADMIN', relatedId, 'STAFF');
         }
       });
     }
   });
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ba57d21 and 5c40b1e.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (1)
  • src/features/scheduling/generation/BunkJamboreeScheduler.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/features/scheduling/generation/BunkJamboreeScheduler.ts (1)
src/types/sessionTypes.ts (6)
  • SectionSchedule (72-75)
  • BunkID (135-135)
  • AdminAttendeeID (41-41)
  • StaffAttendeeID (35-35)
  • Block (85-88)
  • BlockPreferences (151-153)
🪛 Biome (2.1.2)
src/features/scheduling/generation/BunkJamboreeScheduler.ts

[error] 8-8: Duplicate class member name "bunks"

(lint/suspicious/noDuplicateClassMembers)

🔇 Additional comments (2)
src/features/scheduling/generation/BunkJamboreeScheduler.ts (2)

17-31: LGTM!

Builder pattern methods are correctly implemented with proper return types for method chaining.


106-116: LGTM!

The method correctly iterates blocks and delegates to the helper with proper null checking.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

Comment thread src/features/scheduling/generation/BunkJamboreeScheduler.ts Outdated
Comment on lines +34 to +86
assignPeriodsOff(): BunkJamboreeScheduler {
// Get all yesyes relationships
const relationships = this.getYesYesRelationships();

// Get all available staff and admin IDs
const allAttendees = [
...this.staff.map(s => ({ id: s.id, role: 'STAFF' as const })),
...this.admins.map(a => ({ id: a.id, role: 'ADMIN' as const }))
];

// Track who has been assigned periods off
const assignedPeriodsOff = new Set<number>();

// Initialize all blocks if they don't exist
this.blocksToAssign.forEach(blockId => {
if (!this.schedule.blocks[blockId]) {
this.schedule.blocks[blockId] = { activities: [], periodsOff: [] };
}
});

// Continue until all attendees have been assigned a period off
while (assignedPeriodsOff.size < allAttendees.length) {
// Get unassigned attendees
const unassignedAttendees = allAttendees.filter(attendee =>
!assignedPeriodsOff.has(attendee.id)
);

if (unassignedAttendees.length === 0) break;

// Pick a random unassigned attendee
const randomAttendee = unassignedAttendees[
Math.floor(Math.random() * unassignedAttendees.length)
];

// Find the block with the least staff members on break
const blockWithLeastBreaks = this.findBlockWithLeastBreaks();

// Assign the random attendee to this block
this.schedule.blocks[blockWithLeastBreaks].periodsOff.push(randomAttendee.id);
assignedPeriodsOff.add(randomAttendee.id);

// Assign all people in yesyes relationship with this attendee to the same block
const relatedPeople = relationships.get(randomAttendee.id) || [];
relatedPeople.forEach(relatedPerson => {
if (!assignedPeriodsOff.has(relatedPerson.id)) {
this.schedule.blocks[blockWithLeastBreaks].periodsOff.push(relatedPerson.id);
assignedPeriodsOff.add(relatedPerson.id);
}
});
}

return this;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find the Bunk interface and related types
rg -n "interface Bunk|type Bunk|class Bunk" --type=ts -A 10 | head -50

Repository: Hack4Impact-UMD/camp-starfish

Length of output: 3676


🏁 Script executed:

#!/bin/bash
# Find the Schedule and Block types to understand their structure
rg -n "interface Block|type Block|interface Schedule|type Schedule" --type=ts -A 8 | head -80

Repository: Hack4Impact-UMD/camp-starfish

Length of output: 2337


🏁 Script executed:

#!/bin/bash
# Look at the BunkJamboreeScheduler class to understand its properties
fd "BunkJamboreeScheduler" --type=ts | head -1 | xargs cat -n | head -100

Repository: Hack4Impact-UMD/camp-starfish

Length of output: 242


🏁 Script executed:

#!/bin/bash
# Read the BunkJamboreeScheduler.ts file
fd "BunkJamboreeScheduler.ts" --type f | head -1 | xargs wc -l

Repository: Hack4Impact-UMD/camp-starfish

Length of output: 136


🏁 Script executed:

#!/bin/bash
# Read the BunkJamboreeScheduler.ts file
fd "BunkJamboreeScheduler.ts" --type f | head -1 | xargs head -150

Repository: Hack4Impact-UMD/camp-starfish

Length of output: 5869


🏁 Script executed:

#!/bin/bash
# Continue reading the file from line 150
fd "BunkJamboreeScheduler.ts" --type f | head -1 | xargs sed -n '150,250p'

Repository: Hack4Impact-UMD/camp-starfish

Length of output: 3969


🏁 Script executed:

#!/bin/bash
# Check the getYesYesRelationships method
fd "BunkJamboreeScheduler.ts" --type f | head -1 | xargs grep -n "getYesYesRelationships" -A 30

Repository: Hack4Impact-UMD/camp-starfish

Length of output: 2824


🏁 Script executed:

#!/bin/bash
# Look for any staff-to-bunk mapping or constraint checking
fd "BunkJamboreeScheduler.ts" --type f | head -1 | xargs grep -n "staffIds\|bunk\|constraint\|validate" -i

Repository: Hack4Impact-UMD/camp-starfish

Length of output: 3680


🏁 Script executed:

#!/bin/bash
# Check if there's any validation for same-bunk constraint elsewhere
rg -n "staffIds\|same.*bunk\|bunk.*staff" --type ts -B 2 -A 2 | head -100

Repository: Hack4Impact-UMD/camp-starfish

Length of output: 55


🏁 Script executed:

#!/bin/bash
# Look at the rest of the file to see if there are other methods that validate constraints
fd "BunkJamboreeScheduler.ts" --type f | head -1 | xargs sed -n '250,344p'

Repository: Hack4Impact-UMD/camp-starfish

Length of output: 3970


🏁 Script executed:

#!/bin/bash
# Check if Staff type has bunk association
rg -n "interface.*Staff|type.*Staff" --type ts -A 8 | head -80

Repository: Hack4Impact-UMD/camp-starfish

Length of output: 2057


🏁 Script executed:

#!/bin/bash
# Search for any validation related to staff breaks or admin availability
rg -n "periodsOff|admin.*available|same.*bunk|staff.*bunk" --type ts src/features/scheduling/ | grep -i "validate\|check\|constraint"

Repository: Hack4Impact-UMD/camp-starfish

Length of output: 55


🏁 Script executed:

#!/bin/bash
# Check if there's a review or validation step after scheduling
fd ".*Scheduler" --type f | head -5 | xargs grep -l "validate\|constraint" 2>/dev/null

Repository: Hack4Impact-UMD/camp-starfish

Length of output: 57


Add validation to prevent same-bunk staffing gaps and admin shortages during period assignments.

The assignPeriodsOff() method doesn't enforce two critical constraints:

  1. Same-bunk constraint: No more than one staff member from the same bunk should be off during the same period. When assigning related people via yesyesList (lines 77–82), multiple staff from the same bunk could end up assigned to periodsOff in the same block.

  2. Admin availability: Each block should have at least as many available admins as it has activities. The current implementation doesn't validate this before or after assigning periods off.

Add validation checks before assigning attendees to periodsOff:

  • For staff: Verify no other staff from the same bunk are already assigned to that block's periodsOff
  • For admins: Ensure remaining available admins ≥ number of activities in the block
🤖 Prompt for AI Agents
In `@src/features/scheduling/generation/BunkJamboreeScheduler.ts` around lines 34
- 86, The assignPeriodsOff() method must validate two constraints before pushing
an attendee (or their related yes-yes people) into
this.schedule.blocks[blockId].periodsOff: (1) For staff entries (from
this.staff), check the candidate block's periodsOff does not already contain any
staff from the same bunk (use each staff's bunk property) and skip/choose
another block if it would violate the same-bunk rule; (2) For admins (from
this.admins), ensure that after assigning the admin the number of remaining
available admins for that block (total admins minus admins already in periodsOff
for that block) is still >= this.schedule.blocks[blockId].activities.length; if
not, do not assign that admin to that block (try another block or skip). Update
assignPeriodsOff to perform these checks both for the initially chosen
randomAttendee and for each relatedPerson from this.getYesYesRelationships(),
using findBlockWithLeastBreaks (or iterate candidate blocks in
this.blocksToAssign) to select a valid block, and only add ids to
this.schedule.blocks[blockId].periodsOff and assignedPeriodsOff when the checks
pass.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 Fix all issues with AI agents
In `@src/app/LoginPage.tsx`:
- Around line 81-88: The "Generate Test" button in the LoginPage uses
MicrosoftIcon and is exposed in production; update the UI so test data
generation is not shown in prod: either wrap the button rendering in an
environment check (e.g., only render when process.env.NODE_ENV ===
'development') or move the generateTestData control to a separate dev/admin
page, and replace MicrosoftIcon with a more appropriate icon or a neutral
placeholder; locate the button by the JSX that calls generateTestData and
references MicrosoftIcon to apply the change.
- Around line 37-45: generateTestData is marked async and uses await on
generateSession(), but generateSession() is synchronous so await has no effect;
change generateTestData to a normal synchronous function (remove async) and call
generateSession() without await, still wrapping the call in try/catch to catch
synchronous throws and call setError on failure (reference generateTestData,
generateSession, setError).

In `@src/features/scheduling/generation/BunkJamboreeScheduler.ts`:
- Around line 248-252: The MAX_CAPACITY_ADMINS calculation can yield 0 when
availableAdmins.length < activities.length causing the capacity check in the
loop (where activities.find checks a.assignments.adminIds.length >=
MAX_CAPACITY_ADMINS) to always fail; update the calculation of
MAX_CAPACITY_ADMINS (used with availableAdmins and activities) to ensure a
minimum of 1 (e.g., replace Math.floor(...) with Math.ceil(...) or wrap with
Math.max(1, ...)) so first-pass admin assignments can succeed and avoid pushing
all admins into unassignedAdmin for the second pass.

In `@src/features/scheduling/test/generateEmulatorData.tsx`:
- Around line 348-350: Remove the duplicate initialization of
schedule.alternatePeriodsOff["RH"] in generateEmulatorData.tsx: keep a single
assignment to schedule.alternatePeriodsOff["RH"] = [] and delete the redundant
line so the property is only initialized once; verify no other logic depends on
a second assignment to schedule.alternatePeriodsOff["RH"].
♻️ Duplicate comments (2)
src/features/scheduling/generation/BunkJamboreeScheduler.ts (2)

49-54: Missing admin availability validation before period-off assignment.

The past review flagged that each block should have at least as many available admins as activities. The current implementation doesn't validate this constraint before assigning admins to periodsOff, which could result in blocks where available admins < activity count.

Consider adding a check before assigning an admin to periodsOff:

const blockActivitiesCount = this.schedule.blocks[blockId].activities.length;
const adminsInBlock = eligibleAdmins.filter(a => !block.periodsOff.includes(a.id));
if (adminsInBlock.length <= blockActivitiesCount) {
  // Skip assigning this admin to periodsOff for this block
}

360-365: Guard against empty activities still missing.

As flagged in a past review, if block.activities is empty, activityBunkCounts will be empty, causing Math.min(...Array.from(activityBunkCounts.values())) to return Infinity, and subsequent operations will fail or behave unexpectedly.

🐛 Proposed fix
   private assignBunksToActivitiesHelper(
     block: Block<"BUNK-JAMBO">, 
     preferences: BlockPreferences
   ): void {
+    if (block.activities.length === 0) {
+      return;
+    }
     // Get all available bunk IDs
     const availableBunks = this.bunks.map(bunk => bunk.id);
🧹 Nitpick comments (4)
src/features/scheduling/generation/BunkJamboreeScheduler.ts (2)

220-228: Method should return this for chaining consistency.

Other methods in this class (assignPeriodsOff, assignBunksToActivities) are designed for chaining but assignAdmin() doesn't return this. This breaks the builder pattern consistency.

♻️ Suggested fix
   assignAdmin(){
+    // ... existing implementation ...
+    return this;
-  }
+  }

374-374: Biased shuffle algorithm.

The shuffle using sort(() => Math.random() - 0.5) produces biased results. The assignAdmin method (lines 239-242) correctly uses Fisher-Yates shuffle. Consider using the same approach here for consistency and correctness:

♻️ Suggested fix
-    const shuffledBunks = [...availableBunks].sort(() => Math.random() - 0.5);
+    const shuffledBunks = [...availableBunks];
+    for (let i = shuffledBunks.length - 1; i > 0; i--) {
+      const j = Math.floor(Math.random() * (i + 1));
+      [shuffledBunks[i], shuffledBunks[j]] = [shuffledBunks[j], shuffledBunks[i]];
+    }
src/features/scheduling/test/generateEmulatorData.tsx (2)

1-18: File extension should be .ts instead of .tsx.

This file contains no JSX or React components, only utility functions. Using .tsx extension is misleading and unnecessary.


128-190: Consider extracting duplicate logic.

The same nono-list generation logic is repeated three times for campers, staff, and admins. This could be refactored into a helper function:

function assignNonoList(person: CamperAttendeeID | StaffAttendeeID | AdminAttendeeID, allPeople: (CamperAttendeeID | StaffAttendeeID | AdminAttendeeID)[]) {
  const shuffled = allPeople.filter(p => p.id !== person.id).sort(() => Math.random() - 0.5);
  const randomAmount = Math.floor(Math.random() * 4);
  const selected = shuffled.slice(0, randomAmount);
  person.nonoList = selected.map(p => p.id);
  for (const other of selected) {
    if (!other.nonoList.includes(person.id)) {
      other.nonoList.push(person.id);
    }
  }
}
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5c40b1e and c6848b2.

📒 Files selected for processing (3)
  • src/app/LoginPage.tsx
  • src/features/scheduling/generation/BunkJamboreeScheduler.ts
  • src/features/scheduling/test/generateEmulatorData.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
src/app/LoginPage.tsx (1)
src/features/scheduling/test/generateEmulatorData.tsx (1)
  • generateSession (711-845)
src/features/scheduling/generation/BunkJamboreeScheduler.ts (2)
src/types/sessionTypes.ts (9)
  • SectionSchedule (72-75)
  • BunkID (135-135)
  • AdminAttendeeID (41-41)
  • StaffAttendeeID (35-35)
  • CamperAttendeeID (26-26)
  • SectionPreferences (155-157)
  • SchedulingSectionID (70-70)
  • Block (85-88)
  • BlockPreferences (151-153)
src/features/scheduling/generation/schedulingUtils.ts (1)
  • doesConflictExist (3-8)
src/features/scheduling/test/generateEmulatorData.tsx (1)
src/types/sessionTypes.ts (17)
  • CamperAttendeeID (26-26)
  • AgeGroup (149-149)
  • StaffAttendeeID (35-35)
  • AdminAttendeeID (41-41)
  • BunkID (135-135)
  • ProgramAreaID (94-94)
  • SectionSchedule (72-75)
  • BundleActivityWithAssignments (125-125)
  • BunkJamboreeActivityWithAssignments (126-126)
  • NonBunkJamboreeActivityWithAssignments (127-127)
  • PostID (147-147)
  • Freeplay (137-140)
  • SectionPreferences (155-157)
  • SectionID (55-55)
  • SchedulingSectionID (70-70)
  • SessionID (11-11)
  • NightShiftID (49-49)
🔇 Additional comments (6)
src/features/scheduling/generation/BunkJamboreeScheduler.ts (3)

1-2: LGTM on imports!

Good use of the existing doesConflictExist utility from schedulingUtils.ts for conflict checking, as suggested in past review comments.


7-12: Duplicate class member issue resolved.

The duplicate bunks declaration flagged in past reviews has been fixed. Fields are now correctly defined.


102-107: Same-bunk constraint allows 2 staff off instead of 1.

The past review requested that "no more than one staff member from the same bunk should be off during the same period." The current check diff + 1 > 2 allows up to 2 staff from the same bunk to be off simultaneously. If stricter enforcement is needed:

-          if (diff + 1 > 2) {
+          if (diff + 1 > 1) {

Also, the fallback value ?? 100 is a magic number. Consider using a named constant or a more explicit check.

Please confirm whether allowing 2 staff per bunk on period off is intentional or if it should be limited to 1 as originally requested.

src/features/scheduling/test/generateEmulatorData.tsx (3)

222-259: LGTM for test data generation.

The bunk generation logic correctly assigns staff and campers to bunks based on their bunk property.


660-662: assignBunksToActivities() is commented out.

The PR description mentions implementing "Assign bunks to activities based on preferences," but scheduler.assignBunksToActivities() is commented out. Is this intentional or should it be enabled?


711-845: LGTM for test data orchestration.

The function correctly composes the test session with attendees, sections, and scheduler. The commented-out code appears to be placeholders for future scheduler implementations.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

Comment thread src/app/LoginPage.tsx
Comment on lines +37 to +45
const generateTestData = async () => {
try {
const data = await generateSession();
console.log(data);
}
catch {
setError("An error occurred generating data. Please try again.");
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

generateSession() is synchronous; await has no effect.

Looking at generateEmulatorData.tsx, generateSession() is a synchronous function that doesn't return a Promise. The await on line 39 has no practical effect.

   const generateTestData = async () => {
     try {
-      const data = await generateSession();
+      const data = generateSession();
       console.log(data);
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const generateTestData = async () => {
try {
const data = await generateSession();
console.log(data);
}
catch {
setError("An error occurred generating data. Please try again.");
}
}
const generateTestData = async () => {
try {
const data = generateSession();
console.log(data);
}
catch {
setError("An error occurred generating data. Please try again.");
}
}
🤖 Prompt for AI Agents
In `@src/app/LoginPage.tsx` around lines 37 - 45, generateTestData is marked async
and uses await on generateSession(), but generateSession() is synchronous so
await has no effect; change generateTestData to a normal synchronous function
(remove async) and call generateSession() without await, still wrapping the call
in try/catch to catch synchronous throws and call setError on failure (reference
generateTestData, generateSession, setError).

Comment thread src/app/LoginPage.tsx
Comment on lines +81 to +88
<button
onClick={generateTestData}
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
</button>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Test button uses wrong icon and may not belong in production.

  1. The button uses MicrosoftIcon but is labeled "Generate Test" - consider using a different icon or placeholder.
  2. This test data generation button appears in the production login page. Consider:
    • Gating it behind an environment check (process.env.NODE_ENV === 'development')
    • Moving it to a dedicated dev/admin page
{process.env.NODE_ENV === 'development' && (
  <button onClick={generateTestData} ...>
    Generate Test
  </button>
)}
🤖 Prompt for AI Agents
In `@src/app/LoginPage.tsx` around lines 81 - 88, The "Generate Test" button in
the LoginPage uses MicrosoftIcon and is exposed in production; update the UI so
test data generation is not shown in prod: either wrap the button rendering in
an environment check (e.g., only render when process.env.NODE_ENV ===
'development') or move the generateTestData control to a separate dev/admin
page, and replace MicrosoftIcon with a more appropriate icon or a neutral
placeholder; locate the button by the JSX that calls generateTestData and
references MicrosoftIcon to apply the change.

Comment on lines +248 to +252
const MAX_CAPACITY_ADMINS = Math.floor(availableAdmins.length/activities.length);
for (const admin of availableAdmins) {
const activity = activities.find((a) => {
// Check max capacity for admins
if (a.assignments.adminIds.length >= MAX_CAPACITY_ADMINS) return false;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Potential issue with MAX_CAPACITY_ADMINS calculation.

When availableAdmins.length < activities.length, MAX_CAPACITY_ADMINS becomes 0 due to Math.floor(), causing the first-pass assignment to always fail the capacity check. All admins would then be pushed to unassignedAdmin and handled in the second pass.

Consider using Math.ceil() or ensuring at least 1:

-const MAX_CAPACITY_ADMINS = Math.floor(availableAdmins.length/activities.length);
+const MAX_CAPACITY_ADMINS = Math.max(1, Math.ceil(availableAdmins.length / activities.length));
🤖 Prompt for AI Agents
In `@src/features/scheduling/generation/BunkJamboreeScheduler.ts` around lines 248
- 252, The MAX_CAPACITY_ADMINS calculation can yield 0 when
availableAdmins.length < activities.length causing the capacity check in the
loop (where activities.find checks a.assignments.adminIds.length >=
MAX_CAPACITY_ADMINS) to always fail; update the calculation of
MAX_CAPACITY_ADMINS (used with availableAdmins and activities) to ensure a
minimum of 1 (e.g., replace Math.floor(...) with Math.ceil(...) or wrap with
Math.max(1, ...)) so first-pass admin assignments can succeed and avoid pushing
all admins into unassignedAdmin for the second pass.

Comment on lines +348 to +350
schedule.alternatePeriodsOff["RH"] = []

schedule.alternatePeriodsOff["RH"] = []
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Duplicate initialization.

Lines 348 and 350 both set schedule.alternatePeriodsOff["RH"] = []. Remove the duplicate.

   const schedule: SectionSchedule<'BUNK-JAMBO'> = { blocks: {}, alternatePeriodsOff: {} };
   schedule.alternatePeriodsOff["RH"] = []
-
-  schedule.alternatePeriodsOff["RH"] = []
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
schedule.alternatePeriodsOff["RH"] = []
schedule.alternatePeriodsOff["RH"] = []
schedule.alternatePeriodsOff["RH"] = []
🤖 Prompt for AI Agents
In `@src/features/scheduling/test/generateEmulatorData.tsx` around lines 348 -
350, Remove the duplicate initialization of schedule.alternatePeriodsOff["RH"]
in generateEmulatorData.tsx: keep a single assignment to
schedule.alternatePeriodsOff["RH"] = [] and delete the redundant line so the
property is only initialized once; verify no other logic depends on a second
assignment to schedule.alternatePeriodsOff["RH"].

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Fix all issues with AI agents
In `@src/features/scheduling/generation/BunkJamboreeScheduler.ts`:
- Around line 368-369: The lookup this.preferences[blockID][b.id][activity.name]
can throw when any layer is missing; in BunkJamboreeScheduler where bunkPrefs is
computed, guard the access by using optional chaining or explicit existence
checks (e.g., this.preferences?.[blockID]?.[b.id]?.[activity.name]) or check
each key before comparing to i so missing preferences return false instead of
throwing; update the expression that builds bunkPrefs to safely handle absent
blockID, bunk id, or activity entries.
- Line 1: The import line in BunkJamboreeScheduler.ts includes unused types
(Block, BlockPreferences, JamboreeActivity, BunkAssignments) that cause ESLint
failures; remove these four symbols from the import list so only used types
remain (AdminAttendeeID, StaffAttendeeID, CamperAttendeeID, SectionSchedule,
SectionPreferences, BunkID, SchedulingSectionID) in the same import statement to
clear the lint errors.

In `@src/features/scheduling/test/generateEmulatorData.tsx`:
- Around line 327-390: Several helper functions and params are flagged unused;
either export or remove the unused generators (generateBundleBlockSchedule,
generateNonBunkJamboBlockSchedule, generateFreeplayBlockSchedule, generatePosts)
and eliminate dead locals (counselors, nightShifts), and silence unused
parameters by renaming them with a leading underscore (e.g. blockID -> _blockID)
in generateBunkJamboActivities and generateNonBunkJamboActivities; specifically,
decide whether to export the listed functions if they’re intended as public
helpers or delete them if dead code, remove or use the unused locals
(counselors, nightShifts) in their respective generators, and rename unused
params to _blockID to satisfy ESLint (or explicitly reference them like void
_blockID) so CI linting passes.
♻️ Duplicate comments (3)
src/features/scheduling/generation/BunkJamboreeScheduler.ts (2)

56-116: Same-bunk period-off constraint is misapplied and blocks most staff.
ogMapDiff is seeded with camperIds.length - totalStaff, so for typical bunks the diff + 1 > 2 check rejects all staff from those bunks. Also, staff without a bunk value are always rejected because of the ?? 100 fallback. Track per-bunk off counts instead and only apply the check when person.bunk is defined.

✅ Safer per-bunk off-count guard
-    let ogMapDiff = new Map<number, number>();
-    let mapDiff = new Map<number, number>();
+    const bunkOffCounts = new Map<number, number>();

     for (const bunk of this.bunks) {
-      let totalStaff = 0
-      for (const staff of bunk.staffIds) {
-        if (eligibleStaff.some(s => s.id === staff)) {
-          totalStaff += 1
-        }
-      }
-      ogMapDiff.set(bunk.id, bunk.camperIds.length - totalStaff)
+      bunkOffCounts.set(bunk.id, 0);
     }

     ...
-        if ('bunk' in person && mapDiff) {
-          const diff = mapDiff.get(person.bunk) ?? 100;
-          if (diff + 1 > 2) {
-            return false
-          }
-        }
+        if ("bunk" in person && person.bunk != null) {
+          const offCount = bunkOffCounts.get(person.bunk) ?? 0;
+          if (offCount >= 1) return false;
+        }

     ...
-        if ('bunk' in person && mapDiff) 
-        {
-          const diff = mapDiff.get(person.bunk) ?? 100;
-          mapDiff.set(person.bunk, diff + 1);
-        }
+        if ("bunk" in person && person.bunk != null) {
+          const offCount = bunkOffCounts.get(person.bunk) ?? 0;
+          bunkOffCounts.set(person.bunk, offCount + 1);
+        }

248-252: MAX_CAPACITY_ADMINS can be 0, forcing all admins into the fallback pass.
When availableAdmins.length < activities.length, Math.floor(...) yields 0, so every activity fails the capacity check.

✅ Suggested fix
-      const MAX_CAPACITY_ADMINS = Math.floor(availableAdmins.length/activities.length);
+      const MAX_CAPACITY_ADMINS = Math.max(1, Math.ceil(availableAdmins.length / activities.length));
src/features/scheduling/test/generateEmulatorData.tsx (1)

345-350: Remove duplicate initialization of alternatePeriodsOff["RH"].

🧹 Proposed fix
   const schedule: SectionSchedule<'BUNK-JAMBO'> = { blocks: {}, alternatePeriodsOff: {} };
   schedule.alternatePeriodsOff["RH"] = []
-
-  schedule.alternatePeriodsOff["RH"] = []

Comment thread src/features/scheduling/generation/BunkJamboreeScheduler.ts Outdated
Comment on lines +368 to +369
let bunkPrefs = this.bunks.filter(b => this.preferences[blockID][b.id][activity.name] == i);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Guard preference lookups to avoid runtime crashes.
this.preferences[blockID][b.id][activity.name] will throw if any layer is missing. Optional chaining avoids hard failures when prefs are incomplete.

✅ Proposed fix
-          let bunkPrefs = this.bunks.filter(b => this.preferences[blockID][b.id][activity.name] == i);
+          const bunkPrefs = this.bunks.filter(
+            b => this.preferences?.[blockID]?.[b.id]?.[activity.name] === i
+          );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
let bunkPrefs = this.bunks.filter(b => this.preferences[blockID][b.id][activity.name] == i);
const bunkPrefs = this.bunks.filter(
b => this.preferences?.[blockID]?.[b.id]?.[activity.name] === i
);
🧰 Tools
🪛 ESLint

[error] 368-368: 'bunkPrefs' is never reassigned. Use 'const' instead.

(prefer-const)

🤖 Prompt for AI Agents
In `@src/features/scheduling/generation/BunkJamboreeScheduler.ts` around lines 368
- 369, The lookup this.preferences[blockID][b.id][activity.name] can throw when
any layer is missing; in BunkJamboreeScheduler where bunkPrefs is computed,
guard the access by using optional chaining or explicit existence checks (e.g.,
this.preferences?.[blockID]?.[b.id]?.[activity.name]) or check each key before
comparing to i so missing preferences return false instead of throwing; update
the expression that builds bunkPrefs to safely handle absent blockID, bunk id,
or activity entries.

Comment on lines +327 to +390
function generateBundleBlockSchedule(blockIDs: string[]) {

const schedule: SectionSchedule<'BUNDLE'> = { blocks: {}, alternatePeriodsOff: {} };

schedule.alternatePeriodsOff["RH"] = [];


for(let i = 0; i < blockIDs.length; i++)
{
const activities: BundleActivityWithAssignments[] = generateBundleActivities(TOTAL_ACTIVITIES_PER_DAY, blockIDs[i]);
const periodsOff: number[] = [];
schedule.blocks[blockIDs[i]] = { activities: activities, periodsOff: periodsOff };
}

return schedule;

}

function generateBunkJamboBlockSchedule(blockIDs: string[]) {

const schedule: SectionSchedule<'BUNK-JAMBO'> = { blocks: {}, alternatePeriodsOff: {} };
schedule.alternatePeriodsOff["RH"] = []

schedule.alternatePeriodsOff["RH"] = []
for(let i = 0; i < blockIDs.length; i++)
{
const activities: BunkJamboreeActivityWithAssignments[] = generateBunkJamboActivities(TOTAL_ACTIVITIES_PER_DAY, blockIDs[i]);
const periodsOff: number[] = [];
schedule.blocks[blockIDs[i]] = { activities: activities, periodsOff: periodsOff };
}

return schedule;

}

function generateNonBunkJamboBlockSchedule(blockIDs: string[]) {

const schedule: SectionSchedule<'NON-BUNK-JAMBO'> = { blocks: {}, alternatePeriodsOff: {} };

schedule.alternatePeriodsOff["RH"] = []
for(let i = 0; i < blockIDs.length; i++)
{
const activities: NonBunkJamboreeActivityWithAssignments[] = generateNonBunkJamboActivities(TOTAL_ACTIVITIES_PER_DAY, blockIDs[i]);
const periodsOff: number[] = [];
schedule.blocks[blockIDs[i]] = { activities: activities, periodsOff: periodsOff };
}

return schedule;

}

function generateFreeplayBlockSchedule(posts: PostID[]) : Freeplay {

const schedule: Freeplay = { posts: {}, buddies: {} };


for(let i = 0; i < posts.length; i++)
{
const buddies: number[] = [];
schedule.posts[posts[i].id] = buddies;
}

return schedule
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Resolve ESLint unused declarations in the emulator generator.
ESLint reports unused helpers (generateBundleBlockSchedule, generateNonBunkJamboBlockSchedule, generateFreeplayBlockSchedule, generatePosts), unused params (blockID in generateBunkJamboActivities / generateNonBunkJamboActivities), and unused locals (counselors, nightShifts). If lint is enforced, this will fail CI. Either use/export them or remove/suppress.

✅ Example fix for unused params
-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[] {

Also applies to: 493-551, 806-807

🧰 Tools
🪛 ESLint

[error] 327-327: 'generateBundleBlockSchedule' is defined but never used.

(@typescript-eslint/no-unused-vars)


[error] 362-362: 'generateNonBunkJamboBlockSchedule' is defined but never used.

(@typescript-eslint/no-unused-vars)


[error] 378-378: 'generateFreeplayBlockSchedule' is defined but never used.

(@typescript-eslint/no-unused-vars)

🤖 Prompt for AI Agents
In `@src/features/scheduling/test/generateEmulatorData.tsx` around lines 327 -
390, Several helper functions and params are flagged unused; either export or
remove the unused generators (generateBundleBlockSchedule,
generateNonBunkJamboBlockSchedule, generateFreeplayBlockSchedule, generatePosts)
and eliminate dead locals (counselors, nightShifts), and silence unused
parameters by renaming them with a leading underscore (e.g. blockID -> _blockID)
in generateBunkJamboActivities and generateNonBunkJamboActivities; specifically,
decide whether to export the listed functions if they’re intended as public
helpers or delete them if dead code, remove or use the unused locals
(counselors, nightShifts) in their respective generators, and rename unused
params to _blockID to satisfy ESLint (or explicitly reference them like void
_blockID) so CI linting passes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants