#288 Allow updating of registration faction & alignment#298
Merged
ianpaschal merged 4 commits intomainfrom Feb 9, 2026
Merged
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. WalkthroughAdds edit support for tournament registrations: new backend mutation, detailFields schema extraction, Edit action added and gated by role/status, UI form and hooks for editing, and public API/type exports adjusted. (50 words) Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 10
🤖 Fix all issues with AI agents
In
`@convex/_model/tournamentRegistrations/_helpers/deepenTournamentRegistration.ts`:
- Around line 38-41: visibleDetails is manually constructed and must stay in
sync with the separately defined detailFields array; add an inline comment above
the visibleDetails declaration explaining that any new fields added to
detailFields require adding corresponding keys here with their specific
visibility logic (e.g., use alignmentsVisible/factionsVisible and
details?.alignment/details?.faction), and that dynamic iteration is not used
because each field has unique reveal flags like
alignmentsRevealed/factionsRevealed.
In `@convex/_model/tournamentRegistrations/_helpers/getAvailableActions.ts`:
- Around line 7-10: Add a section comment and JSDoc for the
TournamentRegistrationActionKey.Edit enum member to match the style of other
members: add a comment like "// ---- Edit Actions ----" (or reuse existing
section naming convention) and a JSDoc block above Edit describing the action
and explicitly listing the roles that can use it (TO, Captain, Player). Update
the enum declaration for TournamentRegistrationActionKey so the Edit member has
the new comment and JSDoc to improve clarity and consistency with other members.
In
`@convex/_model/tournamentRegistrations/mutations/updateTournamentRegistration.ts`:
- Around line 39-53: The code's comment claims a "captain" can edit a competitor
but the logic only allows tournament organizers and the registration owner
(authorizedUserIds includes tournamentRegistration.userId); either update the
comment to match the current behavior or implement captain authorization:
retrieve the team's captain (e.g., call a helper like getTeamCaptainByTeamId or
similar using tournamentRegistration.teamId), add that captain's userId to
authorizedUserIds (alongside tournamentOrganizers.map(...)), and ensure
getTournamentOrganizersByTournament, tournamentRegistration, authorizedUserIds,
and userId are used consistently; if you choose not to implement captain support
now, change the comment text to only mention organizers and the registration
owner and add a TODO referencing captain support.
- Around line 55-59: The patch currently spreads the entire updated object into
ctx.db.patch which will overwrite ownership fields (userId, tournamentId,
tournamentCompetitorId) and cause unnecessary writes; instead construct the
patch payload to include only mutable fields (e.g., details) and modifiedAt —
either pick/assign allowed keys from updated or delete the ownership keys from
updated before calling ctx.db.patch so ctx.db.patch(id, { /* only mutable fields
*/, modifiedAt: Date.now() }) is used rather than spreading the whole updated
object.
- Around line 27-30: Replace the incorrect error code thrown when a registration
is missing: in the updateTournamentRegistration mutation where you check the
result of ctx.db.get(id) (the tournamentRegistration variable), change the
ConvexError call to use getErrorMessage('TOURNAMENT_REGISTRATION_NOT_FOUND')
instead of 'TOURNAMENT_COMPETITOR_NOT_FOUND' so the error matches the resource.
- Around line 13-16: updateTournamentRegistrationArgs is too permissive because
it spreads editableFields (which contains userId, tournamentId,
tournamentCompetitorId) allowing clients to reassign identity fields; replace
the args schema to only accept details (the editable faction/alignment object)
so the mutation and the DB patch in updateTournamentRegistration only update
details, not identity fields. Change the incorrect error code
TOURNAMENT_COMPETITOR_NOT_FOUND to a registration-specific code such as
TOURNAMENT_REGISTRATION_NOT_FOUND in the lookup/throw path. Finally, fix the
authorization comment that references "tournament competitor's captain" to match
the implemented checks in updateTournamentRegistration (either remove captain
mention or add the captain authorization logic if intended).
In
`@src/components/TournamentRegistrationForm/components/DetailsFields/DetailsFields.tsx`:
- Around line 38-39: The visibility logic uses the truthiness of existingValues
(a DeepPartial) causing showAlignmentField and showFactionField to always be
true in edit mode; change each check to look for the specific field on
existingValues first (e.g., existingValues.alignment and existingValues.faction)
and fall back to tournament.registrationDetails?.alignment and
tournament.registrationDetails?.faction respectively so the field only shows
when the registration or the tournament requires/contains that particular value;
update the boolean assignments for showAlignmentField and showFactionField
accordingly.
In `@src/components/TournamentRegistrationForm/TournamentRegistrationForm.tsx`:
- Around line 149-151: TournamentRegistrationForm currently renders
<DetailsFields> whenever existingValues exists, which causes DetailsFields to
show alignment/faction regardless of tournament.registrationDetails; change the
prop passed so DetailsFields receives only the relevant detail slice (e.g., pass
existingValues?.details or explicit props like existingAlignment and
existingFaction instead of the whole existingValues object) and update
DetailsFields to determine visibility by checking specific fields (e.g.,
tournament.registrationDetails?.alignment || existingAlignment) and
(tournament.registrationDetails?.faction || existingFaction) rather than
!!(existingValues || tournament.registrationDetails?.alignment).
In `@src/components/TournamentRegistrationProvider/actions/useEditAction.tsx`:
- Around line 40-48: The onSubmit handler currently spreads forcedValues into
the mutation payload which can leak ownership fields; update the onSubmit in
useEditAction.tsx so it only sends the id and the details object to mutation
(e.g., mutation({ id: subject._id, details: data.details })) instead of
...forcedValues and spreading editable fields, and keep the param destructuring
for nameVisibilityConsent and tournamentCompetitor (rename to
_nameVisibilityConsent/_tournamentCompetitor if desired) so ownership fields
(tournamentCompetitorId, tournamentId, userId) are not sent from the client.
- Around line 18-23: The onSuccess callback passed into
useUpdateTournamentRegistration references close before it's declared; to avoid
the forward reference, call useFormDialog (to get close) before invoking
useUpdateTournamentRegistration in useEditAction so close is bound before being
used, then pass the onSuccess that calls close and shows the toast;
alternatively, capture close into a local function (e.g., handleSuccess) after
close is declared and pass that to useUpdateTournamentRegistration. Ensure
symbols: useUpdateTournamentRegistration, onSuccess, close, useFormDialog, and
subject.displayName are used as described.
convex/_model/tournamentRegistrations/_helpers/deepenTournamentRegistration.ts
Show resolved
Hide resolved
convex/_model/tournamentRegistrations/mutations/updateTournamentRegistration.ts
Show resolved
Hide resolved
convex/_model/tournamentRegistrations/mutations/updateTournamentRegistration.ts
Outdated
Show resolved
Hide resolved
convex/_model/tournamentRegistrations/mutations/updateTournamentRegistration.ts
Outdated
Show resolved
Hide resolved
convex/_model/tournamentRegistrations/mutations/updateTournamentRegistration.ts
Show resolved
Hide resolved
src/components/TournamentRegistrationForm/components/DetailsFields/DetailsFields.tsx
Show resolved
Hide resolved
src/components/TournamentRegistrationForm/TournamentRegistrationForm.tsx
Outdated
Show resolved
Hide resolved
src/components/TournamentRegistrationProvider/actions/useEditAction.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/TournamentRegistrationForm/TournamentRegistrationForm.tsx (1)
56-61:⚠️ Potential issue | 🟠 MajorSpreading a full
TournamentRegistrationintodefaultValuesleaks non-form fields.
existingValuesis now typed asTournamentRegistration, which includes fields like_id,displayName,availableActions,active,confirmed, etc. that are not part ofFormData. Whilereact-hook-formwon't render them, it will track them as registered values and they'll appear in the submit payload — which is why theonSubmitinuseEditActionends up sending extra fields to the mutation.Consider mapping
existingValuesto only theFormData-compatible subset before spreading:Proposed approach
Either narrow the spread here:
const form = useForm<FormData>({ defaultValues: { ...defaultValues, - ...existingValues, + ...(existingValues && { + userId: existingValues.userId, + tournamentId: existingValues.tournamentId, + tournamentCompetitorId: existingValues.tournamentCompetitorId, + details: existingValues.details, + }), ...forcedValues, }, mode: 'onSubmit', });Or handle it at the
onSubmitboundary inuseEditAction(see related comment on that file).convex/_model/tournamentRegistrations/table.ts (1)
35-36: 🧹 Nitpick | 🔵 TrivialVerify whether
by_tournament_competitorindex is necessary.In Convex, compound indexes support prefix-based queries. The
by_tournament_competitor_userindex on[tournamentCompetitorId, userId]can serve queries that filter only bytournamentCompetitorIdusing the first field as a prefix. According to Convex best practices, you typically only need the compound index unless the single-field index provides different ordering semantics (e.g., different_creationTimeplacement).Check your query patterns: if you never need
by_tournament_competitorwithout also consideringuserIdor creation time ordering, this index is redundant and should be removed.
🤖 Fix all issues with AI agents
In `@convex/_model/tournamentRegistrations/table.ts`:
- Around line 19-23: The table currently duplicates the four fields from
editableFields inside defineTable (userId, tournamentId, tournamentCompetitorId,
details); remove the repeated explicit field declarations and instead spread
editableFields into the defineTable call (e.g., use ...editableFields) so the
schema and editable contract stay in sync, keeping any additional non-editable
fields added directly in the defineTable body.
ianpaschal
deleted the
288-allow-update-registration-faction-alignment
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 #288
Summary by CodeRabbit
New Features
Bug Fixes
UX