#293 Add Tournament Pairing Options Form to Tournament Pairings Page#297
Merged
ianpaschal merged 19 commits intomainfrom Feb 9, 2026
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaces options-based pairing controls with a policies/config model, adds server actions for draft pairing generation and table assignments, introduces pairing configuration UI and form components, strengthens null-safety in helpers, refactors table-assignment logic, and updates page flows and exports. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
…ournament-pairings-page
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 35
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/components/generic/Switch/Switch.scss (1)
34-40:⚠️ Potential issue | 🟡 MinorDuplicate
&-disabledblock.Lines 34–36 and 38–40 are identical. One should be removed.
Proposed fix
&-disabled { opacity: 0.4; // TODO: THIS IS SUPER HACKY } - - &-disabled { - opacity: 0.4; // TODO: THIS IS SUPER HACKY - }src/components/TournamentForm/components/FormatFields.module.scss (1)
118-140:⚠️ Potential issue | 🔴 CriticalMedia query selectors still use the old
.FormatFields_prefix — responsive styles are broken.The root class was renamed to
.PairingFieldson line 12, but the media queries on lines 118–140 still reference.FormatFields_RoundStructureFields,.FormatFields_RoundStructureLegend,.FormatFields_RoundsSection, and.FormatFields_PairingSection. These selectors will never match, so all responsive layout rules are silently dead.Proposed fix
`@media` (width >= 480px) { - .FormatFields_RoundStructureFields { + .PairingFields_RoundStructureFields { `@include` flex.column; } - .FormatFields_RoundStructureLegend { + .PairingFields_RoundStructureLegend { `@include` flex.column($gap: 0.5rem); } - .FormatFields_RoundsSection { + .PairingFields_RoundsSection { display: grid; grid-template-columns: 1fr 1fr; gap: 1rem; } } `@media` (width >= 688px) { - .FormatFields_PairingSection { + .PairingFields_PairingSection { display: grid; grid-template-columns: 1fr 1fr; gap: 1rem; } }convex/_model/tournamentPairings/_helpers/validateTournamentPairing.ts (1)
8-16:⚠️ Potential issue | 🟡 MinorStale JSDoc:
@param optionsshould be@param policies.The parameter was renamed from
optionstopoliciesbut the doc comment still references the old name and description.Proposed fix
/** * Validates a tournament pairing and returns a status with message. * - * `@param` a - First competitor (or null for bye) - * `@param` b - Second competitor (or null for bye) - * `@param` options - Pairing options (allowRepeats, allowSameAlignment) + * `@param` policies - Pairing policies (repeat, sameAlignment) + * `@param` a - First competitor (or null for bye) + * `@param` b - Second competitor (or null for bye) * `@param` table - Optional table assignment for table-related warnings * `@returns` TournamentPairingStatus with 'error', 'warning', or 'ok' status */src/components/TournamentForm/TournamentForm.schema.ts (1)
92-99:⚠️ Potential issue | 🟠 MajorStale error path:
pairingMethodshould bepairingConfig.The field was renamed from
pairingMethodtopairingConfig(line 83), but the error path on line 97 still references the old name. This will prevent the validation error from surfacing on the correct form field.Proposed fix
- path: ['pairingMethod'], + path: ['pairingConfig'],convex/_model/tournamentPairings/queries/getDraftTournamentPairings.ts (1)
22-29:⚠️ Potential issue | 🟡 MinorStale JSDoc:
@param args.methodno longer exists.The
methodparameter was replaced byconfig. Update the doc comment accordingly.Proposed fix
* `@param` ctx - Convex query context * `@param` args - Convex query args - * `@param` args.method - The pairing method to use + * `@param` args.config - The pairing configuration including orderBy and policies * `@param` args.round - The upcoming round index which the generated pairing are for * `@param` args.tournamentId - ID of the Tournamentconvex/_model/tournamentPairings/_helpers/generateDraftPairings.ts (1)
14-24:⚠️ Potential issue | 🟡 MinorStale JSDoc —
@paramand@returnsare outdated.
@param allowRepeatsno longer exists (nowpolicies), and the@returnsdescription is truncated ("An array of "). Please update to reflect the current signature.📝 Proposed fix
* `@param` orderedCompetitors - Ordered array of competitors - * `@param` allowRepeats - Allow repeat match-ups - * `@returns` An array of + * `@param` policies - Pairing constraint policies (repeat, sameAlignment) + * `@returns` An array of competitor pairs
🤖 Fix all issues with AI agents
In `@convex/_model/tournamentPairings/_helpers/assignTables.tsx`:
- Around line 1-4: This file has no JSX but is named assignTables.tsx; rename
the file to assignTables.ts and update all imports that reference this module
accordingly (search for imports pointing to
tournamentPairings/_helpers/assignTables or similar). Ensure the module exports
and any TypeScript typings (e.g., uses of Id, DeepTournamentCompetitor,
TournamentDeep, DraftTournamentPairing) continue to compile after the rename;
run a quick build or typecheck to catch any stale .tsx references.
- Around line 36-72: The loop over draftPairings currently drops pairings with
both competitors null and also normalizes byes to always place the single
competitor into tournamentCompetitor0Id; update the logic in the loop (the block
that references draftPairings, pairing, manuallyAssignedPairings,
autoAssignedPairings, and availableTables) to explicitly handle ids.length === 0
(either skip with a clear comment or push a null-table entry so the pairing
isn’t silently lost) and change the ids.length === 1 branch so it preserves the
original slot positions (use pairing.tournamentCompetitor0Id and
pairing.tournamentCompetitor1Id as-is rather than reordering to always put the
non-null into slot 0) to avoid discarding slot information.
- Line 20: The current tableCount computation uses
(data.tournament?.competitorCount ?? 2) / 2 which is fragile; change it to
derive tableCount from actual data: prefer an explicit tournament.tableCount if
present, otherwise count active/eligible competitors (filter by
status/active/inactive and exclude byes) and compute
Math.ceil(activeCompetitorCount / 2) to handle odd numbers, and fall back to 1
or the existing default if neither is available; update any validation/logic
that uses the tableCount variable (the code around assignTables and the
validation that throws on over-allocation) to use the new tableCount derivation
so byes, inactive competitors, and manual table assignments are handled
correctly.
- Around line 79-83: The loop rebuilds Array.from(availableTables) on every
iteration causing O(n^2) behavior; convert the Set once to an array (e.g., const
tables = Array.from(availableTables)) before the loop and then pick and remove
entries from that array (use pop() or swap-with-last-and-pop or splice at random
index) while assigning to autoAssignedPairings[i].table and optionally keep
availableTables in sync if needed; update references to randomTable and remove
the per-iteration Array.from call in the assignTables logic.
In
`@convex/_model/tournamentPairings/_helpers/checkIfPairingIsSameAlignment.test.ts`:
- Around line 66-72: Add unit tests to cover the new null-guard behavior in
checkIfPairingIsSameAlignment: create cases where the first competitor is null,
the second competitor is null, and both are null, and assert that
checkIfPairingIsSameAlignment returns false for each. Use the existing test
pattern in _helpers/checkIfPairingIsSameAlignment.test.ts (reusing
createMockTournamentCompetitor for non-null arguments) and name the tests
clearly (e.g., "returns false if first competitor is null", "returns false if
second competitor is null", "returns false if both competitors are null") so the
new guard clause is exercised.
In `@convex/_model/tournamentPairings/_helpers/deepenTournamentPairing.ts`:
- Around line 56-64: The playerUserIds array can include undefined because both
tournamentCompetitor0.registrations and tournamentCompetitor1.registrations map
r.user?._id; update the construction of playerUserIds (in
deepenTournamentPairing) to exclude undefined values by mapping to the id then
filtering out null/undefined (or use flatMap to conditionally include r.user._id
only when present) so the resulting array is strictly Id<'users'>[]; ensure you
reference tournamentCompetitor0.registrations,
tournamentCompetitor1.registrations and the playerUserIds field when making the
change.
In `@convex/_model/tournamentPairings/_helpers/generateDraftPairings.test.ts`:
- Line 62: The forEach callback in the test uses a parenthesized assignment `(p)
=> (p.byeRounds = [ 1 ])` which implicitly returns a value; change it to a
block-bodied callback so it does not return anything — locate the
`competitors.forEach` call in generateDraftPairings.test and replace the arrow
function with a block body that performs the assignment (e.g., use `{
p.byeRounds = [1]; }`) so the callback returns void.
- Around line 15-18: The parameter name in generateDraftPairings (function in
generateDraftPairings.ts) is misspelled as "polices" — update it to "policies"
to match the TournamentPairingPolicies type and the test's defaultPolicies
variable; change the function signature parameter name, any internal references
to that parameter, and any exported/declared types or calls that use "polices"
so everything consistently uses "policies" (ensure usages in tests and callers
are updated to reference the corrected name).
In `@convex/_model/tournamentPairings/_helpers/generateDraftPairings.ts`:
- Line 27: Rename the misspelled parameter "polices" to "policies" in the
generateDraftPairings function signature (type TournamentPairingPolicies) and
update every use of that identifier inside the function (including recursivePair
calls and any references to policies within generateDraftPairings) so they all
reference "policies"; ensure the renamed parameter name matches the one used by
recursivePair and any other internal references to avoid inconsistency.
In `@convex/_model/tournamentPairings/_helpers/validateTournamentPairing.test.ts`:
- Around line 26-29: Tests for validateTournamentPairing only cover
TournamentPairingPolicy.Block and Allow, missing the middle-tier behavior; add
test cases that call validateTournamentPairing with
TournamentPairingPolicy.Avoid (for relevant combinations used in existing tests
around lines shown) and assert the result status is "warning" (or the project's
equivalent warning indicator) to verify the Avoid branch; update all similar
test blocks that currently use Block/Allow pairs (the ones covering repeat and
sameAlignment scenarios and the other spots called out) to include parallel
Avoid assertions so the Avoid behavior is exercised for
validateTournamentPairing.
In `@convex/_model/tournamentPairings/_helpers/validateTournamentPairing.ts`:
- Around line 23-26: The code defensively uses "policies ?? { repeat: ...,
sameAlignment: ... }" while the parameter is declared non-nullable
TournamentPairingPolicies; remove the nullish fallback and destructure directly
from policies (const { repeat, sameAlignment } = policies) so runtime behavior
matches the type signature, and ensure the function (validateTournamentPairing)
keeps policies as non-nullable; alternatively, if you prefer allowing null,
change the parameter type to TournamentPairingPolicies | null and update callers
to handle/validate nulls—but do not keep the current mismatch.
In `@convex/_model/tournamentPairings/actions/generateDraftTournamentPairings.ts`:
- Around line 35-41: The ordering logic in generateDraftTournamentPairings is
using two independent ifs so orderedCompetitors may remain empty when
args.config.orderBy is neither 'ranking' nor 'random'; change the block that
sets orderedCompetitors (currently using sortByRank and shuffle) to an if / else
if / else sequence and in the final else throw a clear error (including the
invalid args.config.orderBy value) so the function fails fast instead of
returning zero pairings silently.
In `@convex/_model/tournamentPairings/actions/generateTableAssignments.ts`:
- Around line 13-17: The schema export generateTableAssignmentsArgs declares
args.round but the value is never used; either remove v.optional(v.number())
from generateTableAssignmentsArgs to avoid an unused arg, or thread args.round
into the downstream call-site (e.g., pass args.round into the
generateTableAssignments handler or any helper like assignTablesForRound) so the
round value is consumed; update the matching function signature and any callers
to accept and use the round parameter if you choose to keep it.
In `@convex/_model/tournamentPairings/mutations/createTournamentPairings.ts`:
- Line 80: The code uses unnecessary optional chaining on competitor after a
null-check; in the createTournamentPairings mutation where you already guard
that competitor is non-null (the check just above), replace competitor?.active
with competitor.active to avoid the misleading suggestion that competitor may
still be null.
In `@convex/_model/tournamentPairings/queries/getDraftTournamentPairings.ts`:
- Around line 41-47: The current logic in getDraftTournamentPairings builds
orderedCompetitors only for 'ranking' or 'random', so if args.config.orderBy has
any other value orderedCompetitors stays empty; update the code around the
orderedCompetitors construction to handle invalid values explicitly (e.g.,
replace the two ifs with a switch or an if/else-if/else) and in the else branch
throw a descriptive error referencing args.config.orderBy (or validate earlier)
so callers fail fast; keep use of sortByRank and shuffle for their respective
branches and ensure orderedCompetitors is always populated or an exception is
raised.
In `@convex/_model/utils/createTestTournamentMatchResults.ts`:
- Around line 72-101: Remove the commented-out legacy block that looks up
tournamentRegistrations and sets playerData.player0UserId/player1UserId or
placeholders (the ~30 lines referencing competitor0Id/competitor1Id,
tournamentCompetitor0Registrations/tournamentCompetitor1Registrations, and
usedPlayerIds) from createTestTournamentMatchResults; simply delete that dead
code so the function contains only the active logic (leave surrounding variables
like pairing, playerData, and usedPlayerIds intact).
- Around line 57-71: The loop over [pairing.tournamentCompetitor0Id,
pairing.tournamentCompetitor1Id] currently skips falsy IDs so a "Bye"
placeholder for competitor1 is lost; update the loop in
createTestTournamentMatchResults so when id is falsy you assign
player{index}Placeholder = 'Bye' (or specifically restore the original asymmetry
by only assigning player1Placeholder = 'Bye' when index === 1) instead of
skipping — locate the for (const [index, id] ...) block and add the appropriate
else branch that sets player0Placeholder/player1Placeholder based on index.
In `@src/components/generic/Checkbox/Checkbox.tsx`:
- Line 19: The Checkbox component currently declares a loading?: boolean prop
(in the CheckboxProps interface) but it is never used; either implement the
behavior or remove/comment it: if you want loading semantics, use the loading
prop inside the Checkbox component to disable the input, set
aria-busy/aria-disabled, and optionally render a small spinner or loading
indicator adjacent to the checkbox and ensure onChange is suppressed while
loading; if the prop exists only for API conformance, add an inline comment in
the CheckboxProps and component explaining it is intentionally unused and why,
or remove the prop from the interface and all call sites to avoid dead public
API.
- Line 33: The default prop for variant in Checkbox.tsx is invalid: change the
default from 'solid' to a valid ElementVariant member (e.g., 'primary') so it
matches the variant?: ElementVariant type; update the default assignment where
variant = 'solid' is declared to use 'primary' (or another of 'secondary' |
'outlined' | 'ghost') so the prop type and default align.
In `@src/components/generic/Form/FormField.tsx`:
- Line 76: The prop named "loading" is being spread onto children via
cloneElement in FormField.tsx and can trigger React DOM warnings when children
are native elements; update the code around the cloneElement usage (where
"loading" is included) to either rename the prop to "isLoading" throughout
(props, callers, and usages) or, if keeping the name, filter it out when cloning
native DOM elements by checking the child type (e.g., use
React.isValidElement(child) and if typeof child.type === 'string' omit the
loading prop before cloning); ensure the same change is applied to the other
spread locations referenced (lines ~86-91) so native elements never receive the
loading attribute.
In `@src/components/TournamentForm/components/FormatFields.tsx`:
- Around line 24-28: The effect in FormatFields uses the whole pairingConfig
object which is unstable and causes reruns; change the effect to depend on the
primitive sameAlignment by using watch('pairingConfig.policies.sameAlignment')
(or store that value in a const) and update the effect to
setValue('registrationDetails.alignment','required') when sameAlignment !==
TournamentPairingPolicy.Allow and include an else branch that resets alignment
(e.g., setValue('registrationDetails.alignment','optional' or an empty string)
when sameAlignment === TournamentPairingPolicy.Allow); keep references to
setValue and TournamentPairingPolicy unchanged so you can locate the effect
easily.
In `@src/components/TournamentForm/components/PairingFields.tsx`:
- Around line 19-20: The component is subscribing to the entire form by calling
watch() which causes unnecessary re-renders; update PairingFields to only
subscribe to the specific field by replacing the current usage of watch() that
destructures pairingConfig with a direct call watch('pairingConfig') (keep
setValue from useFormContext as-is) so pairingConfig is obtained via const
pairingConfig = watch('pairingConfig') and the component no longer subscribes to
the whole form.
In
`@src/components/TournamentPairingConfigForm/TournamentPairingConfigFields.hooks.ts`:
- Around line 24-25: Change the untyped useFormContext call to use the form
generic so it returns properly typed methods: call useFormContext<TFormValues>()
so watch, setValue and reset have the correct types; keep the existing cast for
currentConfig where watch() returns the whole form (so leave the cast to
TournamentPairingConfig for currentConfig), but remove the unnecessary casts on
usages of setValue and reset now that useFormContext is typed (update any
occurrences referencing setValue/reset in this file to rely on the typed
signatures).
In
`@src/components/TournamentPairingConfigForm/TournamentPairingConfigFields.tsx`:
- Around line 55-58: The FormField in TournamentPairingConfigFields inside the
TabsContent with value "preset" intentionally omits a name to avoid
react-hook-form Controller because the value is derived and controlled by
usePresetField; add a concise inline comment above that FormField explaining
that omission and referencing usePresetField and presetFieldProps to clarify
this is a virtual selector managed externally, so future readers know the
missing name is deliberate.
- Around line 43-44: Replace the loose FIXME with a tracked task: open an issue
in the project tracker describing the need to add back-end support for the Avoid
pairing policy and note expected API/behavior, then update the comment in
TournamentPairingConfigFields.tsx above the
getTournamentPairingPolicyOptions().filter(...) line to reference that issue
(e.g., "TODO: track backend support for TournamentPairingPolicy.Avoid — see
ISSUE-123") and include a short note to remove the filter when the back-end
implements TournamentPairingPolicy.Avoid; ensure the issue ID is used in the
comment so the TODO is discoverable.
- Around line 35-38: The tabs array is being recreated on every render; hoist it
to module scope as a constant (e.g., rename to TABS) and replace the
in-component reference to tabs with TABS; locate the current tabs declaration in
TournamentPairingConfigFields (the component in
TournamentPairingConfigFields.tsx), move the array definition outside the
component function, export or keep it as a plain const TABS, and update any JSX
that uses tabs to use TABS instead.
In `@src/components/TournamentPairingConfigForm/TournamentPairingConfigForm.tsx`:
- Around line 55-57: The showLoading computation unnecessarily creates a
single-element array and calls .some(); replace the array pattern with a direct
boolean conversion of loading (e.g., use !!loading or Boolean(loading)) to
simplify the logic in TournamentPairingConfigForm where showLoading is defined,
and if you intend to add more loading flags later, add a brief comment above
showLoading explaining the future expansion.
- Around line 34-41: The form's defaultValues are only applied on mount, causing
stale values when async props change; inside TournamentPairingConfigForm add a
useEffect that watches existingValues and forcedValues (and form) and calls
form.reset(...) with the merged object
({...tournamentPairingConfig.defaultValues, ...existingValues, ...forcedValues})
so the useForm instance (referenced as form) is updated when those props change.
In `@src/pages/TournamentPairingsPage/TournamentPairingsPage.module.scss`:
- Line 36: The CSS declaration in TournamentPairingsPage.module.scss has an
extra space in the grid-template-columns value; update the grid-template-columns
property (in the rule that currently reads `grid-template-columns: auto 6rem
1fr;`) to remove the double space so it becomes `auto 6rem 1fr` to fix the
cosmetic spacing issue.
In `@src/pages/TournamentPairingsPage/TournamentPairingsPage.tsx`:
- Line 69: existingValues is stuck using getDefaultValues(tournament) on every
render so validation via getPairingsStatuses uses stale defaults; instead add a
state variable (e.g., activePairingConfig) initialized from
getDefaultValues(tournament), update that state inside handleRegenerate with the
current values emitted from TournamentPairingConfigForm, and replace usages of
existingValues in calls to getPairingsStatuses (and any other validation logic)
to use activePairingConfig so validations reflect the user-selected config.
- Around line 74-98: The dialog content currently closes over
tournamentCompetitors when creating the generateTableAssignments onSuccess
handler, which can yield stale columns; update onSuccess to compute columns at
dialog-open time (not at handler-definition time) by passing a function or thunk
into openConfirmCreateDialog that calls getTableColumns(tournamentCompetitors ??
[]) when the dialog actually renders (or build the columns inside the content
render callback), e.g., refactor the onSuccess/openConfirmCreateDialog call in
useGenerateTableAssignments so getTableColumns is invoked at click/open time and
still passes pairings to handleConfirmCreate.
- Around line 258-269: The code accesses pairings[i].table directly which can be
undefined during form transitions; update the InputSelect value and onChange to
guard against out-of-bounds by using a safe read (e.g. pairings?.[i]?.table ??
-1) for the value and ensure form.setValue still writes to `pairings.${i}.table`
(use the same `i` from fields.map); reference the map callback variables
(fields.map, field, i), the pairings array, the InputSelect props, and
form.setValue to locate and apply the change so the component no longer crashes
when pairings[i] is missing.
- Around line 243-255: When mapping fields (fields.map) avoid destructuring
pairingStatuses[i] directly—wrap access with a guard so if pairingStatuses[i] is
undefined you fall back to a safe default (e.g., {status: 'ok', message: ''})
before using status and message; update the mapping inside
TournamentPairingsPage to compute const statusEntry = pairingStatuses[i] ||
{status: 'ok', message: ''} (or similar) and then use statusEntry.status and
statusEntry.message for the Pulsar and InfoPopover props to prevent
out-of-bounds errors when useFieldArray and getPairingsStatuses diverge.
In `@src/pages/TournamentPairingsPage/TournamentPairingsPage.utils.tsx`:
- Around line 150-166: getDefaultValues currently ignores the incoming
tournament and uses hardcoded options; update getDefaultValues to read the
tournament's pairing config (e.g. tournament.pairingConfig or
tournament.pairingOptions) and merge it with the default TournamentPairingConfig
values (orderBy: 'ranking', policies: { sameAlignment:
TournamentPairingPolicy.Allow, repeat: TournamentPairingPolicy.Block }) so
tournament-provided values override defaults, then apply the isFirstRound
override (set orderBy: 'random' when isFirstRound is true); also remove the
commented-out line referencing tournament.pairingOptions. Use the function name
getDefaultValues and types TournamentPairingConfig and TournamentPairingPolicy
to locate where to implement the merge.
- Around line 168-172: Rename the misleading variable playedTablesMap in
getConfirmDialogTableColumns to a clearer name like competitorMap or
competitorById; update the declaration (currently using new Map(...) from
competitors.map((c) => [c._id, c])) and replace all subsequent references to
playedTablesMap within getConfirmDialogTableColumns (and any exported helpers it
touches) to the new identifier so the map clearly represents competitor ID ->
TournamentCompetitor.
convex/_model/tournamentPairings/_helpers/checkIfPairingIsSameAlignment.test.ts
Show resolved
Hide resolved
src/pages/TournamentPairingsPage/TournamentPairingsPage.module.scss
Outdated
Show resolved
Hide resolved
src/pages/TournamentPairingsPage/TournamentPairingsPage.utils.tsx
Outdated
Show resolved
Hide resolved
…s.test.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…rings.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
convex/_model/tournamentPairings/mutations/createTournamentPairings.ts (2)
73-92: 🧹 Nitpick | 🔵 TrivialMove
activePlayerscomputation inside theid !== nullguard.
activePlayersis derived fromcompetitor, which is only meaningful whenidis non-null. Computing it outside the guard is wasteful on null IDs and slightly misleading.♻️ Proposed fix
for (const id of [pairing.tournamentCompetitor0Id, pairing.tournamentCompetitor1Id]) { - const competitor = competitors.find((c) => c._id === id); - const activePlayers = (competitor?.registrations ?? []).filter((p) => p.active); if (id !== null) { + const competitor = competitors.find((c) => c._id === id); if (!competitor) { throw new ConvexError(getErrorMessage('CANNOT_ADD_PAIRING_FOR_MISSING_COMPETITOR')); } if (!competitor.active) { throw new ConvexError(getErrorMessage('CANNOT_ADD_PAIRING_FOR_INACTIVE_COMPETITOR')); } if (pairedCompetitorIds.has(id)) { throw new ConvexError(getErrorMessage('CANNOT_ADD_PAIRING_FOR_ALREADY_PAIRED_COMPETITOR')); } + const activePlayers = (competitor.registrations ?? []).filter((p) => p.active); if (activePlayers.length < tournament.competitorSize) { throw new ConvexError(getErrorMessage('CANNOT_ADD_PAIRING_FOR_UNDER_STRENGTH_COMPETITOR')); } if (activePlayers.length > tournament.competitorSize) { throw new ConvexError(getErrorMessage('CANNOT_ADD_PAIRING_FOR_OVER_STRENGTH_COMPETITOR')); } } }
112-113: 🧹 Nitpick | 🔵 TrivialThe competitor double-assignment TODO on Line 113 appears already handled.
Lines 83–85 check
pairedCompetitorIds.has(id)and throwCANNOT_ADD_PAIRING_FOR_ALREADY_PAIRED_COMPETITORbefore inserting. If this TODO refers to the same invariant, it can be removed. If it means something else (e.g., cross-round uniqueness), consider clarifying the comment.The table double-assignment TODO (Line 112) is still outstanding.
Would you like me to open an issue to track the table double-assignment validation?
🤖 Fix all issues with AI agents
In `@convex/_model/tournamentPairings/_helpers/assignTables.tsx`:
- Around line 88-120: The loop in assignTables (autoAssignedPairings greedy swap
logic) uses a misleading variable name partnerWouldBenefitFromSwap even though
the check ensures the partner isn't harmed; rename partnerWouldBenefitFromSwap
to swapWouldNotHarmPartner and update the surrounding comment to state "swap
would not harm the partner" (keep the same boolean logic and swap behavior in
the nested for-loop that checks current.playedTables/includes(partner.table) and
partner.playedTables/includes(current.table)).
- Around line 78-79: Remove the duplicated comment line "// Step 2: Randomly
assign tables to pairings that need them:" in the assignTables helper so the
step comment appears only once; locate the duplicate in the assignTables.tsx
helper (search for that exact comment string or the assignTables/_helpers
module) and delete the redundant instance while keeping a single, correctly
positioned Step 2 comment.
- Line 20: The tableCount calculation can be fractional when
data.tournament?.competitorCount is odd (e.g., 5 -> 2.5), causing Array.from({
length: tableCount }) to silently truncate and produce too few table slots;
change the computation of tableCount in assignTables (the variable currently set
as (data.tournament?.competitorCount ?? 2) / 2) to use Math.ceil on the division
so it always yields an integer number of table slots (e.g.,
Math.ceil((data.tournament?.competitorCount ?? 2) / 2)), ensuring downstream
uses (the Array.from length and availability checks) see the correct table
count.
In `@convex/_model/tournamentPairings/_helpers/generateDraftPairings.test.ts`:
- Around line 85-88: The test title "Does not allow repeat pairings by default"
is misleading because it calls generateDraftPairings(competitors,
defaultPolicies) with an explicit defaultPolicies (repeat: Block); update the
test name to reflect the policy being passed (e.g., "Does not allow repeat
pairings when repeat policy is Block") and likewise rename the other test "Does
allow same alignment pairings by default" to indicate it relies on
defaultPolicies; locate the tests referencing generateDraftPairings,
defaultPolicies, and the error constant
errors.NO_VALID_PAIRINGS_POSSIBLE_WITHOUT_REPEAT and rename their describe/it
strings so they accurately state the condition being tested.
convex/_model/tournamentPairings/_helpers/generateDraftPairings.test.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
convex/_model/tournamentPairings/_helpers/generateDraftPairings.ts (1)
14-24:⚠️ Potential issue | 🟡 MinorStale JSDoc:
@param allowRepeatsno longer exists.The parameter was renamed to
policies: TournamentPairingPoliciesbut the doc still references the oldallowRepeatsname and is missing a description forpolicies.Proposed fix
/** * Generates draft pairings for an array of ranked TournamentCompetitors. * * `@remarks` This uses a recursive, depth‑first, order-biased backtracking search. * This means each item is attempted to be paired with the next closest item by index. * In the case of adjacent (Swiss) pairing, the order would be by rank, but for random pairings the order would be randomized. * * `@param` orderedCompetitors - Ordered array of competitors - * `@param` allowRepeats - Allow repeat match-ups - * `@returns` An array of + * `@param` policies - Pairing constraint policies (repeat, sameAlignment) + * `@returns` An array of competitor pairs */convex/_model/tournamentPairings/_helpers/assignTables.ts (1)
86-119: 🧹 Nitpick | 🔵 TrivialSwap optimization: variable name
partnerWouldBenefitFromSwapis misleading.On line 109, the check verifies the swap would not harm the partner (i.e., partner won't gain a new conflict at
current.table), not that the partner would benefit. If the partner has no conflict at its current table, the swap is neutral for it, not beneficial. Consider renaming toswapWouldNotHarmPartnerfor clarity.That said, the swap logic itself is functionally correct — it performs a greedy single-pass that avoids introducing new conflicts while resolving existing ones.
🤖 Fix all issues with AI agents
In `@convex/_model/tournamentPairings/actions/generateTableAssignments.ts`:
- Around line 40-43: The thrown plain Error from assignTables is being redacted
in production; change the failure to surface to the client by either converting
the throw inside assignTables to a ConvexError (e.g., replace Error throws like
"There are more unassigned pairings than available tables!" with a new
ConvexError with the same message) or by catching assignTables in
generateTableAssignments and rethrowing a ConvexError with the assignTables
error message; update references to assignTables and generateTableAssignments
accordingly so the client receives the original validation message.
In `@src/components/TournamentForm/components/PairingFields.tsx`:
- Around line 38-41: PairingFields computes a disabled flag from the tournament
status but does not forward the status to RankingFactorFields; update the JSX in
PairingFields to pass the status prop into <RankingFactorFields> (e.g.,
RankingFactorFields disabled={disabled} status={status}) so RankingFactorFields
can apply its own allowedEditStatuses check; ensure you reference the existing
variables/props named disabled and status and avoid changing
RankingFactorFields' internal logic.
- Around line 28-30: Hoist the static array allowedEditStatuses out of the
PairingFields component to module scope and export or reuse it so it's not
recreated on every render and to remove duplication with RankingFactorFields;
update the component to reference the module-level allowedEditStatuses when
computing disabled (currently const disabled =
!allowedEditStatuses.includes(status)) and update RankingFactorFields to
import/use the same constant instead of redefining it.
- Around line 22-26: The useEffect currently depends on the whole pairingConfig
object, causing runs on any nested change; change its dependency array to watch
only pairingConfig.policies.sameAlignment (and setValue) so the effect runs only
when sameAlignment changes — update the effect that checks
pairingConfig.policies.sameAlignment !== TournamentPairingPolicy.Allow and calls
setValue('registrationDetails.alignment','required') to use
[pairingConfig.policies.sameAlignment, setValue] as dependencies.
In
`@src/components/TournamentPairingConfigForm/TournamentPairingConfigFields.hooks.ts`:
- Around line 15-19: The onChange type in the UsePresetFieldResult declaration
redundantly unions SelectValue with null; update the UsePresetFieldResult type
so onChange is typed as (value: SelectValue) => void (remove the extra "| null")
to match SelectValue's definition and avoid the redundant null union; locate the
UsePresetFieldResult type and the onChange signature in
TournamentPairingConfigFields.hooks.ts and adjust accordingly so TypeScript
remains consistent.
In
`@src/components/TournamentPairingConfigForm/TournamentPairingConfigFields.tsx`:
- Around line 43-45: The option lists are being recomputed each render; hoist
them to module scope as constants (e.g. create ORDER_BY_OPTIONS =
getTournamentPairingOrderMethodOptions() and POLICY_OPTIONS =
getTournamentPairingPolicyOptions().filter(opt => opt.value !==
TournamentPairingPolicy.Avoid)) alongside the existing TABS constant, then
replace the local usages of orderByOptions and the filtered policyOptions in
TournamentPairingConfigFields.tsx with ORDER_BY_OPTIONS and POLICY_OPTIONS so
the lists are computed once.
In `@src/pages/TournamentPairingsPage/TournamentPairingsPage.tsx`:
- Around line 74-98: The columns for the confirm dialog are being created with a
potentially stale tournamentCompetitors value captured by the onSuccess closure;
update the code so getConfirmDialogTableColumns is evaluated at dialog render
time using the latest data — for example, create a ref
(latestTournamentCompetitorsRef) that you keep in sync with
tournamentCompetitors or pass a render function to openConfirmCreateDialog that
calls getConfirmDialogTableColumns(tournamentCompetitors ?? []) inside the
dialog content instead of calling it when the onSuccess closure is created;
adjust useGenerateTableAssignments.onSuccess / openConfirmCreateDialog and
ensure handleConfirmCreate still receives the pairings array.
- Around line 148-157: The confirm dialog opened by openConfirmRegenerateDialog
lacks a content/body message; update the call to include a content property
(e.g., content: 'Your current pairings will be lost.') so the dialog explains
the consequence, keeping the existing title ('Regenerate Pairings') and actions
(the onClick handler onConfirm and danger intent) intact.
- Around line 231-237: The button label logic is inverted: in the Button
component (text prop) using form.formState.isDirty should be flipped so that
when the form is not dirty it shows "Generate" and when dirty it shows
"Regenerate"; update the text expression on the Button (currently referencing
form.formState.isDirty) to the opposite logic (use form.formState.isDirty ?
'Regenerate' : 'Generate') so manual reorders result in "Regenerate" and the
initial empty/clean state shows "Generate".
- Around line 139-161: handleRegenerate currently takes the user-provided config
and passes it to generateTournamentPairings but never saves it, causing stale
validation later; update handleRegenerate to persist the passed config into
component state (e.g., call setActiveConfig(config) or similar) before calling
openConfirmRegenerateDialog or generateTournamentPairings (so the confirmed path
and immediate path both set state), and then change getPairingsStatuses to read
from activeConfig instead of existingValues so validation uses the persisted
config; ensure symbols referenced are handleRegenerate,
generateTournamentPairings, openConfirmRegenerateDialog, activeConfig (state
setter like setActiveConfig), and getPairingsStatuses.
src/components/TournamentPairingConfigForm/TournamentPairingConfigFields.hooks.ts
Show resolved
Hide resolved
src/components/TournamentPairingConfigForm/TournamentPairingConfigFields.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/pages/TournamentPairingsPage/TournamentPairingsPage.tsx`:
- Around line 93-96: The actions array uses an unnecessary async/await wrapper:
replace the onClick handler currently defined as async () => await
handleConfirmCreate(pairings) with a direct promise-returning function () =>
handleConfirmCreate(pairings) to avoid the extra microtask hop; locate the
actions entry (onClick) in TournamentPairingsPage where handleConfirmCreate is
referenced and update it accordingly.
- Around line 244-256: The statusColors mapping is recreated on every iteration
inside the fields.map loop; move the static const out of the loop (e.g., declare
const statusColors = { error: 'red', warning: 'yellow', ok: 'green' } above the
return or at module scope) and keep the rest of the loop using
pairingStatuses[i], InfoPopover and Pulsar as-is so each iteration reuses the
same object instead of allocating a new one per field.
- Around line 183-196: handleProceed currently calls generateTableAssignments
directly so non-Convex errors get swallowed; update the call site or the hook to
surface errors by adding an onError handler to useGenerateTableAssignments or by
wrapping generateTableAssignments in a try/catch that reports errors (e.g.,
shows a toast or calls the same error handler used by createTournamentPairings).
Specifically, locate handleProceed and either pass an onError callback into
useGenerateTableAssignments to mirror createMutationHook behavior, or change
handleProceed to catch any thrown error from generateTableAssignments and invoke
the app's toast/error reporting utility with the error message.
…airings-page' of https://github.com/ianpaschal/combat-command into 293-add-tournament-pairing-options-form-to-tournament-pairings-page
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
convex/_model/tournamentPairings/_helpers/assignTables.ts (1)
108-121: 🧹 Nitpick | 🔵 TrivialSwap condition naming is slightly misleading.
partnerWouldBenefitFromSwap(Line 112) doesn't mean the partner benefits — it means the swap won't harm the partner (i.e., won't introduce a new conflict). The logic itself is correct; the variable name just over-promises. ConsiderswapIsSafeForPartneror similar.src/services/utils/createMutationHook.ts (1)
32-42:⚠️ Potential issue | 🔴 CriticalBug:
loadingis never reset tofalseon success.The
return responseon line 32 exits the function beforesetIsLoading(false)on line 42 is reached. After a successful mutation,loadingwill be stuck astrueforever. Move the reset into afinallyblock.Proposed fix
if (instanceConfig?.onSuccess) { instanceConfig.onSuccess(response, args); } return response; } catch (error) { handleError(error); if (config?.onError) { config.onError(error); } if (instanceConfig?.onError) { instanceConfig.onError(error); } + } finally { + setIsLoading(false); } - setIsLoading(false); },
🤖 Fix all issues with AI agents
In `@src/components/TournamentForm/components/FormatFields.tsx`:
- Around line 32-33: Remove the local duplicate allowedEditStatuses in
FormatFields.tsx and replace its usage with the shared ALLOWED_EDIT_STATUSES
constant from TournamentForm.utils; specifically, import ALLOWED_EDIT_STATUSES
(same constant used by PairingFields and RankingFactorFields) and use it to
compute disableFields (i.e., const disableFields =
!ALLOWED_EDIT_STATUSES.includes(status)) so the file no longer defines its own
allowedEditStatuses.
- Line 21: The current call to watch() in FormatFields.tsx subscribes to the
entire form which causes unnecessary re-renders; update the subscription to only
the needed fields by replacing the generic watch() usage that extracts
roundStructure and competitorSize with a targeted watch for those names (e.g.,
call watch with ['roundStructure','competitorSize']) or use useWatch to
individually subscribe to roundStructure and competitorSize, ensuring only
changes to those fields trigger re-renders.
In `@src/components/TournamentForm/components/PairingFields.tsx`:
- Around line 23-27: Extract the duplicated effect into a new custom hook named
useSameAlignmentConstraint and replace the useEffect in PairingFields and
FormatFields with a call to that hook; the hook should accept the form setter
setValue and the pairingConfig.sameAlignment value (or the whole pairingConfig)
and internally run the effect that checks sameAlignment !==
TournamentPairingPolicy.Allow and then calls
setValue('registrationDetails.alignment', 'required'); update both components to
import and call useSameAlignmentConstraint (e.g.,
useSameAlignmentConstraint(setValue, pairingConfig.sameAlignment)) so the logic
is centralized and no longer duplicated when PairingFields and FormatFields
render together in TournamentForm.
In `@src/components/TournamentForm/TournamentForm.utils.ts`:
- Line 3: Change ALLOWED_EDIT_STATUSES to be a readonly typed constant to
prevent mutation and bind it to the status union: update the declaration of
ALLOWED_EDIT_STATUSES (in TournamentForm.utils.ts) to use a readonly tuple/array
and "as const" so its type becomes readonly ('draft' | 'published')[] (or
equivalent readonly tuple), ensuring any future edits that add invalid strings
will error at compile time.
In `@src/pages/TournamentPairingsPage/TournamentPairingsPage.tsx`:
- Around line 197-203: The parameter name in handleConfirmCreate shadows the
component-scoped pairings from useWatch; rename the function parameter (e.g., to
confirmedPairings or draftedPairings) in handleConfirmCreate and update any call
sites to use the new name, keeping the same type DraftTournamentPairing[] and
the same body that passes it to createTournamentPairings({ tournamentId:
tournament._id, round: nextRound, pairings: <newName> }); this removes shadowing
while preserving behavior.
- Line 75: The pairingConfig state is initialized with
getDefaultValues(tournament) on mount and so misses the actual
tournament.pairingConfig when tournament loads asynchronously; add a useEffect
that watches tournament and, when it becomes defined, calls
setPairingConfig(tournament.pairingConfig ?? getDefaultValues(tournament)) to
sync the state. Import useEffect from React and consider protecting against
overwriting user edits on refetch by using a ref flag (e.g., hasInitialized) so
the effect only sets pairingConfig once after the first successful tournament
load.
In `@src/services/utils/createActionHook.ts`:
- Around line 32-42: The try/catch currently returns response before
setIsLoading(false) runs, so loading never resets on success; modify the async
block in createActionHook (the function using setIsLoading, handleError,
config?.onError and instanceConfig?.onError) to use a try { ... } catch (error)
{ ... } finally { setIsLoading(false); } pattern so setIsLoading(false) always
executes after success or failure while preserving the existing return of
response from inside the try block.
In `@src/services/utils/handleError.ts`:
- Around line 5-10: handleError currently only handles ConvexError and Error,
silently ignoring thrown strings/numbers/objects and uses an unnecessary "as
string" cast; update the handleError function to add a fallback branch that logs
the raw error (e.g., console.error or process logger) and shows a toast with a
useful description by stringifying unknown values (JSON.stringify with a safe
fallback), remove the redundant "as string" cast on error.message, and keep
existing ConvexError/Error branches intact to preserve their specific messages.
| }: FormatFieldsProps): JSX.Element => { | ||
| const { watch, setValue } = useFormContext<TournamentFormData>(); | ||
| const { roundStructure, competitorSize, pairingMethod } = watch(); | ||
| const { roundStructure, competitorSize } = watch(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
watch() without arguments subscribes to the entire form.
Only roundStructure and competitorSize are needed here. Using targeted watches avoids re-renders from unrelated form field changes.
Proposed fix
- const { roundStructure, competitorSize } = watch();
+ const roundStructure = watch('roundStructure');
+ const competitorSize = watch('competitorSize');📝 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 { roundStructure, competitorSize } = watch(); | |
| const roundStructure = watch('roundStructure'); | |
| const competitorSize = watch('competitorSize'); |
🤖 Prompt for AI Agents
In `@src/components/TournamentForm/components/FormatFields.tsx` at line 21, The
current call to watch() in FormatFields.tsx subscribes to the entire form which
causes unnecessary re-renders; update the subscription to only the needed fields
by replacing the generic watch() usage that extracts roundStructure and
competitorSize with a targeted watch for those names (e.g., call watch with
['roundStructure','competitorSize']) or use useWatch to individually subscribe
to roundStructure and competitorSize, ensuring only changes to those fields
trigger re-renders.
Comment on lines
32
to
33
| const allowedEditStatuses = ['draft', 'published']; | ||
| const disableFields = !allowedEditStatuses.includes(status); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Local allowedEditStatuses duplicates the shared ALLOWED_EDIT_STATUSES constant.
PairingFields and RankingFactorFields already import ALLOWED_EDIT_STATUSES from TournamentForm.utils. This file still defines its own local copy.
Proposed fix
+import { ALLOWED_EDIT_STATUSES } from '../TournamentForm.utils';
...
- const allowedEditStatuses = ['draft', 'published'];
- const disableFields = !allowedEditStatuses.includes(status);
+ const disableFields = !ALLOWED_EDIT_STATUSES.includes(status);📝 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 allowedEditStatuses = ['draft', 'published']; | |
| const disableFields = !allowedEditStatuses.includes(status); | |
| import { ALLOWED_EDIT_STATUSES } from '../TournamentForm.utils'; | |
| // ... other imports and code ... | |
| const disableFields = !ALLOWED_EDIT_STATUSES.includes(status); |
🤖 Prompt for AI Agents
In `@src/components/TournamentForm/components/FormatFields.tsx` around lines 32 -
33, Remove the local duplicate allowedEditStatuses in FormatFields.tsx and
replace its usage with the shared ALLOWED_EDIT_STATUSES constant from
TournamentForm.utils; specifically, import ALLOWED_EDIT_STATUSES (same constant
used by PairingFields and RankingFactorFields) and use it to compute
disableFields (i.e., const disableFields =
!ALLOWED_EDIT_STATUSES.includes(status)) so the file no longer defines its own
allowedEditStatuses.
Comment on lines
+23
to
+27
| useEffect(() => { | ||
| if (pairingConfig.policies.sameAlignment !== TournamentPairingPolicy.Allow) { | ||
| setValue('registrationDetails.alignment', 'required'); | ||
| } | ||
| }, [pairingConfig, setValue]); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find and examine both component files
echo "=== PairingFields.tsx (lines 20-35) ==="
sed -n '20,35p' src/components/TournamentForm/components/PairingFields.tsx
echo -e "\n=== FormatFields.tsx (lines 20-35) ==="
sed -n '20,35p' src/components/TournamentForm/components/FormatFields.tsxRepository: ianpaschal/combat-command
Length of output: 1365
🏁 Script executed:
#!/bin/bash
# Find where both components are imported/used in TournamentForm
echo "=== Searching for PairingFields and FormatFields usage ==="
rg -n 'PairingFields|FormatFields' src/components/TournamentForm/ -A2 -B2Repository: ianpaschal/combat-command
Length of output: 8786
🏁 Script executed:
#!/bin/bash
# Check the main TournamentForm to see how these components are rendered
echo "=== TournamentForm structure ==="
fd -e tsx -e ts 'TournamentForm' src/components/ | head -5Repository: ianpaschal/combat-command
Length of output: 259
Extract the sameAlignment → required effect into a shared custom hook.
Both PairingFields (lines 23–27) and FormatFields (lines 25–29) define identical useEffect logic that sets registrationDetails.alignment to 'required' when sameAlignment !== Allow. Since both components render simultaneously within the same form context in TournamentForm.tsx (lines 90 & 94), this effect executes redundantly. Create a custom hook (e.g., useSameAlignmentConstraint) to centralize this logic and eliminate the duplication.
🤖 Prompt for AI Agents
In `@src/components/TournamentForm/components/PairingFields.tsx` around lines 23 -
27, Extract the duplicated effect into a new custom hook named
useSameAlignmentConstraint and replace the useEffect in PairingFields and
FormatFields with a call to that hook; the hook should accept the form setter
setValue and the pairingConfig.sameAlignment value (or the whole pairingConfig)
and internally run the effect that checks sameAlignment !==
TournamentPairingPolicy.Allow and then calls
setValue('registrationDetails.alignment', 'required'); update both components to
import and call useSameAlignmentConstraint (e.g.,
useSameAlignmentConstraint(setValue, pairingConfig.sameAlignment)) so the logic
is centralized and no longer duplicated when PairingFields and FormatFields
render together in TournamentForm.
| round, | ||
| options: pairingOptions, | ||
| } : 'skip'); | ||
| const [pairingConfig, setPairingConfig] = useState<TournamentPairingConfig>(getDefaultValues(tournament)); |
There was a problem hiding this comment.
pairingConfig state ignores tournament's saved config after async load.
useState initializer runs once on mount. At that point tournament is likely undefined (fetched async), so getDefaultValues(undefined) returns hardcoded defaults. When the tournament data arrives, pairingConfig is never updated to reflect tournament.pairingConfig.
Consider using an effect to sync state when tournament loads, or defer rendering until tournament is available (e.g., lift the early-return guard above this hook).
Proposed fix — sync state when tournament loads
const [pairingConfig, setPairingConfig] = useState<TournamentPairingConfig>(getDefaultValues(tournament));
+ // Sync pairingConfig when tournament data first loads
+ useEffect(() => {
+ if (tournament) {
+ setPairingConfig(getDefaultValues(tournament));
+ }
+ }, [tournament]);You'll need to add useEffect to the React import on Line 1. Be mindful that this will also reset user edits if tournament refetches — you may want a ref guard (e.g., hasInitialized) to run only once.
🤖 Prompt for AI Agents
In `@src/pages/TournamentPairingsPage/TournamentPairingsPage.tsx` at line 75, The
pairingConfig state is initialized with getDefaultValues(tournament) on mount
and so misses the actual tournament.pairingConfig when tournament loads
asynchronously; add a useEffect that watches tournament and, when it becomes
defined, calls setPairingConfig(tournament.pairingConfig ??
getDefaultValues(tournament)) to sync the state. Import useEffect from React and
consider protecting against overwriting user edits on refetch by using a ref
flag (e.g., hasInitialized) so the effect only sets pairingConfig once after the
first successful tournament load.
Comment on lines
+32
to
+42
| return response; | ||
| } catch (error) { | ||
| if (error instanceof ConvexError) { | ||
| toast.error('Error', { description: error.data.message }); | ||
| if (config?.onError) { | ||
| config.onError(error); | ||
| } | ||
| handleError(error); | ||
| if (config?.onError) { | ||
| config.onError(error); | ||
| } | ||
| if (instanceConfig?.onError) { | ||
| instanceConfig.onError(error); | ||
| } | ||
| } finally { | ||
| setIsLoading(false); | ||
| } | ||
| setIsLoading(false); |
There was a problem hiding this comment.
Bug: loading is never reset to false on success — same issue as createMutationHook.
return response on line 32 exits before setIsLoading(false) on line 42. Use a finally block, identical to the fix suggested for createMutationHook.
Proposed fix
if (instanceConfig?.onSuccess) {
instanceConfig.onSuccess(response, args);
}
return response;
} catch (error) {
handleError(error);
if (config?.onError) {
config.onError(error);
}
if (instanceConfig?.onError) {
instanceConfig.onError(error);
}
+ } finally {
+ setIsLoading(false);
}
- setIsLoading(false);
},📝 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
| return response; | |
| } catch (error) { | |
| if (error instanceof ConvexError) { | |
| toast.error('Error', { description: error.data.message }); | |
| if (config?.onError) { | |
| config.onError(error); | |
| } | |
| handleError(error); | |
| if (config?.onError) { | |
| config.onError(error); | |
| } | |
| if (instanceConfig?.onError) { | |
| instanceConfig.onError(error); | |
| } | |
| } finally { | |
| setIsLoading(false); | |
| } | |
| setIsLoading(false); | |
| return response; | |
| } catch (error) { | |
| handleError(error); | |
| if (config?.onError) { | |
| config.onError(error); | |
| } | |
| if (instanceConfig?.onError) { | |
| instanceConfig.onError(error); | |
| } | |
| } finally { | |
| setIsLoading(false); | |
| } |
🤖 Prompt for AI Agents
In `@src/services/utils/createActionHook.ts` around lines 32 - 42, The try/catch
currently returns response before setIsLoading(false) runs, so loading never
resets on success; modify the async block in createActionHook (the function
using setIsLoading, handleError, config?.onError and instanceConfig?.onError) to
use a try { ... } catch (error) { ... } finally { setIsLoading(false); } pattern
so setIsLoading(false) always executes after success or failure while preserving
the existing return of response from inside the try block.
Comment on lines
+5
to
+10
| export const handleError = (error: unknown): void => { | ||
| if (error instanceof ConvexError) { | ||
| toast.error('Error', { description: error.data.message }); | ||
| } else if (error instanceof Error) { | ||
| toast.error('Error', { description: error.message as string }); | ||
| } |
There was a problem hiding this comment.
Unknown error types are silently swallowed.
If error is neither a ConvexError nor an Error (e.g., a thrown string, number, or plain object), no toast is shown and nothing is logged. The caller and user both lose all signal that something went wrong.
Consider adding a fallback branch:
Proposed fix
if (error instanceof ConvexError) {
toast.error('Error', { description: error.data.message });
} else if (error instanceof Error) {
- toast.error('Error', { description: error.message as string });
- }
+ toast.error('Error', { description: error.message });
+ } else {
+ toast.error('Error', { description: 'An unknown error occurred.' });
+ }Also, the as string cast on error.message is unnecessary since Error.message is already typed as string.
📝 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
| export const handleError = (error: unknown): void => { | |
| if (error instanceof ConvexError) { | |
| toast.error('Error', { description: error.data.message }); | |
| } else if (error instanceof Error) { | |
| toast.error('Error', { description: error.message as string }); | |
| } | |
| export const handleError = (error: unknown): void => { | |
| if (error instanceof ConvexError) { | |
| toast.error('Error', { description: error.data.message }); | |
| } else if (error instanceof Error) { | |
| toast.error('Error', { description: error.message }); | |
| } else { | |
| toast.error('Error', { description: 'An unknown error occurred.' }); | |
| } |
🤖 Prompt for AI Agents
In `@src/services/utils/handleError.ts` around lines 5 - 10, handleError currently
only handles ConvexError and Error, silently ignoring thrown
strings/numbers/objects and uses an unnecessary "as string" cast; update the
handleError function to add a fallback branch that logs the raw error (e.g.,
console.error or process logger) and shows a toast with a useful description by
stringifying unknown values (JSON.stringify with a safe fallback), remove the
redundant "as string" cast on error.message, and keep existing ConvexError/Error
branches intact to preserve their specific messages.
ianpaschal
deleted the
293-add-tournament-pairing-options-form-to-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 #293
Summary by CodeRabbit
New Features
Bug Fixes
Refactor