Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/pages/welcome.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ const Welcome: NextPage = () => {
};

export const getServerSideProps = async (
context: GetServerSidePropsContext
context: GetServerSidePropsContext,
) => {
const session = await getServerAuthSession(context);

Expand Down
27 changes: 16 additions & 11 deletions src/server/router/judging.ts
Original file line number Diff line number Diff line change
Expand Up @@ -892,6 +892,15 @@ export const timeSlotRouter = router({
};
}

// MLH slot duration in minutes - must divide evenly into slotDurationMinutes
const MLH_SLOT_MINUTES = 2.5;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This changes the MLH slot duration from 5 minutes to 2.5 minutes, which is a breaking behavior change, not just a refactoring. This means:

  1. MLH projects will get half the judging time they previously had
  2. With the default slotDurationMinutes of 10, this changes from scheduling 2 MLH projects per time chunk to 4 MLH projects per time chunk

This is a significant logic change that affects the actual judging schedule and should be:

  • Clearly documented in the PR description explaining why the duration is being changed
  • Potentially made configurable rather than hardcoded
  • Verified with stakeholders that halving MLH judging time is intentional

If this change is intentional, it should be clearly communicated in the PR description.

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/server/router/judging.ts
Line: 896:896

Comment:
This changes the MLH slot duration from 5 minutes to 2.5 minutes, which is a **breaking behavior change**, not just a refactoring. This means:

1. MLH projects will get half the judging time they previously had
2. With the default `slotDurationMinutes` of 10, this changes from scheduling 2 MLH projects per time chunk to 4 MLH projects per time chunk

This is a significant logic change that affects the actual judging schedule and should be:
- Clearly documented in the PR description explaining why the duration is being changed
- Potentially made configurable rather than hardcoded
- Verified with stakeholders that halving MLH judging time is intentional

If this change is intentional, it should be clearly communicated in the PR description.

How can I resolve this? If you propose a fix, please make it concise.

if (input.slotDurationMinutes % MLH_SLOT_MINUTES !== 0) {
throw new TRPCError({
code: "BAD_REQUEST",
message: `slotDurationMinutes (${input.slotDurationMinutes}) must be divisible by MLH slot duration (${MLH_SLOT_MINUTES})`,
});
}
Comment on lines +897 to +902
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The modulo operation with a floating point number (2.5) can have precision issues in JavaScript due to how floating point arithmetic works. While 10 % 2.5 should equal 0, floating point precision errors could cause unexpected failures.

A more robust approach would be to use integer arithmetic:

Suggested change
if (input.slotDurationMinutes % MLH_SLOT_MINUTES !== 0) {
throw new TRPCError({
code: "BAD_REQUEST",
message: `slotDurationMinutes (${input.slotDurationMinutes}) must be divisible by MLH slot duration (${MLH_SLOT_MINUTES})`,
});
}
// MLH slot duration in minutes - must divide evenly into slotDurationMinutes
const MLH_SLOT_MINUTES = 2.5;
// Check divisibility using integer arithmetic to avoid floating point precision issues
const slotMillis = input.slotDurationMinutes * 60 * 1000;
const mlhSlotMillis = MLH_SLOT_MINUTES * 60 * 1000;
if (slotMillis % mlhSlotMillis !== 0) {
throw new TRPCError({
code: "BAD_REQUEST",
message: `slotDurationMinutes (${input.slotDurationMinutes}) must be divisible by MLH slot duration (${MLH_SLOT_MINUTES})`,
});
}

Alternatively, you could multiply both values by 2 to work with integers: (input.slotDurationMinutes * 2) % (MLH_SLOT_MINUTES * 2) !== 0

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/server/router/judging.ts
Line: 897:902

Comment:
The modulo operation with a floating point number (`2.5`) can have precision issues in JavaScript due to how floating point arithmetic works. While `10 % 2.5` should equal `0`, floating point precision errors could cause unexpected failures.

A more robust approach would be to use integer arithmetic:

```suggestion
      // MLH slot duration in minutes - must divide evenly into slotDurationMinutes
      const MLH_SLOT_MINUTES = 2.5;
      // Check divisibility using integer arithmetic to avoid floating point precision issues
      const slotMillis = input.slotDurationMinutes * 60 * 1000;
      const mlhSlotMillis = MLH_SLOT_MINUTES * 60 * 1000;
      if (slotMillis % mlhSlotMillis !== 0) {
        throw new TRPCError({
          code: "BAD_REQUEST",
          message: `slotDurationMinutes (${input.slotDurationMinutes}) must be divisible by MLH slot duration (${MLH_SLOT_MINUTES})`,
        });
      }
```

Alternatively, you could multiply both values by 2 to work with integers: `(input.slotDurationMinutes * 2) % (MLH_SLOT_MINUTES * 2) !== 0`

How can I resolve this? If you propose a fix, please make it concise.


// Separate MLH and non-MLH tracks
const mlhProjectTracks = allProjectTracks.filter(
(pt) => pt.track.name === "MLH",
Expand Down Expand Up @@ -973,7 +982,9 @@ export const timeSlotRouter = router({
);

// Schedule MLH judging slots
const mlhSlotsToSchedule = Math.floor(input.slotDurationMinutes / 5);
const mlhSlotsToSchedule = Math.floor(
input.slotDurationMinutes / MLH_SLOT_MINUTES,
);
let mlhStartTime = currentTimeChunk;

for (
Expand Down Expand Up @@ -1002,9 +1013,7 @@ export const timeSlotRouter = router({
projectId: chosenProject.projectId,
startTime: mlhStartTime,
endTime: new Date(
new Date(mlhStartTime).setMinutes(
mlhStartTime.getMinutes() + 5,
),
mlhStartTime.getTime() + MLH_SLOT_MINUTES * 60 * 1000,
),
dhYear: dhYearConfig.value,
},
Expand Down Expand Up @@ -1036,7 +1045,7 @@ export const timeSlotRouter = router({
});

mlhStartTime = new Date(
new Date(mlhStartTime).setMinutes(mlhStartTime.getMinutes() + 5),
mlhStartTime.getTime() + MLH_SLOT_MINUTES * 60 * 1000,
);
}
currentTimeChunk = new Date(
Expand All @@ -1053,18 +1062,14 @@ export const timeSlotRouter = router({
projectId: mlhProject.projectId,
startTime: currentTimeChunk,
endTime: new Date(
new Date(currentTimeChunk).setMinutes(
currentTimeChunk.getMinutes() + 5, // 5 minute slots for MLH
),
currentTimeChunk.getTime() + MLH_SLOT_MINUTES * 60 * 1000,
),
dhYear: dhYearConfig.value,
},
});

currentTimeChunk = new Date(
new Date(currentTimeChunk).setMinutes(
currentTimeChunk.getMinutes() + 5,
),
currentTimeChunk.getTime() + MLH_SLOT_MINUTES * 60 * 1000,
);
}

Expand Down