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:
WalkthroughThis pull request introduces a 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@convex/_model/lists/mutations/createList.ts`:
- Around line 17-25: The code calls Date.now() twice when setting
computedFields.submittedAt and later computing computedFields.locked, which can
create inconsistent state; fix by capturing a single const now = Date.now() at
the start of createList (or just before computedFields) and use that same now
when assigning computedFields.submittedAt and when evaluating Date.now() >
tournament.listSubmissionClosesAt (replace that call with now) — update
references in the createList mutation around computedFields, submittedAt, locked
and the tournament/tournamentRegistration logic that uses getDocStrict.
In `@convex/_model/lists/table.ts`:
- Line 14: submittedAt is currently declared in editableFields allowing it into
createListArgs and updateListArgs and thus clients can modify it via the
updateList mutation that applies ...updated; move submittedAt from
editableFields to computedFields in the model (reference symbol submittedAt) so
it is excluded from client-provided args, update any code that builds
createListArgs/updateListArgs to stop expecting submittedAt (references:
createList.ts where submittedAt is set, checkListSubmittedOnTime.ts which reads
it), and ensure updateList mutation no longer applies unfiltered ...updated to
the DB (or filters out submittedAt) to prevent client override.
🪄 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: 1545ad02-6d5d-4c6a-b441-087b0c05f37e
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
convex/_model/lists/_helpers/checkListSubmittedOnTime.tsconvex/_model/lists/mutations/createList.tsconvex/_model/lists/table.tsconvex/migrations.tssrc/components/TournamentProvider/actions/useToggleListsRevealedAction.tsx
| const computedFields = { | ||
| locked: false, | ||
| submittedAt: Date.now(), | ||
| }; | ||
|
|
||
| if (args.tournamentRegistrationId) { | ||
| const tournamentRegistration = await getDocStrict(ctx, args.tournamentRegistrationId); | ||
| const tournament = await getDocStrict(ctx, tournamentRegistration.tournamentId); | ||
| computedFields = { | ||
| locked: Date.now() > tournament.listSubmissionClosesAt, | ||
| }; | ||
| computedFields.locked = Date.now() > tournament.listSubmissionClosesAt; |
There was a problem hiding this comment.
Use one timestamp for both submittedAt and lock evaluation.
Lines 19 and 25 call Date.now() separately. Around close-time boundaries (especially with async fetch latency), this can create contradictory state (submittedAt before deadline but locked true, or vice versa). Capture now once and reuse it.
Proposed fix
- const computedFields = {
- locked: false,
- submittedAt: Date.now(),
- };
+ const now = Date.now();
+ const computedFields = {
+ locked: false,
+ submittedAt: now,
+ };
if (args.tournamentRegistrationId) {
const tournamentRegistration = await getDocStrict(ctx, args.tournamentRegistrationId);
const tournament = await getDocStrict(ctx, tournamentRegistration.tournamentId);
- computedFields.locked = Date.now() > tournament.listSubmissionClosesAt;
+ computedFields.locked = now > tournament.listSubmissionClosesAt;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@convex/_model/lists/mutations/createList.ts` around lines 17 - 25, The code
calls Date.now() twice when setting computedFields.submittedAt and later
computing computedFields.locked, which can create inconsistent state; fix by
capturing a single const now = Date.now() at the start of createList (or just
before computedFields) and use that same now when assigning
computedFields.submittedAt and when evaluating Date.now() >
tournament.listSubmissionClosesAt (replace that call with now) — update
references in the createList mutation around computedFields, submittedAt, locked
and the tournament/tournamentRegistration logic that uses getDocStrict.
| storageId: v.id('_storage'), | ||
| userId: v.id('users'), | ||
| tournamentRegistrationId: v.optional(v.id('tournamentRegistrations')), | ||
| submittedAt: v.optional(v.number()), |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find all places where lists editableFields are reused for args validation.
rg -n --type=ts -C3 '\b(v\.object\(editableFields\)|v\.partial\(editableFields\)|editableFields)\b' convex/_model/lists convex
# 2) Inspect list mutations that patch/replace list docs to confirm whether args flow directly into writes.
rg -n --type=ts -C4 'ctx\.db\.(patch|replace)\(' convex/_model/listsRepository: ianpaschal/combat-command
Length of output: 37898
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find all references to submittedAt in the codebase to understand its intent
rg -n --type=ts 'submittedAt' convex/_model/lists
# 2) Check if there's a migration file or initial creation logic setting submittedAt
fd -t f 'migration|Migration' convex/_model/lists && rg -n --type=ts 'submittedAt' convex || true
# 3) Look at the getAvailableActions helper for lists to understand permission model
cat -n convex/_model/lists/_helpers/getAvailableActions.ts 2>/dev/null || echo "File not found"Repository: ianpaschal/combat-command
Length of output: 3581
Move submittedAt from editableFields to computedFields to prevent client modification.
submittedAt is server-managed (set only during creation in createList.ts:19) and used for deadline checks in checkListSubmittedOnTime.ts. However, because it's in editableFields, it flows into both createListArgs and updateListArgs. The updateList mutation then patches the document directly with ...updated without filtering, allowing clients to modify the submission timestamp.
Proposed refactor
export const editableFields = {
gameSystem,
storageId: v.id('_storage'),
userId: v.id('users'),
tournamentRegistrationId: v.optional(v.id('tournamentRegistrations')),
- submittedAt: v.optional(v.number()),
// FUTURE:
// data: listData,
};
export const computedFields = {
modifiedAt: v.optional(v.number()),
+ submittedAt: v.optional(v.number()),
locked: v.boolean(),
};📝 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.
| submittedAt: v.optional(v.number()), | |
| export const editableFields = { | |
| gameSystem, | |
| storageId: v.id('_storage'), | |
| userId: v.id('users'), | |
| tournamentRegistrationId: v.optional(v.id('tournamentRegistrations')), | |
| // FUTURE: | |
| // data: listData, | |
| }; | |
| export const computedFields = { | |
| modifiedAt: v.optional(v.number()), | |
| submittedAt: v.optional(v.number()), | |
| locked: v.boolean(), | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@convex/_model/lists/table.ts` at line 14, submittedAt is currently declared
in editableFields allowing it into createListArgs and updateListArgs and thus
clients can modify it via the updateList mutation that applies ...updated; move
submittedAt from editableFields to computedFields in the model (reference symbol
submittedAt) so it is excluded from client-provided args, update any code that
builds createListArgs/updateListArgs to stop expecting submittedAt (references:
createList.ts where submittedAt is set, checkListSubmittedOnTime.ts which reads
it), and ensure updateList mutation no longer applies unfiltered ...updated to
the DB (or filters out submittedAt) to prevent client override.
1 participant
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor