CC-2: Improve Pairing Workflow#322
Open
ianpaschal wants to merge 2 commits into
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughLocal pairing config/schema/defaults with tableCount were added, plus a migration to backfill it. Database queries switched to the by_tournament index. A getLastVisibleRound helper with tests was implemented and integrated in deepenTournament. Draft pairings now accept include lists, and a mutation updates pairing config. UI infrastructure moved to an actions prop, identity components were refactored, and rankings/roster were replaced with unified competitors/registrations tables. The pairings page was overhauled with a drag-and-drop grid and new dialogs. ✨ Finishing Touches🧪 Generate unit tests (beta)
|
Fixes CC-2
ianpaschal
force-pushed
the
ian/cc-2-improve-pairing-workflow
branch
from
2b01393 to
4bac10b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/components/PageWrapper/PageWrapper.tsx (1)
57-68:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
actionscan be dropped when no title/back button is shown.The header container is currently not rendered unless back button or visible title is present, so
actionsalone will never render.💡 Suggested fix
- {(showBackButton || (title && !hideTitle)) && ( + {(showBackButton || (title && !hideTitle) || actions != null) && ( <div className={styles.PageWrapper_Header}> {showBackButton && ( <Button icon={<ArrowLeft />} variant="shaded" onClick={handleClickBack} /> )} {title && ( <h1>{title}</h1> )} - <div className={styles.PageWrapper_Header_Right}> - {actions} - </div> + {actions != null && ( + <div className={styles.PageWrapper_Header_Right}> + {actions} + </div> + )} </div> )}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/PageWrapper/PageWrapper.tsx` around lines 57 - 68, The header currently only renders when showBackButton or a visible title exists, so actions passed to PageWrapper are never shown alone; update the render condition around the div with class PageWrapper_Header to also check for actions (i.e., render when showBackButton || (title && !hideTitle) || actions), keeping the internal structure (Button using ArrowLeft and handleClickBack, the h1 for title, and the PageWrapper_Header_Right wrapper for actions) unchanged so actions can display even when no back button or title is present.convex/_model/utils/deleteTestTournament.ts (1)
21-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
Promise.allinstead offorEachto ensure deletions complete before mutation returns.At lines 21, 29, and 37,
forEach(async ...)does not await the async callbacks—the mutation may return before all deletions finish. Replace withPromise.all()paired withmap():Proposed fix
- competitors.forEach(async ({ _id }) => { - await ctx.db.delete(_id); - }); + await Promise.all(competitors.map(({ _id }) => ctx.db.delete(_id))); - pairings.forEach(async ({ _id }) => { - await ctx.db.delete(_id); - }); + await Promise.all(pairings.map(({ _id }) => ctx.db.delete(_id))); - matchResults.forEach(async ({ _id }) => { - await ctx.db.delete(_id); - }); + await Promise.all(matchResults.map(({ _id }) => ctx.db.delete(_id)));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@convex/_model/utils/deleteTestTournament.ts` around lines 21 - 39, The code uses forEach with async callbacks for deleting competitors, pairings, and matchResults so deletions may not complete before the mutation returns; change each block to build an array of deletion promises (e.g., using competitors.map(c => ctx.db.delete(c._id))) and await Promise.all(...) for the competitors, pairings, and matchResults collections so ctx.db.delete is awaited before the function returns; update the blocks that reference competitors, pairings, matchResults and ctx.db.delete accordingly (in the deleteTestTournament logic).convex/_model/tournaments/_helpers/deepenTournament.ts (1)
22-29: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueJSDoc
@paramname is stale.The parameter was renamed to
doc, but the JSDoc still referencestournament. Worth syncing so editor tooltips don't mislead.♻️ Proposed refactor
* `@param` ctx - Convex query context - * `@param` tournament - Raw Tournament document + * `@param` doc - Raw Tournament document * `@returns` A deep Tournament */ export const deepenTournament = async ( ctx: QueryCtx, doc: Doc<'tournaments'>, ) => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@convex/_model/tournaments/_helpers/deepenTournament.ts` around lines 22 - 29, Update the JSDoc for deepenTournament to match the current parameter names: change the `@param` that currently says "tournament" to "doc" and adjust its description to "Raw Tournament document" (or similar) so the JSDoc matches the function signature deepenTournament(ctx: QueryCtx, doc: Doc<'tournaments'>) and editor tooltips show correct info.src/components/TournamentCompetitorIdentity/TournamentCompetitorIdentity.tsx (1)
31-46:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDisable the control when it has no actionable subject.
Right now this still renders as an enabled
<button>whensubjectis missing,onClickis absent, or the row is loading.handleClickbecomes a no-op, but the control still advertises itself as interactive. Derive bothdisabledanddata-clickablefrom a single actionable flag instead.Suggested fix
const { displayName } = subject ?? placeholder ?? { displayName: 'Unknown Competitor', details: { alignments: [], factions: [] } }; + const isActionable = !loading && !!subject && !!onClick && !disabled; const handleClick = (e: MouseEvent): void => { e.preventDefault(); // Prevent submit if in a form - if (subject && onClick) { + if (isActionable && subject && onClick) { onClick(subject._id); } }; @@ - disabled={disabled} - data-clickable={!!onClick} + disabled={!isActionable} + data-clickable={isActionable}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/TournamentCompetitorIdentity/TournamentCompetitorIdentity.tsx` around lines 31 - 46, Compute a single "actionable" flag (e.g. const actionable = Boolean(subject && onClick && !loading)) and use it to drive interactivity: set the button disabled={!actionable}, set data-clickable={actionable}, and update handleClick to return immediately unless actionable before calling onClick(subject._id); reference handleClick, subject, onClick, the disabled prop and data-clickable attribute and include any existing loading/isLoading prop in the actionable check.src/components/TournamentPairingGenerationForm/components/TournamentPairingConfigForm/TournamentPairingConfigForm.tsx (1)
48-57:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winSubmit handler bypasses validation and double-fires
onSubmit.When validation succeeds,
onSubmitis invoked twice — first with the validated payload, then again unconditionally with the raw form data merged withforcedValues, overwriting the validated result and re-triggering any downstream side-effects. When validation fails,validateFormpopulates field errors but the unconditional second call still submits the unvalidated data, defeating the schema entirely.The unconditional block needs to be inside the success branch and should be the only
onSubmitcall, withforcedValuesmerged into the validated payload.🐛 Proposed fix
const handleSubmit: SubmitHandler<TournamentPairingConfig> = async (formData): Promise<void> => { const validFormData = validateForm(tournamentPairingConfigSchema, formData, form.setError); - if (validFormData) { - onSubmit?.(validFormData); - } - onSubmit?.({ - ...formData, - ...forcedValues, - }); + if (!validFormData) { + return; + } + onSubmit?.({ + ...validFormData, + ...forcedValues, + }); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/TournamentPairingGenerationForm/components/TournamentPairingConfigForm/TournamentPairingConfigForm.tsx` around lines 48 - 57, The submit handler handleSubmit currently calls onSubmit twice and bypasses validation; change it so that after calling validateForm(tournamentPairingConfigSchema, formData, form.setError) you only call onSubmit when validFormData is truthy, and call it once with the validated payload merged with forcedValues (e.g., onSubmit?.({ ...validFormData, ...forcedValues })); ensure there is no unconditional onSubmit outside that success branch so invalid submissions do not fire.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@convex/_fixtures/createMockTournament.ts`:
- Around line 51-54: The tableCount calculation in pairingConfig incorrectly
applies the division only to the fallback due to operator precedence; change the
expression so the nullish coalescing picks the competitor count first and then
divide by 2 (e.g. compute Math.ceil((overrides?.maxCompetitors ?? 48) / 2)), and
remove the redundant trailing ?? 1 since Math.ceil always returns a number;
update the expression in the pairingConfig block that references
tournamentPairingConfigDefaultValues and tableCount accordingly.
In `@convex/_model/tournamentPairings/_helpers/assignTables.ts`:
- Line 23: The code uses a non-null assertion on
data.tournament.pairingConfig.tableCount which will break tournaments not yet
migrated; replace the assertion with a runtime-safe fallback (e.g., use optional
chaining and nullish coalescing: data.tournament.pairingConfig?.tableCount ??
<fallback>) and pick an appropriate fallback (a constant DEFAULT_TABLE_COUNT, a
value derived from data.tournament/player count, or a domain-specific safe
default) inside the assignTables logic so pair generation still works until
migration is complete; remove the "!" on tableCount and reference the
pairingConfig?.tableCount and the chosen DEFAULT_TABLE_COUNT or computed
fallback in the same function.
In `@convex/_model/tournaments/_helpers/getLastVisibleRound.ts`:
- Around line 25-27: In getLastVisibleRound, avoid the truthy check on
doc.lastRound; change the condition "if (doc.lastRound && doc.lastRound + 1 ===
doc.roundCount)" to explicitly check for undefined (e.g., "if (doc.lastRound !==
undefined && doc.lastRound + 1 === doc.roundCount)") so a lastRound of 0 is
handled correctly and still returns Math.max(doc.lastRound - 1, 0); leave the
fallback return doc.lastRound as-is.
In `@convex/migrations.ts`:
- Around line 145-147: The guard that skips backfilling uses a truthy check on
doc.pairingConfig.tableCount which will treat 0 as missing; change the
conditional to explicitly check for undefined or null (e.g., use !== undefined
or != null) so only truly missing values are backfilled; locate the check that
reads "if (doc.pairingConfig.tableCount) { return; }" and replace it with an
explicit undefined/null check on doc.pairingConfig.tableCount.
In `@src/components/FactionIndicator/FactionIndicator.tsx`:
- Around line 25-28: primaryAlignment can become undefined when alignment ===
[], which breaks getAlignmentColor (expects Alignment | null). Update the
primaryAlignment computation (the variable used by getAlignmentColor and
getAlignmentDisplayName) to explicitly guard empty arrays and normalize to null
instead of undefined (e.g., when Array.isArray(alignment) use alignment.length ?
alignment[0] : null, and when alignment is a scalar use alignment ?? null) so
getAlignmentColor(primaryAlignment) always receives Alignment | null and
displayName fallback logic continues to work.
In `@src/components/FactionIndicator/FactionIndicator.utils.ts`:
- Around line 6-11: The function getAlignmentColor treats undefined as a valid
alignment and falls through to return 'mixed'; update the null check to treat
both null and undefined as "unknown" by changing the guard (alignment === null)
to a check that catches both (e.g., alignment == null or alignment === null ||
alignment === undefined) so missing alignment values return 'gray' instead of
'mixed'; locate this change inside getAlignmentColor in
FactionIndicator.utils.ts.
In
`@src/components/TournamentPairingGenerationForm/TournamentPairingGenerationForm.tsx`:
- Around line 78-80: The expression that computes showLoading is overly verbose:
it uses [loading].some((l) => !!l) to check a single flag; simplify by replacing
that array/some pattern with a direct boolean cast (!!loading) so showLoading
becomes const showLoading = !tournament || !!loading, keeping the existing
variables tournament and loading (and preserving behavior).
- Around line 65-76: handleSubmit currently bypasses validation and sends raw
form data; re-enable the commented validation so the pairing config is validated
against tournamentPairingConfigSchema before calling onSubmit. Use
validateForm(tournamentPairingConfigSchema, formData, form.setError) and
validateForm(tournamentPairingConfigSchema, formData.config, form.setError) (as
previously attempted) and only invoke onSubmit?.({...formData, ...forcedValues})
when validation returns a truthy valid object; ensure you propagate
form.setError for field errors and keep the merge with forcedValues intact,
preserving the SubmitHandler signature in handleSubmit and the
GenerateDraftTournamentPairingsArgs type.
- Around line 46-58: The form's defaultValues for tournamentId is captured once
by useForm in TournamentPairingGenerationForm and can remain '' if tournament
loads after mount; fix by adding an effect that watches tournament?._id and
calls form.reset(...) to merge the same defaults (config:
tournamentPairingConfigDefaultValues, tournamentId: tournament?._id ?? '',
round: 0, include: [], plus existingValues and forcedValues) so the live form
state is updated when tournament._id becomes available (alternatively, gate the
component render on tournament truthiness so useForm only mounts after
tournament is loaded—choose one approach and implement it in
TournamentPairingGenerationForm, referencing useForm and form.reset).
In
`@src/components/TournamentRegistrationIdentity/TournamentRegistrationIdentity.tsx`:
- Line 29: The fallback object used when destructuring displayName in the
TournamentRegistrationIdentity component contains an unused details property;
update the expression that assigns const { displayName } = subject ??
placeholder ?? { displayName: 'Unknown Competitor' } by removing the redundant
details: { alignments: [], factions: [] } so the fallback only provides
displayName, keeping the rest of the component logic unchanged.
- Line 51: Update the Avatar URL access in TournamentRegistrationIdentity to use
defensive optional chaining: change occurrences of subject?.user.avatarUrl to
subject?.user?.avatarUrl so the leaf component still guards against a missing
user; reference the TournamentRegistrationIdentity component and the
deepenTournamentRegistration helper in the message for context.
In `@src/hooks/useAsyncState.ts`:
- Line 14: Add a one-line comment above the initialized ref declaration to
explain that useRef(asyncValue !== undefined) runs only on the first render and
is intentionally used to record whether an initial synchronous asyncValue was
provided (so the effect won't reapply it); reference the initialized constant
and the asyncValue parameter/name in your comment to make the intention clear to
future readers.
- Around line 14-20: The hook useAsyncState now only initializes value once
using the initialized ref and then ignores further asyncValue updates, which can
cause stale config in callers like TournamentPairingsPage (pairingConfig →
config); update the implementation or documentation: either restore
resync-on-asyncValue-change by removing the initialized gate and allowing
setValue(asyncValue) whenever asyncValue changes, or add explicit JSDoc on
useAsyncState explaining the once-only initialization semantics (mention
initialized, asyncValue, value, setValue) so callers know local edits win and
external updates are ignored.
In
`@src/pages/TournamentDetailPage/components/TournamentCompetitorsCard/TournamentCompetitorsCard.tsx`:
- Around line 25-34: The component currently calls
useGetTournamentCompetitorsByTournament and
useGetTournamentRegistrationsByTournament in TournamentCompetitorsCard while
TournamentCompetitorTable and TournamentRegistrationTable also re-run the same
queries; pick one owner: either lift data up or delegate to child. To fix,
remove the duplicate hook calls from the child components if you decide the card
should own data, and pass competitors, competitorsLoading, registrations,
registrationsLoading (or a single active view's rows/loading) into
TournamentCompetitorTable and TournamentRegistrationTable via props;
alternatively remove the hooks from TournamentCompetitorsCard and derive
showLoadingState/showEmptyState from the active child’s returned rows/loading so
only the active table issues the query. Ensure you update prop names used by
TournamentCompetitorTable and TournamentRegistrationTable to accept the passed
rows and loading values.
In
`@src/pages/TournamentDetailPage/components/TournamentCompetitorsCard/TournamentCompetitorsCard.utils.tsx`:
- Around line 26-101: Remove the large commented-out implementation of
getTableConfig in TournamentCompetitorsCard.utils.tsx: delete the entire
commented block that begins with the commented export const getTableConfig and
ends at its closing comment so the file only exposes the active exported
types/APIs; rely on git history if you need the previous implementation later.
Ensure no other code is accidentally removed and run a quick build/lint to
confirm no references to getTableConfig remain.
In
`@src/pages/TournamentDetailPage/components/TournamentRegistrationTable/TournamentRegistrationTable.tsx`:
- Around line 42-45: The identity column label in TournamentRegistrationTable is
misleading for team events because the rows render TournamentRegistration via
TournamentRegistrationIdentity (which is player-oriented); change the column
definition (the object with key: 'identity' in TournamentRegistrationTable) to
always use the player-oriented label (e.g., 'Player') instead of using
tournament.useTeams ? 'Team' : 'Player' so the header matches the rendered
content; update any related tests or snapshots that assert the header text if
present.
In `@src/pages/TournamentDetailPage/TournamentDetailPage.tsx`:
- Around line 106-108: Update the stale inline comment that references the
removed 'roster' tab: remove the literal 'roster' and instead refer to the
actual behavior (the default tab returned by getDefaultTab) or the current
default tab name; update the comment near the if (activeTab !== queryTab &&
!loading) check to accurately describe that the first render occurs before
tournament status is known so getDefaultTab determines the initial tab, and
remove any mention of the old 'roster' identifier.
In `@src/pages/TournamentPairingsPage/components/Draggable/Draggable.tsx`:
- Around line 33-39: The drag handle button in Draggable.tsx is an unlabeled
icon-only control (GripVertical) so add an accessible name to the <button> by
providing an aria-label (or aria-labelledby to a nearby visible label) so screen
readers can announce the control; update the button that uses attributes and
listeners (the element with class styles.Draggable_Handle and props
{...attributes} {...listeners}) to include a descriptive aria-label such as
"Drag handle" or context-specific text like "Move pairing".
In
`@src/pages/TournamentPairingsPage/components/PairingsGrid/PairingsGrid.utils.ts`:
- Around line 56-58: The code assumes destPairingIndex is valid but findIndex
can return -1; add a guard after computing destPairingIndex (using the existing
variables destPairingIndex, destSlotIndex, destOccupant, parts, and pairings)
that checks for destPairingIndex === -1 and bails out (e.g., return early, skip
the drop handling, or set destOccupant to undefined) so you never access
pairings[-1]. Ensure any downstream logic that uses destOccupant handles the
early-exit case consistently.
In `@src/pages/TournamentPairingsPage/TournamentPairingsPage.tsx`:
- Around line 92-94: The current isDirty only checks whether every local pairing
exists on the server and short-circuits when either pairings or
tournamentPairings is empty, missing deletions and create-from-empty cases;
replace that logic with a symmetric comparison: compute isDirty as true if any
local pairing has no match in tournamentPairings OR any server pairing has no
match in pairings (e.g., isDirty = pairings.some(p =>
!tournamentPairings?.some(sp => pairingsMatch(p, sp))) ||
tournamentPairings?.some(sp => !pairings.some(p => pairingsMatch(p, sp))));
ensure you do not short-circuit on empty arrays so adding the first pairing or
clearing all pairings is detected.
- Around line 68-79: The effect seeds local pairings from
useGetTournamentPairings too eagerly and when local pairings are cleared; fix by
tracking which round you've initialized: add a state like initializedRound:
number | null, and change the useEffect (watching tournamentPairings, nextRound,
initializedRound) to only call setPairings(sanitize(tournamentPairings)) when
tournamentPairings?.length && initializedRound === null && nextRound === the
round corresponding to tournamentPairings (and/or nextRound !== 0 if
applicable), then set initializedRound to nextRound; remove pairings from the
effect deps so clearing local pairings doesn't re-seed. Ensure you reference
useGetTournamentPairings, nextRound, pairings, setPairings, sanitize and
initializedRound in your change.
---
Outside diff comments:
In `@convex/_model/tournaments/_helpers/deepenTournament.ts`:
- Around line 22-29: Update the JSDoc for deepenTournament to match the current
parameter names: change the `@param` that currently says "tournament" to "doc" and
adjust its description to "Raw Tournament document" (or similar) so the JSDoc
matches the function signature deepenTournament(ctx: QueryCtx, doc:
Doc<'tournaments'>) and editor tooltips show correct info.
In `@convex/_model/utils/deleteTestTournament.ts`:
- Around line 21-39: The code uses forEach with async callbacks for deleting
competitors, pairings, and matchResults so deletions may not complete before the
mutation returns; change each block to build an array of deletion promises
(e.g., using competitors.map(c => ctx.db.delete(c._id))) and await
Promise.all(...) for the competitors, pairings, and matchResults collections so
ctx.db.delete is awaited before the function returns; update the blocks that
reference competitors, pairings, matchResults and ctx.db.delete accordingly (in
the deleteTestTournament logic).
In `@src/components/PageWrapper/PageWrapper.tsx`:
- Around line 57-68: The header currently only renders when showBackButton or a
visible title exists, so actions passed to PageWrapper are never shown alone;
update the render condition around the div with class PageWrapper_Header to also
check for actions (i.e., render when showBackButton || (title && !hideTitle) ||
actions), keeping the internal structure (Button using ArrowLeft and
handleClickBack, the h1 for title, and the PageWrapper_Header_Right wrapper for
actions) unchanged so actions can display even when no back button or title is
present.
In
`@src/components/TournamentCompetitorIdentity/TournamentCompetitorIdentity.tsx`:
- Around line 31-46: Compute a single "actionable" flag (e.g. const actionable =
Boolean(subject && onClick && !loading)) and use it to drive interactivity: set
the button disabled={!actionable}, set data-clickable={actionable}, and update
handleClick to return immediately unless actionable before calling
onClick(subject._id); reference handleClick, subject, onClick, the disabled prop
and data-clickable attribute and include any existing loading/isLoading prop in
the actionable check.
In
`@src/components/TournamentPairingGenerationForm/components/TournamentPairingConfigForm/TournamentPairingConfigForm.tsx`:
- Around line 48-57: The submit handler handleSubmit currently calls onSubmit
twice and bypasses validation; change it so that after calling
validateForm(tournamentPairingConfigSchema, formData, form.setError) you only
call onSubmit when validFormData is truthy, and call it once with the validated
payload merged with forcedValues (e.g., onSubmit?.({ ...validFormData,
...forcedValues })); ensure there is no unconditional onSubmit outside that
success branch so invalid submissions do not fire.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e4a7a6b4-7a61-43da-88af-26e54f8f5885
⛔ Files ignored due to path filters (1)
convex/_generated/api.d.tsis excluded by!**/_generated/**
📒 Files selected for processing (97)
convex/_fixtures/createMockTournament.tsconvex/_model/common/tournamentPairingConfig.tsconvex/_model/tournamentCompetitors/mutations/createTournamentCompetitor.tsconvex/_model/tournamentCompetitors/queries/getTournamentCompetitorsByTournament.tsconvex/_model/tournamentCompetitors/table.tsconvex/_model/tournamentPairings/_helpers/assignTables.tsconvex/_model/tournamentPairings/actions/generateDraftTournamentPairings.tsconvex/_model/tournamentPairings/index.tsconvex/_model/tournamentPairings/mutations/createTournamentPairings.tsconvex/_model/tournamentRegistrations/_helpers/deepenTournamentRegistration.tsconvex/_model/tournamentRegistrations/_helpers/sortTournamentCompetitorsByName.tsconvex/_model/tournamentRegistrations/queries/getTournamentRegistrationsByTournament.tsconvex/_model/tournamentResults/_helpers/aggregateTournamentData.tsconvex/_model/tournamentResults/mutations/refreshTournamentResult.tsconvex/_model/tournaments/_helpers/deepenTournament.tsconvex/_model/tournaments/_helpers/getAvailableActions.tsconvex/_model/tournaments/_helpers/getLastVisibleRound.test.tsconvex/_model/tournaments/_helpers/getLastVisibleRound.tsconvex/_model/tournaments/index.tsconvex/_model/tournaments/mutations/deleteTournament.tsconvex/_model/tournaments/mutations/updateTournamentPairingConfig.tsconvex/_model/utils/deleteTestTournament.tsconvex/migrations.tsconvex/tournaments.tssrc/api.tssrc/components/FactionIndicator/FactionIndicator.tsxsrc/components/FactionIndicator/FactionIndicator.utils.tssrc/components/FooterBar/FooterBar.tsxsrc/components/PageWrapper/PageWrapper.module.scsssrc/components/PageWrapper/PageWrapper.tsxsrc/components/ScoreAdjustmentFields/ScoreAdjustmentFields.tsxsrc/components/TournamentCompetitorIdentity/TournamentCompetitorIdentity.module.scsssrc/components/TournamentCompetitorIdentity/TournamentCompetitorIdentity.tsxsrc/components/TournamentCompetitorIdentity/index.tssrc/components/TournamentForm/TournamentForm.schema.tssrc/components/TournamentForm/components/PairingFields.tsxsrc/components/TournamentPairingConfigForm/index.tssrc/components/TournamentPairingGenerationForm/TournamentPairingGenerationForm.module.scsssrc/components/TournamentPairingGenerationForm/TournamentPairingGenerationForm.tsxsrc/components/TournamentPairingGenerationForm/components/TournamentCompetitorChecklist/TournamentCompetitorChecklist.module.scsssrc/components/TournamentPairingGenerationForm/components/TournamentCompetitorChecklist/TournamentCompetitorChecklist.tsxsrc/components/TournamentPairingGenerationForm/components/TournamentCompetitorChecklist/index.tssrc/components/TournamentPairingGenerationForm/components/TournamentPairingConfigFields/TournamentPairingConfigFields.hooks.tssrc/components/TournamentPairingGenerationForm/components/TournamentPairingConfigFields/TournamentPairingConfigFields.module.scsssrc/components/TournamentPairingGenerationForm/components/TournamentPairingConfigFields/TournamentPairingConfigFields.tsxsrc/components/TournamentPairingGenerationForm/components/TournamentPairingConfigFields/index.tssrc/components/TournamentPairingGenerationForm/components/TournamentPairingConfigForm/TournamentPairingConfigForm.tsxsrc/components/TournamentPairingGenerationForm/components/TournamentPairingConfigForm/index.tssrc/components/TournamentPairingGenerationForm/index.tssrc/components/TournamentProvider/TournamentContextMenu.tsxsrc/components/TournamentProvider/TournamentProvider.hooks.tsxsrc/components/TournamentProvider/actions/useConfigureRoundAction.tsxsrc/components/TournamentProvider/actions/useStartAction.tsxsrc/components/TournamentProvider/index.tssrc/components/TournamentRegistrationIdentity/TournamentRegistrationIdentity.module.scsssrc/components/TournamentRegistrationIdentity/TournamentRegistrationIdentity.tsxsrc/components/TournamentRegistrationIdentity/index.tssrc/hooks/useAsyncState.tssrc/hooks/useFormDialog.tsxsrc/pages/MatchResultDetailPage/MatchResultDetailPage.tsxsrc/pages/TournamentCompetitorDetailPage/TournamentCompetitorDetailPage.tsxsrc/pages/TournamentDetailPage/TournamentDetailPage.tsxsrc/pages/TournamentDetailPage/components/TournamentCompetitorTable/TournamentCompetitorTable.module.scsssrc/pages/TournamentDetailPage/components/TournamentCompetitorTable/TournamentCompetitorTable.tsxsrc/pages/TournamentDetailPage/components/TournamentCompetitorTable/index.tssrc/pages/TournamentDetailPage/components/TournamentCompetitorsCard/TournamentCompetitorsCard.module.scsssrc/pages/TournamentDetailPage/components/TournamentCompetitorsCard/TournamentCompetitorsCard.tsxsrc/pages/TournamentDetailPage/components/TournamentCompetitorsCard/TournamentCompetitorsCard.utils.tsxsrc/pages/TournamentDetailPage/components/TournamentCompetitorsCard/index.tssrc/pages/TournamentDetailPage/components/TournamentRankingsCard/TournamentRankingsCard.tsxsrc/pages/TournamentDetailPage/components/TournamentRankingsCard/TournamentRankingsCard.utils.tsxsrc/pages/TournamentDetailPage/components/TournamentRankingsCard/index.tssrc/pages/TournamentDetailPage/components/TournamentRegistrationTable/TournamentRegistrationTable.module.scsssrc/pages/TournamentDetailPage/components/TournamentRegistrationTable/TournamentRegistrationTable.tsxsrc/pages/TournamentDetailPage/components/TournamentRegistrationTable/index.tssrc/pages/TournamentDetailPage/components/TournamentRosterCard/TournamentRosterCard.module.scsssrc/pages/TournamentDetailPage/components/TournamentRosterCard/TournamentRosterCard.tsxsrc/pages/TournamentDetailPage/components/TournamentRosterCard/TournamentRosterCard.utils.tsxsrc/pages/TournamentDetailPage/components/TournamentRosterCard/index.tssrc/pages/TournamentPairingDetailPage/TournamentPairingDetailPage.tsxsrc/pages/TournamentPairingsPage/TournamentPairingsPage.module.scsssrc/pages/TournamentPairingsPage/TournamentPairingsPage.schema.tssrc/pages/TournamentPairingsPage/TournamentPairingsPage.tsxsrc/pages/TournamentPairingsPage/TournamentPairingsPage.utils.tsxsrc/pages/TournamentPairingsPage/components/Draggable/Draggable.module.scsssrc/pages/TournamentPairingsPage/components/Draggable/Draggable.tsxsrc/pages/TournamentPairingsPage/components/Draggable/index.tssrc/pages/TournamentPairingsPage/components/Droppable/Droppable.module.scsssrc/pages/TournamentPairingsPage/components/Droppable/Droppable.tsxsrc/pages/TournamentPairingsPage/components/Droppable/index.tssrc/pages/TournamentPairingsPage/components/PairingsGrid/PairingsGrid.hooks.tssrc/pages/TournamentPairingsPage/components/PairingsGrid/PairingsGrid.module.scsssrc/pages/TournamentPairingsPage/components/PairingsGrid/PairingsGrid.tsxsrc/pages/TournamentPairingsPage/components/PairingsGrid/PairingsGrid.utils.tssrc/pages/TournamentPairingsPage/components/PairingsGrid/index.tssrc/services/tournaments.tssrc/utils/common/getRoundOptions.ts
💤 Files with no reviewable changes (14)
- src/pages/TournamentDetailPage/components/TournamentRankingsCard/index.ts
- src/components/TournamentProvider/index.ts
- src/components/TournamentProvider/TournamentProvider.hooks.tsx
- src/pages/TournamentDetailPage/components/TournamentRosterCard/TournamentRosterCard.module.scss
- src/pages/TournamentDetailPage/components/TournamentRosterCard/TournamentRosterCard.utils.tsx
- src/pages/TournamentDetailPage/components/TournamentRosterCard/index.ts
- src/components/TournamentPairingConfigForm/index.ts
- src/components/TournamentProvider/actions/useStartAction.tsx
- src/pages/TournamentDetailPage/components/TournamentRosterCard/TournamentRosterCard.tsx
- src/pages/TournamentPairingsPage/TournamentPairingsPage.module.scss
- src/components/TournamentProvider/TournamentContextMenu.tsx
- convex/_model/tournamentPairings/mutations/createTournamentPairings.ts
- src/pages/TournamentDetailPage/components/TournamentRankingsCard/TournamentRankingsCard.utils.tsx
- src/pages/TournamentDetailPage/components/TournamentRankingsCard/TournamentRankingsCard.tsx
Comment on lines
+51
to
+54
| pairingConfig: { | ||
| ...tournamentPairingConfigDefaultValues, | ||
| tableCount: Math.ceil(overrides?.maxCompetitors ?? 48 / 2) ?? 1, | ||
| }, |
There was a problem hiding this comment.
Operator-precedence bug: division is only applied to the fallback, not to the override.
Due to JavaScript's operator precedence, ?? binds less tightly than /, so overrides?.maxCompetitors ?? 48 / 2 is parsed as overrides?.maxCompetitors ?? (48 / 2) — i.e. overrides?.maxCompetitors ?? 24. When maxCompetitors is supplied as an override (e.g. 32), Math.ceil(32) = 32, but tableCount should be Math.ceil(32 / 2) = 16. The trailing ?? 1 is also dead code since Math.ceil always returns a number.
🐛 Proposed fix
- tableCount: Math.ceil(overrides?.maxCompetitors ?? 48 / 2) ?? 1,
+ tableCount: Math.ceil((overrides?.maxCompetitors ?? 48) / 2),📝 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
| pairingConfig: { | |
| ...tournamentPairingConfigDefaultValues, | |
| tableCount: Math.ceil(overrides?.maxCompetitors ?? 48 / 2) ?? 1, | |
| }, | |
| pairingConfig: { | |
| ...tournamentPairingConfigDefaultValues, | |
| tableCount: Math.ceil((overrides?.maxCompetitors ?? 48) / 2), | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@convex/_fixtures/createMockTournament.ts` around lines 51 - 54, The
tableCount calculation in pairingConfig incorrectly applies the division only to
the fallback due to operator precedence; change the expression so the nullish
coalescing picks the competitor count first and then divide by 2 (e.g. compute
Math.ceil((overrides?.maxCompetitors ?? 48) / 2)), and remove the redundant
trailing ?? 1 since Math.ceil always returns a number; update the expression in
the pairingConfig block that references tournamentPairingConfigDefaultValues and
tableCount accordingly.
| playedTablesMap.set(null, []); | ||
|
|
||
| const tableCount = (data.tournament?.competitorCount ?? 2) / 2; // TODO: Use actual table count | ||
| const tableCount = data.tournament.pairingConfig.tableCount!; // TODO remove assertion post MIGRATION |
There was a problem hiding this comment.
Avoid hard dependency on migration completion here.
At Line 23, removing the fallback and forcing pairingConfig.tableCount can break pair generation for tournaments not yet backfilled. Keep a runtime fallback until migration is guaranteed complete in all environments.
Suggested fix
- const tableCount = data.tournament.pairingConfig.tableCount!; // TODO remove assertion post MIGRATION
+ const tableCount = data.tournament.pairingConfig.tableCount
+ ?? Math.ceil((data.tournament.competitorCount ?? 2) / 2);📝 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 tableCount = data.tournament.pairingConfig.tableCount!; // TODO remove assertion post MIGRATION | |
| const tableCount = data.tournament.pairingConfig.tableCount | |
| ?? Math.ceil((data.tournament.competitorCount ?? 2) / 2); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@convex/_model/tournamentPairings/_helpers/assignTables.ts` at line 23, The
code uses a non-null assertion on data.tournament.pairingConfig.tableCount which
will break tournaments not yet migrated; replace the assertion with a
runtime-safe fallback (e.g., use optional chaining and nullish coalescing:
data.tournament.pairingConfig?.tableCount ?? <fallback>) and pick an appropriate
fallback (a constant DEFAULT_TABLE_COUNT, a value derived from
data.tournament/player count, or a domain-specific safe default) inside the
assignTables logic so pair generation still works until migration is complete;
remove the "!" on tableCount and reference the pairingConfig?.tableCount and the
chosen DEFAULT_TABLE_COUNT or computed fallback in the same function.
Comment on lines
+25
to
+27
| if (doc.lastRound && doc.lastRound + 1 === doc.roundCount) { | ||
| return Math.max(doc.lastRound - 1, 0); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Prefer explicit !== undefined over truthy check on doc.lastRound.
When doc.lastRound === 0 (a one-round tournament that just completed), doc.lastRound && short-circuits to falsy and the code falls through to return doc.lastRound, returning 0. The result happens to match the intended clamped penultimate (Math.max(0 - 1, 0) === 0), so the corresponding test passes by coincidence rather than by exercising this branch. An explicit undefined check makes the contract for round 0 unambiguous and survives future tweaks.
♻️ Proposed refactor
// Show penultimate round to players if last round has been completed:
- if (doc.lastRound && doc.lastRound + 1 === doc.roundCount) {
+ if (doc.lastRound !== undefined && doc.lastRound + 1 === doc.roundCount) {
return Math.max(doc.lastRound - 1, 0);
}📝 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
| if (doc.lastRound && doc.lastRound + 1 === doc.roundCount) { | |
| return Math.max(doc.lastRound - 1, 0); | |
| } | |
| if (doc.lastRound !== undefined && doc.lastRound + 1 === doc.roundCount) { | |
| return Math.max(doc.lastRound - 1, 0); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@convex/_model/tournaments/_helpers/getLastVisibleRound.ts` around lines 25 -
27, In getLastVisibleRound, avoid the truthy check on doc.lastRound; change the
condition "if (doc.lastRound && doc.lastRound + 1 === doc.roundCount)" to
explicitly check for undefined (e.g., "if (doc.lastRound !== undefined &&
doc.lastRound + 1 === doc.roundCount)") so a lastRound of 0 is handled correctly
and still returns Math.max(doc.lastRound - 1, 0); leave the fallback return
doc.lastRound as-is.
Comment on lines
+145
to
+147
| if (doc.pairingConfig.tableCount) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Use an explicit undefined check for tableCount guard.
At Line 145, a truthy check can incorrectly backfill when tableCount is 0. Use !== undefined (or != null) to only backfill truly missing values.
Suggested fix
- if (doc.pairingConfig.tableCount) {
+ if (doc.pairingConfig.tableCount !== undefined) {
return;
}📝 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
| if (doc.pairingConfig.tableCount) { | |
| return; | |
| } | |
| if (doc.pairingConfig.tableCount !== undefined) { | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@convex/migrations.ts` around lines 145 - 147, The guard that skips
backfilling uses a truthy check on doc.pairingConfig.tableCount which will treat
0 as missing; change the conditional to explicitly check for undefined or null
(e.g., use !== undefined or != null) so only truly missing values are
backfilled; locate the check that reads "if (doc.pairingConfig.tableCount) {
return; }" and replace it with an explicit undefined/null check on
doc.pairingConfig.tableCount.
Comment on lines
+25
to
+28
| const primaryAlignment = Array.isArray(alignment) ? alignment[0] : alignment; | ||
|
|
||
| const color = getAlignmentColor(primaryAlignment); | ||
| const displayName = getAlignmentDisplayName(primaryAlignment) ?? 'Unknown Alignment'; | ||
| const displayName = primaryAlignment && getAlignmentDisplayName(primaryAlignment) || 'Unknown Alignment'; |
There was a problem hiding this comment.
Empty array produces undefined primaryAlignment, which getAlignmentColor doesn't handle.
When alignment is [], alignment[0] is undefined at runtime. TypeScript (without noUncheckedIndexedAccess) infers the type as Alignment and won't catch this. getAlignmentColor is typed to accept Alignment | null, not undefined, so passing undefined produces unspecified behavior. The displayName fallback on line 28 is safe (falsy short-circuit), but the color derivation on line 27 is not.
🛡️ Proposed fix — guard against empty array
- const primaryAlignment = Array.isArray(alignment) ? alignment[0] : alignment;
+ const primaryAlignment = Array.isArray(alignment) ? (alignment[0] ?? null) : alignment;📝 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 primaryAlignment = Array.isArray(alignment) ? alignment[0] : alignment; | |
| const color = getAlignmentColor(primaryAlignment); | |
| const displayName = getAlignmentDisplayName(primaryAlignment) ?? 'Unknown Alignment'; | |
| const displayName = primaryAlignment && getAlignmentDisplayName(primaryAlignment) || 'Unknown Alignment'; | |
| const primaryAlignment = Array.isArray(alignment) ? (alignment[0] ?? null) : alignment; | |
| const color = getAlignmentColor(primaryAlignment); | |
| const displayName = primaryAlignment && getAlignmentDisplayName(primaryAlignment) || 'Unknown Alignment'; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/FactionIndicator/FactionIndicator.tsx` around lines 25 - 28,
primaryAlignment can become undefined when alignment === [], which breaks
getAlignmentColor (expects Alignment | null). Update the primaryAlignment
computation (the variable used by getAlignmentColor and getAlignmentDisplayName)
to explicitly guard empty arrays and normalize to null instead of undefined
(e.g., when Array.isArray(alignment) use alignment.length ? alignment[0] : null,
and when alignment is a scalar use alignment ?? null) so
getAlignmentColor(primaryAlignment) always receives Alignment | null and
displayName fallback logic continues to work.
Comment on lines
106
to
108
| /* Only do this when loading. Otherwise since the first render happens before we know the | ||
| * tournament status, getDefaultTab ALWAYS returns 'roster'. */ | ||
| * tournament status, getDefaultTab ALWAYS returns 'roster'. */ | ||
| if (activeTab !== queryTab && !loading) { |
There was a problem hiding this comment.
Stale comment still references the removed 'roster' tab.
The inline comment was written for the old tab name and now misleads readers.
📝 Proposed fix
- /* Only do this when loading. Otherwise since the first render happens before we know the
- * tournament status, getDefaultTab ALWAYS returns 'roster'. */
+ /* Only do this when loading. Otherwise since the first render happens before we know the
+ * tournament status, getDefaultTab ALWAYS returns 'competitors'. */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/TournamentDetailPage/TournamentDetailPage.tsx` around lines 106 -
108, Update the stale inline comment that references the removed 'roster' tab:
remove the literal 'roster' and instead refer to the actual behavior (the
default tab returned by getDefaultTab) or the current default tab name; update
the comment near the if (activeTab !== queryTab && !loading) check to accurately
describe that the first render occurs before tournament status is known so
getDefaultTab determines the initial tab, and remove any mention of the old
'roster' identifier.
Comment on lines
+33
to
+39
| <button | ||
| className={sx({ variant: 'ghost', corners: 'normal' }, styles.Draggable_Handle)} | ||
| {...attributes} | ||
| {...listeners} | ||
| > | ||
| <GripVertical /> | ||
| </button> |
There was a problem hiding this comment.
Add an accessible name to the drag handle.
This is an unlabeled icon-only <button>, so screen-reader users have no way to identify the control that starts the drag interaction.
Suggested fix
<button
+ type="button"
+ aria-label="Drag competitor"
className={sx({ variant: 'ghost', corners: 'normal' }, styles.Draggable_Handle)}
{...attributes}
{...listeners}
>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/TournamentPairingsPage/components/Draggable/Draggable.tsx` around
lines 33 - 39, The drag handle button in Draggable.tsx is an unlabeled icon-only
control (GripVertical) so add an accessible name to the <button> by providing an
aria-label (or aria-labelledby to a nearby visible label) so screen readers can
announce the control; update the button that uses attributes and listeners (the
element with class styles.Draggable_Handle and props {...attributes}
{...listeners}) to include a descriptive aria-label such as "Drag handle" or
context-specific text like "Move pairing".
Comment on lines
+56
to
+58
| const destPairingIndex = pairings.findIndex((p) => p.id === parts[1]); | ||
| const destSlotIndex = Number(parts[2]) as 0 | 1; | ||
| const destOccupant = destSlotIndex === 0 ? pairings[destPairingIndex].tournamentCompetitor0Id : pairings[destPairingIndex].tournamentCompetitor1Id; |
There was a problem hiding this comment.
destPairingIndex === -1 is not guarded, causing a runtime TypeError.
pairings.findIndex(...) returns -1 when the pairing ID embedded in overId is not found (e.g. a pairing is removed while a drag is in-flight). The very next line then evaluates pairings[-1].tournamentCompetitor0Id, which throws because pairings[-1] is undefined.
🐛 Proposed fix
const destPairingIndex = pairings.findIndex((p) => p.id === parts[1]);
+ if (destPairingIndex === -1) {
+ return null;
+ }
const destSlotIndex = Number(parts[2]) as 0 | 1;
const destOccupant = destSlotIndex === 0 ? pairings[destPairingIndex].tournamentCompetitor0Id : pairings[destPairingIndex].tournamentCompetitor1Id;📝 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 destPairingIndex = pairings.findIndex((p) => p.id === parts[1]); | |
| const destSlotIndex = Number(parts[2]) as 0 | 1; | |
| const destOccupant = destSlotIndex === 0 ? pairings[destPairingIndex].tournamentCompetitor0Id : pairings[destPairingIndex].tournamentCompetitor1Id; | |
| const destPairingIndex = pairings.findIndex((p) => p.id === parts[1]); | |
| if (destPairingIndex === -1) { | |
| return null; | |
| } | |
| const destSlotIndex = Number(parts[2]) as 0 | 1; | |
| const destOccupant = destSlotIndex === 0 ? pairings[destPairingIndex].tournamentCompetitor0Id : pairings[destPairingIndex].tournamentCompetitor1Id; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/pages/TournamentPairingsPage/components/PairingsGrid/PairingsGrid.utils.ts`
around lines 56 - 58, The code assumes destPairingIndex is valid but findIndex
can return -1; add a guard after computing destPairingIndex (using the existing
variables destPairingIndex, destSlotIndex, destOccupant, parts, and pairings)
that checks for destPairingIndex === -1 and bails out (e.g., return early, skip
the drop handling, or set destOccupant to undefined) so you never access
pairings[-1]. Ensure any downstream logic that uses destOccupant handles the
early-exit case consistently.
Comment on lines
+68
to
+79
| const { data: tournamentPairings } = useGetTournamentPairings({ | ||
| tournamentId, | ||
| round: nextRound, | ||
| }); | ||
|
|
||
| const form = useForm<FormData>({ | ||
| resolver: zodResolver(schema), | ||
| defaultValues: { | ||
| pairings: [], | ||
| }, | ||
| mode: 'onSubmit', | ||
| }); | ||
| const { fields } = useFieldArray({ | ||
| control: form.control, | ||
| name: 'pairings', | ||
| }); | ||
| const pairings = useWatch({ | ||
| control: form.control, | ||
| name: 'pairings', | ||
| }); | ||
| // State: | ||
| const [pairings, setPairings] = useState<TournamentPairingFormItem[]>([]); | ||
| useEffect(() => { | ||
| if (!pairings.length && tournamentPairings?.length) { | ||
| setPairings(sanitize(tournamentPairings)); | ||
| } | ||
| }, [tournamentPairings, pairings, setPairings]); |
There was a problem hiding this comment.
This hydration logic can restore the wrong pairings state.
Before tournament finishes loading, nextRound is 0, so this query can fetch round 0 pairings and seed local state with them. The same effect also re-hydrates server pairings whenever the user deletes all local slots, because pairings.length goes back to 0.
Suggested fix
const [pairings, setPairings] = useState<TournamentPairingFormItem[]>([]);
+ const [hasHydratedPairings, setHasHydratedPairings] = useState(false);
useEffect(() => {
- if (!pairings.length && tournamentPairings?.length) {
- setPairings(sanitize(tournamentPairings));
- }
- }, [tournamentPairings, pairings, setPairings]);
+ if (!tournament || hasHydratedPairings || tournamentPairings == null) {
+ return;
+ }
+
+ setPairings(sanitize(tournamentPairings));
+ setHasHydratedPairings(true);
+ }, [tournament, tournamentPairings, hasHydratedPairings, setPairings]);📝 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 { data: tournamentPairings } = useGetTournamentPairings({ | |
| tournamentId, | |
| round: nextRound, | |
| }); | |
| const form = useForm<FormData>({ | |
| resolver: zodResolver(schema), | |
| defaultValues: { | |
| pairings: [], | |
| }, | |
| mode: 'onSubmit', | |
| }); | |
| const { fields } = useFieldArray({ | |
| control: form.control, | |
| name: 'pairings', | |
| }); | |
| const pairings = useWatch({ | |
| control: form.control, | |
| name: 'pairings', | |
| }); | |
| // State: | |
| const [pairings, setPairings] = useState<TournamentPairingFormItem[]>([]); | |
| useEffect(() => { | |
| if (!pairings.length && tournamentPairings?.length) { | |
| setPairings(sanitize(tournamentPairings)); | |
| } | |
| }, [tournamentPairings, pairings, setPairings]); | |
| const { data: tournamentPairings } = useGetTournamentPairings({ | |
| tournamentId, | |
| round: nextRound, | |
| }); | |
| // State: | |
| const [pairings, setPairings] = useState<TournamentPairingFormItem[]>([]); | |
| const [hasHydratedPairings, setHasHydratedPairings] = useState(false); | |
| useEffect(() => { | |
| if (!tournament || hasHydratedPairings || tournamentPairings == null) { | |
| return; | |
| } | |
| setPairings(sanitize(tournamentPairings)); | |
| setHasHydratedPairings(true); | |
| }, [tournament, tournamentPairings, hasHydratedPairings, setPairings]); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/TournamentPairingsPage/TournamentPairingsPage.tsx` around lines 68
- 79, The effect seeds local pairings from useGetTournamentPairings too eagerly
and when local pairings are cleared; fix by tracking which round you've
initialized: add a state like initializedRound: number | null, and change the
useEffect (watching tournamentPairings, nextRound, initializedRound) to only
call setPairings(sanitize(tournamentPairings)) when tournamentPairings?.length
&& initializedRound === null && nextRound === the round corresponding to
tournamentPairings (and/or nextRound !== 0 if applicable), then set
initializedRound to nextRound; remove pairings from the effect deps so clearing
local pairings doesn't re-seed. Ensure you reference useGetTournamentPairings,
nextRound, pairings, setPairings, sanitize and initializedRound in your change.
Comment on lines
+92
to
+94
| const isDirty = pairings.length && tournamentPairings?.length && ( | ||
| pairings.some((p) => !tournamentPairings.some((sp) => pairingsMatch(p, sp))) | ||
| ); |
There was a problem hiding this comment.
Dirty-state detection misses deletions and create-from-empty edits.
This only checks whether each local pairing exists in the server list. Deleting a pairing, clearing all pairings, or adding the first pairing to an empty round leaves isDirty false, so the cancel confirmation can be skipped and edits lost.
Suggested fix
- const isDirty = pairings.length && tournamentPairings?.length && (
- pairings.some((p) => !tournamentPairings.some((sp) => pairingsMatch(p, sp)))
- );
+ const serverPairings = tournamentPairings ?? [];
+ const isDirty =
+ pairings.length !== serverPairings.length ||
+ pairings.some((p) => !serverPairings.some((sp) => pairingsMatch(p, sp))) ||
+ serverPairings.some((sp) => !pairings.some((p) => pairingsMatch(p, sp)));📝 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 isDirty = pairings.length && tournamentPairings?.length && ( | |
| pairings.some((p) => !tournamentPairings.some((sp) => pairingsMatch(p, sp))) | |
| ); | |
| const serverPairings = tournamentPairings ?? []; | |
| const isDirty = | |
| pairings.length !== serverPairings.length || | |
| pairings.some((p) => !serverPairings.some((sp) => pairingsMatch(p, sp))) || | |
| serverPairings.some((sp) => !pairings.some((p) => pairingsMatch(p, sp))); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/TournamentPairingsPage/TournamentPairingsPage.tsx` around lines 92
- 94, The current isDirty only checks whether every local pairing exists on the
server and short-circuits when either pairings or tournamentPairings is empty,
missing deletions and create-from-empty cases; replace that logic with a
symmetric comparison: compute isDirty as true if any local pairing has no match
in tournamentPairings OR any server pairing has no match in pairings (e.g.,
isDirty = pairings.some(p => !tournamentPairings?.some(sp => pairingsMatch(p,
sp))) || tournamentPairings?.some(sp => !pairings.some(p => pairingsMatch(p,
sp)))); ensure you do not short-circuit on empty arrays so adding the first
pairing or clearing all pairings is detected.
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.
Fixes CC-2: Improve Pairing Workflow
Summary by CodeRabbit
New Features
Refactor