#287 Auto-fill faction field on match results if player faction is known #299
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughUpdated internal package versions. Refactored match result form schema into a game-system-parameterized factory. Added optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/components/MatchResultDetailFields/gameSystems/FlamesOfWarV4MatchResultDetailFields/FlamesOfWarV4MatchResultDetailFields.tsx (1)
134-141:⚠️ Potential issue | 🟡 MinorPre-existing: missing
keyprop in.map()render.The
FormFieldelements rendered inside the.map()at Line 134 lack akeyprop. This will trigger a React warning. While not introduced by this PR, it's worth addressing.Proposed fix
{([0, 1] as const).map((i) => ( <FormField + key={i} name={`details.scoreOverride.player${i}Score`}src/components/MatchResultDetailFields/gameSystems/TeamYankeeV2MatchResultDetailFields/TeamYankeeV2MatchResultDetailFields.tsx (1)
134-141:⚠️ Potential issue | 🟡 MinorPre-existing: missing
keyprop in.map()render — same as FlamesOfWarV4.Proposed fix
{([0, 1] as const).map((i) => ( <FormField + key={i} name={`details.scoreOverride.player${i}Score`}src/utils/validateForm.ts (1)
16-22:⚠️ Potential issue | 🟠 MajorDuplicate toasts:
toast.erroris called once per validation issue inside the loop.When a form has multiple validation errors, this fires N identical "Please check the form for errors." toasts. Move the toast call outside the
forEachso it fires at most once.Proposed fix
if (!result.success) { result.error.issues.forEach((issue) => { const fieldPath = issue.path.join('.') as Path<TFieldValues>; setError(fieldPath, { message: issue.message }); - toast.error('Error', { description: 'Please check the form for errors.' }); }); + toast.error('Error', { description: 'Please check the form for errors.' }); }src/components/MatchResultForm/MatchResultForm.schema.ts (1)
32-40:⚠️ Potential issue | 🟠 MajorWhen creating a new single match, changing the game system from FlamesOfWarV4 will cause form state to be inconsistent with the UI.
The
defaultValuesare hardcoded for FlamesOfWarV4. When a user creates a new single match and selects a different game system from the dropdown (line 223), the form'sdetailsandgameSystemConfigfields remain set to FlamesOfWarV4 defaults rather than the selected system's defaults. While tournament-initiated matches have their game system updated viasetValue(lines 125-126), single matches lack this automatic update when the gameSystem field is manually changed.The mismatch is caught during validation on submit, but the form state will be out of sync with the rendered UI fields.
🤖 Fix all issues with AI agents
In
`@src/components/MatchResultForm/components/TournamentPlayerFields/TournamentPlayerFields.tsx`:
- Around line 33-34: player0Options and player1Options are being recreated each
render because getCompetitorPlayerOptions returns new arrays, causing dependent
useEffect hooks to re-run; wrap both calls in useMemo using
selectedPairing?.tournamentCompetitor0 and
selectedPairing?.tournamentCompetitor1 as their dependency inputs so the arrays
are stable between renders (similar to how registrations is memoized). Update
the symbols player0Options, player1Options, getCompetitorPlayerOptions and use
the selectedPairing?tournamentCompetitor0/1 dependencies to ensure effects only
run when the underlying competitor changes.
- Around line 30-31: The two long ternary label lines (player0Label and
player1Label) should be simplified: add a small helper function or intermediate
variables (e.g., getPlayerLabel(tournament, selectedPairing, index) or const
comp0Name = selectedPairing?.tournamentCompetitor0?.displayName) and use that to
build the label, referencing tournament.useTeams, selectedPairing, and
tournamentCompetitor0.displayName / tournamentCompetitor1.displayName; replace
the inline nested ternaries with calls to the helper or the intermediate
variables to improve readability.
- Around line 50-55: The auto-fill effect for player0 is overwriting user
choices because useEffect watches player0Faction and resets it; modify the logic
so the effect only auto-sets once when the registration changes: add a local
flag state (e.g., player0FactionAutoFilled) and change the effect to depend on
player0Registration (not player0Faction); if player0Registration?.factions[0]
exists and the autoFilled flag is false, call setValue('details.player0Faction',
player0Registration.factions[0]) and then set the flag true; alternatively, if
you want to prevent edits after auto-fill, set the existing disableFactions prop
when the flag is true so the field becomes read-only.
In `@src/components/MatchResultForm/MatchResultForm.schema.ts`:
- Line 28: The current MatchResultSubmitData alias uses
z.infer<ReturnType<typeof getMatchResultFormSchema>> which collapses
game-system-specific shapes; change the typing so the submit type is tied to the
runtime gameSystem selection — either (A) make getMatchResultFormSchema
generic/overloaded (e.g. getMatchResultFormSchema<TGame extends
GameSystem>(gameSystem: TGame) => z.ZodType<...specific to TGame>) and replace
MatchResultSubmitData with a generic <TGame> alias that infers from
ReturnType<typeof getMatchResultFormSchema<TGame>> so details and
gameSystemConfig stay narrow, or (B) export a generic type alias like
MatchResultSubmitData<TGame extends GameSystem> = z.infer<ReturnType<typeof
getMatchResultFormSchema<TGame>>> and use that at call sites; ensure references
to getMatchResultFormSchema, MatchResultSubmitData, details, and
gameSystemConfig are updated to the new generic/overload form so consumers
retain correct per-game-system types.
In `@src/components/MatchResultForm/MatchResultForm.tsx`:
- Line 237: Inside the MatchResultForm component remove the leftover
commented-out debug line ("{/*
<pre>{JSON.stringify(form.formState.errors)}</pre> */}") so there is no
dead/debug code; update the JSX in MatchResultForm (the component function
MatchResultForm) to delete that comment and ensure formatting/linting remains
valid after removal.
- Around line 133-135: onSubmit currently force-casts selectedGameSystem to
GameSystem when calling getMatchResultFormSchema, which can be undefined; add a
guard before calling getMatchResultFormSchema: read selectedGameSystem from
form.watch('gameSystem') (or the existing selectedGameSystem variable), if it's
undefined set a form error via form.setError (or return early) and do not call
getMatchResultFormSchema; only call getMatchResultFormSchema(selectedGameSystem)
and proceed to validateForm when selectedGameSystem is a defined GameSystem to
avoid passing undefined into getGameSystem.
src/components/MatchResultForm/components/TournamentPlayerFields/TournamentPlayerFields.tsx
Show resolved
Hide resolved
| const player0Options = getCompetitorPlayerOptions(selectedPairing?.tournamentCompetitor0); | ||
| const player1Options = getCompetitorPlayerOptions(selectedPairing?.tournamentCompetitor1); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
player0Options / player1Options are recomputed every render, causing effects to re-evaluate unnecessarily.
getCompetitorPlayerOptions returns a new array reference on each render, so the useEffect hooks listing these as dependencies will re-run every cycle. The guard conditions prevent infinite loops, but wrapping these in useMemo (as you did for the registrations) would be more efficient and idiomatic.
Proposed fix
- const player0Options = getCompetitorPlayerOptions(selectedPairing?.tournamentCompetitor0);
- const player1Options = getCompetitorPlayerOptions(selectedPairing?.tournamentCompetitor1);
+ const player0Options = useMemo(
+ () => getCompetitorPlayerOptions(selectedPairing?.tournamentCompetitor0),
+ [selectedPairing?.tournamentCompetitor0],
+ );
+ const player1Options = useMemo(
+ () => getCompetitorPlayerOptions(selectedPairing?.tournamentCompetitor1),
+ [selectedPairing?.tournamentCompetitor1],
+ );Also applies to: 44-48
🤖 Prompt for AI Agents
In
`@src/components/MatchResultForm/components/TournamentPlayerFields/TournamentPlayerFields.tsx`
around lines 33 - 34, player0Options and player1Options are being recreated each
render because getCompetitorPlayerOptions returns new arrays, causing dependent
useEffect hooks to re-run; wrap both calls in useMemo using
selectedPairing?.tournamentCompetitor0 and
selectedPairing?.tournamentCompetitor1 as their dependency inputs so the arrays
are stable between renders (similar to how registrations is memoized). Update
the symbols player0Options, player1Options, getCompetitorPlayerOptions and use
the selectedPairing?tournamentCompetitor0/1 dependencies to ensure effects only
run when the underlying competitor changes.
src/components/MatchResultForm/components/TournamentPlayerFields/TournamentPlayerFields.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/MatchResultForm/MatchResultForm.tsx (1)
109-122:⚠️ Potential issue | 🟡 MinorUnsafe cast
matchResult.gameSystem as GameSystemcould fail for corrupted data.If a persisted
matchResulthas an invalid or missinggameSystem,getMatchResultFormSchemawill callgetGameSystem(...)with a bad value, potentially throwing during form initialization. Atry/catcharound the IIFE (or a pre-check) would make this more resilient.Suggested safeguard
...(matchResult ? (() => { + try { const result = getMatchResultFormSchema(matchResult.gameSystem as GameSystem).safeParse(matchResult); if (!result.success) { console.error('MatchResultForm schema parsing failed:', result.error); console.error('MatchResult data:', matchResult); } return result.success ? result.data : {}; + } catch (e) { + console.error('Failed to initialize form from matchResult:', e); + return {}; + } })() : {}),
🤖 Fix all issues with AI agents
In
`@src/components/MatchResultForm/components/TournamentPlayerFields/TournamentPlayerFields.tsx`:
- Around line 36-41: The useMemo hooks for player0Registration and
player1Registration depend on the whole selectedPairing object which causes
unnecessary recomputations; update the dependency arrays to reference only the
specific competitor/registrations used (e.g., replace selectedPairing in
player0Registration's deps with selectedPairing?.tournamentCompetitor0 or
selectedPairing?.tournamentCompetitor0?.registrations, and similarly use
selectedPairing?.tournamentCompetitor1 or
selectedPairing?.tournamentCompetitor1?.registrations for player1Registration)
so the memo only recalculates when that competitor's registrations or the
playerUserId actually change.
- Around line 50-69: TournamentPlayerFields currently auto-sets factions (using
player0Registration/player1Registration and setValue) but never signals that the
field should be disabled; compute boolean flags like player0DisableFaction =
player0Registration?.factions.length === 1 and player1DisableFaction =
player1Registration?.factions.length === 1 (use the same registration checks you
already have and consider factions.length > 1 to leave enabled) and lift them to
MatchResultForm by either invoking a provided callback or returning them via
props; then ensure MatchResultForm passes those flags as disableFactions to
MatchResultDetailFields so the UI disables the input when exactly one faction is
available while still pre-selecting the first faction via the existing setValue
logic.
src/components/MatchResultForm/components/TournamentPlayerFields/TournamentPlayerFields.tsx
Outdated
Show resolved
Hide resolved
src/components/MatchResultForm/components/TournamentPlayerFields/TournamentPlayerFields.tsx
Show resolved
Hide resolved
…ds/TournamentPlayerFields.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
Resolves: #287
Summary by CodeRabbit
New Features
Improvements