#289 Show Alignments on Tournament Pairings Page#292
Merged
ianpaschal merged 2 commits intomainfrom Jan 31, 2026
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedAn error occurred during the review process. Please try again later. WalkthroughAdds alignment- and repeat-aware pairing validation and options, refactors pairing generation to use the validator, exposes pairing types and validator in the public API, adds unit tests, and updates pairing UI components and identity/avatar rendering. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
convex/_model/tournamentPairings/queries/getDraftTournamentPairings.ts (1)
17-24:⚠️ Potential issue | 🟠 MajorMake
optionsoptional to avoid breaking existing callers.The validator now requires
options, which will reject older clients/tests that omit it. Since the options already have defaults downstream, this can remain optional.🛠️ Suggested fix
export const getDraftTournamentPairingsArgs = v.object({ method: tournamentPairingMethod, round: v.number(), tournamentId: v.id('tournaments'), - options: v.object({ - allowSameAlignment: v.optional(v.boolean()), - allowRepeats: v.optional(v.boolean()), - }), + options: v.optional(v.object({ + allowSameAlignment: v.optional(v.boolean()), + allowRepeats: v.optional(v.boolean()), + })), });
🤖 Fix all issues with AI agents
In `@convex/_model/tournamentPairings/_helpers/checkIfPairingIsRepeat.ts`:
- Around line 3-10: Remove the extra blank line between the JSDoc comment and
the function declaration for checkIfPairingIsRepeat; place the JSDoc immediately
above the export const checkIfPairingIsRepeat(...) declaration so the comment is
directly associated with the function.
In
`@convex/_model/tournamentPairings/_helpers/checkIfPairingIsSameAlignment.test.ts`:
- Around line 66-73: The test shows checkIfPairingIsSameAlignment treats two
empty alignment arrays as "same" (returning true), which blocks pairings when
allowSameAlignment is false; change the function checkIfPairingIsSameAlignment
to treat empty or missing alignment arrays as "unknown" (not matching) by
returning false when either competitor has an empty alignments array (or when
alignments are not visible), so empty/missing alignment data does not block
pairings—update the logic inside checkIfPairingIsSameAlignment (and any place it
reads details.alignments or alignmentsVisible) to short-circuit and return false
for empty/missing alignments or add a clear parameter/flag to control that
behavior if callers need explicit toggling.
In `@convex/_model/tournamentPairings/_helpers/checkIfPairingIsSameAlignment.ts`:
- Around line 13-33: The function checkIfPairingIsSameAlignment currently treats
empty alignment arrays as a conflict; update it so missing alignment data allows
pairing by adding an early check on a.details.alignments and
b.details.alignments: if either aAlignments.length === 0 || bAlignments.length
=== 0 return false (meaning not same alignment / allow pairing). Keep the
existing checks for 'flexible', multiple alignments, and differing single
alignments, but ensure the new empty-array guard runs before those checks to
avoid incorrectly blocking pairings.
In `@convex/_model/tournamentPairings/_helpers/validateTournamentPairing.test.ts`:
- Line 79: Create a tracked issue for the "TODO: Tests beyond here may need some
clean-up" in
convext/_model/tournamentPairings/_helpers/validateTournamentPairing.test.ts:
open a ticket in our tracker describing what needs cleanup (scope, why, rough
acceptance criteria) and then update the TODO comment in
validateTournamentPairing.test.ts to include the new issue number or link (e.g.,
"TODO (ISSUE-123): ...") so future readers can find the task; also add a brief
note in the PR description referencing the new issue.
In `@convex/_model/tournamentPairings/queries/getDraftTournamentPairings.ts`:
- Around line 47-56: The code computes orderBy from args.method but doesn't
normalize the AdjacentAlignment enum variant, so passing AdjacentAlignment
leaves orderedCompetitors empty; update the derivation of orderBy (used in
getDraftTournamentPairings where args.method, TournamentPairingMethod, and
orderBy are referenced) to map AdjacentAlignment to
TournamentPairingMethod.Adjacent before the branch checks (or otherwise
normalize args.method into Adjacent) so the existing branches that call
sortByRank and shuffle and then pass orderedCompetitors into
generateDraftPairings and sortCompetitorPairs run as intended.
In
`@src/components/TournamentCompetitorIdentity/TournamentCompetitorIdentity.tsx`:
- Around line 69-78: The button in the TournamentCompetitorIdentity component
currently relies on the default type (submit); update the JSX for the button
element in TournamentCompetitorIdentity to include an explicit type="button"
attribute (next to className, onClick, disabled, data-clickable) so clicks won't
trigger form submits; keep handleClick and the existing disabled/data-clickable
logic unchanged.
- Around line 64-66: The click handler currently calls onClick without passing
the competitor id; update handleClick to pass the resolved competitor id to the
callback (e.g., change onClick?.() to onClick?.(competitorId) or onClick?.(id)
depending on the local variable that holds the resolved id). Ensure you use the
existing identifier for the resolved competitor id in scope so consumers of the
onClick prop receive the id.
In `@src/components/TournamentProvider/TournamentProvider.hooks.tsx`:
- Line 43: The change commented out useSubmitMatchResultAction(subject) in
TournamentProvider.hooks.tsx removes match result submission and is unrelated to
the pairing alignment PR; either revert that change (uncomment and restore
calling useSubmitMatchResultAction with the same subject so MatchResult
submission behavior remains intact) or move the removal into a separate
branch/PR and leave a clear TODO comment referencing the need to migrate the
match result form; also open a tracking issue that mentions
useSubmitMatchResultAction and the match result form migration so the broken
functionality is filed for follow-up.
In `@src/pages/TournamentPairingsPage/TournamentPairingsPage.tsx`:
- Around line 21-25: pairingOptions currently uses a stale allowSameAlignment
value instead of reflecting tournament.pairingMethod; update pairingOptions
wherever it's built or used (including the pairing-method dropdown change
handler, draft generation call, and status validation path) to compute
allowSameAlignment = tournament.pairingMethod === 'AdjacentAlignment' (or the
correct enum/string for adjacent) based directly on tournament.pairingMethod
before creating TournamentPairingOptions/DraftTournamentPairing; ensure the
dropdown onChange re-computes pairingOptions and passes the fresh object into
the draft generation and status validation routines so switching methods
immediately changes allowed same-alignment behavior.
Comment on lines
+3
to
+10
| /** | ||
| * Checks if two competitors have already played each other. | ||
| */ | ||
|
|
||
| export const checkIfPairingIsRepeat = ( | ||
| a: DeepTournamentCompetitor, | ||
| b: DeepTournamentCompetitor, | ||
| ): boolean => a.opponentIds.includes(b._id) || b.opponentIds.includes(a._id); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: Remove extra blank line between JSDoc and function.
There's an extra blank line (Line 6) between the JSDoc comment and the function declaration.
🧹 Suggested cleanup
/**
* Checks if two competitors have already played each other.
*/
-
export const checkIfPairingIsRepeat = (
a: DeepTournamentCompetitor,
b: DeepTournamentCompetitor,
): boolean => a.opponentIds.includes(b._id) || b.opponentIds.includes(a._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.
Suggested change
| /** | |
| * Checks if two competitors have already played each other. | |
| */ | |
| export const checkIfPairingIsRepeat = ( | |
| a: DeepTournamentCompetitor, | |
| b: DeepTournamentCompetitor, | |
| ): boolean => a.opponentIds.includes(b._id) || b.opponentIds.includes(a._id); | |
| /** | |
| * Checks if two competitors have already played each other. | |
| */ | |
| export const checkIfPairingIsRepeat = ( | |
| a: DeepTournamentCompetitor, | |
| b: DeepTournamentCompetitor, | |
| ): boolean => a.opponentIds.includes(b._id) || b.opponentIds.includes(a._id); |
🤖 Prompt for AI Agents
In `@convex/_model/tournamentPairings/_helpers/checkIfPairingIsRepeat.ts` around
lines 3 - 10, Remove the extra blank line between the JSDoc comment and the
function declaration for checkIfPairingIsRepeat; place the JSDoc immediately
above the export const checkIfPairingIsRepeat(...) declaration so the comment is
directly associated with the function.
convex/_model/tournamentPairings/_helpers/checkIfPairingIsSameAlignment.test.ts
Show resolved
Hide resolved
convex/_model/tournamentPairings/_helpers/checkIfPairingIsSameAlignment.ts
Show resolved
Hide resolved
| }); | ||
| }); | ||
|
|
||
| // TODO: Tests beyond here may need some clean-up: |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider tracking this TODO.
The TODO suggests these tests may need cleanup. Consider creating an issue to track this technical debt.
Would you like me to open a new issue to track this cleanup task?
🤖 Prompt for AI Agents
In `@convex/_model/tournamentPairings/_helpers/validateTournamentPairing.test.ts`
at line 79, Create a tracked issue for the "TODO: Tests beyond here may need
some clean-up" in
convext/_model/tournamentPairings/_helpers/validateTournamentPairing.test.ts:
open a ticket in our tracker describing what needs cleanup (scope, why, rough
acceptance criteria) and then update the TODO comment in
validateTournamentPairing.test.ts to include the new issue number or link (e.g.,
"TODO (ISSUE-123): ...") so future readers can find the task; also add a brief
note in the PR description referencing the new issue.
convex/_model/tournamentPairings/queries/getDraftTournamentPairings.ts
Outdated
Show resolved
Hide resolved
src/components/TournamentCompetitorIdentity/TournamentCompetitorIdentity.tsx
Outdated
Show resolved
Hide resolved
src/components/TournamentCompetitorIdentity/TournamentCompetitorIdentity.tsx
Show resolved
Hide resolved
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@src/components/TournamentCompetitorIdentity/TournamentCompetitorIdentity.tsx`:
- Around line 62-79: The button is still focusable/announced interactive when no
onClick is provided; update the TournamentCompetitorIdentity component so the
button is disabled whenever onClick is undefined (in addition to the existing
showDisabled check). Change the disabled logic (useDisabledName) to incorporate
onClick (e.g., disabled = showDisabled || !onClick), ensure handleClick remains
safe (it already uses onClick?.(...)) and data-clickable reflects !!onClick so
assistive tech and styles correctly indicate non-interactive state; adjust any
className/attribute logic tied to interactivity accordingly.
src/components/TournamentCompetitorIdentity/TournamentCompetitorIdentity.tsx
Show resolved
Hide resolved
ianpaschal
deleted the
289-show-alignments-on-tournament-pairings-page
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #289
Summary by CodeRabbit
New Features
Refactor
Style
Tests
✏️ Tip: You can customize this high-level summary in your review settings.