diff --git a/packages/docs/audits/checklist-data-access-migration.md b/packages/docs/audits/checklist-data-access-migration.md new file mode 100644 index 000000000..9efa8c0a8 --- /dev/null +++ b/packages/docs/audits/checklist-data-access-migration.md @@ -0,0 +1,77 @@ +# Checklist data access: migration state after `useChecklistAnswers` + +## Context + +`ChecklistYjsWrapper` now reads answers via `useChecklistAnswers` (issue #480). The broader refactor (issues #481–#483) is not yet done. This doc enumerates the remaining callers of `getChecklistData`, explains what each one uses the data for, and whether it should eventually migrate to the hook or stay imperative. + +## Status: is there old code to delete? + +No. The only thing #480 removed from `ChecklistYjsWrapper` is the `getChecklistData` destructuring. The function itself is still exported from `createChecklistOperations` and still has four callers. No `refreshKey` remains in the tree (already removed by an earlier pass). Nothing is orphaned. + +## Callers of `getChecklistData` + +### 1. `project.checklist.getData()` — public typed API + +- File: `packages/web/src/project/actions.ts:82-86` +- Shape: one-shot read, imperative, called outside render. +- Currently consumed by `OverviewTab` (see #4 below) — no other direct callers. +- **Keep.** This is the imperative facade. It is the correct home for "grab the answers right now" calls that don't participate in a component's subscription model. Issue #483 will refine its types; the call shape is fine. + +### 2. `calculateInterRaterReliability` (utility) + +- File: `packages/web/src/lib/inter-rater-reliability.ts:34-39, 84-85` +- Shape: pure function, takes `getChecklistData` as a parameter, loops over every dual-reviewer AMSTAR2 study in a project and reads both reviewers' completed answers. +- **Keep imperative.** A per-checklist subscription hook doesn't fit: this computes across N studies × 2 reviewers in one pass. A hook would require either N×2 hook calls (illegal dynamically) or a second, project-scoped subscription that re-derives the whole table on any change. +- **Optional future sharpening:** if we want this to be live (metrics update as reviewers finalize), expose a `useProjectChecklistVersion(projectId)` hook that bumps on any `reviews` change, add it as a `useMemo` dep, and keep the imperative read inside the memo. That's the minimal cost for reactivity without breaking the computation shape. + +### 3. `OverviewTab` + +- File: `packages/web/src/components/project/overview-tab/OverviewTab.tsx:171-175` +- Shape: wraps `project.checklist.getData` in a lambda and passes it to `calculateInterRaterReliability`. +- **Keep.** Follows directly from #2. + +### 4. `PreviousReviewersView` + +- File: `packages/web/src/components/project/completed-tab/PreviousReviewersView.tsx:39, 66, 68` +- Shape: reads two completed-reviewer checklists in `useEffect` on dialog open, stashes them in local state, renders read-only. +- Target checklists have status `REVIEWER_COMPLETED` or later — editing is locked by `isEditable()`. +- **Migrate to `useChecklistAnswers` eventually, low priority.** No correctness bug today (the checklists are locked, so the one-shot read is accurate). Migration value is consistency and killing one more `useEffect` that syncs Y.Doc → local state. +- Blocker: the component reads *two* checklists. Calling the hook twice conditionally would violate Rules of Hooks. Solution: either (a) always call it twice with `null`-safe IDs, or (b) pass `checklistId` as an argument and split into `` children so each panel calls the hook once. (b) is cleaner. + +### 5. `ReconciliationWrapper` + +- File: `packages/web/src/components/project/reconcile-tab/ReconciliationWrapper.tsx:69, 200-217, 219-237, 380-391` +- Shape: reads three checklists (checklist1, checklist2, reconciled), builds UI payloads including reviewer names and createdAt, feeds into the reconciliation view. +- Reactivity **matters here**: during reconciliation both source checklists are typically `REVIEWER_COMPLETED` (locked) but the reconciled checklist is actively edited. If another tab or collaborator writes to the reconciled Y.Doc, this view needs to reflect it. +- Current invalidation: `getChecklistData` is pulled from destructured `ops.checklist` and used as a `useMemo` dep. It is a stable reference — so the memos only re-run when `currentStudy` flips in Zustand, which is the same fragile path #480 just moved away from. +- **Migrate.** Replace the three `useMemo(() => getChecklistData(...))` blocks with three `useChecklistAnswers` calls and spread the returned answers into the existing UI-shape objects. Same transformation as `ChecklistYjsWrapper`. +- Scope: ~40 lines touched in one component. Low risk. Good candidate to pair with the #481 local/collab unify work or land separately right after #480. + +## Summary table + +| Caller | Keep `getChecklistData`? | Reason | +|---|---|---| +| `project.checklist.getData()` | Yes | Imperative facade for one-shot reads | +| `calculateInterRaterReliability` | Yes | Cross-checklist aggregation; hook doesn't fit | +| `OverviewTab` | Yes | Thin wrapper over the above | +| `PreviousReviewersView` | Migrate (low priority) | Locked data, but consistency + removes effect | +| `ReconciliationWrapper` | Migrate (medium priority) | Reconciled checklist is live-edited; reactivity matters | + +`getChecklistData` stays in the surface area indefinitely — the imperative path is legitimate. The hook is the right default for reactive components, not a total replacement. + +## Local-practice migration (issue #481) + +**Not yet written.** Local-practice checklists live in the Dexie `localChecklists` table (`packages/web/src/stores/localChecklistsStore.ts`) as flat JSON blobs with a `data: unknown` field containing the full checklist template shape. They do not use Y.Doc, y-dexie, or the sync layer. + +When issue #481 lands, the migration step is: + +1. On first app load after upgrade, detect entries in `localChecklists`. +2. For each entry, create a local Y.Doc (persisted via y-dexie, no WebSocket provider) under a stable ID. +3. Convert the stored `data` using the appropriate `handler.createAnswersYMap` + write flat fields into the Y.Doc. +4. Mark the migration done via a one-shot flag (e.g., `localStorage['local-checklists-migrated-v1'] = '1'`) so we don't re-run. +5. Leave the old `localChecklists` and `localChecklistPdfs` tables in place for one release as rollback insurance. +6. Next release: drop the tables, delete `localChecklistsStore.ts`, delete `LocalChecklistView`, delete `CreateLocalChecklist`, re-point `/checklist/$checklistId` and dashboard sidebar entries to the unified view. + +Migration correctness should be verified by a round-trip test: every `data` shape that has appeared in the wild, run through the migration, then `handler.serializeAnswers(migrated)` equals the original `data` modulo Y.Text vs. string for note fields. + +Until #481 ships, no user-facing local-checklist data is at risk: the old store and old view keep working unchanged. `useChecklistAnswers` is only exercised by collaborative (project-scoped) checklists. diff --git a/packages/web/src/components/checklist/ChecklistYjsWrapper.tsx b/packages/web/src/components/checklist/ChecklistYjsWrapper.tsx index 1c57471f3..ddbf397a0 100644 --- a/packages/web/src/components/checklist/ChecklistYjsWrapper.tsx +++ b/packages/web/src/components/checklist/ChecklistYjsWrapper.tsx @@ -9,6 +9,7 @@ import { ChevronLeftIcon } from 'lucide-react'; import { ChecklistWithPdf } from '@/components/checklist/ChecklistWithPdf'; import { useProjectContext } from '@/components/project/ProjectContext'; import { connectionPool } from '@/project/ConnectionPool'; +import { useChecklistAnswers } from '@/primitives/useProject/checklists/useChecklistAnswers'; import { useProjectStore, selectConnectionPhase } from '@/stores/projectStore'; import { useAuthStore, selectUser } from '@/stores/authStore'; import { ACCESS_DENIED_ERRORS } from '@/constants/errors.js'; @@ -82,14 +83,8 @@ export function ChecklistYjsWrapper({ projectId, studyId, checklistId }: Checkli const ops = connectionPool.getOps(projectId); if (!ops) throw new Error(`No connection for project ${projectId}`); - const { - updateChecklistAnswer, - updateChecklist, - getChecklistData, - getQuestionNote, - getRobinsText, - getRob2Text, - } = ops.checklist; + const { updateChecklistAnswer, updateChecklist, getQuestionNote, getRobinsText, getRob2Text } = + ops.checklist; const { addPdfToStudy } = ops.pdf; const { addAnnotation, updateAnnotation, deleteAnnotation } = ops.annotation; @@ -220,19 +215,18 @@ export function ChecklistYjsWrapper({ projectId, studyId, checklistId }: Checkli [orgId, projectId, studyId, studyPdfs, user?.id, addPdfToStudy], ); - // Build checklist data for UI + const answers = useChecklistAnswers(projectId, studyId, checklistId); + const checklistForUI = useMemo(() => { - if (!currentChecklist) return null; - const data = getChecklistData(studyId, checklistId); - if (!data) return null; + if (!currentChecklist || !answers) return null; return { id: currentChecklist.id, name: currentStudy?.name || 'Checklist', reviewerName: '', createdAt: currentChecklist.createdAt, - ...((data.answers as Record) ?? {}), + ...answers, }; - }, [currentChecklist, currentStudy, getChecklistData, studyId, checklistId]); + }, [currentChecklist, currentStudy, answers]); const checklistType = useMemo(() => { if (currentChecklist?.type) return currentChecklist.type; diff --git a/packages/web/src/primitives/useProject/checklists/useChecklistAnswers.ts b/packages/web/src/primitives/useProject/checklists/useChecklistAnswers.ts new file mode 100644 index 000000000..8337d28de --- /dev/null +++ b/packages/web/src/primitives/useProject/checklists/useChecklistAnswers.ts @@ -0,0 +1,109 @@ +/** + * Subscription-based read of a checklist's answers from the project Y.Doc. + * + * Replaces the useMemo+Zustand indirection in ChecklistYjsWrapper. The hook + * observes the project's `reviews` map and serializes the requested checklist's + * answers on demand. The returned reference is stable between Y.Doc updates, so + * consumers only re-render when answers actually change. + */ + +import { useCallback, useRef, useSyncExternalStore } from 'react'; +import * as Y from 'yjs'; +import { connectionPool } from '@/project/ConnectionPool'; +import { CHECKLIST_TYPES } from '@/checklist-registry'; +import type { ChecklistHandler } from './handlers/base'; +import { AMSTAR2Handler } from './handlers/amstar2'; +import { ROBINSIHandler } from './handlers/robins-i'; +import { ROB2Handler } from './handlers/rob2'; + +const handlers: Record = { + [CHECKLIST_TYPES.AMSTAR2]: new AMSTAR2Handler(), + [CHECKLIST_TYPES.ROBINS_I]: new ROBINSIHandler(), + [CHECKLIST_TYPES.ROB2]: new ROB2Handler(), +}; + +interface ResolvedAnswers { + answersYMap: Y.Map; + checklistType: string; +} + +function resolveAnswers( + ydoc: Y.Doc | null, + studyId: string, + checklistId: string, +): ResolvedAnswers | null { + if (!ydoc) return null; + const studiesMap = ydoc.getMap('reviews'); + const studyYMap = studiesMap.get(studyId) as Y.Map | undefined; + if (!studyYMap) return null; + const checklistsMap = studyYMap.get('checklists') as Y.Map | undefined; + if (!checklistsMap) return null; + const checklistYMap = checklistsMap.get(checklistId) as Y.Map | undefined; + if (!checklistYMap) return null; + const answersYMap = checklistYMap.get('answers') as Y.Map | undefined; + if (!answersYMap) return null; + const checklistType = (checklistYMap.get('type') as string) || 'AMSTAR2'; + return { answersYMap, checklistType }; +} + +function serialize(resolved: ResolvedAnswers): Record { + const handler = handlers[resolved.checklistType]; + if (handler) return handler.serializeAnswers(resolved.answersYMap); + const fallback: Record = {}; + for (const [key, section] of resolved.answersYMap.entries()) { + const s = section as { toJSON?: () => unknown }; + fallback[key] = s.toJSON ? s.toJSON() : section; + } + return fallback; +} + +export function useChecklistAnswers( + projectId: string, + studyId: string, + checklistId: string, +): Record | null { + const ydoc = connectionPool.getEntry(projectId)?.ydoc ?? null; + + const versionRef = useRef(0); + const cacheRef = useRef<{ + version: number; + studyId: string | null; + checklistId: string | null; + value: Record | null; + }>({ version: -1, studyId: null, checklistId: null, value: null }); + + const subscribe = useCallback( + (onStoreChange: () => void) => { + if (!ydoc) return () => {}; + // observeDeep on `reviews` catches both "checklist arrives via sync" + // and "answers inside this checklist change". Serialization happens in + // getSnapshot, so unrelated writes only bump a counter here. + const reviewsMap = ydoc.getMap('reviews'); + const observer = () => { + versionRef.current += 1; + onStoreChange(); + }; + reviewsMap.observeDeep(observer); + return () => reviewsMap.unobserveDeep(observer); + }, + [ydoc], + ); + + const getSnapshot = useCallback((): Record | null => { + const cached = cacheRef.current; + if ( + cached.version === versionRef.current && + cached.studyId === studyId && + cached.checklistId === checklistId + ) { + return cached.value; + } + + const resolved = resolveAnswers(ydoc, studyId, checklistId); + const value = resolved ? serialize(resolved) : null; + cacheRef.current = { version: versionRef.current, studyId, checklistId, value }; + return value; + }, [ydoc, studyId, checklistId]); + + return useSyncExternalStore(subscribe, getSnapshot); +}