Skip to content

Commit 44c2407

Browse files
Give the other reviewer their checklist when a cell is planned from To-Do (#791)
A reviewer's ad hoc checklist plans the cell, and a filled slot holds every planned cell, so checklist.create now materializes the other slot holder's checklist too. The second reviewer no longer adds a matching checklist by hand; the e2e specs reflect that, and a new plan-first-assignment spec covers the partner's copy appearing, the in-progress swap prompt, hand-over carrying answers, and the old reviewer's To-Do emptying. The data model diagram gains the appraisals entity and checklist kind. Claude-Session: https://claude.ai/code/session_01LqxkXwhjRDsJ1N9cBYpU1n
1 parent 59043d2 commit 44c2407

11 files changed

Lines changed: 272 additions & 66 deletions

‎packages/docs/architecture/diagrams/04-data-model.md‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -84,12 +84,20 @@ erDiagram
8484
date createdAt
8585
}
8686
87+
APPRAISAL {
88+
string id PK "studyId:outcomeKey"
89+
string studyId FK
90+
string type "AMSTAR2, ROB2, ROBINS_I"
91+
string outcomeId "nullable"
92+
}
93+
8794
CHECKLIST {
8895
string id PK
8996
string title
90-
string assignedTo
97+
string kind "reviewer, consensus"
98+
string assignedTo "null on consensus"
9199
string status
92-
string type "AMSTAR2, ROBINS-I"
100+
string type "AMSTAR2, ROB2, ROBINS_I"
93101
}
94102
95103
ANSWER {
@@ -119,9 +127,13 @@ Research project container belonging to an organization. Basic metadata (id, nam
119127

120128
A systematic review or research paper being assessed. Stored entirely in the workspace Durable Object (sync-engine rows). Can have an associated PDF stored in R2.
121129

130+
### Appraisal
131+
132+
The plan: one row per (study, instrument, outcome) cell that the project intends to appraise. A planned cell nobody owns yet has no checklist rows; reviewer checklists materialize when a study's reviewer slot is filled. Stored in the workspace Durable Object.
133+
122134
### Checklist
123135

124-
An assessment using a specific tool (AMSTAR-2, ROBINS-I). Stored entirely in the workspace Durable Object (sync-engine rows). Assigned to a team member.
136+
An assessment using a specific tool (AMSTAR-2, RoB 2, ROBINS-I). Stored entirely in the workspace Durable Object (sync-engine rows). `kind` separates a reviewer's own appraisal (assigned to a team member) from the reconciled consensus (no assignee).
125137

126138
### Answer
127139

‎packages/shared/src/sync/__tests__/appraisals.test.ts‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,48 @@ describe('checklist.create with the plan', () => {
7676
});
7777
});
7878

