Add lazy participant auto-timeouts - #1443
Conversation
|
A preview of is uploaded and can be seen here: ✨ https://revisit.dev/study/PR1443 ✨ Changes may take a few minutes to propagate. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ebc054e30
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| onBlur={() => { | ||
| if (autoTimeoutMinutes !== undefined) { | ||
| saveAutoTimeout(autoTimeoutDraftMinutes).catch(() => undefined); | ||
| } |
There was a problem hiding this comment.
When auto-timeout is enabled and the minutes input has focus, clicking the switch first fires this onBlur, which calls saveAutoTimeout and sets timeoutSettingsSaving to true. The resulting render disables the switch before its click/change event, so the user's first attempt to turn auto-timeout off is swallowed and merely resaves the current duration. Only persist on blur when the value changed, or keep the switch interactive while that save is pending.
Useful? React with 👍 / 👎.
JackWilb
left a comment
JackWilb
left a comment
There was a problem hiding this comment.
This feature has the right intent, but I found several correctness and consistency problems that should be fixed before merging. The existing unresolved comment on the minutes-input blur/switch race is also still valid.
The exact-head checks I ran passed typechecking, but the focused unit run had two failures caused by this change: the summary participant-count shape is stale, and the no-Firebase Live Monitor expectation is stale. A third focused failure is in the unchanged ParticipantTimeoutModal test and is not attributable to this PR. The PR’s Closes #194 link also does not match the issue: #194 asks for run labels and a target participant count, and explicitly says the target should not turn participation off.
Please address the inline comments, add coverage for the Local/Supabase interleavings and the full status model, and rerun the relevant unit and browser checks before merging.
| return; | ||
| } | ||
|
|
||
| const deadline = Date.now() - timeoutMinutes * 60_000; |
There was a problem hiding this comment.
[P1] Please do not use the participant’s browser clock to decide whether an assignment has expired. This code runs when a new participant opens the study, so a clock that is ahead can time out active participants, while a clock that is behind can leave old assignments consuming capacity. Compare against a trusted backend time or perform the eligibility check in a backend transaction.
| const sequenceAssignments = await this.studyDatabase.getItem<Record<string, SequenceAssignment>>(sequenceAssignmentPath) || {}; | ||
| if (sequenceAssignments[participantId]) { | ||
| sequenceAssignments[participantId] = { | ||
| ...sequenceAssignment, |
There was a problem hiding this comment.
[P1] This spreads the assignment object captured before the lock. If the participant completes, is rejected, or is timed out after that read but before this write, this replaces the newer record and erases that status. Re-read the assignment inside the lock and merge only the claimed field. Please add a test for completion racing with slot reuse.
| // late completion on the source assignment. | ||
| const { data, error } = await this.supabase | ||
| .from('revisit') | ||
| .update({ data: { ...sequenceAssignment, claimed: true } }) |
There was a problem hiding this comment.
[P1] This is the same stale full-record write in the Supabase engine. The lock starts after the caller already selected sequenceAssignment, so a late completion, rejection, or timeout can be lost here. Re-read the source row while holding the lock and update only claimed, or merge into that fresh row.
| percent: (Object.entries(row.answers).length - incompleteEntries.length) / denominator, | ||
| completed: row.completed, | ||
| rejected: row.rejected, | ||
| timedOut: !!row.timedOut, |
There was a problem hiding this comment.
[P2] Auto-timed-out participants are still offered to the existing manual timeout dialog, which filters only incomplete and non-rejected participants. That dialog can then call rejectParticipant on this participant, changing the new timed-out status to rejected even though the UI says timed-out participants can finish normally. Please exclude timedOut participants from that dialog and add a regression test.
| const comp = includedParticipants.includes('completed') ? expList.filter((d) => !d.rejected && !d.timedOut && d.completed) : []; | ||
| const prog = includedParticipants.includes('inProgress') ? expList.filter((d) => !d.rejected && !d.timedOut && !d.completed) : []; | ||
| const rej = includedParticipants.includes('rejected') ? expList.filter((d) => d.rejected) : []; | ||
| const timedOut = includedParticipants.includes('timedOut') ? expList.filter((d) => !d.rejected && d.timedOut) : []; |
There was a problem hiding this comment.
[P2] This groups late completions with incomplete timeouts because both have timedOut === true. The new completedLate field is therefore lost in the main analysis filter, and older consumers such as tidy downloads, snapshot counts, and participant badges still use only the old three statuses. Please define one canonical status mapping and use it everywhere, keeping rejected, timed out, and completed-late distinct.
| if (minutes !== undefined && (!Number.isInteger(minutes) || minutes < 1)) { | ||
| throw new Error('Auto-timeout must be a whole number of minutes greater than zero'); | ||
| } | ||
| const modes = await this.getModes(studyId); |
There was a problem hiding this comment.
[P2] This reads and later rewrites the entire settings document to change one field. A concurrent stage or mode update can land between these calls and then be overwritten by this stale copy. Please update only autoTimeoutMinutes (including an atomic removal when disabling) with a backend merge or transaction.
| } | ||
| }) | ||
| .catch(() => { | ||
| if (!cancelled) setTimeoutSettingsLoading(false); |
There was a problem hiding this comment.
[P2] A settings read failure is silently converted into a usable-looking default value. The admin can edit or toggle a value while the backend value is unknown, and a save failure below is also only logged. Please show an error with a retry path and roll back the draft/toggle when a save fails.
| completed: filteredParticipants.filter((p) => p.completed && !p.rejected && !p.timedOut).length, | ||
| inProgress: filteredParticipants.filter((p) => !p.completed && !p.rejected && !p.timedOut).length, | ||
| rejected: filteredParticipants.filter((p) => p.rejected).length, | ||
| timedOut: filteredParticipants.filter((p) => !p.rejected && p.timedOut).length, |
There was a problem hiding this comment.
[P2] This intentionally changes the participant-count shape, but the existing summaryUtils.spec.ts assertion still expects the old object and fails on this exact head. Please update that expectation and add cases for timed-out and completed-late participants so the new counts are locked down.
| ...(showParticipantLimits ? [{ | ||
| accessorKey: 'desiredParticipants', | ||
| header: 'Total / Maximum', | ||
| size: 240, |
There was a problem hiding this comment.
[P3] These table sizing and padding changes are unrelated to auto-timeout behavior. Please remove this styling churn or move it to a separate change so the feature diff stays focused and easier to validate.
2 participants
Summary
Closes #194