Skip to content

feat: add dynamic mlh timing#434

Merged
Krish120003 merged 6 commits into
mainfrom
arian/make-mlh-time-dynamic
Jan 11, 2026
Merged

feat: add dynamic mlh timing#434
Krish120003 merged 6 commits into
mainfrom
arian/make-mlh-time-dynamic

Conversation

@arian81
Copy link
Copy Markdown
Member

@arian81 arian81 commented Jan 11, 2026

No description provided.

@vercel
Copy link
Copy Markdown

vercel Bot commented Jan 11, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Review Updated (UTC)
portal Ready Ready Preview, Comment Jan 11, 2026 4:26pm

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Jan 11, 2026

Greptile Overview

Greptile Summary

This PR modifies the MLH (Major League Hacking) judging slot scheduling logic by:

  1. Extracting a constant for MLH slot duration (MLH_SLOT_MINUTES = 2.5)
  2. Changing the behavior from 5-minute slots to 2.5-minute slots
  3. Adding validation to ensure slotDurationMinutes is divisible by the MLH slot duration
  4. Refactoring date arithmetic from using setMinutes() to using getTime() + milliseconds for better clarity

Key Concerns

Breaking Behavior Change

The most significant issue is that this PR changes the MLH slot duration from 5 minutes to 2.5 minutes, which:

  • Reduces judging time per MLH project by 50%
  • Doubles the number of MLH projects scheduled per time chunk (from 2 to 4 with the default 10-minute slot duration)
  • Is not mentioned in the PR title/description as a breaking change

Missing Issue Reference

The PR description does not include "resolves #issue-id" to link back to a corresponding issue, which violates the project's PR template requirements (rule: 683a435d-4a37-4a5b-bdba-1a62dd037f86).

Floating Point Precision

The validation uses modulo operation with a floating point number (2.5), which can have precision issues in JavaScript.

Positive Changes

  • The date arithmetic refactoring using getTime() + milliseconds is cleaner and more explicit than the previous setMinutes() approach
  • Extracting the constant improves code maintainability
  • Adding validation prevents invalid configurations

Confidence Score: 2/5

  • This PR has significant concerns including a breaking behavior change that's not clearly communicated and potential floating point precision issues
  • Score of 2/5 reflects: (1) An undocumented breaking change that reduces MLH judging time by 50%, (2) Floating point modulo validation that could have precision issues, (3) Missing PR template requirement for issue linking. While the date arithmetic refactoring is an improvement, the behavior change needs to be intentional and clearly documented before merging.
  • src/server/router/judging.ts requires careful review - specifically the change from 5-minute to 2.5-minute MLH slots and the floating point validation logic

Important Files Changed

File Analysis

Filename Score Overview
src/server/router/judging.ts 2/5 Changes MLH slot duration from 5 to 2.5 minutes (breaking change), extracts constant, and refactors date arithmetic. Contains logic issues with behavior change and floating point validation.

Sequence Diagram

sequenceDiagram
    participant Admin as Admin User
    participant Frontend as Frontend (index.tsx)
    participant API as createTimeSlots API
    participant Validation as Validation Logic
    participant Scheduler as Scheduling Logic
    participant DB as Database

    Admin->>Frontend: Configure judging schedule
    Frontend->>API: createTimeSlots(startTime, slotDurationMinutes=10)
    
    API->>Validation: Check slotDurationMinutes % MLH_SLOT_MINUTES
    alt Invalid (not divisible by 2.5)
        Validation-->>API: Throw BAD_REQUEST error
        API-->>Frontend: Error response
    else Valid
        Validation->>Scheduler: Proceed with scheduling
        
        Scheduler->>DB: Clear existing time slots
        Scheduler->>DB: Fetch tables & project tracks
        
        loop For each time chunk
            loop For each table
                Scheduler->>Scheduler: Find most starved project for track
                Scheduler->>DB: Create time slot (slotDurationMinutes)
            end
            
            Scheduler->>Scheduler: Calculate MLH slots to schedule
            Note right of Scheduler: mlhSlotsToSchedule = floor(10 / 2.5) = 4
            
            loop For each MLH slot (up to 4)
                Scheduler->>Scheduler: Find most starved MLH project
                Scheduler->>DB: Create MLH time slot (2.5 minutes)
                Scheduler->>Scheduler: Increment mlhStartTime by 2.5 min
            end
            
            Scheduler->>Scheduler: Increment currentTimeChunk by slotDurationMinutes
        end
        
        loop For remaining MLH projects
            Scheduler->>DB: Create MLH time slot (2.5 minutes)
        end
        
        Scheduler-->>API: Return success with endTime & numTables
        API-->>Frontend: Display schedule summary
    end
Loading

Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot left a comment

Choose a reason for hiding this comment

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

2 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

}

// 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.

Comment on lines +897 to +902
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})`,
});
}
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.

@Krish120003 Krish120003 merged commit 2238ee3 into main Jan 11, 2026
11 checks passed
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