79+
it('gives the other slot holder a checklist for the newly planned cell', () => {
80+
const engine = newEngine();
81+
seedStudy(engine, 's1', { reviewer1: 'alice', reviewer2: 'bob' });
82+
seedOutcome(engine, 'o1');
83+
const result = engine.mutate('checklist.create', {
84+
id: 'chk-alice',
85+
studyId: 's1',
86+
type: 'ROB2',
87+
assignedTo: 'alice',
88+
outcomeId: 'o1',
89+
now: NOW,
90+
});
91+
expect(result.error).toBeUndefined();
92+
const bobs = checklistsOf(engine, 's1').filter(c => c.assignedTo === 'bob');
93+
expect(bobs).toHaveLength(1);
94+
expect(bobs[0]).toMatchObject({
95+
id: materializedChecklistId('s1:o1', 'bob'),
96+
status: 'pending',
97+
});
98+
// Alice keeps the id she chose and gets no second checklist.
99+
expect(
100+
checklistsOf(engine, 's1')
101+
.filter(c => c.assignedTo === 'alice')
102+
.map(c => c.id),
103+
).toEqual(['chk-alice']);
104+
});
105+
106+
it('a consensus row plans nothing new and materializes nothing', () => {
107+
const engine = newEngine();
108+
seedStudy(engine, 's1', { reviewer1: 'alice', reviewer2: 'bob' });
109+
engine.mutate('checklist.create', {
110+
id: 'chk-c',
111+
studyId: 's1',
112+
type: 'AMSTAR2',
113+
kind: 'consensus',
114+
assignedTo: null,
115+
outcomeId: null,
116+
now: NOW,
117+
});
118+
expect(checklistsOf(engine, 's1')).toHaveLength(1);
119+
});
120+
79121
it('allows a consensus row beside a reviewer row for the same cell', () => {
80122
const engine = newEngine();
81123
seedStudy(engine, 's1');

‎packages/shared/src/sync/mutators.ts‎

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -481,7 +481,8 @@ export const syncMutators = defineMutators(
481481
/**
482482
* One checklist for one cell — the To-Do escape hatch and the consensus
483483
* row. Also plans the cell, so the plan stays truthful whichever path
484-
* created the work.
484+
* created the work, and gives every other reviewer on the study their
485+
* checklist for it: a filled slot always holds every planned cell.
485486
*/
486487
'checklist.create': {
487488
args: z.object({
@@ -518,8 +519,18 @@ export const syncMutators = defineMutators(
518519
}
519520
}
520521

521-
ensureAppraisal(tx, { studyId, type, outcomeId }, now);
522-
createChecklistRow(tx, { id, studyId, type, kind, assignedTo, outcomeId }, now);
522+
const held = heldCellIndex(tx);
523+
const cell = { studyId, type, outcomeId };
524+
ensureAppraisal(tx, cell, now);
525+
createChecklistRow(tx, { id, ...cell, kind, assignedTo }, now);
526+
if (kind === 'reviewer') {
527+
if (assignedTo) held.add(`${cellKey(studyId, type, outcomeId)}|${assignedTo}`);
528+
for (const holder of new Set([study.reviewer1, study.reviewer2])) {
529+
if (holder && holder !== assignedTo) {
530+
materializeForReviewer(tx, studyId, holder, held, now);
531+
}
532+
}
533+
}
523534
tx.put('studies', studyId, { ...study, updatedAt: now });
524535
},
525536
},

‎packages/web/e2e/amstar2-workflow.spec.ts‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -59,13 +59,9 @@ test('Dual-Reviewer AMSTAR2 Workflow', async ({ context, page }) => {
5959
await page.goto(`/projects/${projectId}`);
6060
await expect(page.getByText('AMSTAR2 E2E Test').first()).toBeVisible({ timeout: 15_000 });
6161

62+
// User A's checklist planned the cell, so User B's own checklist is
63+
// already waiting: nothing to add.
6264
await page.getByRole('tab', { name: /To-Do/i }).click();
63-
await expect(page.getByRole('button', { name: /Select Checklist/i })).toBeVisible({
64-
timeout: 10_000,
65-
});
66-
67-
await page.getByRole('button', { name: /Select Checklist/i }).click();
68-
await page.getByRole('button', { name: /Add Checklist/i }).click();
6965
await expect(page.getByRole('button', { name: 'Open', exact: true })).toBeVisible({
7066
timeout: 10_000,
7167
});

‎packages/web/e2e/change-outcome.spec.ts‎

Lines changed: 20 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -34,26 +34,33 @@ test.afterAll(async () => {
3434
if (scenario) await cleanupScenario(scenario);
3535
});
3636

37-
/** Add a ROB2 checklist for the given outcome, fill it, and mark it complete. */
37+
/**
38+
* Fill a ROB2 checklist for the given outcome and mark it complete. The first
39+
* reviewer adds it; that plans the cell, so the second reviewer's copy is
40+
* already waiting and `create` is false.
41+
*/
3842
async function fillROB2ChecklistForOutcome(
3943
page: Page,
4044
projectId: string,
4145
outcomeName: string,
4246
answer: string,
47+
create = true,
4348
) {
4449
await page.getByRole('tab', { name: /To-Do/i }).click();
45-
await expect(page.getByRole('button', { name: /Select Checklist/i })).toBeVisible({
46-
timeout: 30_000,
47-
});
48-
49-
await page.getByRole('button', { name: /Select Checklist/i }).click();
50-
await page.getByText(/AMSTAR 2/i).click();
51-
await page.getByRole('option', { name: /RoB 2/i }).click();
52-
await page.getByText(/Select outcome/i).click();
53-
await page.getByRole('option', { name: outcomeName }).click();
54-
await page.getByRole('button', { name: /Add Checklist/i }).click();
50+
if (create) {
51+
await expect(page.getByRole('button', { name: /Select Checklist/i })).toBeVisible({
52+
timeout: 30_000,
53+
});
54+
55+
await page.getByRole('button', { name: /Select Checklist/i }).click();
56+
await page.getByText(/AMSTAR 2/i).click();
57+
await page.getByRole('option', { name: /RoB 2/i }).click();
58+
await page.getByText(/Select outcome/i).click();
59+
await page.getByRole('option', { name: outcomeName }).click();
60+
await page.getByRole('button', { name: /Add Checklist/i }).click();
61+
}
5562
await expect(page.getByRole('button', { name: 'Open', exact: true })).toBeVisible({
56-
timeout: 10_000,
63+
timeout: 30_000,
5764
});
5865

5966
await page.getByRole('button', { name: 'Open', exact: true }).last().click();
@@ -105,7 +112,7 @@ test('Change outcome from Reconcile and Completed tabs', async ({ context, page
105112

106113
await switchUser(context, scenario.cookiesB);
107114
await page.goto(`/projects/${projectId}`);
108-
await fillROB2ChecklistForOutcome(page, projectId, 'Employment', 'N');
115+
await fillROB2ChecklistForOutcome(page, projectId, 'Employment', 'N', false);
109116

110117
// ================================================================
111118
// Reconcile tab: ready pair shows under Employment; move it to

‎packages/web/e2e/concurrent-crdt.spec.ts‎

Lines changed: 2 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -279,11 +279,7 @@ test.describe('Concurrent CRDT: AMSTAR2', () => {
279279
await expect(setupPage.getByText('AMSTAR2 CRDT Test').first()).toBeVisible({ timeout: 15_000 });
280280

281281
await setupPage.getByRole('tab', { name: /To-Do/i }).click();
282-
await expect(setupPage.getByRole('button', { name: /Select Checklist/i })).toBeVisible({
283-
timeout: 10_000,
284-
});
285-
await setupPage.getByRole('button', { name: /Select Checklist/i }).click();
286-
await setupPage.getByRole('button', { name: /Add Checklist/i }).click();
282+
// User A's checklist planned the cell; User B's copy is already waiting.
287283
await expect(setupPage.getByRole('button', { name: 'Open', exact: true })).toBeVisible({
288284
timeout: 10_000,
289285
});
@@ -385,15 +381,7 @@ test.describe('Concurrent CRDT: ROB2', () => {
385381
await expect(setupPage.getByText('ROB2 CRDT Test').first()).toBeVisible({ timeout: 15_000 });
386382

387383
await setupPage.getByRole('tab', { name: /To-Do/i }).click();
388-
await expect(setupPage.getByRole('button', { name: /Select Checklist/i })).toBeVisible({
389-
timeout: 10_000,
390-
});
391-
await setupPage.getByRole('button', { name: /Select Checklist/i }).click();
392-
await setupPage.getByText(/AMSTAR 2/i).click();
393-
await setupPage.getByRole('option', { name: /RoB 2/i }).click();
394-
await setupPage.getByText(/Select outcome/i).click();
395-
await setupPage.getByRole('option', { name: /Primary outcome/i }).click();
396-
await setupPage.getByRole('button', { name: /Add Checklist/i }).click();
384+
// User A's RoB 2 for this outcome planned the cell; User B's copy is waiting.
397385
await expect(setupPage.getByRole('button', { name: 'Open', exact: true })).toBeVisible({
398386
timeout: 10_000,
399387
});
Lines changed: 152 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,152 @@
1+
/**
2+
* E2E Test: Plan-first appraisals (#791)
3+
*
4+
* A reviewer's ad hoc checklist plans the cell, so the other reviewer's own
5+
* checklist appears on their To-Do tab without creating anything. Swapping a
6+
* slot holder who has work in progress asks whether to hand it over; the new
7+
* reviewer then continues from the same answers and the old one has nothing
8+
* left to do.
9+
*
10+
* Prerequisites:
11+
* pnpm --filter web dev (localhost:3010, DEV_MODE=true)
12+
*/
13+
14+
import { test, expect } from './test';
15+
import {
16+
addProjectMember,
17+
cleanupByEmail,
18+
cleanupScenario,
19+
seedDualReviewerScenario,
20+
switchUser,
21+
testApi,
22+
uniquePrefix,
23+
type DualReviewerScenario,
24+
type SeededUser,
25+
type SessionCookie,
26+
} from './helpers';
27+
import { setupProjectWithStudy, waitForSynced } from './shared-steps';
28+
29+
let scenario: DualReviewerScenario;
30+
let carol: SeededUser;
31+
let cookiesC: SessionCookie[];
32+
33+
/** A third org member; the seed route ignores conflicts, so the org row is reused. */
34+
async function seedThirdReviewer(orgId: string) {
35+
const prefix = uniquePrefix('e2e');
36+
const id = `${prefix}-user-c`;
37+
const email = `carol-${prefix}@test.corates.org`;
38+
const seedRes = await testApi('/api/test/seed', {
39+
method: 'POST',
40+
headers: { 'Content-Type': 'application/json' },
41+
body: JSON.stringify({
42+
users: [{ id, name: 'Carol Reviewer', email, givenName: 'Carol', familyName: 'Reviewer' }],
43+
org: { id: orgId, name: 'E2E Test Org' },
44+
orgMembers: [{ userId: id, role: 'member' }],
45+
}),
46+
});
47+
if (!seedRes.ok) throw new Error(`Seed failed: ${seedRes.status} ${await seedRes.text()}`);
48+
const sessionRes = await testApi('/api/test/session', {
49+
method: 'POST',
50+
headers: { 'Content-Type': 'application/json' },
51+
body: JSON.stringify({ userId: id }),
52+
});
53+
if (!sessionRes.ok) throw new Error(`Session failed: ${sessionRes.status}`);
54+
const { cookies } = (await sessionRes.json()) as { cookies: SessionCookie[] };
55+
return { user: { id, name: 'Carol Reviewer', email }, cookies };
56+
}
57+
58+
test.beforeAll(async () => {
59+
scenario = await seedDualReviewerScenario();
60+
const third = await seedThirdReviewer(scenario.orgId);
61+
carol = third.user;
62+
cookiesC = third.cookies;
63+
});
64+
65+
test.afterAll(async () => {
66+
if (carol) await cleanupByEmail(carol.email);
67+
if (scenario) await cleanupScenario(scenario);
68+
});
69+
70+
test('A planned cell reaches every reviewer, and a swap hands work over', async ({
71+
context,
72+
page,
73+
}) => {
74+
const projectId = await setupProjectWithStudy(context, page, scenario, 'Plan First E2E');
75+
await addProjectMember(scenario.orgId, projectId, carol.id, scenario.cookiesA);
76+
77+
// ================================================================
78+
// Alice adds an AMSTAR2 checklist from To-Do: this plans the cell
79+
// ================================================================
80+
await page.getByRole('tab', { name: /To-Do/i }).click();
81+
await expect(page.getByRole('button', { name: /Select Checklist/i })).toBeVisible({
82+
timeout: 10_000,
83+
});
84+
await page.getByRole('button', { name: /Select Checklist/i }).click();
85+
await page.getByRole('button', { name: /Add Checklist/i }).click();
86+
await expect(page.getByRole('button', { name: 'Open', exact: true })).toBeVisible({
87+
timeout: 10_000,
88+
});
89+
await waitForSynced(page);
90+
91+
// ================================================================
92+
// Bob's To-Do already holds his copy; he starts answering
93+
// ================================================================
94+
await switchUser(context, scenario.cookiesB);
95+
await page.goto(`/projects/${projectId}`);
96+
await page.getByRole('tab', { name: /To-Do/i }).click();
97+
await expect(page.getByRole('button', { name: 'Open', exact: true })).toBeVisible({
98+
timeout: 15_000,
99+
});
100+
await expect(page.getByRole('button', { name: /Select Checklist/i })).toHaveCount(0);
101+
102+
await page.getByRole('button', { name: 'Open', exact: true }).click();
103+
await expect(page).toHaveURL(/\/checklists\//, { timeout: 10_000 });
104+
const firstYes = page.getByRole('radio', { name: 'Yes' }).first();
105+
await expect(firstYes).toBeVisible({ timeout: 10_000 });
106+
await firstYes.click();
107+
await expect(firstYes).toBeChecked({ timeout: 5_000 });
108+
await waitForSynced(page);
109+
110+
// ================================================================
111+
// Alice replaces Bob with Carol; the sheet asks about Bob's work
112+
// ================================================================
113+
await switchUser(context, scenario.cookiesA);
114+
await page.goto(`/projects/${projectId}`);
115+
await page.getByRole('tab', { name: /All Studies/i }).click();
116+
await expect(page.getByTestId('study-card').first()).toBeVisible({ timeout: 15_000 });
117+
await page.getByTestId('study-card').first().getByTestId('study-card-menu').click();
118+
await page.getByRole('menuitem', { name: /Assign Reviewers/i }).click();
119+
120+
const sheet = page.getByTestId('assign-reviewers-sheet');
121+
await expect(sheet).toBeVisible({ timeout: 5_000 });
122+
await sheet.getByTestId('reviewer-picker-2').click();
123+
await expect(page.getByRole('option', { name: /Carol/i })).toBeVisible({ timeout: 15_000 });
124+
await page.getByRole('option', { name: /Carol/i }).click();
125+
await expect(page.getByRole('listbox')).toBeHidden({ timeout: 5_000 });
126+
await sheet.getByRole('button', { name: 'Save reviewers' }).click();
127+
128+
const prompt = page.getByTestId('in-progress-prompt');
129+
await expect(prompt).toBeVisible({ timeout: 5_000 });
130+
await prompt.getByRole('button', { name: 'Hand over' }).click();
131+
await expect(sheet).toBeHidden({ timeout: 10_000 });
132+
await waitForSynced(page);
133+
134+
// ================================================================
135+
// Carol continues from Bob's answers; Bob has nothing left to do
136+
// ================================================================
137+
await switchUser(context, cookiesC);
138+
await page.goto(`/projects/${projectId}`);
139+
await page.getByRole('tab', { name: /To-Do/i }).click();
140+
await expect(page.getByRole('button', { name: 'Open', exact: true })).toBeVisible({
141+
timeout: 15_000,
142+
});
143+
await page.getByRole('button', { name: 'Open', exact: true }).click();
144+
await expect(page).toHaveURL(/\/checklists\//, { timeout: 10_000 });
145+
await expect(page.getByRole('radio', { name: 'Yes' }).first()).toBeChecked({ timeout: 10_000 });
146+
147+
await switchUser(context, scenario.cookiesB);
148+
await page.goto(`/projects/${projectId}`);
149+
await page.getByRole('tab', { name: /To-Do/i }).click();
150+
await expect(page.getByRole('tab', { name: /To-Do/i })).toBeVisible({ timeout: 15_000 });
151+
await expect(page.getByRole('button', { name: 'Open', exact: true })).toHaveCount(0);
152+
});

‎packages/web/e2e/realtime-collaboration.spec.ts‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -128,17 +128,13 @@ test('Presence avatars, cursor sync, and text editing sync during reconciliation
128128
await page.goto(`${BASE_URL}/projects/${projectId}`);
129129
await expect(page.getByRole('tab', { name: /To-Do/i })).toBeVisible({ timeout: 15_000 });
130130

131-
// User B: add AMSTAR2 checklist, answer No to everything, mark complete
131+
// User B: open the AMSTAR2 checklist User A's addition planned for them,
132+
// answer No to everything, mark complete
132133
await switchUser(setupCtx, scenario.cookiesB);
133134
await page.goto(`${BASE_URL}/projects/${projectId}`);
134135
await expect(page.getByText('Realtime Reconcile Test').first()).toBeVisible({ timeout: 15_000 });
135136

136137
await page.getByRole('tab', { name: /To-Do/i }).click();
137-
await expect(page.getByRole('button', { name: /Select Checklist/i })).toBeVisible({
138-
timeout: 10_000,
139-
});
140-
await page.getByRole('button', { name: /Select Checklist/i }).click();
141-
await page.getByRole('button', { name: /Add Checklist/i }).click();
142138
await expect(page.getByRole('button', { name: 'Open', exact: true })).toBeVisible({
143139
timeout: 10_000,
144140
});

0 commit comments

Comments
 (0)