-
Notifications
You must be signed in to change notification settings - Fork 2
Fix/judge redistribution #346
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…orithm checks for that)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request modifies the judge-to-team matching algorithm to assign 2 judges per match instead of 3, and implements logic to avoid re-pairing judges with teams they've previously evaluated. The changes retrieve existing judge-team pairings from the submissions collection and use them to prevent duplicate assignments when generating new matches.
Changes:
- Reduced the number of judges assigned per team from 3 to 2
- Added logic to fetch previous judge-team pairings and prevent duplicate assignments
- Updated validation logic to reflect the new 2-judge requirement
- Removed safety check for existing submissions to allow re-matching
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| app/(api)/_utils/matching/judgesToTeamsAlgorithm.ts | Changed rounds from 3 to 2, integrated GetJudgeToTeamPairings to retrieve existing pairings, added String() conversions for ID comparison, and implemented splice logic to remove previous pairings after matching |
| app/(api)/_datalib/judgeToTeam/getJudgeToTeamPairings.ts | New function that retrieves all existing judge-team pairings from the submissions collection and converts them to the JudgeToTeam format |
| app/(api)/_actions/logic/matchTeams.ts | Commented out the check that prevented matching when submissions exist, added debug logging, and updated error messages |
| app/(api)/_actions/logic/checkMatches.ts | Updated validation to require 2 judges per team instead of 3 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Also add a comment so Copilot Code Review doesn't complain again.
|
Seems ready to merge, will just check w copilot once |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const maxRounds = 4; | ||
| const maxSubmissions = teams.length * maxRounds; | ||
| const isSecondRound = existingCount > 0; | ||
|
|
||
| if (existingCount >= maxSubmissions) { | ||
| return { | ||
| ok: false, | ||
| body: null, | ||
| error: `GetManyTeams error: ${teamsRes.error}`, | ||
| error: | ||
| 'Two rounds have already been completed. Clear submissions to rerun.', | ||
| }; |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable name maxRounds is misleading. It's set to 4, but it actually represents the maximum total number of judge assignments per team across multiple executions, not the number of rounds per execution (which is 2). Consider renaming to maxJudgesPerTeam or maxTotalAssignments for clarity. Additionally, the error message "Two rounds have already been completed" is confusing because it suggests two execution rounds, but the check allows for exactly that (2 executions × 2 judges per execution = 4 total). Consider changing the message to "Maximum judge assignments reached (4 judges per team). Clear submissions to rerun."
| if (!previousPairings.ok || !previousPairings.body) { | ||
| throw new Error( | ||
| `Failed to load existing judge-to-team pairings: ${ | ||
| previousPairings.error ?? 'Unknown error' | ||
| }` | ||
| ); | ||
| } |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check for !previousPairings.body is redundant and potentially confusing. When previousPairings.ok is true, the body will always be an array (empty or populated), never null. The current condition correctly handles the empty array case (no previous pairings) since an empty array is truthy. However, for clarity and type safety, consider removing the !previousPairings.body check and relying solely on !previousPairings.ok, or explicitly check for null if that's the intended behavior.
| @@ -65,7 +66,7 @@ export default async function matchTeams( | |||
| return { | |||
| ok: false, | |||
| body: null, | |||
| error: 'Invalid submissions.', | |||
| error: 'Invalid submissions.2', | |||
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error messages 'Invalid submissions.1' and 'Invalid submissions.2' are not descriptive enough for debugging. Consider using more specific messages that explain what validation failed or include the actual error from the underlying operation. For example, 'Invalid submissions.1' could be 'Failed to create submissions in database' and include the actual error details.
| // Remove the previous pairings | ||
| if (previousPairings.ok && previousPairings.body) { | ||
| judgeToTeam.splice(0, previousPairings.body.length); |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The splice operation that removes previous pairings from the judgeToTeam array is fragile and relies on the array maintaining insertion order. While this currently works because only push operations are used, consider a more robust approach such as tracking the index where new pairings start, or filtering out previous pairings by creating a Set of previous pairing IDs and filtering the array based on that. This would make the code more maintainable and less prone to bugs if the array manipulation logic changes in the future.
| // Remove the previous pairings | |
| if (previousPairings.ok && previousPairings.body) { | |
| judgeToTeam.splice(0, previousPairings.body.length); | |
| // Remove the previous pairings without relying on array insertion order. | |
| if (previousPairings.ok && previousPairings.body) { | |
| const previousPairingKeySet = new Set( | |
| previousPairings.body.map((pairing) => { | |
| const judgeId = String(pairing.judge_id); | |
| const teamId = String(pairing.team_id); | |
| return `${judgeId}::${teamId}`; | |
| }) | |
| ); | |
| const filteredJudgeToTeam = judgeToTeam.filter((entry) => { | |
| const judgeId = String(entry.judge_id); | |
| const teamId = String(entry.team_id); | |
| const key = `${judgeId}::${teamId}`; | |
| return !previousPairingKeySet.has(key); | |
| }); | |
| // Mutate the existing array in place so any references remain valid. | |
| judgeToTeam.length = 0; | |
| judgeToTeam.push(...filteredJudgeToTeam); |
ReehalS
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not having Copilot review it again otherwise this PR will never get merged. Its comments so far have been mostly useful but doing more will lead to this getting delayed more and more
changed to 2 judges assigned per match
to match again: then get prev pairings and not pair them, move onto next judge that isnt already paired with them