Skip to content

Commit 51d968c

Browse files
scriptcodedclaude
andcommitted
fix: stop "subjects have already been set" during pre-checkin
Confirming subject selection failed for anyone who used the back button. base:selectSubjects wrote CheckinSubject rows but its onStepRollback was an empty stub, so goBack deleted the step data and left the rows behind. On the re-run, setSubjects saw existing rows and threw. The pre-checkin flow renders a back button on every screen and has two informational steps right after subject selection, so this was easy to hit. - selectSubjects: clear subjects on rollback instead of no-op. - setSubjects: replace the set in a transaction rather than throwing. Resolves the TODO there. Safe because nothing downstream of subject selection has run yet, and it also unsticks sessions that died between setSubjects and setCompleted -- those hit the error on every retry via the resume path. - SelectSubjectScreen: block double submits, which could otherwise put two confirmSubjects calls in flight before the step advanced. Also guard goBack against completed/aborted sessions. Rollback hooks undo real side effects, and step:goBack can be sent directly (admin debug panel) even though the kiosk hides the button once the session finishes. undoCheckin stays the one way to reverse a finished check-in. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent a674819 commit 51d968c

4 files changed

Lines changed: 51 additions & 22 deletions

File tree

packages/backend/src/core/workflow/step.ts

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { prisma } from "../../app/prisma.ts";
12
import {
23
deleteStepData,
34
findLastCompletedStep,
@@ -79,6 +80,25 @@ export async function goBack(
7980
throw new Error("No session ID found in context");
8081
}
8182

83+
// A finished session is a durable record, not something to unwind: rollback
84+
// hooks undo real side effects (subject links, check-in timestamps), so
85+
// walking backwards through one would quietly damage a completed check-in.
86+
// The kiosk hides its back button once `session:completed` arrives, but
87+
// `step:goBack` can still be sent directly (see the admin debug panel), so
88+
// the guard belongs here rather than in the UI. Use undoCheckin to reverse a
89+
// finished check-in.
90+
const session = await prisma.checkinSession.findUniqueOrThrow({
91+
where: { id: sessionId },
92+
select: { completedAt: true, abortedAt: true },
93+
});
94+
if (session.completedAt || session.abortedAt) {
95+
getLogger(c).warn(
96+
{ completedAt: session.completedAt, abortedAt: session.abortedAt },
97+
"Ignoring goBack for a session that is already completed or aborted",
98+
);
99+
return;
100+
}
101+
82102
// Walk backwards, skipping steps marked skipOnGoBack.
83103
const maxSteps = 100;
84104
for (let i = 0; i < maxSteps; i++) {

packages/backend/src/core/workflow/stepContext.ts

Lines changed: 18 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -224,25 +224,24 @@ export function createStepContext(
224224
throw new Error("No session ID found in context");
225225
}
226226

227-
const existingSubjects = await prisma.checkinSubject.findMany({
228-
where: { checkinSessionId: sessionId },
229-
});
230-
231-
if (existingSubjects.length > 0) {
232-
// TODO: Can we handle this gracefully? We could delete old subjects and
233-
// set new ones, but that could have unintended consequences. Maybe we
234-
// should just let the user start a new session?
235-
throw new Error(
236-
"Subjects have already been set for this session, cannot set again",
237-
);
238-
}
239-
240-
await prisma.checkinSubject.createMany({
241-
data: participantIds.map((participantId) => ({
242-
checkinSessionId: sessionId,
243-
participantId,
244-
})),
245-
});
227+
// Idempotent by design: the same set can legitimately be submitted twice.
228+
// A rollback to the selecting step leaves the old rows behind, and a
229+
// double-tapped submit sends two calls before the step advances (the
230+
// stale-method guard in session.socket.ts only catches calls arriving
231+
// after advancement). Replacing the set is safe here because no step
232+
// after subject selection has run yet — anything that acts on subjects
233+
// runs downstream and will see the new set.
234+
await prisma.$transaction([
235+
prisma.checkinSubject.deleteMany({
236+
where: { checkinSessionId: sessionId },
237+
}),
238+
prisma.checkinSubject.createMany({
239+
data: participantIds.map((participantId) => ({
240+
checkinSessionId: sessionId,
241+
participantId,
242+
})),
243+
}),
244+
]);
246245
},
247246
async clearSubjects() {
248247
const sessionId = c.get("wsSessionId");

plugins/base/src/selectSubjects/backend/selectSubjects.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,8 +66,11 @@ export const selectSubjects: StepImplementation = {
6666
subGroups,
6767
});
6868
},
69-
async onStepRollback(_ctx) {
70-
// await ctx.clearActor();
69+
async onStepRollback(ctx) {
70+
// Rolling back deletes this step's data but not the subject rows it
71+
// wrote, so they must be cleared here — otherwise the re-run selection
72+
// is layered on top of the previous one.
73+
await ctx.clearSubjects();
7174
},
7275
},
7376
publicMethods: {

plugins/base/src/selectSubjects/frontend/screens/SelectSubjectScreen.tsx

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@ export function SelectSubjectScreen({ payload }: { payload: object }) {
6363

6464
const validPayload = Payload(payload);
6565

66+
const [submitted, setSubmitted] = useState(false);
6667
const [selectedParticipantIds, setSelectedParticipantIds] = useState<
6768
string[]
6869
>(() => {
@@ -118,6 +119,12 @@ export function SelectSubjectScreen({ payload }: { payload: object }) {
118119
};
119120

120121
const submitSelected = () => {
122+
// Guard against double-tapping: two confirmSubjects calls can otherwise be
123+
// in flight at once, before the backend has advanced the step. The screen
124+
// unmounts on step advancement, so this never needs resetting.
125+
if (submitted) return;
126+
setSubmitted(true);
127+
121128
socket?.send({
122129
name: "step:callMethod",
123130
data: {
@@ -218,7 +225,7 @@ export function SelectSubjectScreen({ payload }: { payload: object }) {
218225
variant="primary"
219226
icon={ArrowRightIcon}
220227
iconPosition="after"
221-
disabled={selectedParticipantIds.length === 0}
228+
disabled={selectedParticipantIds.length === 0 || submitted}
222229
onClick={submitSelected}
223230
>
224231
{t("submit", {

0 commit comments

Comments
 (0)