From 144a4d8a73f2a02377a63c1ac7a3f90f3a495289 Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Mon, 5 Oct 2026 20:02:50 -0700 Subject: [PATCH 01/30] fix(api): clear the 30 no-unnecessary-condition lint errors. Index lookups that can miss at runtime (returning() rows, row-index maps) now use .at(), so their guards type-check as needed instead of being flagged. Casts that hid null now say so, and dead branches are removed. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/api/src/lib/bulk-combined-validate.ts | 6 +++--- .../api/src/lib/collection-delete-cascade.ts | 17 +++++++---------- .../src/lib/collections/collection-create.ts | 16 ++++++++-------- .../src/lib/collections/collection-lifecycle.ts | 8 ++++---- packages/api/src/lib/container-projection.ts | 2 +- packages/api/src/lib/container-read-view.ts | 2 +- .../api/src/lib/container-write-placement.ts | 2 +- packages/api/src/lib/controls/batch-create.ts | 4 ++-- .../src/lib/controls/batch-with-specimens.ts | 6 +++--- packages/api/src/lib/controls/control-read.ts | 2 +- packages/api/src/lib/derivations.ts | 4 ++-- packages/api/src/lib/export-config-migration.ts | 4 ++-- packages/api/src/lib/registration-prepare.ts | 2 +- packages/api/src/routes/containers.ts | 6 +++--- packages/api/src/routes/controls.ts | 4 ++-- 15 files changed, 41 insertions(+), 44 deletions(-) diff --git a/packages/api/src/lib/bulk-combined-validate.ts b/packages/api/src/lib/bulk-combined-validate.ts index acb184ad..2d7a7f71 100644 --- a/packages/api/src/lib/bulk-combined-validate.ts +++ b/packages/api/src/lib/bulk-combined-validate.ts @@ -174,7 +174,7 @@ export async function validateBulkCombinedPayload( { messages: bulkCombinedCollectionMessages } ) if (!containerPrep.valid) { - const failed = resolvedForPrepare[containerPrep.specimenIndex] + const failed = resolvedForPrepare.at(containerPrep.specimenIndex) add( subjectIndex, failed?.specimenIndex ?? containerPrep.specimenIndex, @@ -187,7 +187,7 @@ export async function validateBulkCombinedPayload( let placementRowIndex = 0 for (const resolvedRow of resolvedForPrepare) { if (!resolvedRow.container?.containerType) continue - const prepRow = containerPrep.result.placementRows[placementRowIndex] + const prepRow = containerPrep.result.placementRows.at(placementRowIndex) if (!prepRow) break const c = resolvedRow.container.collection @@ -210,7 +210,7 @@ export async function validateBulkCombinedPayload( const placementErrors = await collectContainerPlacementErrors(database, placementRows) for (const placementError of placementErrors) { - const context = placementContexts[placementError.rowIndex] + const context = placementContexts.at(placementError.rowIndex) if (context) { add(context.subjectIndex, context.specimenIndex, placementError.message, context.rowIndex) } diff --git a/packages/api/src/lib/collection-delete-cascade.ts b/packages/api/src/lib/collection-delete-cascade.ts index 1c5cf9e5..776578f0 100644 --- a/packages/api/src/lib/collection-delete-cascade.ts +++ b/packages/api/src/lib/collection-delete-cascade.ts @@ -87,16 +87,13 @@ export async function collectContainerIdsInCollection( .all() return rows.map((x) => x.id) } - if (type === 'bag') { - const rows = await database - .select({ id: paper.id }) - .from(paper) - .innerJoin(sheet, eq(paper.sheetId, sheet.id)) - .where(eq(sheet.bagId, id)) - .all() - return rows.map((x) => x.id) - } - return [] + const rows = await database + .select({ id: paper.id }) + .from(paper) + .innerJoin(sheet, eq(paper.sheetId, sheet.id)) + .where(eq(sheet.bagId, id)) + .all() + return rows.map((x) => x.id) } async function ensureCollectionRowExists( diff --git a/packages/api/src/lib/collections/collection-create.ts b/packages/api/src/lib/collections/collection-create.ts index 3023d54f..cf80fe65 100644 --- a/packages/api/src/lib/collections/collection-create.ts +++ b/packages/api/src/lib/collections/collection-create.ts @@ -28,7 +28,7 @@ export async function createMicronixPlate(database: Database, input: CreateColle if (existing) throw new CollectionNameExistsError('Plate with this name already exists') const now = utcNow() - const [newPlate] = await database + const inserted = await database .insert(micronixPlate) .values({ name: input.name, @@ -40,7 +40,7 @@ export async function createMicronixPlate(database: Database, input: CreateColle updatedBy: input.userId, }) .returning() - + const newPlate = inserted.at(0) if (!newPlate) throw new Error('Insert did not return plate row') return { plate: { ...newPlate, locationPath: formatLocationPath(loc) } } } @@ -51,7 +51,7 @@ export async function createCryovialBox(database: Database, input: CreateCollect if (existing) throw new CollectionNameExistsError('Cryovial box with this name already exists') const now = utcNow() - const [newBox] = await database + const inserted = await database .insert(cryovialBox) .values({ name: input.name, @@ -63,7 +63,7 @@ export async function createCryovialBox(database: Database, input: CreateCollect updatedBy: input.userId, }) .returning() - + const newBox = inserted.at(0) if (!newBox) throw new Error('Insert did not return cryovial box row') return { box: { ...newBox, locationPath: formatLocationPath(loc) } } } @@ -74,7 +74,7 @@ export async function createGenericBox(database: Database, input: CreateCollecti if (existing) throw new CollectionNameExistsError('Box with this name already exists') const now = utcNow() - const [newBox] = await database + const inserted = await database .insert(box) .values({ name: input.name, @@ -85,7 +85,7 @@ export async function createGenericBox(database: Database, input: CreateCollecti updatedBy: input.userId, }) .returning() - + const newBox = inserted.at(0) if (!newBox) throw new Error('Insert did not return box row') return { box: { ...newBox, locationPath: formatLocationPath(loc) } } } @@ -96,7 +96,7 @@ export async function createBag(database: Database, input: CreateCollectionInput if (existing) throw new CollectionNameExistsError('Bag with this name already exists') const now = utcNow() - const [newBag] = await database + const inserted = await database .insert(bag) .values({ name: input.name, @@ -107,7 +107,7 @@ export async function createBag(database: Database, input: CreateCollectionInput updatedBy: input.userId, }) .returning() - + const newBag = inserted.at(0) if (!newBag) throw new Error('Insert did not return bag row') return { bag: { ...newBag, locationPath: formatLocationPath(loc) } } } diff --git a/packages/api/src/lib/collections/collection-lifecycle.ts b/packages/api/src/lib/collections/collection-lifecycle.ts index e6f7ef02..7f686d01 100644 --- a/packages/api/src/lib/collections/collection-lifecycle.ts +++ b/packages/api/src/lib/collections/collection-lifecycle.ts @@ -94,7 +94,7 @@ export async function createOrResolveCollection( updatedBy: input.userId, }) .returning() - const row = inserted[0] + const row = inserted.at(0) if (!row) throw new Error('Insert did not return box row') collectionMap?.set(key, row.id) return row.id @@ -112,7 +112,7 @@ export async function createOrResolveCollection( updatedBy: input.userId, }) .returning() - const row = inserted[0] + const row = inserted.at(0) if (!row) throw new Error('Insert did not return bag row') collectionMap?.set(key, row.id) return row.id @@ -131,7 +131,7 @@ export async function createOrResolveCollection( updatedBy: input.userId, }) .returning() - const row = inserted[0] + const row = inserted.at(0) if (!row) throw new Error('Insert did not return micronix plate row') collectionMap?.set(key, row.id) return row.id @@ -149,7 +149,7 @@ export async function createOrResolveCollection( updatedBy: input.userId, }) .returning() - const row = inserted[0] + const row = inserted.at(0) if (!row) throw new Error('Insert did not return cryovial box row') collectionMap?.set(key, row.id) return row.id diff --git a/packages/api/src/lib/container-projection.ts b/packages/api/src/lib/container-projection.ts index 63bafbe9..c874d6aa 100644 --- a/packages/api/src/lib/container-projection.ts +++ b/packages/api/src/lib/container-projection.ts @@ -31,7 +31,7 @@ export type ContainerPlacementFields = { /** Placement-only collection info from a resolved placement bundle row. */ export function projectContainerCollection(placement: ContainerPlacement): CollectionInfo | null { - if (placement.containerType === 'unknown' || !placement.collection) { + if (placement.containerType === 'unknown') { return null } diff --git a/packages/api/src/lib/container-read-view.ts b/packages/api/src/lib/container-read-view.ts index 7203dcb4..33c54dc7 100644 --- a/packages/api/src/lib/container-read-view.ts +++ b/packages/api/src/lib/container-read-view.ts @@ -72,7 +72,7 @@ export async function loadContainerReadViews( ]) const specimenTypeIds = [ - ...new Set(specimens.map((row) => row.specimenTypeId).filter((id): id is number => id != null)), + ...new Set(specimens.map((row) => row.specimenTypeId)), ] const specimenTypes = specimenTypeIds.length > 0 diff --git a/packages/api/src/lib/container-write-placement.ts b/packages/api/src/lib/container-write-placement.ts index 267f9c6a..9ee0a7ff 100644 --- a/packages/api/src/lib/container-write-placement.ts +++ b/packages/api/src/lib/container-write-placement.ts @@ -321,7 +321,7 @@ async function ensureSheetId( updatedBy: options?.userId, }) .returning() - const row = inserted[0] + const row = inserted.at(0) if (!row) throw new Error('Insert did not return sheet row') return row.id } diff --git a/packages/api/src/lib/controls/batch-create.ts b/packages/api/src/lib/controls/batch-create.ts index 3d63af58..629cf897 100644 --- a/packages/api/src/lib/controls/batch-create.ts +++ b/packages/api/src/lib/controls/batch-create.ts @@ -42,7 +42,7 @@ export async function createBloodControlBatch( } try { - const [newBatch] = await database + const inserted = await database .insert(controlBatch) .values({ controlDefinitionId: definitionId, @@ -53,7 +53,7 @@ export async function createBloodControlBatch( updatedBy: userId, }) .returning() - + const newBatch = inserted.at(0) if (!newBatch) { throw new Error('Insert did not return control batch row') } diff --git a/packages/api/src/lib/controls/batch-with-specimens.ts b/packages/api/src/lib/controls/batch-with-specimens.ts index 7b03b5f3..7d57eabf 100644 --- a/packages/api/src/lib/controls/batch-with-specimens.ts +++ b/packages/api/src/lib/controls/batch-with-specimens.ts @@ -238,7 +238,7 @@ export async function createBatchWithSpecimens( }) .returning() - const batchRecord = batchResult[0] + const batchRecord = batchResult.at(0) if (!batchRecord) { throw new Error('Failed to create batch - no record returned') } @@ -256,7 +256,7 @@ export async function createBatchWithSpecimens( }) .returning() - const specimenRecord = specimenResult[0] + const specimenRecord = specimenResult.at(0) if (!specimenRecord) { throw new Error('Failed to create specimen - no record returned') } @@ -346,7 +346,7 @@ export async function addSpecimensToBatch( }) .returning() - const specimenRecord = specimenResult[0] + const specimenRecord = specimenResult.at(0) if (!specimenRecord) { throw new Error('Failed to create specimen - no record returned') } diff --git a/packages/api/src/lib/controls/control-read.ts b/packages/api/src/lib/controls/control-read.ts index f468639d..fc6909b6 100644 --- a/packages/api/src/lib/controls/control-read.ts +++ b/packages/api/src/lib/controls/control-read.ts @@ -87,7 +87,7 @@ export async function listBloodControlBatches(database: Database) { const batches = batchesResults.map((row: (typeof batchesResults)[number]) => { const props = row.properties as Record | null - const strains = (props?.strains as unknown[]) || [] + const strains = (props?.strains as unknown[] | undefined) ?? [] return { ...row, strains: strains.map((s) => (typeof s === 'number' ? { id: s } : s)), diff --git a/packages/api/src/lib/derivations.ts b/packages/api/src/lib/derivations.ts index be6b1a84..6c53ea02 100644 --- a/packages/api/src/lib/derivations.ts +++ b/packages/api/src/lib/derivations.ts @@ -111,7 +111,7 @@ async function findOrCreateDerivedSpecimen( return existing.id } - const [created] = await database + const inserted = await database .insert(specimen) .values({ studySubjectId: parentSpecimen.studySubjectId, @@ -122,7 +122,7 @@ async function findOrCreateDerivedSpecimen( lastUpdated: utcNow(), }) .returning() - + const created = inserted.at(0) if (!created) throw new Error('Insert did not return specimen row') return created.id } diff --git a/packages/api/src/lib/export-config-migration.ts b/packages/api/src/lib/export-config-migration.ts index 0c6b78e2..44c3c84c 100644 --- a/packages/api/src/lib/export-config-migration.ts +++ b/packages/api/src/lib/export-config-migration.ts @@ -49,16 +49,16 @@ function appendSublabelWhenNeeded(columns: string[]): { columns: string[]; chang export function migrateExportConfigurationColumnKeys( configs: ExportConfigurations, ): { configs: ExportConfigurations; changed: boolean } { - let changed = false const configurations = configs.configurations.map((config) => { const retired = rewriteRetiredColumnKeys(config.columns) const withSublabel = appendSublabelWhenNeeded(retired.columns) if (!retired.changed && !withSublabel.changed) { return config } - changed = true return { ...config, columns: withSublabel.columns } }) + // Unchanged configs are returned as-is, so identity tells us whether anything was rewritten. + const changed = configurations.some((config, i) => config !== configs.configurations[i]) if (!changed) { return { configs, changed: false } diff --git a/packages/api/src/lib/registration-prepare.ts b/packages/api/src/lib/registration-prepare.ts index 0e34e992..4f9a1a73 100644 --- a/packages/api/src/lib/registration-prepare.ts +++ b/packages/api/src/lib/registration-prepare.ts @@ -269,7 +269,7 @@ export async function prepareRegistrationBatchForSpecimens( const placementErrors = await collectContainerPlacementErrors(database, placementRows) for (const placementError of placementErrors) { - const originalIndex = placementIndexToRowIndex[placementError.rowIndex] + const originalIndex = placementIndexToRowIndex.at(placementError.rowIndex) if (originalIndex !== undefined) { errors.push({ index: originalIndex, message: placementError.message }) } diff --git a/packages/api/src/routes/containers.ts b/packages/api/src/routes/containers.ts index b20a905b..f0bb1915 100644 --- a/packages/api/src/routes/containers.ts +++ b/packages/api/src/routes/containers.ts @@ -121,7 +121,7 @@ containers.get('/:id', authMiddleware, async (c) => { return c.json({ error: 'Container not found' }, 404) } - const [view] = await loadContainerReadViews(database, [container]) + const view = (await loadContainerReadViews(database, [container])).at(0) if (!view) { return c.json({ error: 'Container not found' }, 404) } @@ -224,7 +224,7 @@ containers.patch('/:id', memberMiddleware, async (c) => { } } - if (newBarcode !== null && newBarcode !== undefined) { + if (newBarcode !== null) { const takenBy = await resolveContainerByBarcode(database, newBarcode) if (takenBy !== null && takenBy !== id) { return c.json({ error: `Barcode '${newBarcode}' is already in use` }, 400) @@ -242,7 +242,7 @@ containers.patch('/:id', memberMiddleware, async (c) => { const trimmed = rawSublabel.trim() newSublabel = trimmed.length === 0 ? null : trimmed } - if (newSublabel !== null && newSublabel !== undefined) { + if (newSublabel !== null) { const takenBy = await resolveContainerByBarcode(database, newSublabel) if (takenBy !== null && takenBy !== id) { return c.json({ error: `Sublabel '${newSublabel}' is already in use` }, 400) diff --git a/packages/api/src/routes/controls.ts b/packages/api/src/routes/controls.ts index 7328f40c..1e4969a1 100644 --- a/packages/api/src/routes/controls.ts +++ b/packages/api/src/routes/controls.ts @@ -519,7 +519,7 @@ controls.post('/', memberMiddleware, async (c) => { }) .returning() - const newControl = result[0] + const newControl = result.at(0) if (!newControl) { throw new Error('Insert did not return control definition row') } @@ -562,7 +562,7 @@ controls.patch('/:id', memberMiddleware, async (c) => { const data = schema.parse(body) const { strains, targetDensity, targetDensityUnitId, properties, ...baseData } = data - const existingProps = (existing.properties as Record) || {} + const existingProps = (existing.properties as Record | null) ?? {} const newProps: Record = { ...existingProps, ...(properties || {}) } if (strains !== undefined) { From 6720d9d4234d18173c15ed8c4ae46d4f7b29b766 Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Mon, 5 Oct 2026 20:02:53 -0700 Subject: [PATCH 02/30] chore(api): lint against async database.transaction callbacks. Drizzle's bun-sqlite transaction commits at the callback's first await, which db/README.md already warned about in prose; the audit still found eleven call sites. The two remaining move transactions never awaited, so their callbacks are now synchronous. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/api/eslint.config.mjs | 9 +++++++++ packages/api/src/lib/collection-move.ts | 2 +- packages/api/src/lib/container-move.ts | 2 +- 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/packages/api/eslint.config.mjs b/packages/api/eslint.config.mjs index e8531944..8b0d012c 100644 --- a/packages/api/eslint.config.mjs +++ b/packages/api/eslint.config.mjs @@ -19,6 +19,15 @@ export default [ }, rules: { "@typescript-eslint/no-unnecessary-condition": "error", + "no-restricted-syntax": [ + "error", + { + selector: + "CallExpression[callee.property.name='transaction'] > :matches(ArrowFunctionExpression, FunctionExpression)[async=true]", + message: + "Drizzle's bun-sqlite transaction commits at the callback's first await. Use withWriteTransaction from src/db/write-transaction.ts, or a synchronous callback.", + }, + ], }, }, ]; diff --git a/packages/api/src/lib/collection-move.ts b/packages/api/src/lib/collection-move.ts index ff6b17dd..3eaf927d 100644 --- a/packages/api/src/lib/collection-move.ts +++ b/packages/api/src/lib/collection-move.ts @@ -507,7 +507,7 @@ export async function executeCollectionMoves( } // Execute moves in transaction - await database.transaction(async (tx) => { + database.transaction((tx) => { const now = utcNow() for (const { collectionInfo, targetLocationId } of validMoves) { diff --git a/packages/api/src/lib/container-move.ts b/packages/api/src/lib/container-move.ts index e10bbeb6..9ce17efb 100644 --- a/packages/api/src/lib/container-move.ts +++ b/packages/api/src/lib/container-move.ts @@ -504,7 +504,7 @@ export async function executeMoves(database: Database, request: BatchMoveRequest return { success: false, moved: 0, errors } } - await database.transaction(async (tx) => { + database.transaction((tx) => { for (const { info, targetCollectionId } of finalMoves) { if (!targetCollectionId) continue switch (info.containerType) { From c0262dbe51940ed88529caa39d4871948b113df2 Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Mon, 5 Oct 2026 20:58:25 -0700 Subject: [PATCH 03/30] fix(web): clear the 64 no-unnecessary-condition lint errors. Where the rule exposed a type that lies about the wire, fix the type and keep the guard: - a control source's definition is null for batches without one, and the API omits nulls, so ContainerSourceInfo.definition is optional; - GET /derivations/containers/:id/source has no derivation key for containers that were not derived, so getSource's fields are optional; - scan-move CSV cells read through csvCell, since a column missing from the file is undefined despite the row's string index type. Elsewhere the guards were dead: records that 404 when missing, list routes that always send an array, TanStack isSuccess narrowing, arrays already checked non-empty, and a map whose key 0 is always seeded. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../src/__tests__/helpers/mock-api-domains.ts | 2 +- .../CollectionDestinationPicker.tsx | 5 ++-- packages/web/src/components/SearchModal.tsx | 2 +- packages/web/src/components/SetupGuard.tsx | 2 +- .../components/wizards/SpecimenTypesStep.tsx | 4 +-- packages/web/src/hooks/useDashboard.ts | 4 +-- packages/web/src/hooks/useExportWorkflow.ts | 2 +- packages/web/src/hooks/useMoveWorkflow.ts | 2 +- .../web/src/hooks/useReferenceDataPage.ts | 3 +- packages/web/src/hooks/useSpecimens.ts | 7 ++--- packages/web/src/hooks/useStudies.ts | 2 +- packages/web/src/lib/api/client.ts | 2 +- packages/web/src/lib/api/containers.ts | 3 +- packages/web/src/lib/api/derivations.ts | 7 +++-- packages/web/src/lib/bulk-csv-workflow.ts | 28 ++++++++----------- packages/web/src/lib/container-display.ts | 2 +- packages/web/src/lib/location-tree.ts | 2 +- packages/web/src/lib/scan-move/variants.ts | 14 +++++++--- packages/web/src/pages/BagDetail.tsx | 4 +-- packages/web/src/pages/BoxDetail.tsx | 4 +-- packages/web/src/pages/ContainerDetail.tsx | 18 ++++++------ .../web/src/pages/ContainerMoveMicronix.tsx | 2 +- packages/web/src/pages/ControlBatchWizard.tsx | 6 ---- packages/web/src/pages/CryovialBoxDetail.tsx | 8 ++---- .../web/src/pages/DerivationsBulkImport.tsx | 2 +- packages/web/src/pages/Locations.tsx | 5 ++-- .../web/src/pages/MicronixPlateDetail.tsx | 4 +-- .../web/src/pages/PlateScanValidation.tsx | 2 +- .../web/src/pages/QpcrExperimentDetail.tsx | 10 ++----- packages/web/src/pages/SheetDetail.tsx | 2 +- packages/web/src/pages/Statistics.tsx | 2 +- packages/web/src/pages/Studies.tsx | 2 +- 32 files changed, 75 insertions(+), 89 deletions(-) diff --git a/packages/web/src/__tests__/helpers/mock-api-domains.ts b/packages/web/src/__tests__/helpers/mock-api-domains.ts index f3af5544..99d93c7f 100644 --- a/packages/web/src/__tests__/helpers/mock-api-domains.ts +++ b/packages/web/src/__tests__/helpers/mock-api-domains.ts @@ -122,7 +122,7 @@ const MODULE_API_KEYS: Record = { export function modulesForOverrides(overrides: MockApiOverrides): ApiModuleId[] { const mods = new Set() for (const key of Object.keys(overrides)) { - const mod = API_KEY_TO_MODULE[key] + const mod = API_KEY_TO_MODULE[key] as ApiModuleId | undefined if (mod) mods.add(mod) } return [...mods] diff --git a/packages/web/src/components/CollectionDestinationPicker.tsx b/packages/web/src/components/CollectionDestinationPicker.tsx index dc357850..b215953b 100644 --- a/packages/web/src/components/CollectionDestinationPicker.tsx +++ b/packages/web/src/components/CollectionDestinationPicker.tsx @@ -445,12 +445,11 @@ export default function CollectionDestinationPicker({ <> {rootsWithBoxes.map((root) => renderLocationNode(root, 0))} - {/* eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- collectionsByLocation[0] may be missing when key 0 not in map */} - {(collectionsByLocation[0] ?? []).length > 0 && ( + {collectionsByLocation[0].length > 0 && (
{labels.unlocated}
- {(collectionsByLocation[0] ?? []) + {collectionsByLocation[0] .filter((box) => { if (!debouncedSearch.trim()) return true const searchLower = debouncedSearch.toLowerCase() diff --git a/packages/web/src/components/SearchModal.tsx b/packages/web/src/components/SearchModal.tsx index b59abf4c..34252756 100644 --- a/packages/web/src/components/SearchModal.tsx +++ b/packages/web/src/components/SearchModal.tsx @@ -69,7 +69,7 @@ export default function SearchModal({ isOpen, onClose, initialQuery }: SearchMod setLoading(true) const response = await searchApi.search(query, 'all') if (!ignore) { - setResults(response.results || []) + setResults(response.results) } } catch (error) { if (!ignore) { diff --git a/packages/web/src/components/SetupGuard.tsx b/packages/web/src/components/SetupGuard.tsx index a40427cb..02e0bbee 100644 --- a/packages/web/src/components/SetupGuard.tsx +++ b/packages/web/src/components/SetupGuard.tsx @@ -54,7 +54,7 @@ export default function SetupGuard({ children }: { children: React.ReactNode }) ) } - if (statusQuery.data?.initialized === false && location.pathname !== '/setup') { + if (statusQuery.data.initialized === false && location.pathname !== '/setup') { return null } diff --git a/packages/web/src/components/wizards/SpecimenTypesStep.tsx b/packages/web/src/components/wizards/SpecimenTypesStep.tsx index 4c8cc9e3..6b0028fa 100644 --- a/packages/web/src/components/wizards/SpecimenTypesStep.tsx +++ b/packages/web/src/components/wizards/SpecimenTypesStep.tsx @@ -51,7 +51,7 @@ export default function SpecimenTypesStep({ setAllowedContainerTypes([]) return } - if (containerTypesQuery.isSuccess && containerTypesQuery.data) { + if (containerTypesQuery.isSuccess) { const containerTypes = containerTypesQuery.data.containerTypes setAllowedContainerTypes(containerTypes) if (newSpecimenType.containerType && containerTypes.length > 0) { @@ -72,7 +72,7 @@ export default function SpecimenTypesStep({ setAllowedSpecimenTypes([]) return } - if (specimenTypesByContainerQuery.isSuccess && specimenTypesByContainerQuery.data) { + if (specimenTypesByContainerQuery.isSuccess) { const types = specimenTypesByContainerQuery.data setAllowedSpecimenTypes(types) if (newSpecimenType.specimenTypeId && types.length > 0) { diff --git a/packages/web/src/hooks/useDashboard.ts b/packages/web/src/hooks/useDashboard.ts index de48c209..243dfa02 100644 --- a/packages/web/src/hooks/useDashboard.ts +++ b/packages/web/src/hooks/useDashboard.ts @@ -108,7 +108,7 @@ export function useDashboardSecondary() { controlsApi.list().catch(() => ({ controls: [] as unknown[] })), ]) - const activities = (activityRes.activity ?? []) as Array<{ + const activities = activityRes.activity as Array<{ id: number type: string timestamp: string @@ -126,7 +126,7 @@ export function useDashboardSecondary() { label: item.label, context: item.context, })), - hasControls: (controlsRes.controls?.length ?? 0) > 0, + hasControls: controlsRes.controls.length > 0, } }, }) diff --git a/packages/web/src/hooks/useExportWorkflow.ts b/packages/web/src/hooks/useExportWorkflow.ts index dd7090eb..7aea9dd1 100644 --- a/packages/web/src/hooks/useExportWorkflow.ts +++ b/packages/web/src/hooks/useExportWorkflow.ts @@ -188,7 +188,7 @@ export function usePlateScanBootstrap() { queryKey: [...exportWorkflowKeys.plateScanBootstrap(), 'plates'] as const, queryFn: async (): Promise => { const res = await collectionsApi.listCollectionsByType('micronix_plate') - return (res.collections ?? []).map((c) => ({ + return res.collections.map((c) => ({ id: c.id, name: c.name, })) diff --git a/packages/web/src/hooks/useMoveWorkflow.ts b/packages/web/src/hooks/useMoveWorkflow.ts index 97a87e1f..90505545 100644 --- a/packages/web/src/hooks/useMoveWorkflow.ts +++ b/packages/web/src/hooks/useMoveWorkflow.ts @@ -175,7 +175,7 @@ export function usePaperMoveSheets( : await collectionsApi.getBag(sourceCollectionId!) const contents = response.contents as { sheets?: Array<{ id: number; name: string; papers?: unknown[] }> } - if (!contents?.sheets) return [] + if (!contents.sheets) return [] return contents.sheets.map((s) => ({ id: s.id, diff --git a/packages/web/src/hooks/useReferenceDataPage.ts b/packages/web/src/hooks/useReferenceDataPage.ts index 88d1b903..ba4e5128 100644 --- a/packages/web/src/hooks/useReferenceDataPage.ts +++ b/packages/web/src/hooks/useReferenceDataPage.ts @@ -26,7 +26,8 @@ async function loadTabData( } const res = await config.list() - const data = res.data + // Typed loosely on purpose: callers pass unvalidated response bodies. + const data: unknown = res.data if (Array.isArray(data)) return data if (typeof data === 'object' && data !== null) { return ((data as Record)[config.getDataKey()] as unknown[] | undefined) ?? [] diff --git a/packages/web/src/hooks/useSpecimens.ts b/packages/web/src/hooks/useSpecimens.ts index 5175546f..62f39796 100644 --- a/packages/web/src/hooks/useSpecimens.ts +++ b/packages/web/src/hooks/useSpecimens.ts @@ -126,10 +126,8 @@ export function useSpecimenSourceInfo(specimen: Specimen | null | undefined) { if (specimen.studySubjectId) { const subjectRes = await subjectsApi.get(specimen.studySubjectId) const subject = subjectRes.subject - if (!subject) return null const studyRes = await studiesApi.get(subject.studyId) const study = studyRes.study - if (!study) return null return { type: 'subject', id: specimen.studySubjectId, @@ -140,14 +138,13 @@ export function useSpecimenSourceInfo(specimen: Specimen | null | undefined) { if (specimen.controlBatchId) { const batchRes = await controlsApi.getBatch(specimen.controlBatchId) const batch = batchRes.batch - if (!batch) return null const defRes = await controlsApi.get(batch.controlDefinitionId) const control = defRes.control return { type: 'control', id: specimen.controlBatchId, name: batch.name, - definition: control ? { id: control.id, name: control.name } : undefined, + definition: { id: control.id, name: control.name }, } } return null @@ -161,7 +158,7 @@ export function useContainersForSpecimen(specimenId: string | number | undefined queryKey: specimenKeys.containers(specimenId!), queryFn: async () => { const res = await containersApi.list({ specimen_id: specimenId }) - return res.containers ?? [] + return res.containers }, enabled: !!specimenId, }) diff --git a/packages/web/src/hooks/useStudies.ts b/packages/web/src/hooks/useStudies.ts index 7974f4b7..ed9511eb 100644 --- a/packages/web/src/hooks/useStudies.ts +++ b/packages/web/src/hooks/useStudies.ts @@ -68,7 +68,7 @@ export function useStudySummaryCards() { useEffect(() => { return queryClient.getQueryCache().subscribe((event) => { - const key = event.query?.queryKey + const key = event.query.queryKey if ( key?.[0] === studyKeys.all[0] && (key[1] === 'summary-card' || key[1] === 'summaries-batch') diff --git a/packages/web/src/lib/api/client.ts b/packages/web/src/lib/api/client.ts index 16610488..53960214 100644 --- a/packages/web/src/lib/api/client.ts +++ b/packages/web/src/lib/api/client.ts @@ -49,7 +49,7 @@ function isAuthEndpoint(url: string | undefined): boolean { function attachRequestIdInterceptor(instance: AxiosInstance): void { instance.interceptors.request.use((config) => { - const headers = config.headers ?? {} + const headers = config.headers const existing = readRequestIdHeader(headers) if (!existing) { ;(headers as Record)[REQUEST_ID_HEADER] = generateClientRequestId() diff --git a/packages/web/src/lib/api/containers.ts b/packages/web/src/lib/api/containers.ts index 894fdac6..f62b673c 100644 --- a/packages/web/src/lib/api/containers.ts +++ b/packages/web/src/lib/api/containers.ts @@ -24,7 +24,8 @@ export type ContainerSourceInfo = id: number name: string productionDate?: string | null - definition: { id: number; name: string } + /** Absent when the batch has no definition (the API omits null fields). */ + definition?: { id: number; name: string } } /** Stable container detail shape for GET /containers/:id (normalized on the client). */ diff --git a/packages/web/src/lib/api/derivations.ts b/packages/web/src/lib/api/derivations.ts index 3c809144..02ee81dc 100644 --- a/packages/web/src/lib/api/derivations.ts +++ b/packages/web/src/lib/api/derivations.ts @@ -92,10 +92,11 @@ export const derivationsApi = { params, }), getSource: (containerId: number) => + // Containers that were not derived get { type: 'original', source, ... } with no derivation. api.get<{ - derivation: Derivation - parentContainer: any - parentSpecimen: Specimen + derivation?: Derivation + parentContainer?: any + parentSpecimen?: Specimen }>(`/derivations/containers/${containerId}/source`), getChain: (containerId: number) => api.get<{ diff --git a/packages/web/src/lib/bulk-csv-workflow.ts b/packages/web/src/lib/bulk-csv-workflow.ts index 72879739..e64a7f16 100644 --- a/packages/web/src/lib/bulk-csv-workflow.ts +++ b/packages/web/src/lib/bulk-csv-workflow.ts @@ -62,22 +62,18 @@ export async function runBulkCsvServerValidation( return validateRes.errors.map((e) => ({ row: e.index + 1, error: e.message })) } - if (ctx.importType === 'combined') { - const payload = buildBulkCombinedRequestPayload(data, { - containerType: ctx.containerType, - fixedStudyShortCode: ctx.fixedStudyShortCode, - missingCollections: ctx.missingCollections, - atomicMode: ctx.atomicMode, - }) - const validateRes = await importsApi.bulkCombinedValidate(payload) - if (validateRes.valid || validateRes.errors.length === 0) return [] - return validateRes.errors.map((e) => ({ - row: e.rowIndex ?? e.subjectIndex + 1, - error: e.message, - })) - } - - return [] + const payload = buildBulkCombinedRequestPayload(data, { + containerType: ctx.containerType, + fixedStudyShortCode: ctx.fixedStudyShortCode, + missingCollections: ctx.missingCollections, + atomicMode: ctx.atomicMode, + }) + const validateRes = await importsApi.bulkCombinedValidate(payload) + if (validateRes.valid || validateRes.errors.length === 0) return [] + return validateRes.errors.map((e) => ({ + row: e.rowIndex ?? e.subjectIndex + 1, + error: e.message, + })) } export async function runBulkCsvImport( diff --git a/packages/web/src/lib/container-display.ts b/packages/web/src/lib/container-display.ts index ac732193..c81b5436 100644 --- a/packages/web/src/lib/container-display.ts +++ b/packages/web/src/lib/container-display.ts @@ -69,5 +69,5 @@ export function containerDisplayLabel( return name } - return getContainerTypeName(container.containerType) ?? fallback ?? 'Container' + return getContainerTypeName(container.containerType) } diff --git a/packages/web/src/lib/location-tree.ts b/packages/web/src/lib/location-tree.ts index 474d72a3..3d753577 100644 --- a/packages/web/src/lib/location-tree.ts +++ b/packages/web/src/lib/location-tree.ts @@ -135,7 +135,7 @@ export function getLocationSearchHighlightQuery(raw: string): string { const t = raw.trim() if (!t) return '' const segs = parseLocationPathSegments(t) - if (segs) return segs[segs.length - 1]! ?? t + if (segs) return segs[segs.length - 1] return t } diff --git a/packages/web/src/lib/scan-move/variants.ts b/packages/web/src/lib/scan-move/variants.ts index f0e3e481..a0b98276 100644 --- a/packages/web/src/lib/scan-move/variants.ts +++ b/packages/web/src/lib/scan-move/variants.ts @@ -1,7 +1,7 @@ import { inferDestinationPlateForScan } from '../plate-destination-inference' import { parseScannerPlateCsv, validateScannerPlateCsv } from '../scanner-plate-csv' import { parseBuiltinMoveCsv } from './ingest' -import type { ScanMoveIdentifier, ScanMoveInference, ScanMoveVariant } from './types' +import type { ScanMoveCsvRow, ScanMoveIdentifier, ScanMoveInference, ScanMoveVariant } from './types' /** * Built-in cryovial move CSV spec (ADR 0008): fixed columns, not a @@ -12,6 +12,12 @@ export const CRYOVIAL_MOVE_CSV_SPEC = { skipRows: 0, } as const +/** Trimmed cell value; a column missing from the file reads as ''. */ +function csvCell(row: ScanMoveCsvRow, column: string): string { + const value = row[column] as string | undefined + return (value ?? '').trim() +} + function positionKey(sourceCollectionName: string, sourcePosition: string): string { return `${sourceCollectionName}:${sourcePosition}` } @@ -54,7 +60,7 @@ export const micronixScanMoveVariant: ScanMoveVariant = { }, identifierFromRow(row): ScanMoveIdentifier | null { - const barcode = (row.container_barcode ?? '').trim() + const barcode = csvCell(row, 'container_barcode') if (barcode === '') return null // empty well; no move for this row return { type: 'barcode', barcode } }, @@ -116,8 +122,8 @@ export const cryovialScanMoveVariant: ScanMoveVariant = { identifierFromRow(row): ScanMoveIdentifier { return { type: 'position', - sourceCollectionName: (row.source_collection_name ?? '').trim(), - sourcePosition: (row.source_position ?? '').trim(), + sourceCollectionName: csvCell(row, 'source_collection_name'), + sourcePosition: csvCell(row, 'source_position'), } }, diff --git a/packages/web/src/pages/BagDetail.tsx b/packages/web/src/pages/BagDetail.tsx index fc90af7e..ab37a734 100644 --- a/packages/web/src/pages/BagDetail.tsx +++ b/packages/web/src/pages/BagDetail.tsx @@ -40,7 +40,7 @@ export default function BagDetail() { // Initialize expanded sheets (default to collapsed if more than 3 sheets) // This hook must be called before any conditional returns to maintain hook order - const sheets = (data?.contents?.sheets || []) as Array<{ + const sheets = (data?.contents.sheets || []) as Array<{ id: number name?: string papers?: CollectionTableEntry[] @@ -58,7 +58,7 @@ export default function BagDetail() { const tableRows = useMemo(() => { const bag = data?.bag const context = bag - ? { collectionName: bag.name ?? undefined, locationPath: bag.locationPath ?? undefined } + ? { collectionName: bag.name, locationPath: bag.locationPath ?? undefined } : undefined const list: ReturnType[] = [] sheets.forEach((sheet: { id: number; name?: string; papers?: CollectionTableEntry[] }) => { diff --git a/packages/web/src/pages/BoxDetail.tsx b/packages/web/src/pages/BoxDetail.tsx index d1e49c5d..25776541 100644 --- a/packages/web/src/pages/BoxDetail.tsx +++ b/packages/web/src/pages/BoxDetail.tsx @@ -40,7 +40,7 @@ export default function BoxDetail() { // Initialize expanded sheets (default to collapsed if more than 3 sheets) // This hook must be called before any conditional returns to maintain hook order - const sheets = (data?.contents?.sheets || []) as Array<{ + const sheets = (data?.contents.sheets || []) as Array<{ id: number name?: string papers?: CollectionTableEntry[] @@ -58,7 +58,7 @@ export default function BoxDetail() { const tableRows = useMemo(() => { const box = data?.box const context = box - ? { collectionName: box.name ?? undefined, locationPath: box.locationPath ?? undefined } + ? { collectionName: box.name, locationPath: box.locationPath ?? undefined } : undefined const list: ReturnType[] = [] sheets.forEach((sheet: { id: number; name?: string; papers?: CollectionTableEntry[] }) => { diff --git a/packages/web/src/pages/ContainerDetail.tsx b/packages/web/src/pages/ContainerDetail.tsx index f634cb20..b0b65d39 100644 --- a/packages/web/src/pages/ContainerDetail.tsx +++ b/packages/web/src/pages/ContainerDetail.tsx @@ -95,7 +95,7 @@ export default function ContainerDetail() { ) } - if (!data?.container?.id) { + if (!data?.container.id) { return (
@@ -137,9 +137,7 @@ export default function ContainerDetail() { breadcrumbItems.push({ label: source.name, to: `/blood-controls/batches/${source.id}` }) } else if (source?.type === 'subject') { breadcrumbItems.push({ label: 'Studies', to: '/studies' }) - if (source.study) { - breadcrumbItems.push({ label: source.study.title, to: `/studies/${source.study.id}` }) - } + breadcrumbItems.push({ label: source.study.title, to: `/studies/${source.study.id}` }) breadcrumbItems.push({ label: source.name, to: `/subjects/${source.id}` }) } @@ -401,7 +399,7 @@ export default function ContainerDetail() { )}
- {source.type === 'subject' && source.study && ( + {source.type === 'subject' && (
Study
@@ -470,7 +468,7 @@ export default function ContainerDetail() { {/* Derivation Sections */}
{/* Source Derivation Information (if this container is a child via derivation) */} - {sourceDerivation && sourceDerivation.type === 'derivation' && sourceDerivation.derivation && ( + {sourceDerivation?.derivation && (
@@ -586,11 +584,11 @@ export default function ContainerDetail() { )}
- {container?.remainingQuantity !== undefined && container?.unit && ( + {container?.remainingQuantity !== undefined && container.unit && (
Quantity: - {container.remainingQuantity?.toLocaleString() || '0'} {container.unit?.symbol || 'units'} + {container.remainingQuantity.toLocaleString() || '0'} {container.unit.symbol || 'units'}
)} @@ -704,7 +702,7 @@ export default function ContainerDetail() { )} {/* Edit Modal */} - {showEditModal && container && ( + {showEditModal && ( setShowEditModal(false)} @@ -726,7 +724,7 @@ export default function ContainerDetail() { {/* Derivation Chain View - Always show when there's derivation data */} {id && - ((sourceDerivation && sourceDerivation.type === 'derivation' && sourceDerivation.derivation) || + (sourceDerivation?.derivation || derivations.length > 0) && (
diff --git a/packages/web/src/pages/ContainerMoveMicronix.tsx b/packages/web/src/pages/ContainerMoveMicronix.tsx index ee3c5568..441da23c 100644 --- a/packages/web/src/pages/ContainerMoveMicronix.tsx +++ b/packages/web/src/pages/ContainerMoveMicronix.tsx @@ -42,7 +42,7 @@ export default function ContainerMoveMicronix() { if (scannerConfigurations.length === 0 || selectedConfigId !== null) return const defaultConfig = scannerConfigurations.find((c) => c.isDefault === true) ?? scannerConfigurations[0] - if (defaultConfig) setSelectedConfigId(defaultConfig.id) + setSelectedConfigId(defaultConfig.id) }, [scannerConfigurations, selectedConfigId]) if (!canWrite) { diff --git a/packages/web/src/pages/ControlBatchWizard.tsx b/packages/web/src/pages/ControlBatchWizard.tsx index dfd475fc..8508afb0 100644 --- a/packages/web/src/pages/ControlBatchWizard.tsx +++ b/packages/web/src/pages/ControlBatchWizard.tsx @@ -175,7 +175,6 @@ export default function ControlBatchWizard() { const seed = definitionSeedQuery.data if (!seed) return const { control, composition } = seed.summaryResponse - if (!control) return setBatchInfo({ controlDefinitionId: control.id, controlDefinition: { ...control, strains: composition?.strains }, @@ -202,11 +201,6 @@ export default function ControlBatchWizard() { if (isCreateFromDefinition && definitionSeedQuery.isError) { return getQueryErrorMessage(definitionSeedQuery.error, 'Failed to load control definition') } - if (isCreateFromDefinition && definitionSeedQuery.isSuccess) { - if (!definitionSeedQuery.data.summaryResponse.control) { - return 'Control definition not found' - } - } if (isCompositionCsvFlow && compositionQuery.isError) { return getQueryErrorMessage(compositionQuery.error, 'Failed to load composition') } diff --git a/packages/web/src/pages/CryovialBoxDetail.tsx b/packages/web/src/pages/CryovialBoxDetail.tsx index cffef7c0..81311e3c 100644 --- a/packages/web/src/pages/CryovialBoxDetail.tsx +++ b/packages/web/src/pages/CryovialBoxDetail.tsx @@ -110,10 +110,8 @@ export default function CryovialBoxDetail() { const tableRows = useMemo(() => { if (!layout || !data?.positions) return [] const positions = data.positions as Record - const box = data?.box - const context = box - ? { collectionName: box.name ?? undefined, locationPath: box.locationPath ?? undefined } - : undefined + const box = data.box + const context = { collectionName: box.name, locationPath: box.locationPath ?? undefined } const rows: ReturnType[] = [] layout.rows.forEach((row) => { layout.cols.forEach((col) => { @@ -302,7 +300,7 @@ export default function CryovialBoxDetail() { getKey={(row, col) => `${row}${col.padStart(2, '0')}`} getCell={(row, col) => { const key = `${row}${col.padStart(2, '0')}` - return positions[key] || [] + return (positions[key] as (typeof positions)[string] | undefined) ?? [] }} renderCell={(value, coords) => { const entries: any[] = Array.isArray(value) ? value : value ? [value as any] : [] diff --git a/packages/web/src/pages/DerivationsBulkImport.tsx b/packages/web/src/pages/DerivationsBulkImport.tsx index a13e99b2..fc5133b0 100644 --- a/packages/web/src/pages/DerivationsBulkImport.tsx +++ b/packages/web/src/pages/DerivationsBulkImport.tsx @@ -153,7 +153,7 @@ export default function DerivationsBulkImport() { sourceType, parentContainerType, specimenTypes, - exampleDerivationType: settings.derivationType || DERIVATION_TYPES[0]?.value, + exampleDerivationType: settings.derivationType || DERIVATION_TYPES[0].value, exampleProtocol: settings.protocol || undefined, }) diff --git a/packages/web/src/pages/Locations.tsx b/packages/web/src/pages/Locations.tsx index 64da1ed6..8c27e4ae 100644 --- a/packages/web/src/pages/Locations.tsx +++ b/packages/web/src/pages/Locations.tsx @@ -64,7 +64,7 @@ export default function Locations() { const locationsQuery = useLocationsList() const listStatus = fromQuery(locationsQuery, { - isEmpty: locationsQuery.isSuccess && (locationsQuery.data?.length ?? 0) === 0, + isEmpty: locationsQuery.isSuccess && locationsQuery.data.length === 0, }) const preserveOnNextSync = useRef(false) const [locations, setLocations] = useState([]) @@ -158,7 +158,8 @@ export default function Locations() { if (nodeRef && tree) { // Small delay to ensure DOM is updated setTimeout(() => { - nodeRef?.scrollIntoView?.({ + // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- jsdom has no scrollIntoView + nodeRef.scrollIntoView?.({ behavior: 'smooth', block: 'center', inline: 'nearest', diff --git a/packages/web/src/pages/MicronixPlateDetail.tsx b/packages/web/src/pages/MicronixPlateDetail.tsx index 780bff98..5cd07b2b 100644 --- a/packages/web/src/pages/MicronixPlateDetail.tsx +++ b/packages/web/src/pages/MicronixPlateDetail.tsx @@ -93,9 +93,7 @@ export default function MicronixPlateDetail() { if (!layout || !data?.wells) return [] const wells = data.wells as Record const plate = data.plate - const context = plate - ? { collectionName: plate.name ?? undefined, locationPath: plate.locationPath ?? undefined } - : undefined + const context = { collectionName: plate.name, locationPath: plate.locationPath ?? undefined } const rows: ReturnType[] = [] layout.rows.forEach((row) => { layout.cols.forEach((col) => { diff --git a/packages/web/src/pages/PlateScanValidation.tsx b/packages/web/src/pages/PlateScanValidation.tsx index 47d82c9c..8012e6a7 100644 --- a/packages/web/src/pages/PlateScanValidation.tsx +++ b/packages/web/src/pages/PlateScanValidation.tsx @@ -207,7 +207,7 @@ export default function PlateScanValidation() { useEffect(() => { if (selectedConfigId != null || scannerConfigurations.length === 0) return const defaultConfig = scannerConfigurations.find((c) => c.isDefault === true) - setSelectedConfigId(defaultConfig?.id ?? scannerConfigurations[0]?.id ?? null) + setSelectedConfigId((defaultConfig ?? scannerConfigurations[0]).id) }, [scannerConfigurations, selectedConfigId]) useEffect(() => { diff --git a/packages/web/src/pages/QpcrExperimentDetail.tsx b/packages/web/src/pages/QpcrExperimentDetail.tsx index 665c5437..0b364c71 100644 --- a/packages/web/src/pages/QpcrExperimentDetail.tsx +++ b/packages/web/src/pages/QpcrExperimentDetail.tsx @@ -169,7 +169,7 @@ export default function QpcrExperimentDetail() { useEffect(() => { if (scannerConfigs.length === 0) return const defaultConfig = scannerConfigs.find((c) => c.isDefault === true) ?? scannerConfigs[0] - if (defaultConfig && selectedConfigId === null) { + if (selectedConfigId === null) { setSelectedConfigId(defaultConfig.id) } }, [scannerConfigs, selectedConfigId]) @@ -708,16 +708,12 @@ export default function QpcrExperimentDetail() {
- {wellDetails.container.containerType != null - ? getContainerTypeIcon(wellDetails.container.containerType) - : null} + {getContainerTypeIcon(wellDetails.container.containerType)}

Container

- {wellDetails.container.containerType != null - ? getContainerTypeName(wellDetails.container.containerType) - : '—'} + {getContainerTypeName(wellDetails.container.containerType)} {wellDetails.container.collection?.name != null && ( <> · {wellDetails.container.collection.name} )} diff --git a/packages/web/src/pages/SheetDetail.tsx b/packages/web/src/pages/SheetDetail.tsx index da962f7f..6af88076 100644 --- a/packages/web/src/pages/SheetDetail.tsx +++ b/packages/web/src/pages/SheetDetail.tsx @@ -54,7 +54,7 @@ export default function SheetDetail() { const tableRows = useMemo(() => { const context = sheet - ? { collectionName: sheet.name ?? undefined, locationPath: sheet.locationPath ?? undefined } + ? { collectionName: sheet.name, locationPath: sheet.locationPath ?? undefined } : undefined return papers.map((p: CollectionTableEntry) => buildCollectionTableRow({ diff --git a/packages/web/src/pages/Statistics.tsx b/packages/web/src/pages/Statistics.tsx index dff8ca35..84de135e 100644 --- a/packages/web/src/pages/Statistics.tsx +++ b/packages/web/src/pages/Statistics.tsx @@ -396,7 +396,7 @@ export default function Statistics() { )} {/* Loading overlay when refreshing with data */} - {statisticsQuery.isFetching && data && ( + {statisticsQuery.isFetching && (

diff --git a/packages/web/src/pages/Studies.tsx b/packages/web/src/pages/Studies.tsx index 96fbda21..94814933 100644 --- a/packages/web/src/pages/Studies.tsx +++ b/packages/web/src/pages/Studies.tsx @@ -156,7 +156,7 @@ export default function Studies() { ) cardRefs.current.forEach((element) => { - if (element && summaryObserverRef.current) { + if (summaryObserverRef.current) { summaryObserverRef.current.observe(element) } }) From 4c7580998d2a2ec7cc6be431868f98ac4254e9bc Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Mon, 5 Oct 2026 21:00:55 -0700 Subject: [PATCH 04/30] chore: run lint and tests in CI and pre-commit, and point agents at package docs. - ci:verify now runs lint and unit tests as well as typecheck, build, and the docs check; a new Checks workflow runs it on every PR. - The pre-commit hook lints staged API and web source files. - AGENTS.md points at the API test, web test, and db READMEs, which agents never opened in recent sessions, and describes the new checks. Co-Authored-By: Claude Opus 5.5 (1M context) --- .githooks/pre-commit-checks.sh | 17 +++++++++++++++-- .github/workflows/checks.yml | 35 ++++++++++++++++++++++++++++++++++ AGENTS.md | 14 ++++++++++---- package.json | 2 +- 4 files changed, 61 insertions(+), 7 deletions(-) create mode 100644 .github/workflows/checks.yml diff --git a/.githooks/pre-commit-checks.sh b/.githooks/pre-commit-checks.sh index d09c19a8..005135d7 100755 --- a/.githooks/pre-commit-checks.sh +++ b/.githooks/pre-commit-checks.sh @@ -1,6 +1,6 @@ #!/usr/bin/env bash -# Pre-commit checks mirroring CI compile steps. Runs typecheck on packages -# touched by staged files so Docker `bun run build` (tsc) failures are caught locally. +# Pre-commit checks mirroring CI. Typechecks packages touched by staged files +# and lints the staged API/web source files. set -euo pipefail ROOT="$(git rev-parse --show-toplevel)" @@ -10,6 +10,8 @@ needs_contract=false needs_api=false needs_web=false has_staged=false +api_lint_files=() +web_lint_files=() while IFS= read -r file; do [[ -z "$file" ]] && continue @@ -22,9 +24,11 @@ while IFS= read -r file; do ;; packages/api/*) needs_api=true + [[ "$file" == packages/api/src/*.ts ]] && api_lint_files+=("${file#packages/api/}") ;; packages/web/*) needs_web=true + [[ "$file" == packages/web/src/*.ts || "$file" == packages/web/src/*.tsx ]] && web_lint_files+=("${file#packages/web/}") ;; Dockerfile|.dockerignore|package.json|bun.lock|packages/*/package.json|packages/*/tsconfig*.json) needs_contract=true @@ -51,3 +55,12 @@ if $needs_web; then fi echo "pre-commit: typecheck passed." + +if (( ${#api_lint_files[@]} > 0 )); then + echo "pre-commit: linting staged API files..." + (cd packages/api && bunx eslint --no-warn-ignored "${api_lint_files[@]}") +fi +if (( ${#web_lint_files[@]} > 0 )); then + echo "pre-commit: linting staged web files..." + (cd packages/web && bunx eslint --no-warn-ignored "${web_lint_files[@]}") +fi diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml new file mode 100644 index 00000000..f4efb77b --- /dev/null +++ b/.github/workflows/checks.yml @@ -0,0 +1,35 @@ +name: Checks + +on: + pull_request: + push: + branches: [main] + workflow_dispatch: + +permissions: + contents: read + +concurrency: + group: checks-${{ github.ref }} + cancel-in-progress: true + +jobs: + verify: + runs-on: ubuntu-latest + timeout-minutes: 30 + steps: + - name: Checkout + uses: actions/checkout@v5 + + - name: Set up Bun + uses: oven-sh/setup-bun@v2 + with: + # Matches the oven/bun:1 images in the Dockerfile. + bun-version: "1.x" + + - name: Install dependencies + run: bun install --frozen-lockfile + + # Typecheck, lint, build, unit tests, and the docs stale-claims check. + - name: Verify + run: bun run ci:verify diff --git a/AGENTS.md b/AGENTS.md index 7bd97214..41ab1940 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -12,6 +12,12 @@ Canonical triage roles mapped to GitHub labels; see `docs/agents/triage-labels.m Single-context layout — `CONTEXT.md` and `docs/adr/` at repo root when present. See `docs/agents/domain.md`. +### Package docs + +- Writing an API test (test database, factories, authenticated routes): `packages/api/src/__tests__/README.md` +- Writing a web test (render helper, API mocks, React Query hooks): `packages/web/src/__tests__/README.md` +- Changing the schema, a migration, or a transaction: `packages/api/src/db/README.md` + ### Tests | Package | Command | Runner | @@ -28,9 +34,9 @@ Example (completes in ~1s): cd packages/web && bun run test -- src/components/__tests__/BulkImportFlow.test.tsx ``` -### Pre-commit hooks +### Pre-commit hooks and CI -CI builds the Docker image with `bun run build`, which runs `tsc` on API and web. Type errors that tests miss will fail the nightly build. +The `Checks` workflow runs `bun run ci:verify` on every PR: typecheck, lint, build, unit tests, and the docs stale-claims check. Lint is clean, so any lint error is yours. Install repo hooks once per clone: @@ -38,8 +44,8 @@ Install repo hooks once per clone: bun run setup:hooks ``` -The pre-commit hook runs `typecheck` on `@sampledb/contract`, `@sampledb/api`, and/or `@sampledb/web` when staged files touch those packages (or Docker/build config). To mirror CI fully before pushing: +The pre-commit hook runs `typecheck` on `@sampledb/contract`, `@sampledb/api`, and/or `@sampledb/web` when staged files touch those packages (or Docker/build config), and lints the staged API/web source files. To mirror CI fully before pushing: ```bash -bun run ci:verify # typecheck + build all packages +bun run ci:verify ``` diff --git a/package.json b/package.json index f7943ddb..1285dbf3 100644 --- a/package.json +++ b/package.json @@ -15,7 +15,7 @@ "start": "bun --env-file=.env --env-file=.env.local --filter @sampledb/api start", "lint": "bun --filter '*' lint", "typecheck": "bun --filter '*' typecheck", - "ci:verify": "bun run typecheck && bun run build && bash packages/docs/scripts/check-stale-claims.sh", + "ci:verify": "bun run typecheck && bun run lint && bun run build && bun run test && bash packages/docs/scripts/check-stale-claims.sh", "setup:hooks": "bash .githooks/install.sh", "test": "bun --env-file=.env --env-file=.env.local run --filter @sampledb/contract test && bun --env-file=.env --env-file=.env.local run --filter @sampledb/api test && bun --env-file=.env --env-file=.env.local run --filter @sampledb/web test", "test:coverage": "bun --env-file=.env --env-file=.env.local run --filter @sampledb/api test:coverage && bun --env-file=.env --env-file=.env.local run --filter @sampledb/web test:coverage", From d84fafedbe900b045a1962ae138582477ef4b879 Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Mon, 5 Oct 2026 21:16:49 -0700 Subject: [PATCH 05/30] test(db): fail when initial_schema.sql falls behind schema.ts. New installs apply the snapshot alone, so it must record the current schema version and hold every Drizzle table and column. Against the snapshot before a3bddaa this reports version 6 and the three missing qPCR tables. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../__tests__/initial-schema-snapshot.test.ts | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 packages/api/src/db/__tests__/initial-schema-snapshot.test.ts diff --git a/packages/api/src/db/__tests__/initial-schema-snapshot.test.ts b/packages/api/src/db/__tests__/initial-schema-snapshot.test.ts new file mode 100644 index 00000000..019c50c8 --- /dev/null +++ b/packages/api/src/db/__tests__/initial-schema-snapshot.test.ts @@ -0,0 +1,49 @@ +import { describe, it, expect } from 'vitest' +import { Database } from 'bun:sqlite' +import { is } from 'drizzle-orm' +import { SQLiteTable, getTableConfig } from 'drizzle-orm/sqlite-core' +import * as schema from '../schema' +import { applyInitialSchema } from '../apply-initial-schema' +import { CURRENT_SCHEMA_VERSION, getRecordedSchemaVersion } from '../schema-evolution' + +/** + * New installs get initial_schema.sql and nothing else when its version is current, + * so the snapshot alone must hold every table and column in schema.ts. + */ +describe('initial_schema.sql snapshot', () => { + const sqlite = new Database(':memory:') + applyInitialSchema(sqlite) + + const drizzleTables = Object.values(schema) + .filter((value): value is SQLiteTable => is(value, SQLiteTable)) + .map((table) => getTableConfig(table)) + + it('records the current schema version', () => { + expect(getRecordedSchemaVersion(sqlite)).toBe(CURRENT_SCHEMA_VERSION) + }) + + it('creates every table and column defined in schema.ts', () => { + const missing: string[] = [] + for (const { name, columns } of drizzleTables) { + const snapshotColumns = new Set( + sqlite + .query<{ name: string }, []>(`SELECT name FROM pragma_table_info('${name}')`) + .all() + .map((row) => row.name), + ) + if (snapshotColumns.size === 0) { + missing.push(`table ${name}`) + continue + } + for (const column of columns) { + if (!snapshotColumns.has(column.name)) missing.push(`column ${name}.${column.name}`) + } + } + // Fix by refreshing initial_schema.sql (step 6 in src/db/README.md). + expect(missing).toEqual([]) + }) + + it('finds the schema.ts tables', () => { + expect(drizzleTables.length).toBeGreaterThan(20) + }) +}) From a307f1a8580e9acc86a472bf6be980b7c27349bc Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Mon, 5 Oct 2026 21:17:24 -0700 Subject: [PATCH 06/30] docs(agents): trim the domain doc and add the steps agents kept rediscovering. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - docs/agents/domain.md was the generic template (CONTEXT-MAP.md, ordering/billing example, /grill-with-docs); keep what applies here. - db/README.md: the snapshot test backs step 6; step 7 covers the legacy migration fixtures; note the transaction lint rule. - AGENTS.md: a UI label → code pointer, and state the web test runner positively (its hang warning sent agents to `timeout`, absent on macOS). Co-Authored-By: Claude Opus 5.5 (1M context) --- AGENTS.md | 5 ++-- docs/agents/domain.md | 48 +++++------------------------------ packages/api/src/db/README.md | 5 ++-- 3 files changed, 13 insertions(+), 45 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 41ab1940..0ae94ecc 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -10,13 +10,14 @@ Canonical triage roles mapped to GitHub labels; see `docs/agents/triage-labels.m ### Domain docs -Single-context layout — `CONTEXT.md` and `docs/adr/` at repo root when present. See `docs/agents/domain.md`. +`CONTEXT.md` (glossary) and `docs/adr/` (decisions) at repo root. See `docs/agents/domain.md`. ### Package docs - Writing an API test (test database, factories, authenticated routes): `packages/api/src/__tests__/README.md` - Writing a web test (render helper, API mocks, React Query hooks): `packages/web/src/__tests__/README.md` - Changing the schema, a migration, or a transaction: `packages/api/src/db/README.md` +- Finding the code behind a UI label: `packages/web/src/components/Sidebar.tsx` (label → path) → `packages/web/src/App.tsx` (path → page) → the page's hooks and `lib/` → `packages/api/src/routes` → `packages/api/src/lib` ### Tests @@ -26,7 +27,7 @@ Single-context layout — `CONTEXT.md` and `docs/adr/` at repo root when present | `@sampledb/api` | `cd packages/api && bun test src/...` | **Bun** (needs `bun:sqlite`) | | repo root | `bun run test` | both, via package scripts | -**Web tests must use `bun run test` (Vitest), not `bun test`.** Bun's runner hangs on React component tests (no `vi.importActual`, no jsdom). Piping a hung process to `tail` blocks until EOF — avoid `| tail` on web tests unless you know the runner exits. +**Run web tests with `bun run test` (Vitest).** It runs `vitest run`, which exits on its own, so piping to `tail` or `grep` is safe. Bun's own runner (`bun test`) hangs on React component tests. Example (completes in ~1s): diff --git a/docs/agents/domain.md b/docs/agents/domain.md index 05d38682..1005bb42 100644 --- a/docs/agents/domain.md +++ b/docs/agents/domain.md @@ -1,51 +1,17 @@ # Domain Docs -How the engineering skills should consume this repo's domain documentation when exploring the codebase. +Read these before naming or changing domain concepts: -## Before exploring, read these - -- **`CONTEXT.md`** at the repo root — the canonical domain glossary (entity names, hierarchies, scope). Read this before naming concepts in issues, tests, or refactors. -- **`CONTEXT-MAP.md`** at the repo root if it exists — it points at one `CONTEXT.md` per context. Read each one relevant to the topic. -- **`docs/adr/`** — read ADRs that touch the area you're about to work in. In multi-context repos, also check `src//docs/adr/` for context-scoped decisions. - -`CONTEXT.md` exists in this repo. Start there for vocabulary; use the guides under `packages/docs/` for user-facing workflow detail. - -## File structure - -Single-context repo (most repos): - -``` -/ -├── CONTEXT.md -├── docs/adr/ -│ ├── 0001-event-sourced-orders.md -│ └── 0002-postgres-for-write-model.md -└── src/ -``` - -Multi-context repo (presence of `CONTEXT-MAP.md` at the root): - -``` -/ -├── CONTEXT-MAP.md -├── docs/adr/ ← system-wide decisions -└── src/ - ├── ordering/ - │ ├── CONTEXT.md - │ └── docs/adr/ ← context-specific decisions - └── billing/ - ├── CONTEXT.md - └── docs/adr/ -``` +- **`CONTEXT.md`** is the domain glossary: entity names, hierarchies, and scope. +- **`docs/adr/`** records design decisions. Read the ADRs that touch the area you're changing. +- **`packages/docs/`** holds the user guide, for user-facing workflow detail. ## Use the glossary's vocabulary -When your output names a domain concept (in an issue title, a refactor proposal, a hypothesis, a test name), use the term as defined in `CONTEXT.md`. Don't drift to synonyms the glossary explicitly avoids. - -If the concept you need isn't in the glossary yet, that's a signal — either you're inventing language the project doesn't use (reconsider) or there's a real gap (note it for `/grill-with-docs`). +When your output names a domain concept (an issue title, a test name, a refactor proposal), use the term as `CONTEXT.md` defines it, not a synonym it lists under _Avoid_. If the concept isn't in the glossary, say so rather than inventing a term. ## Flag ADR conflicts -If your output contradicts an existing ADR, surface it explicitly rather than silently overriding: +If your change contradicts an ADR, say so explicitly instead of silently overriding it: -> _Contradicts ADR-0007 (event-sourced orders) — but worth reopening because…_ +> _Contradicts ADR-0006 (unified container inbound write), but worth reopening because…_ diff --git a/packages/api/src/db/README.md b/packages/api/src/db/README.md index a96f3d93..bbefac77 100644 --- a/packages/api/src/db/README.md +++ b/packages/api/src/db/README.md @@ -8,7 +8,7 @@ See [ADR-0003: SQLite schema evolution](../../../docs/adr/0003-sqlite-schema-evo Use `openOperationalDatabase(optionalPath?)` from the client module. It resolves the file path, enables WAL, turns on foreign keys, runs schema evolution, and returns Drizzle + raw `bun:sqlite` handles. Importing the client module does **not** open a connection. -Multi-statement writes that `await` must use `withWriteTransaction` from `write-transaction.ts`. Drizzle's bun-sqlite `db.transaction(async () => …)` commits when the callback first yields; it does not await the returned Promise. +Multi-statement writes that `await` must use `withWriteTransaction` from `write-transaction.ts`. Drizzle's bun-sqlite `db.transaction(async () => …)` commits when the callback first yields; it does not await the returned Promise. ESLint rejects an `async` callback passed to `.transaction(...)`. Tests should use the same entry point (e.g. `openOperationalDatabase(':memory:')` via `setupTestDatabase()`). @@ -35,7 +35,8 @@ Migration `001` is **only** for legacy files; fresh installs from the snapshot s 3. Add `packages/api/src/db/migrations/00N_short_name.sql` (statements separated with `--> statement-breakpoint`). 4. Bump `CURRENT_SCHEMA_VERSION` in `schema-evolution.ts`. 5. Run `bun run build` in `packages/api` — `scripts/runtime-assets.ts` copies migrations into `dist/db/migrations` and verifies the dist layout. If you add other on-disk runtime assets, register them in `RUNTIME_ASSETS` in that script. -6. Refresh `initial_schema.sql` so **new** empty databases match the latest shape (merge generated SQL; keep `INSERT INTO schema_version` at `CURRENT_SCHEMA_VERSION`). +6. Refresh `initial_schema.sql` so **new** empty databases match the latest shape (merge generated SQL; keep `INSERT INTO schema_version` at `CURRENT_SCHEMA_VERSION`). `__tests__/initial-schema-snapshot.test.ts` fails until the snapshot holds every table and column in `schema.ts`. +7. Extend the legacy fixtures if the migration touches a table they don't create. Older-version tests in `__tests__/schema-evolution.test.ts` build minimal databases through `createTablesForLaterMigrations`, and `__tests__/client.test.ts` creates its own tables inline; both must contain every table and column a later migration alters. Do **not** add inline DDL to `open.ts` or connection code. From ac6e111ed18c129584f96858698e85b71b230d3d Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Mon, 5 Oct 2026 21:25:50 -0700 Subject: [PATCH 07/30] fix(web): enforce the React hooks lint rules. eslint-plugin-react-hooks was loaded but none of its rules were on. Turning on rules-of-hooks and exhaustive-deps found 86 problems: - Four components returned early before their hooks (CollectionMove and ControlBatchWizard on !canWrite, ReferenceData on an unknown tab, StatChart on empty data), so a permission or tab change mid-session changed the hook count and React threw. The guards now sit in a thin outer component, or after the hooks. - Effects that deliberately run on a narrower trigger than what they read (load-on-open, debounced search, defaults on type change) read through useEffectEvent, so their triggers are unchanged. - Fallback arrays (`data ?? []`) feeding memos and effects are memoized, so effects such as the scanner-config default stop re-running every render. - The rest: StudyStats bin helpers move to module scope, DataTable's cleanup uses the node it attached to, useCommands memoizes on destructured fields, and the three theme-keyed CSS-variable memos keep theme with a reason. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/web/eslint.config.mjs | 2 + .../components/CollectionMoveTreePicker.tsx | 1 + .../components/ContainerDerivationModal.tsx | 14 +- .../src/components/ContainerRegistration.tsx | 16 ++- .../src/components/ContainerTypeManager.tsx | 6 +- .../components/ContainerTypeUnitsManager.tsx | 8 +- packages/web/src/components/DataTable.tsx | 11 +- .../src/components/DerivationChainView.tsx | 6 +- .../web/src/components/LocationPicker.tsx | 6 +- .../web/src/components/LocationTreePicker.tsx | 6 +- packages/web/src/components/StatChart.tsx | 23 ++-- .../web/src/components/StatisticsFilter.tsx | 13 +- packages/web/src/components/StudyStats.tsx | 128 +++++++++--------- packages/web/src/components/StudyTimeline.tsx | 3 +- .../web/src/components/SubjectMergeModal.tsx | 6 +- .../TableViewConfigurationsManager.tsx | 2 +- .../components/dashboard/SystemInsights.tsx | 1 + .../forms/ControlDefinitionForm.tsx | 41 +++--- .../src/components/wizards/BatchInfoStep.tsx | 2 +- packages/web/src/contexts/ToastContext.tsx | 10 +- packages/web/src/contexts/UserContext.tsx | 2 +- .../web/src/lib/command-registry/registry.ts | 62 ++++++--- .../AdminDataIntegrityEmptyCollections.tsx | 2 +- packages/web/src/pages/AdminUsers.tsx | 2 +- packages/web/src/pages/BagDetail.tsx | 16 ++- packages/web/src/pages/BloodControls.tsx | 4 +- packages/web/src/pages/BoxDetail.tsx | 16 ++- packages/web/src/pages/CollectionMove.tsx | 13 +- packages/web/src/pages/Collections.tsx | 2 +- .../web/src/pages/ContainerMoveMicronix.tsx | 7 +- packages/web/src/pages/ControlBatchWizard.tsx | 16 ++- packages/web/src/pages/LocationDetail.tsx | 5 +- .../web/src/pages/QpcrExperimentDetail.tsx | 4 +- packages/web/src/pages/ReferenceData.tsx | 28 ++-- packages/web/src/pages/SheetDetail.tsx | 2 +- packages/web/src/pages/Studies.tsx | 27 ++-- 36 files changed, 294 insertions(+), 219 deletions(-) diff --git a/packages/web/eslint.config.mjs b/packages/web/eslint.config.mjs index de3116fe..9afeb3da 100644 --- a/packages/web/eslint.config.mjs +++ b/packages/web/eslint.config.mjs @@ -21,6 +21,8 @@ export default [ }, rules: { "@typescript-eslint/no-unnecessary-condition": "error", + "react-hooks/rules-of-hooks": "error", + "react-hooks/exhaustive-deps": "error", }, }, ]; diff --git a/packages/web/src/components/CollectionMoveTreePicker.tsx b/packages/web/src/components/CollectionMoveTreePicker.tsx index b450649c..4492a60c 100644 --- a/packages/web/src/components/CollectionMoveTreePicker.tsx +++ b/packages/web/src/components/CollectionMoveTreePicker.tsx @@ -354,6 +354,7 @@ export default function CollectionMoveTreePicker({ locationChildrenMap, effectiveExpandedIds, collectionsByLocation, + locationsWithCollections, selectedKeys, leafLocations, search, diff --git a/packages/web/src/components/ContainerDerivationModal.tsx b/packages/web/src/components/ContainerDerivationModal.tsx index c44236a0..1f1b92c0 100644 --- a/packages/web/src/components/ContainerDerivationModal.tsx +++ b/packages/web/src/components/ContainerDerivationModal.tsx @@ -143,12 +143,14 @@ function ContainerDerivationModalContent({ const containerTypes = response.containerTypes setAllowedContainerTypes(containerTypes) - if (!containerTypes.includes(formData.containerType)) { - setFormData(prev => ({ - ...prev, - containerType: containerTypes.length > 0 ? (containerTypes[0] as FlatDerivationContainerForm['containerType']) : 'micronix_tube', - })) - } + setFormData(prev => + containerTypes.includes(prev.containerType) + ? prev + : { + ...prev, + containerType: containerTypes.length > 0 ? (containerTypes[0] as FlatDerivationContainerForm['containerType']) : 'micronix_tube', + }, + ) } catch (err: unknown) { if (!ignore) { console.error('Failed to fetch allowed container types:', err) diff --git a/packages/web/src/components/ContainerRegistration.tsx b/packages/web/src/components/ContainerRegistration.tsx index 7cf57409..f2dd06de 100644 --- a/packages/web/src/components/ContainerRegistration.tsx +++ b/packages/web/src/components/ContainerRegistration.tsx @@ -1,4 +1,4 @@ -import { useState, useEffect, useRef } from 'react' +import { useState, useEffect, useRef, useEffectEvent } from 'react' import CollectionSelectOrCreate, { type CollectionOption, type CollectionSelectValue, @@ -140,8 +140,10 @@ export default function ContainerRegistration({ setContainerType(first) setFormData((prev) => ({ ...prev, containerType: first })) } - }, [allowedContainerTypes]) + }, [allowedContainerTypes, containerType]) + // Read at use time, so a unit picked while defaults load is kept. + const hasChosenUnit = useEffectEvent(() => Boolean(selectedUnitId || defaultValue?.unitId)) useEffect(() => { const cancelled = { current: false } loadUnits() @@ -158,7 +160,7 @@ export default function ContainerRegistration({ totalQuantity: typeDefaults.totalQuantity, remainingQuantity: typeDefaults.remainingQuantity, }) - if (!selectedUnitId && !defaultValue?.unitId) { + if (!hasChosenUnit()) { const resUnits = await unitsApi.listAll() // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- effect cleanup can set cancelled.current between awaits if (cancelled.current) return @@ -215,7 +217,7 @@ export default function ContainerRegistration({ } }, [effectiveCollectionType, containerType, paperParentType]) - const loadUnits = async () => { + const loadUnits = useEffectEvent(async () => { try { setUnitsError(null) // Load units filtered by container type @@ -228,7 +230,7 @@ export default function ContainerRegistration({ console.error('Failed to load units:', err) // Don't fallback - show error to user instead } - } + }) const isInitialMount = useRef(true) // When container type or unit changes (after mount), clear quantity fields so placeholder (default) text shows again @@ -282,7 +284,7 @@ export default function ContainerRegistration({ return isValid } - const updateContainerData = () => { + const updateContainerData = useEffectEvent(() => { const data: ContainerData = { ...formData, unitId: selectedUnitId, @@ -293,7 +295,7 @@ export default function ContainerRegistration({ } else { onChange(null) } - } + }) const handleFieldChange = (field: keyof ContainerData, value: any) => { setFormData((prev) => { diff --git a/packages/web/src/components/ContainerTypeManager.tsx b/packages/web/src/components/ContainerTypeManager.tsx index 97db3f76..6e601450 100644 --- a/packages/web/src/components/ContainerTypeManager.tsx +++ b/packages/web/src/components/ContainerTypeManager.tsx @@ -1,4 +1,4 @@ -import { useState, useEffect } from 'react' +import { useState, useEffect, useEffectEvent } from 'react' import { specimenTypesApi } from '../lib/api/reference-data'; import { getContainerTypeName } from '../lib/icons' import { useHotkey } from '../hooks/useHotkey' @@ -53,7 +53,7 @@ export default function ContainerTypeManager({ } }, [isOpen, specimenTypeId]) - const loadContainerTypes = async () => { + const loadContainerTypes = useEffectEvent(async () => { setLoading(true) setError(null) try { @@ -67,7 +67,7 @@ export default function ContainerTypeManager({ } finally { setLoading(false) } - } + }) const handleToggle = (containerType: string) => { setSelectedContainerTypes((prev) => { diff --git a/packages/web/src/components/ContainerTypeUnitsManager.tsx b/packages/web/src/components/ContainerTypeUnitsManager.tsx index 31485b56..1c6a31e2 100644 --- a/packages/web/src/components/ContainerTypeUnitsManager.tsx +++ b/packages/web/src/components/ContainerTypeUnitsManager.tsx @@ -1,4 +1,4 @@ -import { useState, useEffect } from 'react' +import { useState, useEffect, useEffectEvent } from 'react' import { settingsApi } from '../lib/api/settings'; import { unitsApi } from '../lib/api/reference-data'; import type { ContainerDefaults } from '../lib/api/settings'; @@ -52,8 +52,12 @@ export default function ContainerTypeUnitsManager({ // All available units const [allUnits, setAllUnits] = useState([]) + // loadData is also called from handlers, so the mount effect goes through an Effect Event. + const loadInitialData = useEffectEvent(() => { + void loadData() + }) useEffect(() => { - loadData() + loadInitialData() }, []) const loadData = async () => { diff --git a/packages/web/src/components/DataTable.tsx b/packages/web/src/components/DataTable.tsx index eef162a7..9d4f0521 100644 --- a/packages/web/src/components/DataTable.tsx +++ b/packages/web/src/components/DataTable.tsx @@ -156,14 +156,15 @@ export default function DataTable({ } // Make table focusable - if (tableRef.current) { - tableRef.current.setAttribute('tabindex', '0') - tableRef.current.addEventListener('keydown', handleKeyDown) + const table = tableRef.current + if (table) { + table.setAttribute('tabindex', '0') + table.addEventListener('keydown', handleKeyDown) } return () => { - if (tableRef.current) { - tableRef.current.removeEventListener('keydown', handleKeyDown) + if (table) { + table.removeEventListener('keydown', handleKeyDown) } } }, [paginatedData, onRowClick, selectedRowIndex]) diff --git a/packages/web/src/components/DerivationChainView.tsx b/packages/web/src/components/DerivationChainView.tsx index ebade47b..c8d49b7e 100644 --- a/packages/web/src/components/DerivationChainView.tsx +++ b/packages/web/src/components/DerivationChainView.tsx @@ -1,4 +1,4 @@ -import { useEffect, useState } from 'react' +import { useEffect, useState, useEffectEvent } from 'react' import { useNavigate } from 'react-router-dom' import type { EnrichedContainerWire } from '@sampledb/contract/wire' import { derivationsApi } from '../lib/api/derivations' @@ -29,7 +29,7 @@ export default function DerivationChainView({ containerId, onClose }: Derivation loadChain() }, [containerId]) - const loadChain = async () => { + const loadChain = useEffectEvent(async () => { try { setLoading(true) setError(null) @@ -41,7 +41,7 @@ export default function DerivationChainView({ containerId, onClose }: Derivation } finally { setLoading(false) } - } + }) if (loading) { return ( diff --git a/packages/web/src/components/LocationPicker.tsx b/packages/web/src/components/LocationPicker.tsx index c4b5bb71..47d8d538 100644 --- a/packages/web/src/components/LocationPicker.tsx +++ b/packages/web/src/components/LocationPicker.tsx @@ -1,4 +1,4 @@ -import { useState, useEffect, useMemo, useRef } from 'react' +import { useState, useEffect, useMemo, useRef, useEffectEvent } from 'react' import { locationsApi } from '../lib/api/locations'; import type { Location } from '../lib/api/types'; import { buildLocationTree, filterLocationTree, getLocationLabel, getRootLocations, getLocationChildren, getLocationAncestors, locationParentId } from '../lib/location-tree' @@ -46,7 +46,7 @@ export default function LocationPicker({ value, onChange, filterCollectionsOnly }) } - const loadLocations = async () => { + const loadLocations = useEffectEvent(async () => { try { setLoading(true) // Call without pagination params to get all locations @@ -97,7 +97,7 @@ export default function LocationPicker({ value, onChange, filterCollectionsOnly } finally { setLoading(false) } - } + }) const tree = useMemo(() => buildLocationTree(locations), [locations]) const filteredTree = useMemo( diff --git a/packages/web/src/components/LocationTreePicker.tsx b/packages/web/src/components/LocationTreePicker.tsx index 5cd8eb6c..cc424f2a 100644 --- a/packages/web/src/components/LocationTreePicker.tsx +++ b/packages/web/src/components/LocationTreePicker.tsx @@ -1,4 +1,4 @@ -import { useState, useEffect, useMemo, useCallback, useRef } from 'react' +import { useState, useEffect, useMemo, useCallback, useRef, useEffectEvent } from 'react' import { locationsApi } from '../lib/api/locations'; import type { Location } from '../lib/api/types'; import { @@ -38,7 +38,7 @@ export default function LocationTreePicker({ selected, onChange, filterCollectio } }, [open]) - const loadLocations = async () => { + const loadLocations = useEffectEvent(async () => { try { setLoading(true) // Fetch all locations in a single request (no pagination params) @@ -65,7 +65,7 @@ export default function LocationTreePicker({ selected, onChange, filterCollectio } finally { setLoading(false) } - } + }) const locationChildrenMap = useMemo( () => buildLocationChildrenMap(locations), diff --git a/packages/web/src/components/StatChart.tsx b/packages/web/src/components/StatChart.tsx index af1ec569..d37daebd 100644 --- a/packages/web/src/components/StatChart.tsx +++ b/packages/web/src/components/StatChart.tsx @@ -49,19 +49,9 @@ export default function StatChart({ cardClassName = 'bg-app-card rounded-lg shadow p-6', }: StatChartProps) { const { theme } = useTheme() + // eslint-disable-next-line react-hooks/exhaustive-deps -- reads CSS variables; theme changes them const tooltipStyles = useMemo(() => getAppTooltipStyles(), [theme]) - if (data.length === 0) { - return ( -
- {title &&

{title}

} -
- No data available -
-
- ) - } - // Calculate total for percentage calculations const total = data.reduce((sum, entry) => sum + (entry[dataKey] as number), 0) @@ -137,6 +127,17 @@ export default function StatChart({ return [min - padding, max + padding] }, [data, dateKey]) + if (data.length === 0) { + return ( +
+ {title &&

{title}

} +
+ No data available +
+
+ ) + } + // Format date ticks for display const formatDateTick = (tickItem: number) => { if (!dateKey) return String(tickItem) diff --git a/packages/web/src/components/StatisticsFilter.tsx b/packages/web/src/components/StatisticsFilter.tsx index d2da79fd..aab5dd0c 100644 --- a/packages/web/src/components/StatisticsFilter.tsx +++ b/packages/web/src/components/StatisticsFilter.tsx @@ -1,4 +1,4 @@ -import { useState, useEffect, useMemo, useRef } from 'react' +import { useState, useEffect, useMemo, useRef, useEffectEvent } from 'react' import { studiesApi } from '../lib/api/studies'; import type { Study } from '../lib/api/studies'; import { specimenTypesApi } from '../lib/api/reference-data'; @@ -96,15 +96,18 @@ export default function StatisticsFilter({ filters, onChange, onSubmit, isLoadin } }, [studyPickerOpen]) + // Only a search change should restart the debounce; opening the picker loads above. + const reloadForSearch = useEffectEvent(() => { + if (studyPickerOpen) void loadStudies() + }) useEffect(() => { - if (!studyPickerOpen) return const timeout = setTimeout(() => { - void loadStudies() + reloadForSearch() }, 300) return () => clearTimeout(timeout) }, [studySearch]) - const loadStudies = async () => { + const loadStudies = useEffectEvent(async () => { try { setStudyLoading(true) const response = await studiesApi.list(studySearch || undefined) @@ -114,7 +117,7 @@ export default function StatisticsFilter({ filters, onChange, onSubmit, isLoadin } finally { setStudyLoading(false) } - } + }) const selectedStudy = studies.find((s) => s.shortCode === localFilters.study) diff --git a/packages/web/src/components/StudyStats.tsx b/packages/web/src/components/StudyStats.tsx index 51505a98..565ea880 100644 --- a/packages/web/src/components/StudyStats.tsx +++ b/packages/web/src/components/StudyStats.tsx @@ -16,6 +16,69 @@ interface StudyStatsProps { type BinSize = 'day' | 'week' | 'month' | 'quarter' | 'year' +// Helper function to get ISO week number for a date +function getISOWeek(date: Date): { year: number; week: number } { + const d = new Date(Date.UTC(date.getFullYear(), date.getMonth(), date.getDate())) + const dayNum = d.getUTCDay() || 7 + d.setUTCDate(d.getUTCDate() + 4 - dayNum) + const yearStart = new Date(Date.UTC(d.getUTCFullYear(), 0, 1)) + const weekNo = Math.ceil((((d.getTime() - yearStart.getTime()) / 86400000) + 1) / 7) + return { year: d.getUTCFullYear(), week: weekNo } +} + +// Helper function to get bin key for a date based on bin size +const getBinKey = (dateString: string, size: BinSize): string => { + const date = new Date(dateString) + const year = date.getFullYear() + const month = date.getMonth() + 1 + const day = date.getDate() + + switch (size) { + case 'day': + return `${year}-${String(month).padStart(2, '0')}-${String(day).padStart(2, '0')}` + case 'week': { + // Get ISO week number + const { year: weekYear, week } = getISOWeek(date) + return `${weekYear}-W${String(week).padStart(2, '0')}` + } + case 'month': + return `${year}-${String(month).padStart(2, '0')}` + case 'quarter': { + const quarter = Math.floor((month - 1) / 3) + 1 + return `${year}-Q${quarter}` + } + case 'year': + return String(year) + default: + return dateString.split('T')[0] + } +} + +// Helper function to format bin key for display +const formatBinKey = (key: string, size: BinSize): string => { + switch (size) { + case 'day': { + const date = new Date(key) + return date.toLocaleDateString('en-US', { month: 'short', day: 'numeric', year: 'numeric' }) + } + case 'week': { + const [year, week] = key.split('-W') + return `${year} Week ${week}` + } + case 'month': { + const [year, month] = key.split('-') + const date = new Date(parseInt(year), parseInt(month) - 1, 1) + return date.toLocaleDateString('en-US', { month: 'short', year: 'numeric' }) + } + case 'quarter': + return key.replace('-Q', ' Q') + case 'year': + return key + default: + return key + } +} + export default function StudyStats({ summary, timelineData, statCardClassName, cardClassName }: StudyStatsProps) { const { settings } = useDateFilter() const { minDate, maxDate } = settings @@ -54,69 +117,6 @@ export default function StudyStats({ summary, timelineData, statCardClassName, c return filteredSpecimens }, [timelineData, minDate, maxDate]) - // Helper function to get ISO week number for a date - const getISOWeek = (date: Date): { year: number; week: number } => { - const d = new Date(Date.UTC(date.getFullYear(), date.getMonth(), date.getDate())) - const dayNum = d.getUTCDay() || 7 - d.setUTCDate(d.getUTCDate() + 4 - dayNum) - const yearStart = new Date(Date.UTC(d.getUTCFullYear(), 0, 1)) - const weekNo = Math.ceil((((d.getTime() - yearStart.getTime()) / 86400000) + 1) / 7) - return { year: d.getUTCFullYear(), week: weekNo } - } - - // Helper function to get bin key for a date based on bin size - const getBinKey = (dateString: string, size: BinSize): string => { - const date = new Date(dateString) - const year = date.getFullYear() - const month = date.getMonth() + 1 - const day = date.getDate() - - switch (size) { - case 'day': - return `${year}-${String(month).padStart(2, '0')}-${String(day).padStart(2, '0')}` - case 'week': { - // Get ISO week number - const { year: weekYear, week } = getISOWeek(date) - return `${weekYear}-W${String(week).padStart(2, '0')}` - } - case 'month': - return `${year}-${String(month).padStart(2, '0')}` - case 'quarter': { - const quarter = Math.floor((month - 1) / 3) + 1 - return `${year}-Q${quarter}` - } - case 'year': - return String(year) - default: - return dateString.split('T')[0] - } - } - - // Helper function to format bin key for display - const formatBinKey = (key: string, size: BinSize): string => { - switch (size) { - case 'day': { - const date = new Date(key) - return date.toLocaleDateString('en-US', { month: 'short', day: 'numeric', year: 'numeric' }) - } - case 'week': { - const [year, week] = key.split('-W') - return `${year} Week ${week}` - } - case 'month': { - const [year, month] = key.split('-') - const date = new Date(parseInt(year), parseInt(month) - 1, 1) - return date.toLocaleDateString('en-US', { month: 'short', year: 'numeric' }) - } - case 'quarter': - return key.replace('-Q', ' Q') - case 'year': - return key - default: - return key - } - } - // Check if date filtering is reducing the specimen count const isFilteringSpecimens = useMemo(() => { if (!filteredData || !timelineData) return false @@ -213,7 +213,7 @@ export default function StudyStats({ summary, timelineData, statCardClassName, c // Keep enrollment timeline from original summary (it's based on subject creation dates, not filtered) enrollmentTimeline: summary.enrollmentTimeline, } - }, [summary, filteredData, timelineData, minDate, maxDate, binSize]) + }, [summary, filteredData, timelineData, minDate, maxDate]) // Transform data for charts using filtered summary const specimenTypeChartData = useMemo(() => { return displaySummary.specimenTypes.map((type) => ({ diff --git a/packages/web/src/components/StudyTimeline.tsx b/packages/web/src/components/StudyTimeline.tsx index cca48d4b..c32f5cc2 100644 --- a/packages/web/src/components/StudyTimeline.tsx +++ b/packages/web/src/components/StudyTimeline.tsx @@ -353,6 +353,7 @@ export default function StudyTimeline({ data }: StudyTimelineProps) { .style('font-size', '14px') .style('text-anchor', 'middle') .text('Subject') + // eslint-disable-next-line react-hooks/exhaustive-deps -- reads CSS variables; theme changes them }, [theme, allPoints, dateRange, filteredSubjects, yAxisLabels, seriesData]) // Handle mouse move for tooltip @@ -500,7 +501,7 @@ export default function StudyTimeline({ data }: StudyTimelineProps) { const point = nearestPoint as ScatterDataPoint navigate(`/specimens/${point.specimenId}`) } - }, [dateRange, filteredSubjects, spatialIndex, navigate]) + }, [allPoints.length, dateRange, filteredSubjects, spatialIndex, navigate]) // Set up canvas and event listeners useEffect(() => { diff --git a/packages/web/src/components/SubjectMergeModal.tsx b/packages/web/src/components/SubjectMergeModal.tsx index 33c0b6ec..51bcd788 100644 --- a/packages/web/src/components/SubjectMergeModal.tsx +++ b/packages/web/src/components/SubjectMergeModal.tsx @@ -1,4 +1,4 @@ -import { useState, useEffect, useRef } from 'react' +import { useState, useEffect, useRef, useEffectEvent } from 'react' import { subjectsApi } from '../lib/api/subjects'; import { studiesApi } from '../lib/api/studies'; import type { StudySubject } from '../lib/api/types'; @@ -88,7 +88,7 @@ function SubjectMergeModalContent({ } } - const loadSubjects = async () => { + const loadSubjects = useEffectEvent(async () => { if (!studyId) { setError('Study ID is required') setLoading(false) @@ -114,7 +114,7 @@ function SubjectMergeModalContent({ } finally { setLoading(false) } - } + }) const handleMerge = async () => { if (!sourceSubjectId || !targetSubjectId) { diff --git a/packages/web/src/components/TableViewConfigurationsManager.tsx b/packages/web/src/components/TableViewConfigurationsManager.tsx index 66f37f28..72b436bd 100644 --- a/packages/web/src/components/TableViewConfigurationsManager.tsx +++ b/packages/web/src/components/TableViewConfigurationsManager.tsx @@ -39,7 +39,7 @@ export default function TableViewConfigurationsManager({ setLoading(true) try { const configsValue = await settingsApi.getValue('table_view_configurations') - const configs = configsValue?.configurations ?? [] // eslint-disable-line @typescript-eslint/no-unnecessary-condition + const configs = configsValue?.configurations ?? [] setConfigurations(configs) } catch (err: unknown) { const msg = diff --git a/packages/web/src/components/dashboard/SystemInsights.tsx b/packages/web/src/components/dashboard/SystemInsights.tsx index e40f0f8a..e53ae96e 100644 --- a/packages/web/src/components/dashboard/SystemInsights.tsx +++ b/packages/web/src/components/dashboard/SystemInsights.tsx @@ -13,6 +13,7 @@ interface SystemInsightsProps { export default function SystemInsights({ data, loading }: SystemInsightsProps) { const { theme } = useTheme() + // eslint-disable-next-line react-hooks/exhaustive-deps -- reads CSS variables; theme changes them const chartColors = useMemo(() => getAppChartColors(), [theme]) if (loading || !data) { diff --git a/packages/web/src/components/forms/ControlDefinitionForm.tsx b/packages/web/src/components/forms/ControlDefinitionForm.tsx index 76e01c44..af2d1fb8 100644 --- a/packages/web/src/components/forms/ControlDefinitionForm.tsx +++ b/packages/web/src/components/forms/ControlDefinitionForm.tsx @@ -1,4 +1,4 @@ -import { useState, useRef, useEffect } from 'react' +import { useState, useRef, useEffect, useEffectEvent } from 'react' import { controlsApi } from '../../lib/api/controls'; import type { ControlDefinition } from '../../lib/api/controls'; import { strainsApi, unitsApi } from '../../lib/api/reference-data'; @@ -81,7 +81,7 @@ export default function ControlDefinitionForm({ controlDefinition: propControlDe } // Sync form when controlDefinition is set from fetch (in loadControlDefinition) - const loadControlDefinition = async () => { + const loadControlDefinition = useEffectEvent(async () => { if (!id) return try { const res = await controlsApi.getDefinitionSummary(parseInt(id)) @@ -111,7 +111,7 @@ export default function ControlDefinitionForm({ controlDefinition: propControlDe console.error('Failed to load control definition:', err) setError('Failed to load control definition') } - } + }) const loadStrains = async () => { try { @@ -148,15 +148,16 @@ export default function ControlDefinitionForm({ controlDefinition: propControlDe strainInputs.length > 0 && Math.abs(strainInputs.reduce((sum, s) => sum + s.percentage, 0) - 100) < 0.01 - useEffect(() => { - if (!canSuggestNames) { - setDefinitionNames([]) - return - } - let cancelled = false + // Refetch only when the values behind the names change, not on every new array. + const targetDensitiesKey = targetDensitiesForNames.join(',') + const strainsKey = strainInputs + .map((s) => `${s.strainId}:${s.percentage}`) + .sort() + .join(',') + const fetchSuggestedNames = useEffectEvent(() => { const unitId = formData.targetDensityUnitId ? parseInt(formData.targetDensityUnitId) : undefined const strainsPayload = strainInputs.map((s) => ({ strainId: s.strainId, percentage: s.percentage })) - Promise.all( + return Promise.all( targetDensitiesForNames.map((d) => controlsApi .suggestName({ @@ -167,21 +168,21 @@ export default function ControlDefinitionForm({ controlDefinition: propControlDe }) .then((r) => r.suggestedName) ) - ).then((names) => { + ) + }) + useEffect(() => { + if (!canSuggestNames) { + setDefinitionNames([]) + return + } + let cancelled = false + void fetchSuggestedNames().then((names) => { if (!cancelled) setDefinitionNames(names) }) return () => { cancelled = true } - }, [ - canSuggestNames, - formData.targetDensityUnitId, - targetDensitiesForNames.join(','), - strainInputs - .map((s) => `${s.strainId}:${s.percentage}`) - .sort() - .join(','), - ]) + }, [canSuggestNames, formData.targetDensityUnitId, targetDensitiesKey, strainsKey]) const handleAddStrain = () => { const availableStrains = strains.filter((s) => !strainInputs.some((si) => si.strainId === s.id)) diff --git a/packages/web/src/components/wizards/BatchInfoStep.tsx b/packages/web/src/components/wizards/BatchInfoStep.tsx index 13556866..f9a888f5 100644 --- a/packages/web/src/components/wizards/BatchInfoStep.tsx +++ b/packages/web/src/components/wizards/BatchInfoStep.tsx @@ -79,7 +79,7 @@ export default function BatchInfoStep({ if (def && batchInfo.controlDefinitionId && !batchInfo.controlDefinition) { onChange({ ...batchInfo, controlDefinition: def }) } - }, [definitionDetailQuery.data, batchInfo.controlDefinitionId, batchInfo.controlDefinition, onChange]) + }, [definitionDetailQuery.data, batchInfo, onChange]) // Client-side filter by search, strains, and density const filteredDefinitions = useMemo(() => { diff --git a/packages/web/src/contexts/ToastContext.tsx b/packages/web/src/contexts/ToastContext.tsx index 268ca3df..9810c458 100644 --- a/packages/web/src/contexts/ToastContext.tsx +++ b/packages/web/src/contexts/ToastContext.tsx @@ -36,6 +36,10 @@ interface ToastProviderProps { export function ToastProvider({ children }: ToastProviderProps) { const [toasts, setToasts] = useState([]) + const removeToast = useCallback((id: string) => { + setToasts((prev) => prev.filter((toast) => toast.id !== id)) + }, []) + const addToast = useCallback((message: string, type: ToastType = 'info', duration = 5000) => { const id = `toast_${Date.now()}_${Math.random().toString(36).substr(2, 9)}` const toast: Toast = { id, message, type, duration } @@ -48,11 +52,7 @@ export function ToastProvider({ children }: ToastProviderProps) { removeToast(id) }, duration) } - }, []) - - const removeToast = useCallback((id: string) => { - setToasts((prev) => prev.filter((toast) => toast.id !== id)) - }, []) + }, [removeToast]) const success = useCallback((message: string, duration?: number) => { addToast(message, 'success', duration) diff --git a/packages/web/src/contexts/UserContext.tsx b/packages/web/src/contexts/UserContext.tsx index fc318211..4ff9e238 100644 --- a/packages/web/src/contexts/UserContext.tsx +++ b/packages/web/src/contexts/UserContext.tsx @@ -138,7 +138,7 @@ export function UserProvider({ children }: { children: ReactNode }) { setLoading(userCachedLoading) setError(userCachedError) } - }, [refreshUser]) + }, [refreshUser, setUser]) // Permission helpers const canWrite = useMemo(() => user?.role === 'admin' || user?.role === 'member', [user]) diff --git a/packages/web/src/lib/command-registry/registry.ts b/packages/web/src/lib/command-registry/registry.ts index 8235bc62..a6f78839 100644 --- a/packages/web/src/lib/command-registry/registry.ts +++ b/packages/web/src/lib/command-registry/registry.ts @@ -21,25 +21,53 @@ export function buildAllCommands(deps: CommandDependencies): Command[] { ] } -export function useCommands(deps: CommandDependencies) { +export function useCommands({ + navigate, + location, + canWrite, + isAdmin, + canManageReferenceData, + theme, + setTheme, + toggleHelpModal, + openSearchModal, + refreshUser, + handleExportSpecimens, + handleExportInventory, + handleClearFilters, +}: CommandDependencies) { + // The caller builds a new deps object each render, so memoize on its fields. return useMemo( - () => buildAllCommands(deps), + () => + buildAllCommands({ + navigate, + location, + canWrite, + isAdmin, + canManageReferenceData, + theme, + setTheme, + toggleHelpModal, + openSearchModal, + refreshUser, + handleExportSpecimens, + handleExportInventory, + handleClearFilters, + }), [ - deps.navigate, - deps.location.pathname, - deps.location.search, - deps.location.key, - deps.canWrite, - deps.isAdmin, - deps.canManageReferenceData, - deps.theme, - deps.setTheme, - deps.toggleHelpModal, - deps.openSearchModal, - deps.refreshUser, - deps.handleExportSpecimens, - deps.handleExportInventory, - deps.handleClearFilters, + navigate, + location, + canWrite, + isAdmin, + canManageReferenceData, + theme, + setTheme, + toggleHelpModal, + openSearchModal, + refreshUser, + handleExportSpecimens, + handleExportInventory, + handleClearFilters, ] ) } diff --git a/packages/web/src/pages/AdminDataIntegrityEmptyCollections.tsx b/packages/web/src/pages/AdminDataIntegrityEmptyCollections.tsx index d4f72652..db3d2790 100644 --- a/packages/web/src/pages/AdminDataIntegrityEmptyCollections.tsx +++ b/packages/web/src/pages/AdminDataIntegrityEmptyCollections.tsx @@ -49,7 +49,7 @@ export default function AdminDataIntegrityEmptyCollections() { const queryClient = useQueryClient() const reportQuery = useAdminIntegrityReport() const reportStatus = fromQuery(reportQuery) - const collections = reportQuery.data?.emptyCollections ?? [] + const collections = useMemo(() => reportQuery.data?.emptyCollections ?? [], [reportQuery.data]) const [mutationError, setMutationError] = useState(null) const [selected, setSelected] = useState>(new Set()) const [showDeleteModal, setShowDeleteModal] = useState(false) diff --git a/packages/web/src/pages/AdminUsers.tsx b/packages/web/src/pages/AdminUsers.tsx index d68d4756..ada126bc 100644 --- a/packages/web/src/pages/AdminUsers.tsx +++ b/packages/web/src/pages/AdminUsers.tsx @@ -48,7 +48,7 @@ export default function AdminUsers() { const [showPassword, setShowPassword] = useState(false) const usersQuery = useAdminUsers(showDeleted) - const users = usersQuery.data ?? [] + const users = useMemo(() => usersQuery.data ?? [], [usersQuery.data]) const listStatus = fromQuery(usersQuery, { isEmpty: usersQuery.isSuccess && users.length === 0, }) diff --git a/packages/web/src/pages/BagDetail.tsx b/packages/web/src/pages/BagDetail.tsx index ab37a734..45795111 100644 --- a/packages/web/src/pages/BagDetail.tsx +++ b/packages/web/src/pages/BagDetail.tsx @@ -40,11 +40,15 @@ export default function BagDetail() { // Initialize expanded sheets (default to collapsed if more than 3 sheets) // This hook must be called before any conditional returns to maintain hook order - const sheets = (data?.contents.sheets || []) as Array<{ - id: number - name?: string - papers?: CollectionTableEntry[] - }> + const sheets = useMemo( + () => + (data?.contents.sheets || []) as Array<{ + id: number + name?: string + papers?: CollectionTableEntry[] + }>, + [data], + ) useEffect(() => { if (sheets.length > 0 && !initializedSheets.current) { initializedSheets.current = true @@ -53,7 +57,7 @@ export default function BagDetail() { setExpandedSheets(new Set(sheets.map((s: any) => s.id))) } } - }, [sheets.length]) + }, [sheets]) const tableRows = useMemo(() => { const bag = data?.bag diff --git a/packages/web/src/pages/BloodControls.tsx b/packages/web/src/pages/BloodControls.tsx index c4794d16..f20d94ee 100644 --- a/packages/web/src/pages/BloodControls.tsx +++ b/packages/web/src/pages/BloodControls.tsx @@ -26,8 +26,8 @@ export default function BloodControls() { const navigate = useNavigate() const { canWrite } = useUser() const overviewQuery = useBloodControlsOverview() - const definitions = overviewQuery.data?.definitions ?? [] - const batches = overviewQuery.data?.batches ?? [] + const definitions = useMemo(() => overviewQuery.data?.definitions ?? [], [overviewQuery.data]) + const batches = useMemo(() => overviewQuery.data?.batches ?? [], [overviewQuery.data]) const strains = overviewQuery.data?.strains ?? [] const overviewStatus = fromQuery(overviewQuery) const [searchParams, setSearchParams] = useSearchParams() diff --git a/packages/web/src/pages/BoxDetail.tsx b/packages/web/src/pages/BoxDetail.tsx index 25776541..b9549b08 100644 --- a/packages/web/src/pages/BoxDetail.tsx +++ b/packages/web/src/pages/BoxDetail.tsx @@ -40,11 +40,15 @@ export default function BoxDetail() { // Initialize expanded sheets (default to collapsed if more than 3 sheets) // This hook must be called before any conditional returns to maintain hook order - const sheets = (data?.contents.sheets || []) as Array<{ - id: number - name?: string - papers?: CollectionTableEntry[] - }> + const sheets = useMemo( + () => + (data?.contents.sheets || []) as Array<{ + id: number + name?: string + papers?: CollectionTableEntry[] + }>, + [data], + ) useEffect(() => { if (sheets.length > 0 && !initializedSheets.current) { initializedSheets.current = true @@ -53,7 +57,7 @@ export default function BoxDetail() { setExpandedSheets(new Set(sheets.map((s: any) => s.id))) } } - }, [sheets.length]) + }, [sheets]) const tableRows = useMemo(() => { const box = data?.box diff --git a/packages/web/src/pages/CollectionMove.tsx b/packages/web/src/pages/CollectionMove.tsx index cd02aa40..d70f9328 100644 --- a/packages/web/src/pages/CollectionMove.tsx +++ b/packages/web/src/pages/CollectionMove.tsx @@ -26,20 +26,25 @@ function fromKey(key: string): { type: CollectionType; id: number } { export default function CollectionMove() { const navigate = useNavigate() - const queryClient = useQueryClient() const { canWrite } = useUser() - const [currentStep, setCurrentStep] = useState('select-collections') - + // Redirect if user doesn't have write permissions useEffect(() => { if (!canWrite) { navigate('/', { replace: true }) } }, [canWrite, navigate]) - + if (!canWrite) { return null } + return +} + +/** Split out so its hooks run on every render, whatever canWrite is. */ +function CollectionMoveContent() { + const queryClient = useQueryClient() + const [currentStep, setCurrentStep] = useState('select-collections') const [mutating, setMutating] = useState(false) const locationsQuery = useLocationsList() const collectionsQuery = useAllCollections() diff --git a/packages/web/src/pages/Collections.tsx b/packages/web/src/pages/Collections.tsx index 7253c0af..5562e195 100644 --- a/packages/web/src/pages/Collections.tsx +++ b/packages/web/src/pages/Collections.tsx @@ -66,7 +66,7 @@ export default function Collections() { ) const collectionsQuery = useAllCollections() - const collections = collectionsQuery.data ?? [] + const collections = useMemo(() => collectionsQuery.data ?? [], [collectionsQuery.data]) const listStatus = fromQuery(collectionsQuery, { isEmpty: collectionsQuery.isSuccess && collections.length === 0, }) diff --git a/packages/web/src/pages/ContainerMoveMicronix.tsx b/packages/web/src/pages/ContainerMoveMicronix.tsx index 441da23c..59b727a8 100644 --- a/packages/web/src/pages/ContainerMoveMicronix.tsx +++ b/packages/web/src/pages/ContainerMoveMicronix.tsx @@ -1,4 +1,4 @@ -import { useState, useEffect, useRef, useCallback } from 'react' +import { useState, useEffect, useRef, useCallback, useMemo } from 'react' import { Navigate } from 'react-router-dom' import { useMicronixMoveBootstrap, moveWorkflowKeys } from '../hooks/useMoveWorkflow' import { useScanMoveWorkflow } from '../hooks/useScanMoveWorkflow' @@ -20,7 +20,10 @@ export default function ContainerMoveMicronix() { const bootstrapStatus = fromQuery(bootstrapQuery) const availablePlates = (bootstrapQuery.data?.plates ?? []) as MicronixPlate[] const locations = bootstrapQuery.data?.locations ?? [] - const scannerConfigurations = bootstrapQuery.data?.scannerConfigurations ?? [] + const scannerConfigurations = useMemo( + () => bootstrapQuery.data?.scannerConfigurations ?? [], + [bootstrapQuery.data], + ) const refreshCollections = useCallback(async () => { await queryClient.invalidateQueries({ queryKey: moveWorkflowKeys.micronixBootstrap() }) diff --git a/packages/web/src/pages/ControlBatchWizard.tsx b/packages/web/src/pages/ControlBatchWizard.tsx index 8508afb0..2e7ccb45 100644 --- a/packages/web/src/pages/ControlBatchWizard.tsx +++ b/packages/web/src/pages/ControlBatchWizard.tsx @@ -105,9 +105,17 @@ export function canProceedToReview( } export default function ControlBatchWizard() { + const { canWrite } = useUser() + if (!canWrite) { + return + } + return +} + +/** Split out so its hooks run on every render, whatever canWrite is. */ +function ControlBatchWizardContent() { const navigate = useNavigate() const queryClient = useQueryClient() - const { canWrite } = useUser() const { id: batchId, definitionId, compositionKey: compositionKeyParam } = useParams<{ id?: string definitionId?: string @@ -115,10 +123,6 @@ export default function ControlBatchWizard() { }>() const [searchParams, setSearchParams] = useSearchParams() - if (!canWrite) { - return - } - const compositionKey = compositionKeyParam ? decodeURIComponent(compositionKeyParam) : null // Composition route is only used for "add batches from CSV"; path alone defines the short flow. const isCompositionCsvFlow = !!compositionKey @@ -219,8 +223,6 @@ export default function ControlBatchWizard() { batchBootstrapQuery.error, definitionSeedQuery.isError, definitionSeedQuery.error, - definitionSeedQuery.isSuccess, - definitionSeedQuery.data, compositionQuery.isError, compositionQuery.error, compositionQuery.isSuccess, diff --git a/packages/web/src/pages/LocationDetail.tsx b/packages/web/src/pages/LocationDetail.tsx index 3a1c0120..b38602eb 100644 --- a/packages/web/src/pages/LocationDetail.tsx +++ b/packages/web/src/pages/LocationDetail.tsx @@ -47,7 +47,10 @@ export default function LocationDetail() { const location = (detailQuery.data?.location as Location | undefined) ?? null const contents = (detailQuery.data?.contents as LocationContents | undefined) ?? null const hierarchyStats = (detailQuery.data?.hierarchyStats as LocationHierarchyStats | undefined) ?? null - const allLocations = (detailQuery.data?.allLocations as Location[] | undefined) ?? [] + const allLocations = useMemo( + () => (detailQuery.data?.allLocations as Location[] | undefined) ?? [], + [detailQuery.data], + ) const pagination = detailQuery.data?.pagination ?? null const pathLabel = useMemo(() => { diff --git a/packages/web/src/pages/QpcrExperimentDetail.tsx b/packages/web/src/pages/QpcrExperimentDetail.tsx index 0b364c71..d8a58706 100644 --- a/packages/web/src/pages/QpcrExperimentDetail.tsx +++ b/packages/web/src/pages/QpcrExperimentDetail.tsx @@ -1,4 +1,4 @@ -import { useEffect, useState, useRef } from 'react' +import { useEffect, useState, useRef, useMemo } from 'react' import { useParams, Link, useNavigate, useSearchParams } from 'react-router-dom' import { useQueryClient } from '@tanstack/react-query' import { api } from '../lib/api/client' @@ -137,7 +137,7 @@ export default function QpcrExperimentDetail() { const experimentQuery = useQpcrExperiment(experimentId) const experimentStatus = fromQuery(experimentQuery) const scannerConfigsQuery = useQpcrScannerConfigurations() - const scannerConfigs = scannerConfigsQuery.data ?? [] + const scannerConfigs = useMemo(() => scannerConfigsQuery.data ?? [], [scannerConfigsQuery.data]) const [data, setData] = useState(null) const [deleting, setDeleting] = useState(false) const [selectedConfigId, setSelectedConfigId] = useState(null) diff --git a/packages/web/src/pages/ReferenceData.tsx b/packages/web/src/pages/ReferenceData.tsx index fc4c53a7..b917eac2 100644 --- a/packages/web/src/pages/ReferenceData.tsx +++ b/packages/web/src/pages/ReferenceData.tsx @@ -7,6 +7,7 @@ import ReferenceDataForm from '../components/ReferenceDataForm' import { referenceDataConfigs, getReferenceDataConfig, + type ReferenceDataConfig, type ReferenceDataType, } from '../config/reference-data-config' import { useStorageTypes } from '../hooks/useReferenceData' @@ -23,10 +24,27 @@ import { PageError, fromQuery, getQueryErrorMessage } from '../ui' import '../styles/reference-data.css' export default function ReferenceData() { - const { canManageReferenceData } = useUser() - const [searchParams, setSearchParams] = useSearchParams() + const [searchParams] = useSearchParams() // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- tab from URL may be missing or invalid const activeTab = (searchParams.get('tab') as ReferenceDataType) || 'specimen-types' + const config = getReferenceDataConfig(activeTab) + + if (!config) { + return
Invalid tab
+ } + return +} + +/** Split out so its hooks run on every render, even when the tab is invalid. */ +function ReferenceDataContent({ + activeTab, + config, +}: { + activeTab: ReferenceDataType + config: ReferenceDataConfig +}) { + const { canManageReferenceData } = useUser() + const [, setSearchParams] = useSearchParams() // Keep isAdmin for backward compatibility in this file (used in multiple places) const isAdmin = canManageReferenceData @@ -38,12 +56,6 @@ export default function ReferenceData() { }) } - const config = getReferenceDataConfig(activeTab) - - if (!config) { - return
Invalid tab
- } - const queryClient = useQueryClient() const [editingItem, setEditingItem] = useState(null) diff --git a/packages/web/src/pages/SheetDetail.tsx b/packages/web/src/pages/SheetDetail.tsx index 6af88076..0d326dd0 100644 --- a/packages/web/src/pages/SheetDetail.tsx +++ b/packages/web/src/pages/SheetDetail.tsx @@ -50,7 +50,7 @@ export default function SheetDetail() { // Derive once so hooks below can run unconditionally (Rules of Hooks) const sheet = data?.sheet ?? null - const papers = (data?.papers ?? []) as CollectionTableEntry[] + const papers = useMemo(() => (data?.papers ?? []) as CollectionTableEntry[], [data]) const tableRows = useMemo(() => { const context = sheet diff --git a/packages/web/src/pages/Studies.tsx b/packages/web/src/pages/Studies.tsx index 94814933..11c13b11 100644 --- a/packages/web/src/pages/Studies.tsx +++ b/packages/web/src/pages/Studies.tsx @@ -1,4 +1,4 @@ -import { useEffect, useState, useRef, useMemo, useCallback } from 'react' +import { useEffect, useState, useRef, useMemo, useCallback, useEffectEvent } from 'react' import { Link, useSearchParams } from 'react-router-dom' import type { Study, StudySummaryBasic } from '../lib/api/studies' import StudyCard from '../components/StudyCard' @@ -166,20 +166,18 @@ export default function Studies() { } }, [studies, prefetchSummaries]) + const loadNextPage = useEffectEvent(() => { + if (infiniteQuery.hasNextPage && !infiniteQuery.isFetchingNextPage) { + void infiniteQuery.fetchNextPage() + } + }) + useEffect(() => { if (hasActiveFilters || !infiniteQuery.hasNextPage) return const observer = new IntersectionObserver( (entries) => { - entries.forEach((entry) => { - if ( - entry.isIntersecting && - infiniteQuery.hasNextPage && - !infiniteQuery.isFetchingNextPage - ) { - void infiniteQuery.fetchNextPage() - } - }) + if (entries.some((entry) => entry.isIntersecting)) loadNextPage() }, { rootMargin: '200px' } ) @@ -189,12 +187,9 @@ export default function Studies() { } return () => observer.disconnect() - }, [ - hasActiveFilters, - infiniteQuery.hasNextPage, - infiniteQuery.isFetchingNextPage, - infiniteQuery.fetchNextPage, - ]) + // isFetchingNextPage re-creates the observer after each page, which fires again + // while the sentinel is still visible, so short pages keep loading. + }, [hasActiveFilters, infiniteQuery.hasNextPage, infiniteQuery.isFetchingNextPage]) const filteredAndSortedStudies = useMemo(() => { let filtered = [...studies] From ba434b370b58a304b6285e80ff063cc3f8492c41 Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Mon, 5 Oct 2026 21:40:14 -0700 Subject: [PATCH 08/30] chore: ignore local database copies, audit notes, and worktrees. These sat untracked at the repo root, so a broad git add could commit them. Co-Authored-By: Claude Opus 5.5 (1M context) --- .gitignore | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/.gitignore b/.gitignore index 9ea36f71..4d54a08e 100644 --- a/.gitignore +++ b/.gitignore @@ -14,6 +14,10 @@ build/ *.sqlite *.sqlite-shm *.sqlite-wal +# Renamed copies, e.g. sampledb.sqlite.old, sampledb.sqlite-shm.old, sampledb.sqlite.old-wal +*.sqlite*.old +*.sqlite.old-* +backup_dbs/ litestream-replicas/ # Python @@ -77,3 +81,7 @@ scripts/ !packages/docs/scripts/ test-csvs/ + +# Local scratch: audit notes and agent worktrees +.audit/ +.worktrees/ From 34ba32f7a838fa28ee8d5a2dacd67c3814f6caeb Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Tue, 6 Oct 2026 16:48:40 -0700 Subject: [PATCH 09/30] test(db): close the snapshot test's database and note why it skips the entry point. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/api/src/db/README.md | 2 +- packages/api/src/db/__tests__/initial-schema-snapshot.test.ts | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/api/src/db/README.md b/packages/api/src/db/README.md index bbefac77..f7771a0e 100644 --- a/packages/api/src/db/README.md +++ b/packages/api/src/db/README.md @@ -10,7 +10,7 @@ Use `openOperationalDatabase(optionalPath?)` from the client module. It resolves Multi-statement writes that `await` must use `withWriteTransaction` from `write-transaction.ts`. Drizzle's bun-sqlite `db.transaction(async () => …)` commits when the callback first yields; it does not await the returned Promise. ESLint rejects an `async` callback passed to `.transaction(...)`. -Tests should use the same entry point (e.g. `openOperationalDatabase(':memory:')` via `setupTestDatabase()`). +Tests should use the same entry point (e.g. `openOperationalDatabase(':memory:')` via `setupTestDatabase()`), except a test that checks the snapshot itself: `__tests__/initial-schema-snapshot.test.ts` applies `initial_schema.sql` alone, because the entry point would run pending migrations on top and hide a stale snapshot. ## Schema version diff --git a/packages/api/src/db/__tests__/initial-schema-snapshot.test.ts b/packages/api/src/db/__tests__/initial-schema-snapshot.test.ts index 019c50c8..b5a972c7 100644 --- a/packages/api/src/db/__tests__/initial-schema-snapshot.test.ts +++ b/packages/api/src/db/__tests__/initial-schema-snapshot.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect } from 'vitest' +import { describe, it, expect, afterAll } from 'vitest' import { Database } from 'bun:sqlite' import { is } from 'drizzle-orm' import { SQLiteTable, getTableConfig } from 'drizzle-orm/sqlite-core' @@ -13,6 +13,7 @@ import { CURRENT_SCHEMA_VERSION, getRecordedSchemaVersion } from '../schema-evol describe('initial_schema.sql snapshot', () => { const sqlite = new Database(':memory:') applyInitialSchema(sqlite) + afterAll(() => sqlite.close()) const drizzleTables = Object.values(schema) .filter((value): value is SQLiteTable => is(value, SQLiteTable)) From 1e7285b0d638646daa4c6a4724e3ce3798a8e6e3 Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Tue, 6 Oct 2026 16:49:25 -0700 Subject: [PATCH 10/30] test(db): compare the snapshot's indexes, foreign keys, NOT NULL and types with the migrated schema. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/api/src/db/README.md | 2 +- .../__tests__/fixtures/initial_schema_v1.sql | 384 ++++++++++++++++++ .../__tests__/initial-schema-snapshot.test.ts | 117 +++++- 3 files changed, 500 insertions(+), 3 deletions(-) create mode 100644 packages/api/src/db/__tests__/fixtures/initial_schema_v1.sql diff --git a/packages/api/src/db/README.md b/packages/api/src/db/README.md index f7771a0e..35c19d29 100644 --- a/packages/api/src/db/README.md +++ b/packages/api/src/db/README.md @@ -35,7 +35,7 @@ Migration `001` is **only** for legacy files; fresh installs from the snapshot s 3. Add `packages/api/src/db/migrations/00N_short_name.sql` (statements separated with `--> statement-breakpoint`). 4. Bump `CURRENT_SCHEMA_VERSION` in `schema-evolution.ts`. 5. Run `bun run build` in `packages/api` — `scripts/runtime-assets.ts` copies migrations into `dist/db/migrations` and verifies the dist layout. If you add other on-disk runtime assets, register them in `RUNTIME_ASSETS` in that script. -6. Refresh `initial_schema.sql` so **new** empty databases match the latest shape (merge generated SQL; keep `INSERT INTO schema_version` at `CURRENT_SCHEMA_VERSION`). `__tests__/initial-schema-snapshot.test.ts` fails until the snapshot holds every table and column in `schema.ts`. +6. Refresh `initial_schema.sql` so **new** empty databases match the latest shape (merge generated SQL; keep `INSERT INTO schema_version` at `CURRENT_SCHEMA_VERSION`). `__tests__/initial-schema-snapshot.test.ts` fails until the snapshot holds every table and column in `schema.ts`, and until its columns, declared types, NOT NULL, indexes and foreign keys match a version 1 install (`__tests__/fixtures/initial_schema_v1.sql`, never edited) brought forward by the migrations. 7. Extend the legacy fixtures if the migration touches a table they don't create. Older-version tests in `__tests__/schema-evolution.test.ts` build minimal databases through `createTablesForLaterMigrations`, and `__tests__/client.test.ts` creates its own tables inline; both must contain every table and column a later migration alters. Do **not** add inline DDL to `open.ts` or connection code. diff --git a/packages/api/src/db/__tests__/fixtures/initial_schema_v1.sql b/packages/api/src/db/__tests__/fixtures/initial_schema_v1.sql new file mode 100644 index 00000000..74a73aac --- /dev/null +++ b/packages/api/src/db/__tests__/fixtures/initial_schema_v1.sql @@ -0,0 +1,384 @@ +PRAGMA foreign_keys=OFF;--> statement-breakpoint +CREATE TABLE IF NOT EXISTS users ( + id INTEGER PRIMARY KEY, + email TEXT NOT NULL UNIQUE, + username TEXT UNIQUE, + name TEXT NOT NULL, + password_hash TEXT NOT NULL, + role TEXT NOT NULL, + created TEXT NOT NULL DEFAULT (datetime('now')), + last_login TEXT, + deleted_at TEXT, + approved_at TEXT +);--> statement-breakpoint +CREATE TABLE IF NOT EXISTS sessions ( + id TEXT PRIMARY KEY, + user_id INTEGER NOT NULL REFERENCES users(id), + expires_at INTEGER NOT NULL +);--> statement-breakpoint +CREATE TABLE IF NOT EXISTS tag ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + name TEXT NOT NULL UNIQUE +);--> statement-breakpoint +CREATE TABLE IF NOT EXISTS unit ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + symbol TEXT NOT NULL UNIQUE, + name TEXT NOT NULL, + category TEXT NOT NULL +);--> statement-breakpoint +CREATE TABLE IF NOT EXISTS study ( + id INTEGER PRIMARY KEY, + title TEXT NOT NULL UNIQUE, + description TEXT, + short_code TEXT NOT NULL UNIQUE, + is_longitudinal INTEGER NOT NULL, + lead_person TEXT NOT NULL, + created TEXT NOT NULL, + last_updated TEXT NOT NULL, + created_by INTEGER REFERENCES users(id), + updated_by INTEGER REFERENCES users(id) +);--> statement-breakpoint +CREATE INDEX IF NOT EXISTS study_title_idx ON study(title);--> statement-breakpoint +CREATE INDEX IF NOT EXISTS study_short_code_idx ON study(short_code);--> statement-breakpoint +CREATE TABLE IF NOT EXISTS study_subject ( + id INTEGER PRIMARY KEY, + study_id INTEGER NOT NULL REFERENCES study(id), + name TEXT NOT NULL, + created TEXT NOT NULL, + last_updated TEXT NOT NULL, + created_by INTEGER REFERENCES users(id), + updated_by INTEGER REFERENCES users(id) +);--> statement-breakpoint +CREATE INDEX IF NOT EXISTS study_subject_study_id_idx ON study_subject(study_id);--> statement-breakpoint +CREATE INDEX IF NOT EXISTS study_subject_name_idx ON study_subject(name);--> statement-breakpoint +CREATE TABLE IF NOT EXISTS control_definition ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + name TEXT NOT NULL UNIQUE, + control_type TEXT NOT NULL CHECK (control_type IN ('blood', 'plasma_positive', 'plasma_negative', 'antibody', 'extraction', 'negative')), + properties TEXT, + created TEXT NOT NULL DEFAULT (datetime('now')), + last_updated TEXT NOT NULL DEFAULT (datetime('now')), + created_by INTEGER REFERENCES users(id), + updated_by INTEGER REFERENCES users(id) +);--> statement-breakpoint +CREATE TABLE IF NOT EXISTS control_batch ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + control_definition_id INTEGER NOT NULL REFERENCES control_definition(id), + name TEXT NOT NULL UNIQUE, + production_date TEXT, + properties TEXT, + created TEXT NOT NULL DEFAULT (datetime('now')), + last_updated TEXT NOT NULL DEFAULT (datetime('now')), + created_by INTEGER REFERENCES users(id), + updated_by INTEGER REFERENCES users(id) +);--> statement-breakpoint +CREATE INDEX IF NOT EXISTS control_batch_control_definition_id_idx ON control_batch(control_definition_id);--> statement-breakpoint +CREATE TABLE IF NOT EXISTS reagent ( + id INTEGER PRIMARY KEY, + name TEXT NOT NULL, + reagent_type TEXT NOT NULL, + vendor TEXT, + catalog_number TEXT, + lot_number TEXT, + received_date TEXT, + expiration_date TEXT, + storage_temp TEXT, + properties TEXT, + created TEXT NOT NULL DEFAULT (datetime('now')), + last_updated TEXT NOT NULL DEFAULT (datetime('now')), + created_by INTEGER REFERENCES users(id), + updated_by INTEGER REFERENCES users(id) +);--> statement-breakpoint +CREATE TABLE IF NOT EXISTS cell_line ( + id INTEGER PRIMARY KEY, + name TEXT NOT NULL UNIQUE, + species TEXT NOT NULL, + strain TEXT, + source TEXT, + properties TEXT, + created TEXT NOT NULL DEFAULT (datetime('now')), + last_updated TEXT NOT NULL DEFAULT (datetime('now')), + created_by INTEGER REFERENCES users(id), + updated_by INTEGER REFERENCES users(id) +);--> statement-breakpoint +CREATE TABLE IF NOT EXISTS plasmid ( + id INTEGER PRIMARY KEY, + name TEXT NOT NULL UNIQUE, + backbone TEXT, + insert_name TEXT, + insert_size_bp INTEGER, + resistance TEXT, + source TEXT, + properties TEXT, + created TEXT NOT NULL DEFAULT (datetime('now')), + last_updated TEXT NOT NULL DEFAULT (datetime('now')), + created_by INTEGER REFERENCES users(id), + updated_by INTEGER REFERENCES users(id) +);--> statement-breakpoint +CREATE TABLE IF NOT EXISTS standard ( + id INTEGER PRIMARY KEY, + name TEXT NOT NULL, + standard_type TEXT NOT NULL, + manufacturer TEXT, + catalog_number TEXT, + lot_number TEXT, + properties TEXT, + created TEXT NOT NULL DEFAULT (datetime('now')), + last_updated TEXT NOT NULL DEFAULT (datetime('now')), + created_by INTEGER REFERENCES users(id), + updated_by INTEGER REFERENCES users(id) +);--> statement-breakpoint +CREATE TABLE IF NOT EXISTS specimen_type ( + id INTEGER PRIMARY KEY, + name TEXT NOT NULL UNIQUE, + created TEXT NOT NULL, + last_updated TEXT NOT NULL +);--> statement-breakpoint +CREATE TABLE IF NOT EXISTS specimen ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + study_subject_id INTEGER REFERENCES study_subject(id), + control_batch_id INTEGER REFERENCES control_batch(id), + specimen_type_id INTEGER NOT NULL REFERENCES specimen_type(id), + collection_date TEXT, + created TEXT NOT NULL DEFAULT (datetime('now')), + last_updated TEXT NOT NULL DEFAULT (datetime('now')), + created_by INTEGER REFERENCES users(id), + updated_by INTEGER REFERENCES users(id), + CHECK ((study_subject_id IS NOT NULL AND control_batch_id IS NULL) OR + (study_subject_id IS NULL AND control_batch_id IS NOT NULL)) +);--> statement-breakpoint +CREATE INDEX IF NOT EXISTS specimen_study_subject_id_idx ON specimen(study_subject_id);--> statement-breakpoint +CREATE INDEX IF NOT EXISTS specimen_control_batch_id_idx ON specimen(control_batch_id);--> statement-breakpoint +CREATE INDEX IF NOT EXISTS specimen_collection_date_idx ON specimen(collection_date);--> statement-breakpoint +CREATE INDEX IF NOT EXISTS specimen_specimen_type_id_idx ON specimen(specimen_type_id);--> statement-breakpoint +CREATE UNIQUE INDEX IF NOT EXISTS idx_specimen_study_subject_type_date ON specimen(study_subject_id, specimen_type_id, collection_date) WHERE study_subject_id IS NOT NULL;--> statement-breakpoint +CREATE UNIQUE INDEX IF NOT EXISTS idx_specimen_control_batch_type_date ON specimen(control_batch_id, specimen_type_id, collection_date) WHERE control_batch_id IS NOT NULL;--> statement-breakpoint +CREATE TABLE IF NOT EXISTS storage_container ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + specimen_id INTEGER NOT NULL REFERENCES specimen(id), + comment TEXT, + total_quantity REAL DEFAULT 1.0, + remaining_quantity REAL DEFAULT 1.0, + unit_id INTEGER NOT NULL REFERENCES unit(id), + created TEXT NOT NULL DEFAULT (datetime('now')), + last_updated TEXT NOT NULL DEFAULT (datetime('now')), + created_by INTEGER REFERENCES users(id), + updated_by INTEGER REFERENCES users(id) +);--> statement-breakpoint +CREATE INDEX IF NOT EXISTS storage_container_specimen_id_idx ON storage_container(specimen_id);--> statement-breakpoint +CREATE TABLE IF NOT EXISTS storage_container_tag ( + storage_container_id INTEGER NOT NULL REFERENCES storage_container(id), + tag_id INTEGER NOT NULL REFERENCES tag(id), + PRIMARY KEY (storage_container_id, tag_id) +);--> statement-breakpoint +CREATE TABLE IF NOT EXISTS location ( + id INTEGER PRIMARY KEY, + parent_id INTEGER REFERENCES location(id), + name TEXT NOT NULL, + storage_type_id TEXT, + description TEXT, + can_contain_collections INTEGER NOT NULL DEFAULT 0, + path TEXT, + created TEXT NOT NULL DEFAULT (datetime('now')), + last_updated TEXT NOT NULL DEFAULT (datetime('now')), + created_by INTEGER REFERENCES users(id), + updated_by INTEGER REFERENCES users(id), + UNIQUE (parent_id, name), + CHECK ((parent_id IS NULL AND storage_type_id IS NOT NULL) OR + (parent_id IS NOT NULL AND storage_type_id IS NULL)) +);--> statement-breakpoint +CREATE INDEX IF NOT EXISTS idx_location_parent_id ON location(parent_id);--> statement-breakpoint +CREATE INDEX IF NOT EXISTS idx_location_path ON location(path);--> statement-breakpoint +CREATE TABLE IF NOT EXISTS micronix_plate ( + id INTEGER PRIMARY KEY, + location_id INTEGER NOT NULL REFERENCES location(id), + name TEXT NOT NULL UNIQUE, + barcode TEXT UNIQUE, + created TEXT NOT NULL DEFAULT (datetime('now')), + last_updated TEXT NOT NULL DEFAULT (datetime('now')), + created_by INTEGER REFERENCES users(id), + updated_by INTEGER REFERENCES users(id) +);--> statement-breakpoint +CREATE INDEX IF NOT EXISTS micronix_plate_location_id_idx ON micronix_plate(location_id);--> statement-breakpoint +CREATE TABLE IF NOT EXISTS micronix_tube ( + id INTEGER PRIMARY KEY REFERENCES storage_container(id), + collection_id INTEGER NOT NULL REFERENCES micronix_plate(id), + barcode TEXT NOT NULL UNIQUE, + position TEXT +);--> statement-breakpoint +CREATE TABLE IF NOT EXISTS cryovial_box ( + id INTEGER PRIMARY KEY, + location_id INTEGER NOT NULL REFERENCES location(id), + name TEXT NOT NULL UNIQUE, + barcode TEXT UNIQUE, + created TEXT NOT NULL DEFAULT (datetime('now')), + last_updated TEXT NOT NULL DEFAULT (datetime('now')), + created_by INTEGER REFERENCES users(id), + updated_by INTEGER REFERENCES users(id) +);--> statement-breakpoint +CREATE INDEX IF NOT EXISTS cryovial_box_location_id_idx ON cryovial_box(location_id);--> statement-breakpoint +CREATE TABLE IF NOT EXISTS cryovial_tube ( + id INTEGER PRIMARY KEY REFERENCES storage_container(id), + collection_id INTEGER NOT NULL REFERENCES cryovial_box(id), + barcode TEXT, + position TEXT +);--> statement-breakpoint +CREATE TABLE IF NOT EXISTS box ( + id INTEGER PRIMARY KEY, + location_id INTEGER NOT NULL REFERENCES location(id), + name TEXT NOT NULL UNIQUE, + created TEXT NOT NULL, + last_updated TEXT NOT NULL, + created_by INTEGER REFERENCES users(id), + updated_by INTEGER REFERENCES users(id) +);--> statement-breakpoint +CREATE TABLE IF NOT EXISTS bag ( + id INTEGER PRIMARY KEY, + location_id INTEGER NOT NULL REFERENCES location(id), + name TEXT NOT NULL UNIQUE, + created TEXT NOT NULL, + last_updated TEXT NOT NULL, + created_by INTEGER REFERENCES users(id), + updated_by INTEGER REFERENCES users(id) +);--> statement-breakpoint +CREATE INDEX IF NOT EXISTS bag_location_id_idx ON bag(location_id);--> statement-breakpoint +CREATE TABLE IF NOT EXISTS sheet ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + name TEXT NOT NULL, + box_id INTEGER REFERENCES box(id), + bag_id INTEGER REFERENCES bag(id), + created TEXT NOT NULL DEFAULT (datetime('now')), + last_updated TEXT NOT NULL DEFAULT (datetime('now')), + created_by INTEGER REFERENCES users(id), + updated_by INTEGER REFERENCES users(id), + UNIQUE (name, box_id, bag_id), + CHECK ((box_id IS NOT NULL AND bag_id IS NULL) OR + (box_id IS NULL AND bag_id IS NOT NULL) OR + (box_id IS NULL AND bag_id IS NULL)) +);--> statement-breakpoint +CREATE TABLE IF NOT EXISTS paper ( + id INTEGER PRIMARY KEY REFERENCES storage_container(id), + sheet_id INTEGER NOT NULL REFERENCES sheet(id), + barcode TEXT, + position TEXT +);--> statement-breakpoint +CREATE TABLE IF NOT EXISTS static_well ( + id INTEGER PRIMARY KEY REFERENCES storage_container(id), + collection_id INTEGER NOT NULL REFERENCES micronix_plate(id), + position TEXT +);--> statement-breakpoint +CREATE TABLE IF NOT EXISTS strain ( + id INTEGER PRIMARY KEY, + name TEXT NOT NULL UNIQUE, + description TEXT +);--> statement-breakpoint +CREATE TABLE IF NOT EXISTS storage_type ( + id INTEGER PRIMARY KEY, + name TEXT NOT NULL UNIQUE, + description TEXT +);--> statement-breakpoint +CREATE TABLE IF NOT EXISTS schema_version ( + version INTEGER NOT NULL +);--> statement-breakpoint +INSERT INTO schema_version (version) VALUES (1);--> statement-breakpoint +CREATE TABLE IF NOT EXISTS settings ( + key TEXT NOT NULL, + user_id INTEGER REFERENCES users(id), + value TEXT NOT NULL, + PRIMARY KEY (key, user_id) +);--> statement-breakpoint +CREATE INDEX IF NOT EXISTS settings_user_id_idx ON settings(user_id);--> statement-breakpoint +CREATE TABLE IF NOT EXISTS specimen_type_container_type ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + specimen_type_id INTEGER NOT NULL REFERENCES specimen_type(id) ON DELETE CASCADE, + container_type TEXT NOT NULL CHECK (container_type IN ('paper', 'cryovial_tube', 'micronix_tube', 'static_well')), + created TEXT NOT NULL DEFAULT (datetime('now')), + UNIQUE (specimen_type_id, container_type) +);--> statement-breakpoint +CREATE TABLE IF NOT EXISTS container_type_unit ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + container_type TEXT NOT NULL CHECK (container_type IN ('paper', 'cryovial_tube', 'micronix_tube', 'static_well')), + unit_id INTEGER NOT NULL REFERENCES unit(id) ON DELETE CASCADE, + created TEXT NOT NULL DEFAULT (datetime('now')), + UNIQUE (container_type, unit_id) +);--> statement-breakpoint +CREATE TABLE IF NOT EXISTS container_derivation ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + parent_container_id INTEGER NOT NULL REFERENCES storage_container(id), + child_container_id INTEGER NOT NULL REFERENCES storage_container(id), + derivation_type TEXT NOT NULL, + derivation_date TEXT, + operator_id INTEGER REFERENCES users(id), + protocol TEXT, + notes TEXT, + properties TEXT, + created TEXT NOT NULL DEFAULT (datetime('now')), + CHECK (parent_container_id != child_container_id), + UNIQUE (child_container_id) +);--> statement-breakpoint +CREATE INDEX IF NOT EXISTS idx_container_derivation_parent ON container_derivation(parent_container_id);--> statement-breakpoint +CREATE INDEX IF NOT EXISTS idx_container_derivation_child ON container_derivation(child_container_id);--> statement-breakpoint +CREATE INDEX IF NOT EXISTS idx_container_derivation_type ON container_derivation(derivation_type);--> statement-breakpoint +CREATE INDEX IF NOT EXISTS idx_container_derivation_date ON container_derivation(derivation_date);--> statement-breakpoint +CREATE TABLE IF NOT EXISTS error_logs ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + timestamp TEXT NOT NULL DEFAULT (datetime('now')), + source TEXT NOT NULL, + level TEXT NOT NULL, + message TEXT NOT NULL, + error_code TEXT, + stack TEXT, + context TEXT, + user_id INTEGER, + url TEXT, + user_agent TEXT, + resolved INTEGER NOT NULL DEFAULT 0, + resolved_at TEXT, + resolved_by INTEGER +);--> statement-breakpoint +CREATE INDEX IF NOT EXISTS error_logs_timestamp_idx ON error_logs(timestamp);--> statement-breakpoint +CREATE INDEX IF NOT EXISTS error_logs_source_idx ON error_logs(source);--> statement-breakpoint +CREATE INDEX IF NOT EXISTS error_logs_level_idx ON error_logs(level);--> statement-breakpoint +CREATE INDEX IF NOT EXISTS error_logs_resolved_idx ON error_logs(resolved);--> statement-breakpoint +CREATE TABLE IF NOT EXISTS qpcr_experiment ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + name TEXT, + template_format TEXT NOT NULL, + status TEXT NOT NULL, + standard_layout TEXT, + plate_barcode TEXT, + instrument_type TEXT, + created TEXT NOT NULL DEFAULT (datetime('now')), + last_updated TEXT NOT NULL DEFAULT (datetime('now')), + created_by INTEGER REFERENCES users(id), + updated_by INTEGER REFERENCES users(id) +);--> statement-breakpoint +CREATE INDEX IF NOT EXISTS qpcr_experiment_status_idx ON qpcr_experiment(status);--> statement-breakpoint +CREATE TABLE IF NOT EXISTS qpcr_experiment_target ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + qpcr_experiment_id INTEGER NOT NULL REFERENCES qpcr_experiment(id) ON DELETE CASCADE, + target_name TEXT NOT NULL, + fluorophore TEXT, + reporter TEXT, + sort_order INTEGER NOT NULL DEFAULT 0, + UNIQUE (qpcr_experiment_id, target_name) +);--> statement-breakpoint +CREATE INDEX IF NOT EXISTS qpcr_experiment_target_experiment_idx ON qpcr_experiment_target(qpcr_experiment_id);--> statement-breakpoint +CREATE TABLE IF NOT EXISTS qpcr_experiment_well ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + qpcr_experiment_id INTEGER NOT NULL REFERENCES qpcr_experiment(id) ON DELETE CASCADE, + well_position TEXT NOT NULL, + barcode TEXT, + storage_container_id INTEGER REFERENCES storage_container(id), + specimen_id INTEGER REFERENCES specimen(id), + content_type TEXT, + standard_density REAL, + cq REAL, + starting_quantity REAL, + cq_mean REAL, + raw_sample_name TEXT, + UNIQUE (qpcr_experiment_id, well_position) +);--> statement-breakpoint +CREATE INDEX IF NOT EXISTS qpcr_experiment_well_experiment_idx ON qpcr_experiment_well(qpcr_experiment_id);--> statement-breakpoint +CREATE INDEX IF NOT EXISTS qpcr_experiment_well_specimen_idx ON qpcr_experiment_well(specimen_id);--> statement-breakpoint +PRAGMA foreign_keys=ON; diff --git a/packages/api/src/db/__tests__/initial-schema-snapshot.test.ts b/packages/api/src/db/__tests__/initial-schema-snapshot.test.ts index b5a972c7..08587049 100644 --- a/packages/api/src/db/__tests__/initial-schema-snapshot.test.ts +++ b/packages/api/src/db/__tests__/initial-schema-snapshot.test.ts @@ -1,14 +1,95 @@ import { describe, it, expect, afterAll } from 'vitest' import { Database } from 'bun:sqlite' +import { readFileSync } from 'fs' +import { dirname, join } from 'path' +import { fileURLToPath } from 'url' import { is } from 'drizzle-orm' import { SQLiteTable, getTableConfig } from 'drizzle-orm/sqlite-core' import * as schema from '../schema' import { applyInitialSchema } from '../apply-initial-schema' -import { CURRENT_SCHEMA_VERSION, getRecordedSchemaVersion } from '../schema-evolution' +import { splitSqlStatements } from '../migration-runner' +import { CURRENT_SCHEMA_VERSION, evolveOperationalSchema, getRecordedSchemaVersion } from '../schema-evolution' + +/** initial_schema.sql as of schema version 1, the first versioned snapshot. Never edit it. */ +const VERSION_1_SNAPSHOT = join(dirname(fileURLToPath(import.meta.url)), 'fixtures', 'initial_schema_v1.sql') + +/** A version 1 install brought to the current version by the numbered migrations. */ +function openMigratedDatabase(): Database { + const sqlite = new Database(':memory:') + for (const statement of splitSqlStatements(readFileSync(VERSION_1_SNAPSHOT, 'utf-8'))) { + sqlite.exec(statement) + } + evolveOperationalSchema(sqlite) + return sqlite +} + +const normaliseSql = (sql: string | null) => sql?.replace(/\s+/g, ' ').replace(/ IF NOT EXISTS/i, '').trim() ?? null + +/** + * Table shape from PRAGMA output, normalised so that two databases with the same schema compare + * equal however they were built: columns and keys are sorted (ALTER TABLE ADD COLUMN appends), + * and SQLite's generated index names give way to their columns. + */ +function describeSchema(sqlite: Database) { + const tables = sqlite + .query<{ name: string }, []>( + `SELECT name FROM sqlite_master WHERE type = 'table' AND name NOT LIKE 'sqlite_%' ORDER BY name`, + ) + .all() + .map(({ name }) => name) + + return Object.fromEntries( + tables.map((table) => { + const columns = sqlite + .query<{ name: string; type: string; notnull: number; dflt_value: string | null; pk: number }, []>( + `SELECT name, type, "notnull", dflt_value, pk FROM pragma_table_info('${table}')`, + ) + .all() + .map((column) => ({ ...column, type: column.type.toUpperCase() })) + .sort((a, b) => a.name.localeCompare(b.name)) + + const indexes = sqlite + .query<{ name: string; unique: number; origin: string; partial: number }, []>( + `SELECT name, "unique", origin, partial FROM pragma_index_list('${table}')`, + ) + .all() + .map((index) => { + const indexColumns = sqlite + .query<{ name: string | null }, []>(`SELECT name FROM pragma_index_info('${index.name}') ORDER BY seqno`) + .all() + .map((column) => column.name) + const generated = index.name.startsWith('sqlite_autoindex_') + const sql = sqlite + .query<{ sql: string | null }, [string]>(`SELECT sql FROM sqlite_master WHERE type = 'index' AND name = ?`) + .get(index.name)?.sql + return { + name: generated ? `(${index.origin} on ${indexColumns.join(', ')})` : index.name, + unique: index.unique, + origin: index.origin, + partial: index.partial, + columns: indexColumns, + sql: normaliseSql(sql ?? null), + } + }) + .sort((a, b) => a.name.localeCompare(b.name)) + + const foreignKeys = sqlite + .query< + { table: string; from: string; to: string | null; on_update: string; on_delete: string; match: string }, + [] + >(`SELECT "table", "from", "to", on_update, on_delete, "match" FROM pragma_foreign_key_list('${table}')`) + .all() + .sort((a, b) => `${a.from}>${a.table}`.localeCompare(`${b.from}>${b.table}`)) + + return [table, { columns, indexes, foreignKeys }] + }), + ) +} /** * New installs get initial_schema.sql and nothing else when its version is current, - * so the snapshot alone must hold every table and column in schema.ts. + * so the snapshot alone must hold every table and column in schema.ts, and must match + * what the numbered migrations build on top of an older install. */ describe('initial_schema.sql snapshot', () => { const sqlite = new Database(':memory:') @@ -47,4 +128,36 @@ describe('initial_schema.sql snapshot', () => { it('finds the schema.ts tables', () => { expect(drizzleTables.length).toBeGreaterThan(20) }) + + it('matches a version 1 install after the migrations: columns, types, NOT NULL, indexes, foreign keys', () => { + const migrated = openMigratedDatabase() + try { + expect(getRecordedSchemaVersion(migrated)).toBe(CURRENT_SCHEMA_VERSION) + // A difference means a migration and initial_schema.sql disagree (step 6 in src/db/README.md). + expect(describeSchema(sqlite)).toEqual(describeSchema(migrated)) + } finally { + migrated.close() + } + }) + + it('notices a snapshot that drifts from the migrations', () => { + const drifted = new Database(':memory:') + try { + applyInitialSchema(drifted) + drifted.exec('DROP INDEX sheet_box_name_idx') + drifted.exec('CREATE INDEX sheet_box_name_idx ON sheet(box_id, name) WHERE box_id IS NOT NULL') + drifted.exec('ALTER TABLE paper ADD COLUMN extra TEXT NOT NULL DEFAULT \'\'') + const migrated = openMigratedDatabase() + try { + const expected = describeSchema(migrated) + const actual = describeSchema(drifted) + expect(actual.sheet.indexes).not.toEqual(expected.sheet.indexes) + expect(actual.paper.columns).not.toEqual(expected.paper.columns) + } finally { + migrated.close() + } + } finally { + drifted.close() + } + }) }) From 14eb9fdab1a4b985d7fb9a2d443d5d030c82f9de Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Tue, 6 Oct 2026 16:52:25 -0700 Subject: [PATCH 11/30] chore(api): lint any transaction callback that returns a Promise, in tests too. The selector only caught async function literals written inline. A typed local rule also catches a named async function and a sync callback that returns a Promise. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../eslint-rules/__tests__/fixtures/file.ts | 1 + .../__tests__/fixtures/tsconfig.json | 4 ++ .../no-promise-transaction-callback.test.ts | 46 +++++++++++++ .../no-promise-transaction-callback.mjs | 67 +++++++++++++++++++ packages/api/eslint.config.mjs | 41 +++++++----- packages/api/package.json | 8 +-- packages/api/src/db/README.md | 2 +- packages/api/tsconfig.eslint.json | 6 ++ 8 files changed, 153 insertions(+), 22 deletions(-) create mode 100644 packages/api/eslint-rules/__tests__/fixtures/file.ts create mode 100644 packages/api/eslint-rules/__tests__/fixtures/tsconfig.json create mode 100644 packages/api/eslint-rules/__tests__/no-promise-transaction-callback.test.ts create mode 100644 packages/api/eslint-rules/no-promise-transaction-callback.mjs create mode 100644 packages/api/tsconfig.eslint.json diff --git a/packages/api/eslint-rules/__tests__/fixtures/file.ts b/packages/api/eslint-rules/__tests__/fixtures/file.ts new file mode 100644 index 00000000..556f134d --- /dev/null +++ b/packages/api/eslint-rules/__tests__/fixtures/file.ts @@ -0,0 +1 @@ +// Placeholder so the fixture tsconfig has a file; RuleTester supplies the code. diff --git a/packages/api/eslint-rules/__tests__/fixtures/tsconfig.json b/packages/api/eslint-rules/__tests__/fixtures/tsconfig.json new file mode 100644 index 00000000..42091bba --- /dev/null +++ b/packages/api/eslint-rules/__tests__/fixtures/tsconfig.json @@ -0,0 +1,4 @@ +{ + "compilerOptions": { "strict": true, "target": "ES2022", "module": "ESNext", "lib": ["ES2022"], "noEmit": true }, + "include": ["*.ts"] +} diff --git a/packages/api/eslint-rules/__tests__/no-promise-transaction-callback.test.ts b/packages/api/eslint-rules/__tests__/no-promise-transaction-callback.test.ts new file mode 100644 index 00000000..2a108898 --- /dev/null +++ b/packages/api/eslint-rules/__tests__/no-promise-transaction-callback.test.ts @@ -0,0 +1,46 @@ +import { describe, it } from 'vitest' +import { join } from 'path' +import { dirname } from 'path' +import { fileURLToPath } from 'url' +import { RuleTester } from 'eslint' +import parser from '@typescript-eslint/parser' +import rule from '../no-promise-transaction-callback.mjs' + +RuleTester.describe = describe +RuleTester.it = it + +const fixtures = join(dirname(fileURLToPath(import.meta.url)), 'fixtures') + +const ruleTester = new RuleTester({ + languageOptions: { + parser, + parserOptions: { project: './tsconfig.json', tsconfigRootDir: fixtures }, + }, +}) + +/** Drizzle's bun-sqlite shape: the callback runs synchronously and its result is returned. */ +const database = ` + type Tx = { insert(value: number): void } + declare const db: { transaction(fn: (tx: Tx) => T): T } + declare function helper(tx: Tx): Promise + declare function syncHelper(tx: Tx): number +` +const filename = join(fixtures, 'file.ts') +const invalid = (code: string) => ({ code: database + code, filename, errors: [{ messageId: 'promiseCallback' }] }) + +ruleTester.run('no-promise-transaction-callback', rule, { + valid: [ + { code: `${database} db.transaction((tx) => { tx.insert(1) })`, filename }, + { code: `${database} db.transaction(syncHelper)`, filename }, + { code: `${database} function doWrites(tx: Tx) { tx.insert(1); return 1 } db.transaction(doWrites)`, filename }, + { code: `${database} declare const other: { run(fn: () => Promise): void }; other.run(async () => {})`, filename }, + ], + invalid: [ + invalid(`db.transaction(async (tx) => { tx.insert(1) })`), + invalid(`db.transaction(async function (tx) { tx.insert(1) })`), + invalid(`async function doWrites(tx: Tx) { tx.insert(1) } db.transaction(doWrites)`), + invalid(`db.transaction((tx) => helper(tx))`), + invalid(`db.transaction(helper)`), + invalid(`declare const maybe: (tx: Tx) => Promise | void; db.transaction(maybe)`), + ], +}) diff --git a/packages/api/eslint-rules/no-promise-transaction-callback.mjs b/packages/api/eslint-rules/no-promise-transaction-callback.mjs new file mode 100644 index 00000000..bc8c09cd --- /dev/null +++ b/packages/api/eslint-rules/no-promise-transaction-callback.mjs @@ -0,0 +1,67 @@ +/** + * Flags a callback passed to `.transaction(...)` whose return type is Promise-like. + * + * Drizzle's bun-sqlite `transaction` (and bun:sqlite's own) runs the callback synchronously and + * commits when it returns, so a callback that returns a Promise commits at its first `await`. + * Checking the callback's return type catches inline `async` functions, named async functions + * (`db.transaction(doWrites)`), and sync functions that return a Promise (`tx => helper(tx)`). + */ + +/** @param {import('typescript').Type} type */ +const constituents = (type) => (type.isUnion() ? type.types : [type]) + +/** + * @param {import('typescript').TypeChecker} checker + * @param {import('typescript').Type} type + * @param {import('typescript').Node} location + */ +function isThenable(checker, type, location) { + return constituents(type).some((part) => { + const then = part.getProperty('then') + return then != null && checker.getTypeOfSymbolAtLocation(then, location).getCallSignatures().length > 0 + }) +} + +/** @type {import('eslint').Rule.RuleModule} */ +export default { + meta: { + type: 'problem', + docs: { description: 'Disallow callbacks that return a Promise in database.transaction(...)' }, + messages: { + promiseCallback: + "Drizzle's bun-sqlite transaction commits when the callback returns, so a callback that returns a Promise commits at its first await. Use withWriteTransaction from src/db/write-transaction.ts, or a synchronous callback.", + }, + schema: [], + }, + create(context) { + const services = context.sourceCode.parserServices + if (!services?.program || !services.esTreeNodeToTSNodeMap) { + throw new Error('no-promise-transaction-callback needs type information (parserOptions.project).') + } + const checker = services.program.getTypeChecker() + + return { + CallExpression(node) { + const { callee } = node + if ( + callee.type !== 'MemberExpression' || + callee.computed || + callee.property.type !== 'Identifier' || + callee.property.name !== 'transaction' + ) { + return + } + const callback = node.arguments[0] + if (!callback || callback.type === 'SpreadElement') return + + const tsNode = services.esTreeNodeToTSNodeMap.get(callback) + const returnsPromise = constituents(checker.getTypeAtLocation(tsNode)).some((part) => + part + .getCallSignatures() + .some((signature) => isThenable(checker, checker.getReturnTypeOfSignature(signature), tsNode)), + ) + if (returnsPromise) context.report({ node: callback, messageId: 'promiseCallback' }) + }, + } + }, +} diff --git a/packages/api/eslint.config.mjs b/packages/api/eslint.config.mjs index 8b0d012c..db97ad50 100644 --- a/packages/api/eslint.config.mjs +++ b/packages/api/eslint.config.mjs @@ -1,33 +1,40 @@ import tseslint from "@typescript-eslint/eslint-plugin"; import parser from "@typescript-eslint/parser"; +import noPromiseTransactionCallback from "./eslint-rules/no-promise-transaction-callback.mjs"; + +// tsconfig.json leaves out tests (they aren't built); tsconfig.eslint.json adds them so the +// transaction rule has type information there too. +const typedLanguageOptions = { + parser, + parserOptions: { + project: "./tsconfig.eslint.json", + tsconfigRootDir: import.meta.dirname, + }, +}; export default [ { - ignores: ["dist/**", "coverage/**", "node_modules/**", "src/**/__tests__/**"], + ignores: ["dist/**", "coverage/**", "node_modules/**"], }, { files: ["src/**/*.ts"], - languageOptions: { - parser, - parserOptions: { - projectService: true, - tsconfigRootDir: import.meta.dirname, - }, - }, + ignores: ["src/**/__tests__/**"], + languageOptions: typedLanguageOptions, plugins: { "@typescript-eslint": tseslint, }, rules: { "@typescript-eslint/no-unnecessary-condition": "error", - "no-restricted-syntax": [ - "error", - { - selector: - "CallExpression[callee.property.name='transaction'] > :matches(ArrowFunctionExpression, FunctionExpression)[async=true]", - message: - "Drizzle's bun-sqlite transaction commits at the callback's first await. Use withWriteTransaction from src/db/write-transaction.ts, or a synchronous callback.", - }, - ], + }, + }, + { + files: ["src/**/*.ts"], + languageOptions: typedLanguageOptions, + plugins: { + local: { rules: { "no-promise-transaction-callback": noPromiseTransactionCallback } }, + }, + rules: { + "local/no-promise-transaction-callback": "error", }, }, ]; diff --git a/packages/api/package.json b/packages/api/package.json index 8c7bae96..5ae1e026 100644 --- a/packages/api/package.json +++ b/packages/api/package.json @@ -9,10 +9,10 @@ "start": "bun dist/index.js", "typecheck": "tsc --noEmit", "lint": "eslint src --ext .ts", - "test": "NODE_ENV=test bun test src", - "test:watch": "NODE_ENV=test bun test --watch src", + "test": "NODE_ENV=test bun test src eslint-rules", + "test:watch": "NODE_ENV=test bun test --watch src eslint-rules", "test:ui": "vitest --ui", - "test:coverage": "NODE_ENV=test bun test src --coverage", + "test:coverage": "NODE_ENV=test bun test src eslint-rules --coverage", "demo:seed": "bun src/lib/demo-seed.ts" }, "dependencies": { @@ -39,4 +39,4 @@ "typescript": "^5.9.3", "vitest": "^4.0.16" } -} \ No newline at end of file +} diff --git a/packages/api/src/db/README.md b/packages/api/src/db/README.md index 35c19d29..3a60d728 100644 --- a/packages/api/src/db/README.md +++ b/packages/api/src/db/README.md @@ -8,7 +8,7 @@ See [ADR-0003: SQLite schema evolution](../../../docs/adr/0003-sqlite-schema-evo Use `openOperationalDatabase(optionalPath?)` from the client module. It resolves the file path, enables WAL, turns on foreign keys, runs schema evolution, and returns Drizzle + raw `bun:sqlite` handles. Importing the client module does **not** open a connection. -Multi-statement writes that `await` must use `withWriteTransaction` from `write-transaction.ts`. Drizzle's bun-sqlite `db.transaction(async () => …)` commits when the callback first yields; it does not await the returned Promise. ESLint rejects an `async` callback passed to `.transaction(...)`. +Multi-statement writes that `await` must use `withWriteTransaction` from `write-transaction.ts`. Drizzle's bun-sqlite `db.transaction(async () => …)` commits when the callback first yields; it does not await the returned Promise. ESLint (`eslint-rules/no-promise-transaction-callback.mjs`, tests included) rejects any callback passed to `.transaction(...)` whose return type is Promise-like: an `async` function, a named async function, or a sync function that returns a Promise. Tests should use the same entry point (e.g. `openOperationalDatabase(':memory:')` via `setupTestDatabase()`), except a test that checks the snapshot itself: `__tests__/initial-schema-snapshot.test.ts` applies `initial_schema.sql` alone, because the entry point would run pending migrations on top and hide a stale snapshot. diff --git a/packages/api/tsconfig.eslint.json b/packages/api/tsconfig.eslint.json new file mode 100644 index 00000000..1333611a --- /dev/null +++ b/packages/api/tsconfig.eslint.json @@ -0,0 +1,6 @@ +{ + "extends": "./tsconfig.json", + "compilerOptions": { "noEmit": true }, + "include": ["src/**/*"], + "exclude": ["node_modules", "dist"] +} From c524782c607beeb6d1e54e041ca3fe5fabbbcdd4 Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Tue, 6 Oct 2026 16:55:11 -0700 Subject: [PATCH 12/30] refactor(api): take an insert's returned row through one insertedRow helper. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../src/lib/__tests__/inserted-row.test.ts | 12 ++++++++ packages/api/src/lib/bulk-combined-import.ts | 13 ++++----- .../src/lib/collections/collection-create.ts | 13 ++++----- .../lib/collections/collection-lifecycle.ts | 13 ++++----- packages/api/src/lib/container-creation.ts | 6 ++-- .../api/src/lib/container-write-placement.ts | 4 +-- packages/api/src/lib/controls/batch-create.ts | 6 ++-- .../src/lib/controls/batch-with-specimens.ts | 16 +++------- packages/api/src/lib/crud-routes.ts | 7 ++--- packages/api/src/lib/demo-seed.ts | 29 +++++-------------- packages/api/src/lib/derivations.ts | 4 +-- packages/api/src/lib/inserted-row.ts | 9 ++++++ .../api/src/lib/registration-orchestrator.ts | 7 ++--- packages/api/src/routes/controls.ts | 6 ++-- packages/api/src/routes/reagents.ts | 25 ++++++++-------- packages/api/src/routes/specimens.ts | 17 +++++------ packages/api/src/routes/studies.ts | 29 +++++++++---------- 17 files changed, 97 insertions(+), 119 deletions(-) create mode 100644 packages/api/src/lib/__tests__/inserted-row.test.ts create mode 100644 packages/api/src/lib/inserted-row.ts diff --git a/packages/api/src/lib/__tests__/inserted-row.test.ts b/packages/api/src/lib/__tests__/inserted-row.test.ts new file mode 100644 index 00000000..a1952174 --- /dev/null +++ b/packages/api/src/lib/__tests__/inserted-row.test.ts @@ -0,0 +1,12 @@ +import { describe, it, expect } from 'vitest' +import { insertedRow } from '../inserted-row' + +describe('insertedRow', () => { + it('returns the first returned row', () => { + expect(insertedRow([{ id: 1 }, { id: 2 }], 'plate')).toEqual({ id: 1 }) + }) + + it('throws, naming the table, when the insert returned nothing', () => { + expect(() => insertedRow([], 'plate')).toThrow('Insert did not return plate row') + }) +}) diff --git a/packages/api/src/lib/bulk-combined-import.ts b/packages/api/src/lib/bulk-combined-import.ts index 6b914cae..83ad00fa 100644 --- a/packages/api/src/lib/bulk-combined-import.ts +++ b/packages/api/src/lib/bulk-combined-import.ts @@ -33,6 +33,7 @@ import { type BulkCombinedContainerInput, } from './container-write-placement' import { withWriteTransaction } from '../db/write-transaction' +import { insertedRow } from './inserted-row' export type { BulkCombinedContainerInput } @@ -245,12 +246,8 @@ export async function createSubjectWithSpecimensInTx( updatedBy: userId, }) .returning() - .get() - const insertedSubject = Array.isArray(newSubjectResult) ? newSubjectResult[0] : newSubjectResult - if (!insertedSubject) { - throw new Error('Insert did not return study subject row') - } - subject = insertedSubject as typeof studySubject.$inferSelect + .all() + subject = insertedRow(newSubjectResult, 'study subject') subjectId = subject.id } @@ -277,8 +274,8 @@ export async function createSubjectWithSpecimensInTx( updatedBy: userId, }) .returning() - .get() - specimenRecord = (Array.isArray(newSpecimenResult) ? newSpecimenResult[0] : newSpecimenResult) as typeof specimen.$inferSelect + .all() + specimenRecord = insertedRow(newSpecimenResult, 'specimen') specimenCreated = true } const specimenId = specimenRecord.id diff --git a/packages/api/src/lib/collections/collection-create.ts b/packages/api/src/lib/collections/collection-create.ts index cf80fe65..b49c3489 100644 --- a/packages/api/src/lib/collections/collection-create.ts +++ b/packages/api/src/lib/collections/collection-create.ts @@ -14,6 +14,7 @@ export { CollectionLocationNotFoundError, CollectionLocationNotAllowedError, } from './collection-lifecycle' +import { insertedRow } from '../inserted-row' export class CollectionNameExistsError extends Error { constructor(message: string) { @@ -40,8 +41,7 @@ export async function createMicronixPlate(database: Database, input: CreateColle updatedBy: input.userId, }) .returning() - const newPlate = inserted.at(0) - if (!newPlate) throw new Error('Insert did not return plate row') + const newPlate = insertedRow(inserted, 'plate') return { plate: { ...newPlate, locationPath: formatLocationPath(loc) } } } @@ -63,8 +63,7 @@ export async function createCryovialBox(database: Database, input: CreateCollect updatedBy: input.userId, }) .returning() - const newBox = inserted.at(0) - if (!newBox) throw new Error('Insert did not return cryovial box row') + const newBox = insertedRow(inserted, 'cryovial box') return { box: { ...newBox, locationPath: formatLocationPath(loc) } } } @@ -85,8 +84,7 @@ export async function createGenericBox(database: Database, input: CreateCollecti updatedBy: input.userId, }) .returning() - const newBox = inserted.at(0) - if (!newBox) throw new Error('Insert did not return box row') + const newBox = insertedRow(inserted, 'box') return { box: { ...newBox, locationPath: formatLocationPath(loc) } } } @@ -107,7 +105,6 @@ export async function createBag(database: Database, input: CreateCollectionInput updatedBy: input.userId, }) .returning() - const newBag = inserted.at(0) - if (!newBag) throw new Error('Insert did not return bag row') + const newBag = insertedRow(inserted, 'bag') return { bag: { ...newBag, locationPath: formatLocationPath(loc) } } } diff --git a/packages/api/src/lib/collections/collection-lifecycle.ts b/packages/api/src/lib/collections/collection-lifecycle.ts index 7f686d01..cb266a52 100644 --- a/packages/api/src/lib/collections/collection-lifecycle.ts +++ b/packages/api/src/lib/collections/collection-lifecycle.ts @@ -6,6 +6,7 @@ import type * as schema from '../../db/schema' import { location, micronixPlate, cryovialBox, box, bag } from '../../db/schema' import { resolveCollection } from './collection-resolve' import { utcNow } from '../datetime' +import { insertedRow } from '../inserted-row' type DatabaseOrTransaction = | Database @@ -94,8 +95,7 @@ export async function createOrResolveCollection( updatedBy: input.userId, }) .returning() - const row = inserted.at(0) - if (!row) throw new Error('Insert did not return box row') + const row = insertedRow(inserted, 'box') collectionMap?.set(key, row.id) return row.id } @@ -112,8 +112,7 @@ export async function createOrResolveCollection( updatedBy: input.userId, }) .returning() - const row = inserted.at(0) - if (!row) throw new Error('Insert did not return bag row') + const row = insertedRow(inserted, 'bag') collectionMap?.set(key, row.id) return row.id } @@ -131,8 +130,7 @@ export async function createOrResolveCollection( updatedBy: input.userId, }) .returning() - const row = inserted.at(0) - if (!row) throw new Error('Insert did not return micronix plate row') + const row = insertedRow(inserted, 'micronix plate') collectionMap?.set(key, row.id) return row.id } @@ -149,8 +147,7 @@ export async function createOrResolveCollection( updatedBy: input.userId, }) .returning() - const row = inserted.at(0) - if (!row) throw new Error('Insert did not return cryovial box row') + const row = insertedRow(inserted, 'cryovial box') collectionMap?.set(key, row.id) return row.id } diff --git a/packages/api/src/lib/container-creation.ts b/packages/api/src/lib/container-creation.ts index 0b19fa63..804d23b6 100644 --- a/packages/api/src/lib/container-creation.ts +++ b/packages/api/src/lib/container-creation.ts @@ -19,6 +19,7 @@ import { ValidationError } from './error-handler' import type { ContainerWriteInput } from '@sampledb/contract' import { ensureContainerPlacement, type EnsuredPlacement } from './container-write-placement' import { checkGridPositionOccupancy, occupiedGridPositionMessage } from './container-occupancy' +import { insertedRow } from './inserted-row' type DatabaseOrTransaction = | Database @@ -138,10 +139,7 @@ async function insertStorageContainer( updatedBy: userId, }).returning() - const container = inserted[0] - // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- runtime invariant per avoid-masking-bugs: insert must return row - if (inserted.length === 0 || container === undefined) throw new Error('Insert did not return container row') - return { id: container.id } + return { id: insertedRow(inserted, 'container').id } } async function createMicronixTube( diff --git a/packages/api/src/lib/container-write-placement.ts b/packages/api/src/lib/container-write-placement.ts index 9ee0a7ff..a1567b25 100644 --- a/packages/api/src/lib/container-write-placement.ts +++ b/packages/api/src/lib/container-write-placement.ts @@ -14,6 +14,7 @@ import { } from './collections/collection-lifecycle' import { utcNow } from './datetime' import { ValidationError } from './error-handler' +import { insertedRow } from './inserted-row' type DatabaseOrTransaction = | Database @@ -321,8 +322,7 @@ async function ensureSheetId( updatedBy: options?.userId, }) .returning() - const row = inserted.at(0) - if (!row) throw new Error('Insert did not return sheet row') + const row = insertedRow(inserted, 'sheet') return row.id } diff --git a/packages/api/src/lib/controls/batch-create.ts b/packages/api/src/lib/controls/batch-create.ts index 629cf897..6f7bae3b 100644 --- a/packages/api/src/lib/controls/batch-create.ts +++ b/packages/api/src/lib/controls/batch-create.ts @@ -4,6 +4,7 @@ import { eq, and } from 'drizzle-orm' import { validateControlBatchName, generateUniqueBatchName } from '../validation' import { NotFoundError, ValidationError } from '../error-handler' import type { CreateBloodControlBatchInput } from './batch-schemas' +import { insertedRow } from '../inserted-row' function isUniqueConstraintError(error: unknown): boolean { if (!(error instanceof Error)) return false @@ -53,10 +54,7 @@ export async function createBloodControlBatch( updatedBy: userId, }) .returning() - const newBatch = inserted.at(0) - if (!newBatch) { - throw new Error('Insert did not return control batch row') - } + const newBatch = insertedRow(inserted, 'control batch') return newBatch } catch (error) { if (isUniqueConstraintError(error)) { diff --git a/packages/api/src/lib/controls/batch-with-specimens.ts b/packages/api/src/lib/controls/batch-with-specimens.ts index 7d57eabf..2f41e999 100644 --- a/packages/api/src/lib/controls/batch-with-specimens.ts +++ b/packages/api/src/lib/controls/batch-with-specimens.ts @@ -27,6 +27,7 @@ import type { BatchContainerInput } from './batch-schemas' import { withWriteTransaction } from '../../db/write-transaction' import { validateCollectionDate } from '../validation' import { ConflictError, ValidationError, sqliteConstraintConflict } from '../error-handler' +import { insertedRow } from '../inserted-row' export interface CreateBatchWithSpecimensRequest { batch: { @@ -238,10 +239,7 @@ export async function createBatchWithSpecimens( }) .returning() - const batchRecord = batchResult.at(0) - if (!batchRecord) { - throw new Error('Failed to create batch - no record returned') - } + const batchRecord = insertedRow(batchResult, 'control batch') const batchId = extractId(batchRecord) const createdSpecimens: CreatedSpecimen[] = [] @@ -256,10 +254,7 @@ export async function createBatchWithSpecimens( }) .returning() - const specimenRecord = specimenResult.at(0) - if (!specimenRecord) { - throw new Error('Failed to create specimen - no record returned') - } + const specimenRecord = insertedRow(specimenResult, 'specimen') const specimenId = extractId(specimenRecord) const containerIds: number[] = [] @@ -346,10 +341,7 @@ export async function addSpecimensToBatch( }) .returning() - const specimenRecord = specimenResult.at(0) - if (!specimenRecord) { - throw new Error('Failed to create specimen - no record returned') - } + const specimenRecord = insertedRow(specimenResult, 'specimen') specimenId = extractId(specimenRecord) } diff --git a/packages/api/src/lib/crud-routes.ts b/packages/api/src/lib/crud-routes.ts index 36d675d2..1144dae2 100644 --- a/packages/api/src/lib/crud-routes.ts +++ b/packages/api/src/lib/crud-routes.ts @@ -8,6 +8,7 @@ import { handleRouteError, NotFoundError, ConflictError, ValidationError } from import { listResponse, successResponse, createdResponse } from './response-helpers' import { requireParam } from './common-validators' import { createAdminMiddleware, createAuthMiddleware } from '../middleware/auth' +import { insertedRow } from './inserted-row' export interface CrudRouteConfig< TTable extends SQLiteTable, @@ -230,11 +231,7 @@ export function createCrudRoutes< .values(insertData as any) .returning() - const row = result[0] - // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- runtime invariant per avoid-masking-bugs: insert must return row - if (!row) { - throw new Error('Insert did not return row') - } + const row = insertedRow(result, entityName) const transformed = transformDetail ? transformDetail(row as TSelect) : row return createdResponse(c, transformed) } catch (error) { diff --git a/packages/api/src/lib/demo-seed.ts b/packages/api/src/lib/demo-seed.ts index 61f3a255..cecc1103 100644 --- a/packages/api/src/lib/demo-seed.ts +++ b/packages/api/src/lib/demo-seed.ts @@ -33,6 +33,7 @@ import { createBatchWithSpecimens } from './controls/batch-with-specimens' import { createDerivation } from './derivations' import { utcNow } from './datetime' import { controlDefinition, box, sheet, micronixPlate, strain } from '../db/schema' +import { insertedRow } from './inserted-row' // Demo defaults (aligned with packages/web setup-defaults, inlined to avoid cross-package deps) const DEMO_SPECIMEN_TYPES = [ @@ -107,9 +108,7 @@ export async function runDemoSeed( .insert(storageType) .values({ id: i + 1, name: s.name, description: s.description }) .returning() - const row = Array.isArray(result) ? result[0] : result - // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- runtime invariant per avoid-masking-bugs: insert must return row - if (!row) throw new Error('Insert did not return storage type row') + const row = insertedRow(result, 'storage type') storageTypeMap.set(s.name, row.id) } @@ -265,9 +264,7 @@ export async function runDemoSeed( createdBy: 1, updatedBy: 1, }).returning() - const negDef = Array.isArray(negResult) ? negResult[0] : negResult - // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- runtime invariant per avoid-masking-bugs: insert must return row - if (!negDef) throw new Error('Insert did not return control definition row') + const negDef = insertedRow(negResult, 'control definition') const negDefId = negDef.id const plasmaPosResult = await database.insert(controlDefinition).values({ @@ -278,9 +275,7 @@ export async function runDemoSeed( createdBy: 1, updatedBy: 1, }).returning() - const plasmaPosDef = Array.isArray(plasmaPosResult) ? plasmaPosResult[0] : plasmaPosResult - // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- runtime invariant per avoid-masking-bugs: insert must return row - if (!plasmaPosDef) throw new Error('Insert did not return control definition row') + const plasmaPosDef = insertedRow(plasmaPosResult, 'control definition') const plasmaPosDefId = plasmaPosDef.id const bloodResult = await database.insert(controlDefinition).values({ @@ -300,9 +295,7 @@ export async function runDemoSeed( createdBy: 1, updatedBy: 1, }).returning() - const bloodDef = Array.isArray(bloodResult) ? bloodResult[0] : bloodResult - // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- runtime invariant per avoid-masking-bugs: insert must return row - if (!bloodDef) throw new Error('Insert did not return control definition row') + const bloodDef = insertedRow(bloodResult, 'control definition') const bloodDefId = bloodDef.id const controlBatchResult1 = await createBatchWithSpecimens(database, { @@ -381,9 +374,7 @@ export async function runDemoSeed( created: now, lastUpdated: now, }).returning() - const dbsBox = Array.isArray(dbsBoxResult) ? dbsBoxResult[0] : dbsBoxResult - // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- runtime invariant per avoid-masking-bugs: insert must return row - if (!dbsBox) throw new Error('Insert did not return box row') + const dbsBox = insertedRow(dbsBoxResult, 'box') const dbsBoxId = dbsBox.id const dbsSheetResult = await database.insert(sheet).values({ @@ -393,9 +384,7 @@ export async function runDemoSeed( created: now, lastUpdated: now, }).returning() - const dbsSheet = Array.isArray(dbsSheetResult) ? dbsSheetResult[0] : dbsSheetResult - // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- runtime invariant per avoid-masking-bugs: insert must return row - if (!dbsSheet) throw new Error('Insert did not return sheet row') + const dbsSheet = insertedRow(dbsSheetResult, 'sheet') const dbsSheetId = dbsSheet.id const bloodDnaPlateResult = await database.insert(micronixPlate).values({ @@ -405,9 +394,7 @@ export async function runDemoSeed( created: now, lastUpdated: now, }).returning() - const bloodDnaPlate = Array.isArray(bloodDnaPlateResult) ? bloodDnaPlateResult[0] : bloodDnaPlateResult - // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- runtime invariant per avoid-masking-bugs: insert must return row - if (!bloodDnaPlate) throw new Error('Insert did not return plate row') + const bloodDnaPlate = insertedRow(bloodDnaPlateResult, 'plate') const bloodDnaPlateId = bloodDnaPlate.id const bloodContainerIds = controlBatchResult3.specimens.flatMap((s) => s.containerIds) diff --git a/packages/api/src/lib/derivations.ts b/packages/api/src/lib/derivations.ts index 6c53ea02..e04e051e 100644 --- a/packages/api/src/lib/derivations.ts +++ b/packages/api/src/lib/derivations.ts @@ -16,6 +16,7 @@ import { getDefaultUnit } from './defaults' import { utcNow } from './datetime' import { createContainerForSpecimen, pickContainerQuantity } from './container-creation' import { withWriteTransaction } from '../db/write-transaction' +import { insertedRow } from './inserted-row' export type DerivationType = string @@ -122,8 +123,7 @@ async function findOrCreateDerivedSpecimen( lastUpdated: utcNow(), }) .returning() - const created = inserted.at(0) - if (!created) throw new Error('Insert did not return specimen row') + const created = insertedRow(inserted, 'specimen') return created.id } diff --git a/packages/api/src/lib/inserted-row.ts b/packages/api/src/lib/inserted-row.ts new file mode 100644 index 00000000..10b7bca8 --- /dev/null +++ b/packages/api/src/lib/inserted-row.ts @@ -0,0 +1,9 @@ +/** + * The row an `INSERT … RETURNING` wrote. SQLite always returns it, so an empty result is a bug: + * throw rather than carry on with `undefined`. + */ +export function insertedRow(rows: readonly T[], table: string): T { + const row = rows.at(0) + if (row === undefined) throw new Error(`Insert did not return ${table} row`) + return row +} diff --git a/packages/api/src/lib/registration-orchestrator.ts b/packages/api/src/lib/registration-orchestrator.ts index 8fe45c8d..50f2b54b 100644 --- a/packages/api/src/lib/registration-orchestrator.ts +++ b/packages/api/src/lib/registration-orchestrator.ts @@ -38,6 +38,7 @@ export { prepareCombinedSubjectContainerBatch, validateSpecimenContainerRegistration, } from './registration-prepare' +import { insertedRow } from './inserted-row' /** * Validate bulk specimen rows without creating records. @@ -117,10 +118,8 @@ export async function createBulkSpecimenRows( updatedBy: userId, }) .returning() - .get() - const inserted = Array.isArray(insertResult) ? insertResult[0] : insertResult - if (!inserted) throw new Error('Insert did not return specimen row') - specimenRecordsByUniqueIndex.push(inserted as typeof specimen.$inferSelect) + .all() + specimenRecordsByUniqueIndex.push(insertedRow(insertResult, 'specimen')) newCount += 1 } } diff --git a/packages/api/src/routes/controls.ts b/packages/api/src/routes/controls.ts index 1e4969a1..d7e2371c 100644 --- a/packages/api/src/routes/controls.ts +++ b/packages/api/src/routes/controls.ts @@ -46,6 +46,7 @@ import { findMatchingDefinitionInList, buildBloodControlPropertiesPayload, } from '../lib/controls/strain-composition' +import { insertedRow } from '../lib/inserted-row' /** * Create controls routes with database injection @@ -519,10 +520,7 @@ controls.post('/', memberMiddleware, async (c) => { }) .returning() - const newControl = result.at(0) - if (!newControl) { - throw new Error('Insert did not return control definition row') - } + const newControl = insertedRow(result, 'control definition') return c.json({ control: newControl }, 201) } catch (error) { return handleRouteError(error, c) diff --git a/packages/api/src/routes/reagents.ts b/packages/api/src/routes/reagents.ts index 5b4f72a4..6e55b57e 100644 --- a/packages/api/src/routes/reagents.ts +++ b/packages/api/src/routes/reagents.ts @@ -8,6 +8,7 @@ import { createAuthMiddleware, createMemberMiddleware } from '../middleware/auth import { utcNow } from '../lib/datetime' import { requireParam } from '../lib/common-validators' import { handleRouteError } from '../lib/error-handler' +import { insertedRow } from '../lib/inserted-row' /** * Create reagents routes with database injection @@ -92,19 +93,17 @@ reagents.post('/', memberMiddleware, async (c) => { const data = schema.parse(body) const user = c.get('user') - const [newReagent] = await database - .insert(reagent) - .values({ - ...data, - createdBy: user?.id, - updatedBy: user?.id, - }) - .returning() - - // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- runtime invariant per avoid-masking-bugs: insert must return row - if (!newReagent) { - throw new Error('Insert did not return reagent row') - } + const newReagent = insertedRow( + await database + .insert(reagent) + .values({ + ...data, + createdBy: user?.id, + updatedBy: user?.id, + }) + .returning(), + 'reagent', + ) return c.json({ reagent: newReagent }, 201) } catch (error) { return handleRouteError(error, c) diff --git a/packages/api/src/routes/specimens.ts b/packages/api/src/routes/specimens.ts index 808b456e..55c421b0 100644 --- a/packages/api/src/routes/specimens.ts +++ b/packages/api/src/routes/specimens.ts @@ -16,6 +16,7 @@ import { createAuthMiddleware, createMemberMiddleware } from '../middleware/auth import { utcNow } from '../lib/datetime' import { requireParam } from '../lib/common-validators' import { validateBulkSpecimenRows, createBulkSpecimenRows } from '../lib/registration-orchestrator' +import { insertedRow } from '../lib/inserted-row' /** * Create specimens routes with database injection @@ -178,15 +179,13 @@ specimens.post('/', memberMiddleware, async (c) => { insertData.collectionDate = validation.resolved.collectionDate } - const [newSpecimen] = await dbInstance - .insert(specimen) - .values(insertData) - .returning() - - // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- runtime invariant per avoid-masking-bugs: insert must return row - if (!newSpecimen) { - throw new Error('Insert did not return specimen row') - } + const newSpecimen = insertedRow( + await dbInstance + .insert(specimen) + .values(insertData) + .returning(), + 'specimen', + ) let containerResult: { success: boolean; containerId?: number; error?: string } | null = null if (data.container) { diff --git a/packages/api/src/routes/studies.ts b/packages/api/src/routes/studies.ts index e006ccd8..840e7fc4 100644 --- a/packages/api/src/routes/studies.ts +++ b/packages/api/src/routes/studies.ts @@ -24,6 +24,7 @@ import { utcNow } from '../lib/datetime' import { requireParam } from '../lib/common-validators' import { withWriteTransaction } from '../db/write-transaction' import { assertNotUsedInQpcr } from '../lib/qpcr-usage' +import { insertedRow } from '../lib/inserted-row' /** Short code prefix for tutorial namespace. Any study whose short code starts with this (case-insensitive) may be deleted by any member. Only admins may rename a study into or out of this namespace. */ const TUTORIAL_SHORT_CODE_PREFIX = 'TUT' @@ -578,21 +579,19 @@ studies.post('/', memberMiddleware, async (c) => { const data = schema.parse(body) const user = c.get('user') - const [newStudy] = await database - .insert(study) - .values({ - ...data, - created: utcNow(), - lastUpdated: utcNow(), - createdBy: user?.id, - updatedBy: user?.id, - }) - .returning() - - // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- runtime invariant per avoid-masking-bugs: insert must return row - if (!newStudy) { - throw new Error('Insert did not return study row') - } + const newStudy = insertedRow( + await database + .insert(study) + .values({ + ...data, + created: utcNow(), + lastUpdated: utcNow(), + createdBy: user?.id, + updatedBy: user?.id, + }) + .returning(), + 'study', + ) return c.json({ study: newStudy }, 201) } catch (error) { return handleRouteError(error, c) From c7d696e72c787f35421848ac6b206082b65b371b Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Tue, 6 Oct 2026 16:56:42 -0700 Subject: [PATCH 13/30] refactor(api): branch on every collection type explicitly when deleting a collection. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../api/src/lib/collection-delete-cascade.ts | 183 ++++++++---------- .../src/routes/__tests__/collections.test.ts | 68 +++++++ 2 files changed, 154 insertions(+), 97 deletions(-) diff --git a/packages/api/src/lib/collection-delete-cascade.ts b/packages/api/src/lib/collection-delete-cascade.ts index 776578f0..82127e73 100644 --- a/packages/api/src/lib/collection-delete-cascade.ts +++ b/packages/api/src/lib/collection-delete-cascade.ts @@ -49,71 +49,98 @@ export interface DeleteCollectionWithContentsResult { subjectsDeleted: number } -/** Collect all storage_container ids in this collection (tubes, wells, papers, etc.) */ -export async function collectContainerIdsInCollection( - database: Database, - type: CollectionEntityType, - id: number -): Promise { - if (type === 'micronix_plate') { - const [tubeRows, wellRows] = await Promise.all([ - database - .select({ id: micronixTube.id }) - .from(micronixTube) - .where(eq(micronixTube.collectionId, id)) - .all(), - database - .select({ id: staticWell.id }) - .from(staticWell) - .where(eq(staticWell.collectionId, id)) - .all(), - ]) - return [...tubeRows.map((x) => x.id), ...wellRows.map((x) => x.id)] - } - if (type === 'cryovial_box') { - const rows = await database - .select({ id: cryovialTube.id }) - .from(cryovialTube) - .where(eq(cryovialTube.collectionId, id)) - .all() - return rows.map((x) => x.id) - } - if (type === 'box') { - const rows = await database - .select({ id: paper.id }) - .from(paper) - .innerJoin(sheet, eq(paper.sheetId, sheet.id)) - .where(eq(sheet.boxId, id)) - .all() - return rows.map((x) => x.id) - } - const rows = await database +/** Per-kind table access. A Record, so adding a CollectionEntityType fails to compile until it has an entry. */ +interface CollectionKind { + label: string + /** All storage_container ids in the collection (tubes, wells, papers). */ + containerIds(database: Database, id: number): number[] + exists(database: Database, id: number): boolean + /** Deletes the collection row (and a box's or bag's sheets); returns the number of sheets deleted. */ + deleteRow(database: Database, id: number): number +} + +function paperIdsOnSheets(database: Database, sheetColumn: typeof sheet.boxId | typeof sheet.bagId, id: number): number[] { + return database .select({ id: paper.id }) .from(paper) .innerJoin(sheet, eq(paper.sheetId, sheet.id)) - .where(eq(sheet.bagId, id)) + .where(eq(sheetColumn, id)) .all() - return rows.map((x) => x.id) + .map((x) => x.id) +} + +function deleteSheets(database: Database, sheetColumn: typeof sheet.boxId | typeof sheet.bagId, id: number): number { + return database.delete(sheet).where(eq(sheetColumn, id)).returning({ id: sheet.id }).all().length +} + +const COLLECTION_KINDS: Record = { + micronix_plate: { + label: 'Micronix plate', + containerIds: (database, id) => [ + ...database.select({ id: micronixTube.id }).from(micronixTube).where(eq(micronixTube.collectionId, id)).all(), + ...database.select({ id: staticWell.id }).from(staticWell).where(eq(staticWell.collectionId, id)).all(), + ].map((x) => x.id), + exists: (database, id) => + database.select({ id: micronixPlate.id }).from(micronixPlate).where(eq(micronixPlate.id, id)).get() != null, + deleteRow: (database, id) => { + requireDeleted(database.delete(micronixPlate).where(eq(micronixPlate.id, id)).returning().all(), 'Micronix plate', id) + return 0 + }, + }, + cryovial_box: { + label: 'Cryovial box', + containerIds: (database, id) => + database + .select({ id: cryovialTube.id }) + .from(cryovialTube) + .where(eq(cryovialTube.collectionId, id)) + .all() + .map((x) => x.id), + exists: (database, id) => + database.select({ id: cryovialBox.id }).from(cryovialBox).where(eq(cryovialBox.id, id)).get() != null, + deleteRow: (database, id) => { + requireDeleted(database.delete(cryovialBox).where(eq(cryovialBox.id, id)).returning().all(), 'Cryovial box', id) + return 0 + }, + }, + box: { + label: 'Box', + containerIds: (database, id) => paperIdsOnSheets(database, sheet.boxId, id), + exists: (database, id) => database.select({ id: box.id }).from(box).where(eq(box.id, id)).get() != null, + deleteRow: (database, id) => { + const sheetsDeleted = deleteSheets(database, sheet.boxId, id) + requireDeleted(database.delete(box).where(eq(box.id, id)).returning().all(), 'Box', id) + return sheetsDeleted + }, + }, + bag: { + label: 'Bag', + containerIds: (database, id) => paperIdsOnSheets(database, sheet.bagId, id), + exists: (database, id) => database.select({ id: bag.id }).from(bag).where(eq(bag.id, id)).get() != null, + deleteRow: (database, id) => { + const sheetsDeleted = deleteSheets(database, sheet.bagId, id) + requireDeleted(database.delete(bag).where(eq(bag.id, id)).returning().all(), 'Bag', id) + return sheetsDeleted + }, + }, +} + +function requireDeleted(rows: unknown[], label: string, id: number): void { + if (rows.length === 0) throw new NotFoundError(label, id) } -async function ensureCollectionRowExists( +/** Collect all storage_container ids in this collection (tubes, wells, papers, etc.) */ +export async function collectContainerIdsInCollection( database: Database, type: CollectionEntityType, id: number -): Promise { - if (type === 'micronix_plate') { - const p = await database.select().from(micronixPlate).where(eq(micronixPlate.id, id)).get() - if (!p) throw new NotFoundError('Micronix plate', id) - } else if (type === 'cryovial_box') { - const b = await database.select().from(cryovialBox).where(eq(cryovialBox.id, id)).get() - if (!b) throw new NotFoundError('Cryovial box', id) - } else if (type === 'box') { - const b = await database.select().from(box).where(eq(box.id, id)).get() - if (!b) throw new NotFoundError('Box', id) - } else { - const b = await database.select().from(bag).where(eq(bag.id, id)).get() - if (!b) throw new NotFoundError('Bag', id) - } +): Promise { + return COLLECTION_KINDS[type].containerIds(database, id) +} + +function ensureCollectionRowExists(database: Database, type: CollectionEntityType, id: number): void { + const kind = COLLECTION_KINDS[type] + if (!kind.exists(database, id)) throw new NotFoundError(kind.label, id) } /** @@ -291,7 +318,7 @@ export async function preflightCollectionDelete( database: Database, request: Pick ): Promise { - await ensureCollectionRowExists(database, request.type, request.id) + ensureCollectionRowExists(database, request.type, request.id) const containerIdList = await collectContainerIdsInCollection(database, request.type, request.id) const containerIdSet = new Set(containerIdList) @@ -318,7 +345,7 @@ export async function deleteCollectionWithContents( ): Promise { const { type, id, removeEmptySubjects } = request - await ensureCollectionRowExists(database, type, id) + ensureCollectionRowExists(database, type, id) const containerIdList = await collectContainerIdsInCollection(database, type, id) const containerIdSet = new Set(containerIdList) @@ -395,45 +422,7 @@ export async function deleteCollectionWithContents( } } - if (type === 'box') { - const sheetRows = await tx - .select({ id: sheet.id }) - .from(sheet) - .where(eq(sheet.boxId, id)) - .all() - sheetsDeleted = sheetRows.length - if (sheetRows.length > 0) { - tx.delete(sheet).where(eq(sheet.boxId, id)).run() - } - const boxDel = await tx.delete(box).where(eq(box.id, id)).returning() - if (boxDel.length === 0) { - throw new NotFoundError('Box', id) - } - } else if (type === 'bag') { - const sheetRows = await tx - .select({ id: sheet.id }) - .from(sheet) - .where(eq(sheet.bagId, id)) - .all() - sheetsDeleted = sheetRows.length - if (sheetRows.length > 0) { - tx.delete(sheet).where(eq(sheet.bagId, id)).run() - } - const bagDel = await tx.delete(bag).where(eq(bag.id, id)).returning() - if (bagDel.length === 0) { - throw new NotFoundError('Bag', id) - } - } else if (type === 'micronix_plate') { - const plateDel = await tx.delete(micronixPlate).where(eq(micronixPlate.id, id)).returning() - if (plateDel.length === 0) { - throw new NotFoundError('Micronix plate', id) - } - } else { - const cryoDel = await tx.delete(cryovialBox).where(eq(cryovialBox.id, id)).returning() - if (cryoDel.length === 0) { - throw new NotFoundError('Cryovial box', id) - } - } + sheetsDeleted = COLLECTION_KINDS[type].deleteRow(tx, id) }) return { diff --git a/packages/api/src/routes/__tests__/collections.test.ts b/packages/api/src/routes/__tests__/collections.test.ts index c7d594f5..205e5f6d 100644 --- a/packages/api/src/routes/__tests__/collections.test.ts +++ b/packages/api/src/routes/__tests__/collections.test.ts @@ -22,6 +22,11 @@ import { storageContainer, micronixTube, box, + bag, + sheet, + paper, + cryovialBox, + cryovialTube, micronixPlate, specimen, qpcrExperiment, @@ -336,6 +341,69 @@ describe('Collections API', () => { expect(res.status).toBe(404) }) + it.each(['micronix_plate', 'cryovial_box', 'box', 'bag'] as const)( + 'returns 404 when the %s does not exist', + async (collectionType) => { + const res = await ctx.request('/api/collections/delete-with-contents', { + method: 'POST', + json: { collectionType, id: 999_999, removeEmptySubjects: false }, + }) + expect(res.status).toBe(404) + }, + ) + + it.each(['cryovial_box', 'box', 'bag'] as const)( + 'deletes a %s with its container, specimen and sheets', + async (collectionType) => { + const storageType = await createTestStorageType(ctx.db, { name: `St-${collectionType}` }) + const loc = await createTestLocation(ctx.db, { + name: `Loc-${collectionType}`, + storageTypeId: String(storageType.id), + canContainCollections: true, + }) + const st = await createTestSpecimenType(ctx.db, { name: `ST-${collectionType}` }) + const sp = await createTestSpecimen(ctx.db, st.id) + const unit = await createTestUnit(ctx.db, { symbol: 'uL', name: 'microliter', category: 'volume' }) + const now = utcNow() + const [c] = await ctx.db + .insert(storageContainer) + .values({ specimenId: sp.id, unitId: unit.id, totalQuantity: 1, remainingQuantity: 1, created: now, lastUpdated: now }) + .returning() + if (!c) throw new Error('no container') + const collection = { name: `Coll-${collectionType}`, locationId: loc.id, created: now, lastUpdated: now } + + let id: number + if (collectionType === 'cryovial_box') { + const [row] = await ctx.db.insert(cryovialBox).values(collection).returning() + id = row!.id + await ctx.db.insert(cryovialTube).values({ id: c.id, collectionId: id, barcode: 'CRYO1', position: 'A1' }) + } else { + const table = collectionType === 'box' ? box : bag + const [row] = await ctx.db.insert(table).values(collection).returning() + id = row!.id + const [sh] = await ctx.db + .insert(sheet) + .values({ name: 'S1', boxId: collectionType === 'box' ? id : null, bagId: collectionType === 'bag' ? id : null, created: now, lastUpdated: now }) + .returning() + await ctx.db.insert(paper).values({ id: c.id, sheetId: sh!.id }) + } + + const res = await ctx.request('/api/collections/delete-with-contents', { + method: 'POST', + json: { collectionType, id, removeEmptySubjects: false }, + }) + expect(res.status).toBe(200) + expect(await res.json()).toMatchObject({ + containersDeleted: 1, + specimensDeleted: 1, + sheetsDeleted: collectionType === 'cryovial_box' ? 0 : 1, + collectionDeleted: true, + }) + expect(await ctx.db.select().from(storageContainer).where(eq(storageContainer.id, c.id)).get()).toBeUndefined() + expect(await ctx.db.select().from(sheet).all()).toEqual([]) + }, + ) + it('deletes a micronix plate, its tubes, and fully-contained specimens', async () => { const storageType = await createTestStorageType(ctx.db, { name: 'StDel' }) const loc = await createTestLocation(ctx.db, { From 97b03270fb920060f1ec6b653ebe3c7b9eae9bcd Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Tue, 6 Oct 2026 16:58:27 -0700 Subject: [PATCH 14/30] refactor(web): list the command dependencies once and rebuild commands only when one changes. useCommands now keeps the caller's deps object while its fields are shallow-equal, and commands take the route as pathname and search, the only parts of the location they read. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/web/src/App.tsx | 3 +- .../hooks/__tests__/useShallowStable.test.ts | 29 ++++++++++ packages/web/src/hooks/useShallowStable.ts | 18 +++++++ .../__tests__/registry.test.ts | 3 +- .../__tests__/useCommands.test.tsx | 45 ++++++++++++++++ .../src/lib/command-registry/command-deps.ts | 7 ++- .../lib/command-registry/groups/contextual.ts | 6 +-- .../src/lib/command-registry/groups/create.ts | 2 +- .../web/src/lib/command-registry/registry.ts | 54 ++----------------- 9 files changed, 110 insertions(+), 57 deletions(-) create mode 100644 packages/web/src/hooks/__tests__/useShallowStable.test.ts create mode 100644 packages/web/src/hooks/useShallowStable.ts create mode 100644 packages/web/src/lib/command-registry/__tests__/useCommands.test.tsx diff --git a/packages/web/src/App.tsx b/packages/web/src/App.tsx index f042beec..77c37bce 100644 --- a/packages/web/src/App.tsx +++ b/packages/web/src/App.tsx @@ -192,7 +192,8 @@ function AppContent() { const commands = useCommands({ navigate, - location, + pathname: location.pathname, + search: location.search, canWrite, isAdmin, canManageReferenceData, diff --git a/packages/web/src/hooks/__tests__/useShallowStable.test.ts b/packages/web/src/hooks/__tests__/useShallowStable.test.ts new file mode 100644 index 00000000..a3c1db75 --- /dev/null +++ b/packages/web/src/hooks/__tests__/useShallowStable.test.ts @@ -0,0 +1,29 @@ +import { describe, it, expect } from 'vitest' +import { renderHook } from '@testing-library/react' +import { useShallowStable } from '../useShallowStable' + +describe('useShallowStable', () => { + it('keeps the first object while a fresh one has the same fields', () => { + const fn = () => {} + const { result, rerender } = renderHook(({ value }) => useShallowStable(value), { + initialProps: { value: { a: 1, fn } }, + }) + const first = result.current + rerender({ value: { a: 1, fn } }) + expect(result.current).toBe(first) + }) + + it('returns the new object when a field changes', () => { + const { result, rerender } = renderHook(({ value }) => useShallowStable(value), { + initialProps: { value: { a: 1 } as Record }, + }) + const changed = { a: 2 } + rerender({ value: changed }) + expect(result.current).toBe(changed) + rerender({ value: { a: 2 } }) + expect(result.current).toBe(changed) + const added = { a: 2, b: 3 } + rerender({ value: added }) + expect(result.current).toBe(added) + }) +}) diff --git a/packages/web/src/hooks/useShallowStable.ts b/packages/web/src/hooks/useShallowStable.ts new file mode 100644 index 00000000..faaecbab --- /dev/null +++ b/packages/web/src/hooks/useShallowStable.ts @@ -0,0 +1,18 @@ +import { useState } from 'react' + +function shallowEqual(a: T, b: T): boolean { + const aKeys = Object.keys(a) as (keyof T)[] + return aKeys.length === Object.keys(b).length && aKeys.every((key) => Object.is(a[key], b[key])) +} + +/** + * Returns the previous object while every property is unchanged (`Object.is`), so a caller that + * builds an object each render can memoize on it without listing its fields. + */ +export function useShallowStable(value: T): T { + const [stable, setStable] = useState(value) + if (stable === value || shallowEqual(stable, value)) return stable + // Storing information from previous renders: https://react.dev/reference/react/useState#storing-information-from-previous-renders + setStable(value) + return value +} diff --git a/packages/web/src/lib/command-registry/__tests__/registry.test.ts b/packages/web/src/lib/command-registry/__tests__/registry.test.ts index e7b509df..052f7b53 100644 --- a/packages/web/src/lib/command-registry/__tests__/registry.test.ts +++ b/packages/web/src/lib/command-registry/__tests__/registry.test.ts @@ -5,7 +5,8 @@ import type { CommandDependencies } from '../command-deps' function mockDeps(overrides: Partial = {}): CommandDependencies { return { navigate: vi.fn() as CommandDependencies['navigate'], - location: { pathname: '/', search: '', hash: '', state: null, key: 'default' }, + pathname: '/', + search: '', canWrite: true, isAdmin: false, canManageReferenceData: false, diff --git a/packages/web/src/lib/command-registry/__tests__/useCommands.test.tsx b/packages/web/src/lib/command-registry/__tests__/useCommands.test.tsx new file mode 100644 index 00000000..1e2c881e --- /dev/null +++ b/packages/web/src/lib/command-registry/__tests__/useCommands.test.tsx @@ -0,0 +1,45 @@ +import { describe, it, expect, vi } from 'vitest' +import { renderHook } from '@testing-library/react' +import { useCommands } from '../registry' +import type { CommandDependencies } from '../command-deps' + +const stable = { + navigate: vi.fn() as CommandDependencies['navigate'], + setTheme: vi.fn(), + toggleHelpModal: vi.fn(), + openSearchModal: vi.fn(), + refreshUser: vi.fn(async () => {}), + handleExportSpecimens: vi.fn(), + handleExportInventory: vi.fn(), + handleClearFilters: vi.fn(), +} + +const deps = (overrides: Partial = {}): CommandDependencies => ({ + ...stable, + pathname: '/', + search: '', + canWrite: true, + isAdmin: false, + canManageReferenceData: false, + theme: 'light', + ...overrides, +}) + +describe('useCommands', () => { + it('keeps the same commands when the caller passes a fresh but equal deps object', () => { + const { result, rerender } = renderHook(({ d }) => useCommands(d), { initialProps: { d: deps() } }) + const first = result.current + rerender({ d: deps() }) + expect(result.current).toBe(first) + }) + + it('rebuilds when the route or a permission changes', () => { + const { result, rerender } = renderHook(({ d }) => useCommands(d), { initialProps: { d: deps() } }) + const first = result.current + rerender({ d: deps({ search: '?q=1' }) }) + const second = result.current + expect(second).not.toBe(first) + rerender({ d: deps({ search: '?q=1', isAdmin: true }) }) + expect(result.current).not.toBe(second) + }) +}) diff --git a/packages/web/src/lib/command-registry/command-deps.ts b/packages/web/src/lib/command-registry/command-deps.ts index 0caf0066..a6082b8e 100644 --- a/packages/web/src/lib/command-registry/command-deps.ts +++ b/packages/web/src/lib/command-registry/command-deps.ts @@ -1,12 +1,15 @@ -import type { NavigateFunction, Location } from 'react-router-dom' +import type { NavigateFunction } from 'react-router-dom' import type { Theme } from '../../contexts/ThemeContext' /** * Dependencies injected from the app shell for building command lists. + * Keep every field a primitive or a stable function: useCommands rebuilds when any field changes. */ export interface CommandDependencies { navigate: NavigateFunction - location: Location + /** Current route; the only parts of the location commands read. */ + pathname: string + search: string canWrite: boolean isAdmin: boolean canManageReferenceData: boolean diff --git a/packages/web/src/lib/command-registry/groups/contextual.ts b/packages/web/src/lib/command-registry/groups/contextual.ts index 610c8d61..d27068ff 100644 --- a/packages/web/src/lib/command-registry/groups/contextual.ts +++ b/packages/web/src/lib/command-registry/groups/contextual.ts @@ -2,14 +2,14 @@ import type { Command } from '../../commands' import type { CommandDependencies } from '../command-deps' function mergeSearch(d: CommandDependencies, mutator: (sp: URLSearchParams) => void): void { - const sp = new URLSearchParams(d.location.search) + const sp = new URLSearchParams(d.search) mutator(sp) const next = sp.toString() - d.navigate({ pathname: d.location.pathname, search: next ? `?${next}` : '' }) + d.navigate({ pathname: d.pathname, search: next ? `?${next}` : '' }) } export function buildContextualCommands(d: CommandDependencies): Command[] { - const p = d.location.pathname + const p = d.pathname const out: Command[] = [] const studyMatch = p.match(/^\/studies\/(\d+)$/) diff --git a/packages/web/src/lib/command-registry/groups/create.ts b/packages/web/src/lib/command-registry/groups/create.ts index 872ff74a..ec0e2ee9 100644 --- a/packages/web/src/lib/command-registry/groups/create.ts +++ b/packages/web/src/lib/command-registry/groups/create.ts @@ -12,7 +12,7 @@ function subjectDetailId(pathname: string): string | null { } export function buildCreateCommands(d: CommandDependencies): Command[] { - const { pathname, search } = d.location + const { pathname, search } = d const out: Command[] = [] if (pathname === '/' || pathname === '/studies' || /^\/studies\/\d+$/.test(pathname)) { diff --git a/packages/web/src/lib/command-registry/registry.ts b/packages/web/src/lib/command-registry/registry.ts index a6f78839..545de8d8 100644 --- a/packages/web/src/lib/command-registry/registry.ts +++ b/packages/web/src/lib/command-registry/registry.ts @@ -1,4 +1,5 @@ import { useMemo } from 'react' +import { useShallowStable } from '../../hooks/useShallowStable' import type { Command } from '../commands' import type { CommandDependencies } from './command-deps' import { buildNavigationCommands } from './groups/navigation' @@ -21,53 +22,8 @@ export function buildAllCommands(deps: CommandDependencies): Command[] { ] } -export function useCommands({ - navigate, - location, - canWrite, - isAdmin, - canManageReferenceData, - theme, - setTheme, - toggleHelpModal, - openSearchModal, - refreshUser, - handleExportSpecimens, - handleExportInventory, - handleClearFilters, -}: CommandDependencies) { - // The caller builds a new deps object each render, so memoize on its fields. - return useMemo( - () => - buildAllCommands({ - navigate, - location, - canWrite, - isAdmin, - canManageReferenceData, - theme, - setTheme, - toggleHelpModal, - openSearchModal, - refreshUser, - handleExportSpecimens, - handleExportInventory, - handleClearFilters, - }), - [ - navigate, - location, - canWrite, - isAdmin, - canManageReferenceData, - theme, - setTheme, - toggleHelpModal, - openSearchModal, - refreshUser, - handleExportSpecimens, - handleExportInventory, - handleClearFilters, - ] - ) +export function useCommands(deps: CommandDependencies): Command[] { + // The caller builds a new deps object each render; keep the last one while its fields are unchanged. + const stableDeps = useShallowStable(deps) + return useMemo(() => buildAllCommands(stableDeps), [stableDeps]) } From 6c761b70ec8cbb10f2e93336e66afd751d03c2fe Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Tue, 6 Oct 2026 16:59:27 -0700 Subject: [PATCH 15/30] refactor(web): fall back to one shared empty list instead of memoizing `data ?? []` per page. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/web/src/lib/empty-list.ts | 5 +++++ .../web/src/pages/AdminDataIntegrityEmptyCollections.tsx | 3 ++- packages/web/src/pages/AdminUsers.tsx | 3 ++- packages/web/src/pages/BloodControls.tsx | 5 +++-- packages/web/src/pages/Collections.tsx | 3 ++- packages/web/src/pages/ContainerMoveMicronix.tsx | 8 +++----- packages/web/src/pages/LocationDetail.tsx | 6 ++---- packages/web/src/pages/QpcrExperimentDetail.tsx | 5 +++-- packages/web/src/pages/SheetDetail.tsx | 3 ++- 9 files changed, 24 insertions(+), 17 deletions(-) create mode 100644 packages/web/src/lib/empty-list.ts diff --git a/packages/web/src/lib/empty-list.ts b/packages/web/src/lib/empty-list.ts new file mode 100644 index 00000000..386f24a9 --- /dev/null +++ b/packages/web/src/lib/empty-list.ts @@ -0,0 +1,5 @@ +/** + * One shared, frozen empty array. `query.data ?? EMPTY_LIST` keeps the same identity on every + * render while data loads, so hooks that depend on the list don't rerun. + */ +export const EMPTY_LIST: never[] = Object.freeze([]) as never[] diff --git a/packages/web/src/pages/AdminDataIntegrityEmptyCollections.tsx b/packages/web/src/pages/AdminDataIntegrityEmptyCollections.tsx index db3d2790..6e70e9a0 100644 --- a/packages/web/src/pages/AdminDataIntegrityEmptyCollections.tsx +++ b/packages/web/src/pages/AdminDataIntegrityEmptyCollections.tsx @@ -6,6 +6,7 @@ import type { EmptyCollectionItem, EmptyCollectionsDeleteIds } from '../lib/api/ import { adminKeys, useAdminIntegrityReport } from '../hooks/useAdmin' import { Modal, PageError, fromQuery, getQueryErrorMessage } from '../ui' import '../styles/admin.css' +import { EMPTY_LIST } from '../lib/empty-list' const COLLECTION_TYPES: EmptyCollectionItem['type'][] = ['micronix_plate', 'cryovial_box', 'box', 'bag'] @@ -49,7 +50,7 @@ export default function AdminDataIntegrityEmptyCollections() { const queryClient = useQueryClient() const reportQuery = useAdminIntegrityReport() const reportStatus = fromQuery(reportQuery) - const collections = useMemo(() => reportQuery.data?.emptyCollections ?? [], [reportQuery.data]) + const collections = reportQuery.data?.emptyCollections ?? EMPTY_LIST const [mutationError, setMutationError] = useState(null) const [selected, setSelected] = useState>(new Set()) const [showDeleteModal, setShowDeleteModal] = useState(false) diff --git a/packages/web/src/pages/AdminUsers.tsx b/packages/web/src/pages/AdminUsers.tsx index ada126bc..47282c0c 100644 --- a/packages/web/src/pages/AdminUsers.tsx +++ b/packages/web/src/pages/AdminUsers.tsx @@ -6,6 +6,7 @@ import { adminKeys, useAdminUsers } from '../hooks/useAdmin' import { useFocusSearchOnSlash } from '../hooks/useHotkey' import { AsyncPresentation, Modal, PageError, fromQuery, getQueryErrorMessage } from '../ui' import '../styles/admin.css' +import { EMPTY_LIST } from '../lib/empty-list' interface CreateUserData { email: string @@ -48,7 +49,7 @@ export default function AdminUsers() { const [showPassword, setShowPassword] = useState(false) const usersQuery = useAdminUsers(showDeleted) - const users = useMemo(() => usersQuery.data ?? [], [usersQuery.data]) + const users = usersQuery.data ?? EMPTY_LIST const listStatus = fromQuery(usersQuery, { isEmpty: usersQuery.isSuccess && users.length === 0, }) diff --git a/packages/web/src/pages/BloodControls.tsx b/packages/web/src/pages/BloodControls.tsx index f20d94ee..4c15de9b 100644 --- a/packages/web/src/pages/BloodControls.tsx +++ b/packages/web/src/pages/BloodControls.tsx @@ -12,6 +12,7 @@ import { getCompositionKey } from '../lib/composition-key' import { DetailPageSkeleton, PageError, fromQuery, getQueryErrorMessage } from '../ui' import '../styles/blood-controls.css' import { parseDisplayDate } from '../lib/date-utils' +import { EMPTY_LIST } from '../lib/empty-list' export interface CompositionGroup { id: number @@ -26,8 +27,8 @@ export default function BloodControls() { const navigate = useNavigate() const { canWrite } = useUser() const overviewQuery = useBloodControlsOverview() - const definitions = useMemo(() => overviewQuery.data?.definitions ?? [], [overviewQuery.data]) - const batches = useMemo(() => overviewQuery.data?.batches ?? [], [overviewQuery.data]) + const definitions = overviewQuery.data?.definitions ?? EMPTY_LIST + const batches = overviewQuery.data?.batches ?? EMPTY_LIST const strains = overviewQuery.data?.strains ?? [] const overviewStatus = fromQuery(overviewQuery) const [searchParams, setSearchParams] = useSearchParams() diff --git a/packages/web/src/pages/Collections.tsx b/packages/web/src/pages/Collections.tsx index 5562e195..b00ff6c5 100644 --- a/packages/web/src/pages/Collections.tsx +++ b/packages/web/src/pages/Collections.tsx @@ -6,6 +6,7 @@ import { useAllCollections } from '../hooks/useCollections' import { useFocusSearchOnSlash } from '../hooks/useHotkey' import { AsyncPresentation, EmptyState, PageError, fromQuery, getQueryErrorMessage } from '../ui' import '../styles/subject-specimen.css' +import { EMPTY_LIST } from '../lib/empty-list' const PAGE_SIZE = 50 const TYPE_TABS: { value: CollectionTypeFilter; label: string }[] = [ @@ -66,7 +67,7 @@ export default function Collections() { ) const collectionsQuery = useAllCollections() - const collections = useMemo(() => collectionsQuery.data ?? [], [collectionsQuery.data]) + const collections = collectionsQuery.data ?? EMPTY_LIST const listStatus = fromQuery(collectionsQuery, { isEmpty: collectionsQuery.isSuccess && collections.length === 0, }) diff --git a/packages/web/src/pages/ContainerMoveMicronix.tsx b/packages/web/src/pages/ContainerMoveMicronix.tsx index 59b727a8..e09085a9 100644 --- a/packages/web/src/pages/ContainerMoveMicronix.tsx +++ b/packages/web/src/pages/ContainerMoveMicronix.tsx @@ -1,4 +1,4 @@ -import { useState, useEffect, useRef, useCallback, useMemo } from 'react' +import { useState, useEffect, useRef, useCallback } from 'react' import { Navigate } from 'react-router-dom' import { useMicronixMoveBootstrap, moveWorkflowKeys } from '../hooks/useMoveWorkflow' import { useScanMoveWorkflow } from '../hooks/useScanMoveWorkflow' @@ -12,6 +12,7 @@ import { MICRONIX_SCAN_MOVE_COPY } from '../components/scan-move/copy' import MicronixMoveInstructions from '../components/scan-move/MicronixMoveInstructions' import MicronixScannerConfigPanel from '../components/scan-move/MicronixScannerConfigPanel' import '../styles/storage.css' +import { EMPTY_LIST } from '../lib/empty-list' export default function ContainerMoveMicronix() { const { canWrite } = useUser() @@ -20,10 +21,7 @@ export default function ContainerMoveMicronix() { const bootstrapStatus = fromQuery(bootstrapQuery) const availablePlates = (bootstrapQuery.data?.plates ?? []) as MicronixPlate[] const locations = bootstrapQuery.data?.locations ?? [] - const scannerConfigurations = useMemo( - () => bootstrapQuery.data?.scannerConfigurations ?? [], - [bootstrapQuery.data], - ) + const scannerConfigurations = bootstrapQuery.data?.scannerConfigurations ?? EMPTY_LIST const refreshCollections = useCallback(async () => { await queryClient.invalidateQueries({ queryKey: moveWorkflowKeys.micronixBootstrap() }) diff --git a/packages/web/src/pages/LocationDetail.tsx b/packages/web/src/pages/LocationDetail.tsx index b38602eb..c5631f70 100644 --- a/packages/web/src/pages/LocationDetail.tsx +++ b/packages/web/src/pages/LocationDetail.tsx @@ -12,6 +12,7 @@ import Pagination from '../components/Pagination' import { DetailPageSkeleton, PageError, fromQuery, getQueryErrorMessage } from '../ui' import ContentCard from '../components/ContentCard' import '../styles/storage.css' +import { EMPTY_LIST } from '../lib/empty-list' interface LocationContents { micronixPlates?: any[] @@ -47,10 +48,7 @@ export default function LocationDetail() { const location = (detailQuery.data?.location as Location | undefined) ?? null const contents = (detailQuery.data?.contents as LocationContents | undefined) ?? null const hierarchyStats = (detailQuery.data?.hierarchyStats as LocationHierarchyStats | undefined) ?? null - const allLocations = useMemo( - () => (detailQuery.data?.allLocations as Location[] | undefined) ?? [], - [detailQuery.data], - ) + const allLocations = (detailQuery.data?.allLocations as Location[] | undefined) ?? EMPTY_LIST const pagination = detailQuery.data?.pagination ?? null const pathLabel = useMemo(() => { diff --git a/packages/web/src/pages/QpcrExperimentDetail.tsx b/packages/web/src/pages/QpcrExperimentDetail.tsx index d8a58706..467a9c6f 100644 --- a/packages/web/src/pages/QpcrExperimentDetail.tsx +++ b/packages/web/src/pages/QpcrExperimentDetail.tsx @@ -1,4 +1,4 @@ -import { useEffect, useState, useRef, useMemo } from 'react' +import { useEffect, useState, useRef } from 'react' import { useParams, Link, useNavigate, useSearchParams } from 'react-router-dom' import { useQueryClient } from '@tanstack/react-query' import { api } from '../lib/api/client' @@ -18,6 +18,7 @@ import { useUser } from '../contexts/UserContext' import { useToast } from '../contexts/ToastContext' import '../styles/qpcr.css' import { bytesToBase64 } from '../lib/base64' +import { EMPTY_LIST } from '../lib/empty-list' const PLATE_ROWS = ['A', 'B', 'C', 'D', 'E', 'F', 'G', 'H'] as const const PLATE_COLS = 12 @@ -137,7 +138,7 @@ export default function QpcrExperimentDetail() { const experimentQuery = useQpcrExperiment(experimentId) const experimentStatus = fromQuery(experimentQuery) const scannerConfigsQuery = useQpcrScannerConfigurations() - const scannerConfigs = useMemo(() => scannerConfigsQuery.data ?? [], [scannerConfigsQuery.data]) + const scannerConfigs = scannerConfigsQuery.data ?? EMPTY_LIST const [data, setData] = useState(null) const [deleting, setDeleting] = useState(false) const [selectedConfigId, setSelectedConfigId] = useState(null) diff --git a/packages/web/src/pages/SheetDetail.tsx b/packages/web/src/pages/SheetDetail.tsx index 0d326dd0..94b8cfbd 100644 --- a/packages/web/src/pages/SheetDetail.tsx +++ b/packages/web/src/pages/SheetDetail.tsx @@ -14,6 +14,7 @@ import { import { useTableViewConfigurations } from '../hooks/useTableViewConfigurations' import { Link } from 'react-router-dom' import '../styles/storage.css' +import { EMPTY_LIST } from '../lib/empty-list' export default function SheetDetail() { const { id } = useParams<{ id: string }>() @@ -50,7 +51,7 @@ export default function SheetDetail() { // Derive once so hooks below can run unconditionally (Rules of Hooks) const sheet = data?.sheet ?? null - const papers = useMemo(() => (data?.papers ?? []) as CollectionTableEntry[], [data]) + const papers = (data?.papers ?? EMPTY_LIST) as CollectionTableEntry[] const tableRows = useMemo(() => { const context = sheet From 34f1167d5313f7bb18d0150bd069b9e849e8f971 Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Tue, 6 Oct 2026 17:00:32 -0700 Subject: [PATCH 16/30] refactor(web): share the box and bag sheet list and expand state in useCollectionSheets. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../__tests__/useCollectionSheets.test.ts | 42 +++++++++++++++ packages/web/src/hooks/useCollectionSheets.ts | 51 +++++++++++++++++++ packages/web/src/pages/BagDetail.tsx | 43 ++-------------- packages/web/src/pages/BoxDetail.tsx | 43 ++-------------- 4 files changed, 99 insertions(+), 80 deletions(-) create mode 100644 packages/web/src/hooks/__tests__/useCollectionSheets.test.ts create mode 100644 packages/web/src/hooks/useCollectionSheets.ts diff --git a/packages/web/src/hooks/__tests__/useCollectionSheets.test.ts b/packages/web/src/hooks/__tests__/useCollectionSheets.test.ts new file mode 100644 index 00000000..69255191 --- /dev/null +++ b/packages/web/src/hooks/__tests__/useCollectionSheets.test.ts @@ -0,0 +1,42 @@ +import { describe, it, expect } from 'vitest' +import { act, renderHook } from '@testing-library/react' +import { useCollectionSheets } from '../useCollectionSheets' + +const contents = (count: number) => ({ sheets: Array.from({ length: count }, (_, i) => ({ id: i + 1, papers: [] })) }) + +describe('useCollectionSheets', () => { + it('expands every sheet when there are 3 or fewer', () => { + const { result } = renderHook(() => useCollectionSheets(1, contents(3))) + expect([...result.current.expandedSheets]).toEqual([1, 2, 3]) + }) + + it('starts collapsed when there are more than 3 sheets', () => { + const { result } = renderHook(() => useCollectionSheets(1, contents(4))) + expect(result.current.expandedSheets.size).toBe(0) + }) + + it('waits for the sheets to load, then keeps the user toggles across refetches', () => { + const { result, rerender } = renderHook(({ id, c }) => useCollectionSheets(id, c), { + initialProps: { id: 1, c: undefined as ReturnType | undefined }, + }) + expect(result.current.sheets).toEqual([]) + rerender({ id: 1, c: contents(2) }) + expect([...result.current.expandedSheets]).toEqual([1, 2]) + + act(() => result.current.toggleSheet(1)) + expect([...result.current.expandedSheets]).toEqual([2]) + rerender({ id: 1, c: contents(2) }) + expect([...result.current.expandedSheets]).toEqual([2]) + }) + + it('starts over for a different collection', () => { + const { result, rerender } = renderHook(({ id, c }) => useCollectionSheets(id, c), { + initialProps: { id: 1, c: contents(4) as ReturnType | undefined }, + }) + act(() => result.current.toggleSheet(2)) + rerender({ id: 2, c: undefined }) + expect(result.current.expandedSheets.size).toBe(0) + rerender({ id: 2, c: contents(1) }) + expect([...result.current.expandedSheets]).toEqual([1]) + }) +}) diff --git a/packages/web/src/hooks/useCollectionSheets.ts b/packages/web/src/hooks/useCollectionSheets.ts new file mode 100644 index 00000000..2547689b --- /dev/null +++ b/packages/web/src/hooks/useCollectionSheets.ts @@ -0,0 +1,51 @@ +import { useEffect, useMemo, useRef, useState } from 'react' +import type { CollectionTableEntry } from '../lib/collection-table-columns' +import { EMPTY_LIST } from '../lib/empty-list' + +export interface CollectionSheet { + id: number + name?: string + papers?: CollectionTableEntry[] +} + +/** + * The sheets of a box or bag, and which are expanded. When the sheets first load, 3 or fewer start + * expanded and more start collapsed; a new `collectionId` starts over. + */ +export function useCollectionSheets( + collectionId: number, + contents: { sheets: readonly unknown[] } | undefined, +) { + const [expandedSheets, setExpandedSheets] = useState>(new Set()) + const initializedSheets = useRef(false) + + useEffect(() => { + initializedSheets.current = false + setExpandedSheets(new Set()) + }, [collectionId]) + + const sheets = useMemo(() => (contents?.sheets ?? EMPTY_LIST) as CollectionSheet[], [contents]) + + useEffect(() => { + if (sheets.length > 0 && !initializedSheets.current) { + initializedSheets.current = true + if (sheets.length <= 3) { + setExpandedSheets(new Set(sheets.map((s) => s.id))) + } + } + }, [sheets]) + + const toggleSheet = (sheetId: number) => { + setExpandedSheets((prev) => { + const next = new Set(prev) + if (next.has(sheetId)) { + next.delete(sheetId) + } else { + next.add(sheetId) + } + return next + }) + } + + return { sheets, expandedSheets, toggleSheet } +} diff --git a/packages/web/src/pages/BagDetail.tsx b/packages/web/src/pages/BagDetail.tsx index 45795111..ae2eb7bb 100644 --- a/packages/web/src/pages/BagDetail.tsx +++ b/packages/web/src/pages/BagDetail.tsx @@ -1,4 +1,4 @@ -import { useEffect, useMemo, useState, useRef } from 'react' +import { useMemo, useState } from 'react' import { useParams, Link, useNavigate } from 'react-router-dom' import EntityBreadcrumbs from '../components/EntityBreadcrumbs' import CollectionTableWithExport from '../components/CollectionTableWithExport' @@ -12,6 +12,7 @@ import { type CollectionTableEntry, } from '../lib/collection-table-columns' import { useTableViewConfigurations } from '../hooks/useTableViewConfigurations' +import { useCollectionSheets } from '../hooks/useCollectionSheets' import CollectionDeleteDialog from '../components/CollectionDeleteDialog' import '../styles/storage.css' @@ -24,8 +25,6 @@ export default function BagDetail() { const detailStatus = fromQuery(dataQuery) const [viewMode, setViewMode] = useState<'sheets' | 'table'>('sheets') const [showDeleteCollection, setShowDeleteCollection] = useState(false) - const [expandedSheets, setExpandedSheets] = useState>(new Set()) - const initializedSheets = useRef(false) const { configurations: viewConfigurations, selectedConfigId, @@ -33,31 +32,7 @@ export default function BagDetail() { loading: loadingConfigs, } = useTableViewConfigurations() - useEffect(() => { - initializedSheets.current = false - setExpandedSheets(new Set()) - }, [bagId]) - - // Initialize expanded sheets (default to collapsed if more than 3 sheets) - // This hook must be called before any conditional returns to maintain hook order - const sheets = useMemo( - () => - (data?.contents.sheets || []) as Array<{ - id: number - name?: string - papers?: CollectionTableEntry[] - }>, - [data], - ) - useEffect(() => { - if (sheets.length > 0 && !initializedSheets.current) { - initializedSheets.current = true - // Default to expanded if 3 or fewer sheets, otherwise collapsed - if (sheets.length <= 3) { - setExpandedSheets(new Set(sheets.map((s: any) => s.id))) - } - } - }, [sheets]) + const { sheets, expandedSheets, toggleSheet } = useCollectionSheets(bagId, data?.contents) const tableRows = useMemo(() => { const bag = data?.bag @@ -147,18 +122,6 @@ export default function BagDetail() { sum + (sheet.papers?.filter((p: any) => p.container?.remainingQuantity > 0).length || 0), 0 ) - const toggleSheet = (sheetId: number) => { - setExpandedSheets(prev => { - const next = new Set(prev) - if (next.has(sheetId)) { - next.delete(sheetId) - } else { - next.add(sheetId) - } - return next - }) - } - return (
diff --git a/packages/web/src/pages/BoxDetail.tsx b/packages/web/src/pages/BoxDetail.tsx index b9549b08..cddc425f 100644 --- a/packages/web/src/pages/BoxDetail.tsx +++ b/packages/web/src/pages/BoxDetail.tsx @@ -1,4 +1,4 @@ -import { useEffect, useMemo, useState, useRef } from 'react' +import { useMemo, useState } from 'react' import { useParams, Link, useNavigate } from 'react-router-dom' import EntityBreadcrumbs from '../components/EntityBreadcrumbs' import CollectionTableWithExport from '../components/CollectionTableWithExport' @@ -12,6 +12,7 @@ import { type CollectionTableEntry, } from '../lib/collection-table-columns' import { useTableViewConfigurations } from '../hooks/useTableViewConfigurations' +import { useCollectionSheets } from '../hooks/useCollectionSheets' import CollectionDeleteDialog from '../components/CollectionDeleteDialog' import '../styles/storage.css' @@ -24,8 +25,6 @@ export default function BoxDetail() { const detailStatus = fromQuery(dataQuery) const [viewMode, setViewMode] = useState<'sheets' | 'table'>('sheets') const [showDeleteCollection, setShowDeleteCollection] = useState(false) - const [expandedSheets, setExpandedSheets] = useState>(new Set()) - const initializedSheets = useRef(false) const { configurations: viewConfigurations, selectedConfigId, @@ -33,31 +32,7 @@ export default function BoxDetail() { loading: loadingConfigs, } = useTableViewConfigurations() - useEffect(() => { - initializedSheets.current = false - setExpandedSheets(new Set()) - }, [boxId]) - - // Initialize expanded sheets (default to collapsed if more than 3 sheets) - // This hook must be called before any conditional returns to maintain hook order - const sheets = useMemo( - () => - (data?.contents.sheets || []) as Array<{ - id: number - name?: string - papers?: CollectionTableEntry[] - }>, - [data], - ) - useEffect(() => { - if (sheets.length > 0 && !initializedSheets.current) { - initializedSheets.current = true - // Default to expanded if 3 or fewer sheets, otherwise collapsed - if (sheets.length <= 3) { - setExpandedSheets(new Set(sheets.map((s: any) => s.id))) - } - } - }, [sheets]) + const { sheets, expandedSheets, toggleSheet } = useCollectionSheets(boxId, data?.contents) const tableRows = useMemo(() => { const box = data?.box @@ -150,18 +125,6 @@ export default function BoxDetail() { sum + (sheet.papers?.filter((p: any) => p.container?.remainingQuantity > 0).length || 0), 0 ) - const toggleSheet = (sheetId: number) => { - setExpandedSheets(prev => { - const next = new Set(prev) - if (next.has(sheetId)) { - next.delete(sheetId) - } else { - next.add(sheetId) - } - return next - }) - } - return (
From 83fd885e66f0705303db208e0ff248ba7422be04 Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Tue, 6 Oct 2026 17:02:38 -0700 Subject: [PATCH 17/30] fix(web): rerun the batch-definition and allowed-container-type effects only on their original triggers. Moving these effects onto the full batchInfo and onto containerType made them run on unrelated edits. Their bodies now read the rest through useEffectEvent. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../src/components/ContainerRegistration.tsx | 10 +++-- .../__tests__/ContainerRegistration.test.tsx | 42 +++++++++++++++++++ .../src/components/wizards/BatchInfoStep.tsx | 10 +++-- .../wizards/__tests__/BatchInfoStep.test.tsx | 22 +++++++++- 4 files changed, 77 insertions(+), 7 deletions(-) create mode 100644 packages/web/src/components/__tests__/ContainerRegistration.test.tsx diff --git a/packages/web/src/components/ContainerRegistration.tsx b/packages/web/src/components/ContainerRegistration.tsx index f2dd06de..6f5beec9 100644 --- a/packages/web/src/components/ContainerRegistration.tsx +++ b/packages/web/src/components/ContainerRegistration.tsx @@ -133,14 +133,18 @@ export default function ContainerRegistration({ } } - // When allowed types are restricted and current type is not allowed, reset to first allowed - useEffect(() => { + // When allowed types are restricted and current type is not allowed, reset to first allowed. + // Runs when the allowed list changes; the select only offers allowed types. + const resetDisallowedContainerType = useEffectEvent(() => { if (allowedContainerTypes.length > 0 && !allowedContainerTypes.includes(containerType)) { const first = allowedContainerTypes[0] as ContainerType setContainerType(first) setFormData((prev) => ({ ...prev, containerType: first })) } - }, [allowedContainerTypes, containerType]) + }) + useEffect(() => { + resetDisallowedContainerType() + }, [allowedContainerTypes]) // Read at use time, so a unit picked while defaults load is kept. const hasChosenUnit = useEffectEvent(() => Boolean(selectedUnitId || defaultValue?.unitId)) diff --git a/packages/web/src/components/__tests__/ContainerRegistration.test.tsx b/packages/web/src/components/__tests__/ContainerRegistration.test.tsx new file mode 100644 index 00000000..773ffb11 --- /dev/null +++ b/packages/web/src/components/__tests__/ContainerRegistration.test.tsx @@ -0,0 +1,42 @@ +import { describe, it, expect, vi } from 'vitest' +import { render, screen, waitFor } from '../../__tests__/helpers/render' +import ContainerRegistration from '../ContainerRegistration' + +vi.mock('../../lib/api/settings', async () => { + const { createMockedDomainModule } = await import('../../__tests__/helpers/mock-api') + return createMockedDomainModule('settings', { + settingsApi: { + getValue: vi.fn().mockResolvedValue(null), + getContainerTypeUnits: vi.fn().mockResolvedValue({ units: [] }), + }, + }) +}) + +vi.mock('../../lib/api/collections', async () => { + const { createMockedDomainModule } = await import('../../__tests__/helpers/mock-api') + return createMockedDomainModule('collections', { + collectionsApi: { listCollectionsByType: vi.fn().mockResolvedValue({ collections: [] }) }, + }) +}) + +const containerTypeSelect = () => screen.getByText('Container Type *').parentElement?.querySelector('select') + +vi.mock('../../lib/api/locations', async () => { + const { createMockedDomainModule } = await import('../../__tests__/helpers/mock-api') + return createMockedDomainModule('locations', { + locationsApi: { list: vi.fn().mockResolvedValue({ locations: [] }) }, + }) +}) + +describe('ContainerRegistration', () => { + it('switches to the first allowed container type when the allowed list excludes the current one', async () => { + const onChange = vi.fn() + const { rerender } = await render( + , + ) + await waitFor(() => expect(containerTypeSelect()).toHaveValue('cryovial_tube')) + + rerender() + await waitFor(() => expect(containerTypeSelect()).toHaveValue('paper')) + }) +}) diff --git a/packages/web/src/components/wizards/BatchInfoStep.tsx b/packages/web/src/components/wizards/BatchInfoStep.tsx index f9a888f5..d9046915 100644 --- a/packages/web/src/components/wizards/BatchInfoStep.tsx +++ b/packages/web/src/components/wizards/BatchInfoStep.tsx @@ -1,4 +1,4 @@ -import { useState, useEffect, useRef, useMemo } from 'react' +import { useState, useEffect, useEffectEvent, useRef, useMemo } from 'react' import { useQueryClient } from '@tanstack/react-query' import { controlsApi } from '../../lib/api/controls' import type { ControlDefinition } from '../../lib/api/controls' @@ -74,12 +74,16 @@ export default function BatchInfoStep({ } }, [definitionsQuery.data]) + // Attach the loaded definition once; edits to other batch fields don't re-run this. + const attachDefinition = useEffectEvent((def: ControlDefinition) => { + onChange({ ...batchInfo, controlDefinition: def }) + }) useEffect(() => { const def = definitionDetailQuery.data if (def && batchInfo.controlDefinitionId && !batchInfo.controlDefinition) { - onChange({ ...batchInfo, controlDefinition: def }) + attachDefinition(def) } - }, [definitionDetailQuery.data, batchInfo, onChange]) + }, [definitionDetailQuery.data, batchInfo.controlDefinitionId, batchInfo.controlDefinition]) // Client-side filter by search, strains, and density const filteredDefinitions = useMemo(() => { diff --git a/packages/web/src/components/wizards/__tests__/BatchInfoStep.test.tsx b/packages/web/src/components/wizards/__tests__/BatchInfoStep.test.tsx index 1ebb56ce..c34171bd 100644 --- a/packages/web/src/components/wizards/__tests__/BatchInfoStep.test.tsx +++ b/packages/web/src/components/wizards/__tests__/BatchInfoStep.test.tsx @@ -1,5 +1,6 @@ import { describe, it, expect, vi, beforeEach } from 'vitest' -import { render, screen } from '../../../__tests__/helpers/render' +import { render, screen, waitFor } from '../../../__tests__/helpers/render' +import { controlsApi } from '../../../lib/api/controls' import BatchInfoStep from '../BatchInfoStep' vi.mock('../../../lib/api/reference-data', async () => { @@ -20,6 +21,7 @@ vi.mock('../../../lib/api/controls', async () => { return createMockedDomainModule('controls', { controlsApi: { list: vi.fn().mockResolvedValue({ controls: [] }), + get: vi.fn(), getDefinitionSummary: vi.fn(), }, strainsApi: { @@ -65,4 +67,22 @@ describe('BatchInfoStep', () => { expect(screen.getByRole('button', { name: /cancel/i })).toBeInTheDocument() expect(screen.getByRole('button', { name: /next|continue/i })).toBeInTheDocument() }) + + it('attaches the loaded definition once, not again when another batch field changes', async () => { + vi.mocked(controlsApi.get).mockResolvedValue({ + control: { id: 5, name: 'Def 5', controlType: 'blood' }, + } as unknown as Awaited>) + const onChange = vi.fn() + const props = { onChange, onNext: vi.fn(), onCancel: vi.fn(), isAddMode: false } + const batchInfo = { ...defaultBatchInfo, controlDefinitionId: 5 } + const { rerender } = await render() + + await waitFor(() => expect(onChange).toHaveBeenCalledTimes(1)) + expect(onChange.mock.calls[0]?.[0]).toMatchObject({ controlDefinitionId: 5, controlDefinition: { id: 5 } }) + + // The parent hasn't stored the definition yet, but only the name changed. + rerender() + await new Promise((resolve) => setTimeout(resolve, 0)) + expect(onChange).toHaveBeenCalledTimes(1) + }) }) From c1d105f31aeafc35bb0a126873a7d60c43e5ef79 Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Tue, 6 Oct 2026 17:03:15 -0700 Subject: [PATCH 18/30] fix(web): set up box and bag sheet expansion on load or collection change, not every refetch. The PR moved this effect from the sheet count to the whole sheet list, so it ran on every refetch. The count alone missed a collection whose sheets were already cached with the same count, so it now runs on the collection id and the count. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../src/hooks/__tests__/useCollectionSheets.test.ts | 9 +++++++++ packages/web/src/hooks/useCollectionSheets.ts | 11 ++++++++--- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/packages/web/src/hooks/__tests__/useCollectionSheets.test.ts b/packages/web/src/hooks/__tests__/useCollectionSheets.test.ts index 69255191..7bcad19e 100644 --- a/packages/web/src/hooks/__tests__/useCollectionSheets.test.ts +++ b/packages/web/src/hooks/__tests__/useCollectionSheets.test.ts @@ -39,4 +39,13 @@ describe('useCollectionSheets', () => { rerender({ id: 2, c: contents(1) }) expect([...result.current.expandedSheets]).toEqual([1]) }) + + it('initializes a collection whose sheets are already cached, even with the same sheet count', () => { + const cached = { sheets: [{ id: 10 }, { id: 11 }] } + const { result, rerender } = renderHook(({ id, c }) => useCollectionSheets(id, c), { + initialProps: { id: 1, c: contents(2) as { sheets: { id: number }[] } }, + }) + rerender({ id: 2, c: cached }) + expect([...result.current.expandedSheets]).toEqual([10, 11]) + }) }) diff --git a/packages/web/src/hooks/useCollectionSheets.ts b/packages/web/src/hooks/useCollectionSheets.ts index 2547689b..a083a309 100644 --- a/packages/web/src/hooks/useCollectionSheets.ts +++ b/packages/web/src/hooks/useCollectionSheets.ts @@ -1,4 +1,4 @@ -import { useEffect, useMemo, useRef, useState } from 'react' +import { useEffect, useEffectEvent, useMemo, useRef, useState } from 'react' import type { CollectionTableEntry } from '../lib/collection-table-columns' import { EMPTY_LIST } from '../lib/empty-list' @@ -26,14 +26,19 @@ export function useCollectionSheets( const sheets = useMemo(() => (contents?.sheets ?? EMPTY_LIST) as CollectionSheet[], [contents]) - useEffect(() => { + const initializeExpanded = useEffectEvent(() => { if (sheets.length > 0 && !initializedSheets.current) { initializedSheets.current = true if (sheets.length <= 3) { setExpandedSheets(new Set(sheets.map((s) => s.id))) } } - }, [sheets]) + }) + // Runs when the sheets first load or the collection changes (its sheets may already be cached), + // not on every refetch. + useEffect(() => { + initializeExpanded() + }, [collectionId, sheets.length]) const toggleSheet = (sheetId: number) => { setExpandedSheets((prev) => { From 3a511d6da95e0cc645c18f6b5a1b275221a48912 Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Tue, 6 Oct 2026 17:05:25 -0700 Subject: [PATCH 19/30] fix(web): read chart colors for the theme being rendered and drop the exhaustive-deps disables. The getters read CSS variables from the document root, which ThemeProvider updates in a layout effect after children render, so a memo keyed on the theme kept the old theme's colors. They now take the theme and read it from a probe element. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/web/src/components/StatChart.tsx | 3 +- packages/web/src/components/StudyTimeline.tsx | 3 +- .../components/dashboard/SystemInsights.tsx | 3 +- .../src/lib/__tests__/chart-colors.test.ts | 33 ++++++++++++ packages/web/src/lib/chart-colors.ts | 53 +++++++++++-------- packages/web/src/styles/tokens.css | 3 +- 6 files changed, 70 insertions(+), 28 deletions(-) create mode 100644 packages/web/src/lib/__tests__/chart-colors.test.ts diff --git a/packages/web/src/components/StatChart.tsx b/packages/web/src/components/StatChart.tsx index d37daebd..c62c6175 100644 --- a/packages/web/src/components/StatChart.tsx +++ b/packages/web/src/components/StatChart.tsx @@ -49,8 +49,7 @@ export default function StatChart({ cardClassName = 'bg-app-card rounded-lg shadow p-6', }: StatChartProps) { const { theme } = useTheme() - // eslint-disable-next-line react-hooks/exhaustive-deps -- reads CSS variables; theme changes them - const tooltipStyles = useMemo(() => getAppTooltipStyles(), [theme]) + const tooltipStyles = useMemo(() => getAppTooltipStyles(theme), [theme]) // Calculate total for percentage calculations const total = data.reduce((sum, entry) => sum + (entry[dataKey] as number), 0) diff --git a/packages/web/src/components/StudyTimeline.tsx b/packages/web/src/components/StudyTimeline.tsx index c32f5cc2..27e7b759 100644 --- a/packages/web/src/components/StudyTimeline.tsx +++ b/packages/web/src/components/StudyTimeline.tsx @@ -219,7 +219,7 @@ export default function StudyTimeline({ data }: StudyTimelineProps) { ctx.save() ctx.translate(margin.left, margin.top) - const axisColors = getAppAxisColors() + const axisColors = getAppAxisColors(theme) // Create scales const xScale = scaleLinear() @@ -353,7 +353,6 @@ export default function StudyTimeline({ data }: StudyTimelineProps) { .style('font-size', '14px') .style('text-anchor', 'middle') .text('Subject') - // eslint-disable-next-line react-hooks/exhaustive-deps -- reads CSS variables; theme changes them }, [theme, allPoints, dateRange, filteredSubjects, yAxisLabels, seriesData]) // Handle mouse move for tooltip diff --git a/packages/web/src/components/dashboard/SystemInsights.tsx b/packages/web/src/components/dashboard/SystemInsights.tsx index e53ae96e..e4025b16 100644 --- a/packages/web/src/components/dashboard/SystemInsights.tsx +++ b/packages/web/src/components/dashboard/SystemInsights.tsx @@ -13,8 +13,7 @@ interface SystemInsightsProps { export default function SystemInsights({ data, loading }: SystemInsightsProps) { const { theme } = useTheme() - // eslint-disable-next-line react-hooks/exhaustive-deps -- reads CSS variables; theme changes them - const chartColors = useMemo(() => getAppChartColors(), [theme]) + const chartColors = useMemo(() => getAppChartColors(theme), [theme]) if (loading || !data) { return ( diff --git a/packages/web/src/lib/__tests__/chart-colors.test.ts b/packages/web/src/lib/__tests__/chart-colors.test.ts new file mode 100644 index 00000000..9c13c4d3 --- /dev/null +++ b/packages/web/src/lib/__tests__/chart-colors.test.ts @@ -0,0 +1,33 @@ +import { describe, it, expect, afterEach } from 'vitest' +import { getAppAxisColors, getAppChartColors, getAppTooltipStyles } from '../chart-colors' + +describe('chart colors', () => { + let style: HTMLStyleElement | null = null + + afterEach(() => { + style?.remove() + style = null + delete document.documentElement.dataset.theme + }) + + it('reads the tokens of the requested theme, not the theme the document shows yet', () => { + style = document.createElement('style') + style.textContent = ` + :root, [data-theme="light"] { --app-text: 10 20 30; --app-border: 40 50 60; --app-card: 1 1 1; --app-chart-1: 7 8 9; } + [data-theme="dark"] { --app-text: 200 210 220; --app-border: 90 90 90; --app-card: 2 2 2; --app-chart-1: 4 5 6; } + ` + document.head.appendChild(style) + document.documentElement.dataset.theme = 'light' + + expect(getAppAxisColors('dark')).toEqual({ text: 'rgb(200, 210, 220)', border: 'rgb(90, 90, 90)' }) + expect(getAppChartColors('dark')[0]).toBe('rgb(4, 5, 6)') + expect(getAppTooltipStyles('dark').contentStyle.backgroundColor).toBe('rgb(2 2 2)') + expect(getAppAxisColors('light').text).toBe('rgb(10, 20, 30)') + }) + + it('leaves no probe element behind', () => { + const before = document.body.childElementCount + getAppChartColors('light') + expect(document.body.childElementCount).toBe(before) + }) +}) diff --git a/packages/web/src/lib/chart-colors.ts b/packages/web/src/lib/chart-colors.ts index 2614f17f..62aeaecf 100644 --- a/packages/web/src/lib/chart-colors.ts +++ b/packages/web/src/lib/chart-colors.ts @@ -1,9 +1,26 @@ import type { CSSProperties } from 'react' +import type { Theme } from '../contexts/theme-types' /** - * Read app chart palette from CSS variables (--app-chart-1 … --app-chart-8). - * Values are theme-aware (light/dark) so charts adapt automatically. + * Read token values for `theme` from a hidden probe element carrying that theme, not from the + * document root: a component rendering for a newly chosen theme runs before ThemeProvider applies + * it to the root. Returns null outside a browser. */ +function readThemeTokens(theme: Theme, names: readonly string[]): string[] | null { + if (typeof document === 'undefined') return null + const probe = document.createElement('div') + probe.dataset.theme = theme + probe.hidden = true + document.body.appendChild(probe) + try { + const style = getComputedStyle(probe) + return names.map((name) => style.getPropertyValue(name).trim()) + } finally { + probe.remove() + } +} + +/** App chart palette tokens (--app-chart-1 … --app-chart-8); each theme sets its own values. */ const CHART_TOKEN_NAMES = [ '--app-chart-1', '--app-chart-2', @@ -24,8 +41,9 @@ function parseTokenRgb(value: string): string { return `rgb(${r}, ${g}, ${b})` } -export function getAppChartColors(): string[] { - if (typeof document === 'undefined') { +export function getAppChartColors(theme: Theme): string[] { + const tokens = readThemeTokens(theme, CHART_TOKEN_NAMES) + if (!tokens) { return [ 'rgb(20, 184, 166)', 'rgb(13, 148, 136)', @@ -37,10 +55,8 @@ export function getAppChartColors(): string[] { 'rgb(71, 85, 105)', ] } - const root = document.documentElement - const style = getComputedStyle(root) - return CHART_TOKEN_NAMES.map((name) => { - const value = style.getPropertyValue(name).trim() + return CHART_TOKEN_NAMES.map((_, i) => { + const value = tokens[i] return value ? parseTokenRgb(value) : 'rgb(100, 116, 139)' }) } @@ -64,14 +80,12 @@ export interface AppAxisColors { } /** Theme-aware colors for chart axes, grid lines, and labels (e.g. StudyTimeline). */ -export function getAppAxisColors(): AppAxisColors { - if (typeof document === 'undefined') { +export function getAppAxisColors(theme: Theme): AppAxisColors { + const tokens = readThemeTokens(theme, [AXIS_TOKEN_NAMES.text, AXIS_TOKEN_NAMES.border]) + if (!tokens) { return { text: 'rgb(30, 41, 59)', border: 'rgb(226, 232, 240)' } } - const root = document.documentElement - const style = getComputedStyle(root) - const text = style.getPropertyValue(AXIS_TOKEN_NAMES.text).trim() - const border = style.getPropertyValue(AXIS_TOKEN_NAMES.border).trim() + const [text, border] = tokens return { text: text ? parseTokenRgb(text) : 'rgb(30, 41, 59)', border: border ? parseTokenRgb(border) : 'rgb(226, 232, 240)', @@ -79,8 +93,9 @@ export function getAppAxisColors(): AppAxisColors { } /** Theme-aware styles for Recharts (and other) chart tooltips. */ -export function getAppTooltipStyles(): AppTooltipStyles { - if (typeof document === 'undefined') { +export function getAppTooltipStyles(theme: Theme): AppTooltipStyles { + const tokens = readThemeTokens(theme, [TOOLTIP_TOKEN_NAMES.card, TOOLTIP_TOKEN_NAMES.text, TOOLTIP_TOKEN_NAMES.border]) + if (!tokens) { return { contentStyle: { margin: 0, @@ -94,11 +109,7 @@ export function getAppTooltipStyles(): AppTooltipStyles { labelStyle: { color: 'rgb(30, 41, 59)' }, } } - const root = document.documentElement - const style = getComputedStyle(root) - const card = style.getPropertyValue(TOOLTIP_TOKEN_NAMES.card).trim() - const text = style.getPropertyValue(TOOLTIP_TOKEN_NAMES.text).trim() - const border = style.getPropertyValue(TOOLTIP_TOKEN_NAMES.border).trim() + const [card, text, border] = tokens const bg = card ? `rgb(${card})` : 'rgb(255, 255, 255)' const textColor = text ? `rgb(${text})` : 'rgb(30, 41, 59)' const borderColor = border ? `rgb(${border})` : 'rgb(226, 232, 240)' diff --git a/packages/web/src/styles/tokens.css b/packages/web/src/styles/tokens.css index 2792f100..f87798f2 100644 --- a/packages/web/src/styles/tokens.css +++ b/packages/web/src/styles/tokens.css @@ -5,7 +5,8 @@ @import url('https://fonts.googleapis.com/css2?family=DM+Sans:ital,opsz,wght@0,9..40,400;0,9..40,500;0,9..40,600;0,9..40,700&family=Source+Sans+3:ital,wght@0,400;0,500;0,600;0,700&display=swap'); -:root { +:root, +[data-theme="light"] { /* Surfaces */ --app-bg: 248 250 252; --app-surface: 241 245 249; From b8e5a0a2398a9b9f0acf0934f33f8b7f550799c0 Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Tue, 6 Oct 2026 17:06:22 -0700 Subject: [PATCH 20/30] refactor(web): let the ContainerTypeUnitsManager effect call its loader directly. Handlers reload by bumping a counter the effect depends on, so loadData can be the Effect Event and the loadInitialData wrapper goes. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../components/ContainerTypeUnitsManager.tsx | 16 +++---- .../ContainerTypeUnitsManager.test.tsx | 45 +++++++++++++++++++ 2 files changed, 52 insertions(+), 9 deletions(-) create mode 100644 packages/web/src/components/__tests__/ContainerTypeUnitsManager.test.tsx diff --git a/packages/web/src/components/ContainerTypeUnitsManager.tsx b/packages/web/src/components/ContainerTypeUnitsManager.tsx index 1c6a31e2..213c565e 100644 --- a/packages/web/src/components/ContainerTypeUnitsManager.tsx +++ b/packages/web/src/components/ContainerTypeUnitsManager.tsx @@ -52,15 +52,13 @@ export default function ContainerTypeUnitsManager({ // All available units const [allUnits, setAllUnits] = useState([]) - // loadData is also called from handlers, so the mount effect goes through an Effect Event. - const loadInitialData = useEffectEvent(() => { - void loadData() - }) + // Bumped by handlers to reload; the effect owns loading. + const [reloadCount, setReloadCount] = useState(0) useEffect(() => { - loadInitialData() - }, []) + void loadData() + }, [reloadCount]) - const loadData = async () => { + const loadData = useEffectEvent(async () => { setLoading(true) setError(null) try { @@ -114,7 +112,7 @@ export default function ContainerTypeUnitsManager({ } finally { setLoading(false) } - } + }) const handleToggleUnit = async (containerType: ContainerType, unitId: number) => { const currentAllowed = allowedUnits[containerType] @@ -152,7 +150,7 @@ export default function ContainerTypeUnitsManager({ setError(errorMsg) onError?.(errorMsg) // Reload to get correct state - await loadData() + setReloadCount((n) => n + 1) } } diff --git a/packages/web/src/components/__tests__/ContainerTypeUnitsManager.test.tsx b/packages/web/src/components/__tests__/ContainerTypeUnitsManager.test.tsx new file mode 100644 index 00000000..a2f5e666 --- /dev/null +++ b/packages/web/src/components/__tests__/ContainerTypeUnitsManager.test.tsx @@ -0,0 +1,45 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest' +import userEvent from '@testing-library/user-event' +import { render, screen, waitFor } from '../../__tests__/helpers/render' +import ContainerTypeUnitsManager from '../ContainerTypeUnitsManager' +import { settingsApi } from '../../lib/api/settings' +import { unitsApi } from '../../lib/api/reference-data' + +vi.mock('../../lib/api/settings', async () => { + const { createMockedDomainModule } = await import('../../__tests__/helpers/mock-api') + return createMockedDomainModule('settings', { + settingsApi: { + getValue: vi.fn(), + getContainerTypeUnits: vi.fn(), + addContainerTypeUnit: vi.fn(), + removeContainerTypeUnit: vi.fn(), + }, + }) +}) + +vi.mock('../../lib/api/reference-data', async () => { + const { createMockedDomainModule } = await import('../../__tests__/helpers/mock-api') + return createMockedDomainModule('reference-data', { unitsApi: { listAll: vi.fn() } }) +}) + +describe('ContainerTypeUnitsManager', () => { + beforeEach(() => { + vi.clearAllMocks() + vi.mocked(unitsApi.listAll).mockResolvedValue([{ id: 1, symbol: 'uL', name: 'microliter', category: 'volume' }]) + vi.mocked(settingsApi.getValue).mockResolvedValue(null) + vi.mocked(settingsApi.getContainerTypeUnits).mockResolvedValue({ units: [] } as never) + }) + + it('loads on mount, and reloads after a failed toggle', async () => { + vi.mocked(settingsApi.addContainerTypeUnit).mockRejectedValue(new Error('nope')) + const onError = vi.fn() + await render() + await waitFor(() => expect(screen.getAllByRole('checkbox').length).toBeGreaterThan(0)) + expect(unitsApi.listAll).toHaveBeenCalledTimes(1) + + await userEvent.setup().click(screen.getAllByRole('checkbox')[0]!) + + await waitFor(() => expect(unitsApi.listAll).toHaveBeenCalledTimes(2)) + expect(onError).toHaveBeenCalledWith('Failed to update unit relationship') + }) +}) From c7b6e9862e0e5a77d96e66779b41e89079dd6a90 Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Tue, 6 Oct 2026 17:06:48 -0700 Subject: [PATCH 21/30] test(web): stub scrollIntoView in the web test setup instead of guarding it in Locations. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/web/src/__tests__/README.md | 2 +- packages/web/src/__tests__/setup.ts | 5 +++++ packages/web/src/pages/Locations.tsx | 3 +-- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/web/src/__tests__/README.md b/packages/web/src/__tests__/README.md index 747bc19f..8d4189a6 100644 --- a/packages/web/src/__tests__/README.md +++ b/packages/web/src/__tests__/README.md @@ -38,7 +38,7 @@ Reference-data list helpers still return `{ data: T[], meta? }` — keep that en - `render.tsx`: Custom render function with providers (QueryClient, BrowserRouter, ToastProvider) - `mock-api.ts`: `createMockedDomainModule()` — `importActual` per `lib/api/` + per-test overrides - `mock-api-templates.ts`: domain presets (`statisticsPageMock`, `studiesPageMock`, etc.) for common page/hook tests - - `setup.ts`: Global test setup (IntersectionObserver, in-memory localStorage; default mocks for `auth` and `settings`) + - `setup.ts`: Global test setup (IntersectionObserver and `scrollIntoView` stubs, in-memory localStorage; default mocks for `auth` and `settings`) - `__tests__/fixtures/` - Test data fixtures - `lib/__tests__/` - Lib unit tests (e.g. commands, constants, plate-filename-match, container-types, localUserHistory, hotkeys, error-logger) - `components/__tests__/` - Component tests (e.g. BulkImportFlow, ContainerDerivationModal) diff --git a/packages/web/src/__tests__/setup.ts b/packages/web/src/__tests__/setup.ts index a527ac3e..d5238692 100644 --- a/packages/web/src/__tests__/setup.ts +++ b/packages/web/src/__tests__/setup.ts @@ -47,6 +47,11 @@ if (typeof window !== 'undefined' && typeof window.IntersectionObserver === 'und window.IntersectionObserver = MockIO } +// jsdom does not implement scrollIntoView; production code calls it unguarded. +if (typeof Element !== 'undefined' && typeof Element.prototype.scrollIntoView !== 'function') { + Element.prototype.scrollIntoView = () => {} +} + // Cleanup after each test afterEach(() => { cleanup() diff --git a/packages/web/src/pages/Locations.tsx b/packages/web/src/pages/Locations.tsx index 8c27e4ae..95a19bc1 100644 --- a/packages/web/src/pages/Locations.tsx +++ b/packages/web/src/pages/Locations.tsx @@ -158,8 +158,7 @@ export default function Locations() { if (nodeRef && tree) { // Small delay to ensure DOM is updated setTimeout(() => { - // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- jsdom has no scrollIntoView - nodeRef.scrollIntoView?.({ + nodeRef.scrollIntoView({ behavior: 'smooth', block: 'center', inline: 'nearest', From c8938b032c8105dafaeca41d67bee9545349c66e Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Tue, 6 Oct 2026 17:07:11 -0700 Subject: [PATCH 22/30] style(web): drop trailing whitespace the lint fixes left behind. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/web/src/components/StudyStats.tsx | 2 +- packages/web/src/components/TableViewConfigurationsManager.tsx | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/web/src/components/StudyStats.tsx b/packages/web/src/components/StudyStats.tsx index 565ea880..6a5ef54c 100644 --- a/packages/web/src/components/StudyStats.tsx +++ b/packages/web/src/components/StudyStats.tsx @@ -32,7 +32,7 @@ const getBinKey = (dateString: string, size: BinSize): string => { const year = date.getFullYear() const month = date.getMonth() + 1 const day = date.getDate() - + switch (size) { case 'day': return `${year}-${String(month).padStart(2, '0')}-${String(day).padStart(2, '0')}` diff --git a/packages/web/src/components/TableViewConfigurationsManager.tsx b/packages/web/src/components/TableViewConfigurationsManager.tsx index 72b436bd..6f654539 100644 --- a/packages/web/src/components/TableViewConfigurationsManager.tsx +++ b/packages/web/src/components/TableViewConfigurationsManager.tsx @@ -39,7 +39,7 @@ export default function TableViewConfigurationsManager({ setLoading(true) try { const configsValue = await settingsApi.getValue('table_view_configurations') - const configs = configsValue?.configurations ?? [] + const configs = configsValue?.configurations ?? [] setConfigurations(configs) } catch (err: unknown) { const msg = From a14d39bc3ce8a34931e7d3cf23bed8f11b3b4f22 Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Tue, 6 Oct 2026 17:08:48 -0700 Subject: [PATCH 23/30] fix(web): say "Control definition not found" when the batch wizard's definition 404s. The PR dropped the client-side check and relied on the summary route's 404, which showed whichever of two server messages arrived first, after a retry. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/web/src/hooks/useControls.ts | 3 + packages/web/src/pages/ControlBatchWizard.tsx | 4 +- .../__tests__/ControlBatchWizard.test.tsx | 63 +++++++++++++++++++ packages/web/src/ui/async.ts | 8 +++ packages/web/src/ui/index.ts | 2 +- 5 files changed, 78 insertions(+), 2 deletions(-) create mode 100644 packages/web/src/pages/__tests__/ControlBatchWizard.test.tsx diff --git a/packages/web/src/hooks/useControls.ts b/packages/web/src/hooks/useControls.ts index 080ea730..8579618f 100644 --- a/packages/web/src/hooks/useControls.ts +++ b/packages/web/src/hooks/useControls.ts @@ -4,6 +4,7 @@ import { strainsApi } from '../lib/api/reference-data' import type { Strain } from '../lib/api/reference-data' import { getCompositionKey } from '../lib/composition-key' import { localToday } from '../lib/date-utils' +import { getHttpStatus } from '../ui/async' export const controlKeys = { all: ['blood-controls'] as const, @@ -121,6 +122,8 @@ export function useControlDefinitionWizardSeed(definitionId: number | undefined) ]) return { summaryResponse, suggestedName: nameResponse.name, productionDate: today } }, + // A missing definition stays missing; show it now instead of after a retry. + retry: (failureCount, error) => getHttpStatus(error) !== 404 && failureCount < 1, enabled: !!definitionId && definitionId > 0, }) } diff --git a/packages/web/src/pages/ControlBatchWizard.tsx b/packages/web/src/pages/ControlBatchWizard.tsx index 2e7ccb45..9343a61c 100644 --- a/packages/web/src/pages/ControlBatchWizard.tsx +++ b/packages/web/src/pages/ControlBatchWizard.tsx @@ -15,7 +15,7 @@ import { useControlDefinitionWizardSeed, } from '../hooks/useControls' import { useUser } from '../contexts/UserContext' -import { PageError, fromQuery, getQueryErrorMessage } from '../ui' +import { PageError, fromQuery, getHttpStatus, getQueryErrorMessage } from '../ui' import '../styles/blood-controls.css' import { localToday } from '../lib/date-utils' @@ -203,6 +203,8 @@ function ControlBatchWizardContent() { return getQueryErrorMessage(batchBootstrapQuery.error, 'Failed to load batch') } if (isCreateFromDefinition && definitionSeedQuery.isError) { + // The definition summary and batch-name routes both 404 for a missing or non-blood definition. + if (getHttpStatus(definitionSeedQuery.error) === 404) return 'Control definition not found' return getQueryErrorMessage(definitionSeedQuery.error, 'Failed to load control definition') } if (isCompositionCsvFlow && compositionQuery.isError) { diff --git a/packages/web/src/pages/__tests__/ControlBatchWizard.test.tsx b/packages/web/src/pages/__tests__/ControlBatchWizard.test.tsx new file mode 100644 index 00000000..6626935a --- /dev/null +++ b/packages/web/src/pages/__tests__/ControlBatchWizard.test.tsx @@ -0,0 +1,63 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest' +import { Route, Routes } from 'react-router-dom' +import { render, screen } from '../../__tests__/helpers/render' +import ControlBatchWizard from '../ControlBatchWizard' +import { controlsApi } from '../../lib/api/controls' + +vi.mock('../../lib/api/controls', async () => { + const { createMockedDomainModule } = await import('../../__tests__/helpers/mock-api') + return createMockedDomainModule('controls', { + controlsApi: { getDefinitionSummary: vi.fn(), suggestBatchName: vi.fn() }, + }) +}) + +vi.mock('../../contexts/UserContext', async () => { + const actual = await vi.importActual('../../contexts/UserContext') + return { ...actual, useUser: () => ({ canWrite: true }) } +}) + +const notFound = (error: string) => Object.assign(new Error('Request failed with status code 404'), { + response: { status: 404, data: { error } }, +}) + +function renderWizard() { + return render( + + } /> + , + { initialEntries: ['/blood-controls/999/batches/new'] }, + ) +} + +describe('ControlBatchWizard', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it('says the control definition was not found when its summary 404s', async () => { + vi.mocked(controlsApi.getDefinitionSummary).mockRejectedValue(notFound('Blood control with id 999 not found')) + vi.mocked(controlsApi.suggestBatchName).mockResolvedValue({ name: 'unused' }) + await renderWizard() + + expect(await screen.findByText('Control definition not found')).toBeInTheDocument() + expect(controlsApi.getDefinitionSummary).toHaveBeenCalledTimes(1) + }) + + it('shows the same message when the batch-name lookup 404s first', async () => { + vi.mocked(controlsApi.getDefinitionSummary).mockReturnValue(new Promise(() => {})) + vi.mocked(controlsApi.suggestBatchName).mockRejectedValue(notFound('Control definition not found')) + await renderWizard() + + expect(await screen.findByText('Control definition not found')).toBeInTheDocument() + }) + + it('shows the server message for other failures', async () => { + vi.mocked(controlsApi.getDefinitionSummary).mockRejectedValue( + Object.assign(new Error('boom'), { response: { status: 500, data: { error: 'Database is locked' } } }), + ) + vi.mocked(controlsApi.suggestBatchName).mockResolvedValue({ name: 'unused' }) + await renderWizard() + + expect(await screen.findByText('Database is locked', {}, { timeout: 3000 })).toBeInTheDocument() + }) +}) diff --git a/packages/web/src/ui/async.ts b/packages/web/src/ui/async.ts index 85a69107..0e7cd4a1 100644 --- a/packages/web/src/ui/async.ts +++ b/packages/web/src/ui/async.ts @@ -18,6 +18,14 @@ export function fromQuery( return 'loading' } +/** HTTP status of a failed API call, or undefined for other errors. */ +export function getHttpStatus(error: unknown): number | undefined { + if (error && typeof error === 'object' && 'response' in error) { + return (error as { response?: { status?: number } }).response?.status + } + return undefined +} + export function getQueryErrorMessage(error: unknown, fallback: string): string { if (error && typeof error === 'object' && 'response' in error) { const response = (error as { response?: { data?: { error?: string } } }).response diff --git a/packages/web/src/ui/index.ts b/packages/web/src/ui/index.ts index f2fa1555..96bd8f6e 100644 --- a/packages/web/src/ui/index.ts +++ b/packages/web/src/ui/index.ts @@ -1,4 +1,4 @@ -export { type PresentationStatus, fromQuery, getQueryErrorMessage } from './async' +export { type PresentationStatus, fromQuery, getHttpStatus, getQueryErrorMessage } from './async' export { AsyncPresentation, type AsyncPresentationProps } from './AsyncPresentation' export { Button, buttonClassName, type ButtonProps, type ButtonVariant } from './Button' export { EmptyState, type EmptyStateProps } from './EmptyState' From ced8760d13704ab52bd34129b5dce12e018b1435 Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Thu, 29 Oct 2026 22:18:02 -0700 Subject: [PATCH 24/30] fix(ci): cancel superseded Checks runs only on pull requests, not on main. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/checks.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml index f4efb77b..de2ee7de 100644 --- a/.github/workflows/checks.yml +++ b/.github/workflows/checks.yml @@ -9,9 +9,11 @@ on: permissions: contents: read +# A newer push to a PR supersedes its older run; runs on main each check a different merge, so +# they all finish. concurrency: group: checks-${{ github.ref }} - cancel-in-progress: true + cancel-in-progress: ${{ github.event_name == 'pull_request' }} jobs: verify: From 2aa411dedfdd6161fc79ddd4082b539774564e5a Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Thu, 29 Oct 2026 22:19:06 -0700 Subject: [PATCH 25/30] fix(ci): refuse partially staged files in pre-commit instead of linting their working-tree copy. Co-Authored-By: Claude Opus 5.5 --- .githooks/pre-commit-checks.sh | 19 ++++++++++++++++++- AGENTS.md | 2 +- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/.githooks/pre-commit-checks.sh b/.githooks/pre-commit-checks.sh index 005135d7..f683f15e 100755 --- a/.githooks/pre-commit-checks.sh +++ b/.githooks/pre-commit-checks.sh @@ -1,6 +1,6 @@ #!/usr/bin/env bash # Pre-commit checks mirroring CI. Typechecks packages touched by staged files -# and lints the staged API/web source files. +# and lints the staged API/web source files (refusing ones with unstaged edits). set -euo pipefail ROOT="$(git rev-parse --show-toplevel)" @@ -42,6 +42,23 @@ if ! $has_staged || { ! $needs_contract && ! $needs_api && ! $needs_web; }; then exit 0 fi +# eslint reads files from disk, and the typed rules can't lint a staged blob over stdin (they take +# type information from the file on disk), so a file with unstaged edits would be linted as it is +# in the working tree rather than as it will be committed. Refuse those instead. +partially_staged=() +for file in ${api_lint_files[@]+"${api_lint_files[@]}"}; do + git diff --quiet -- "packages/api/$file" || partially_staged+=("packages/api/$file") +done +for file in ${web_lint_files[@]+"${web_lint_files[@]}"}; do + git diff --quiet -- "packages/web/$file" || partially_staged+=("packages/web/$file") +done +if (( ${#partially_staged[@]} > 0 )); then + echo "pre-commit: these files have unstaged changes, so the lint would check the working tree, not the commit:" >&2 + printf ' %s\n' "${partially_staged[@]}" >&2 + echo "Stage them fully, or set the unstaged edits aside (git stash push --keep-index) and commit again." >&2 + exit 1 +fi + echo "pre-commit: running typecheck for staged TypeScript changes..." if $needs_contract; then diff --git a/AGENTS.md b/AGENTS.md index 0ae94ecc..1d097171 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -45,7 +45,7 @@ Install repo hooks once per clone: bun run setup:hooks ``` -The pre-commit hook runs `typecheck` on `@sampledb/contract`, `@sampledb/api`, and/or `@sampledb/web` when staged files touch those packages (or Docker/build config), and lints the staged API/web source files. To mirror CI fully before pushing: +The pre-commit hook runs `typecheck` on `@sampledb/contract`, `@sampledb/api`, and/or `@sampledb/web` when staged files touch those packages (or Docker/build config), and lints the staged API/web source files. It refuses a source file with unstaged edits, since eslint would lint the working-tree copy. To mirror CI fully before pushing: ```bash bun run ci:verify From 81bf8ec56131a87f5c538674b6ce34076f68e8be Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Thu, 29 Oct 2026 22:19:35 -0700 Subject: [PATCH 26/30] test(db): say why the snapshot test opens its database without openOperationalDatabase. Co-Authored-By: Claude Opus 5.5 --- packages/api/src/db/__tests__/initial-schema-snapshot.test.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/api/src/db/__tests__/initial-schema-snapshot.test.ts b/packages/api/src/db/__tests__/initial-schema-snapshot.test.ts index 08587049..d9168e8f 100644 --- a/packages/api/src/db/__tests__/initial-schema-snapshot.test.ts +++ b/packages/api/src/db/__tests__/initial-schema-snapshot.test.ts @@ -92,6 +92,8 @@ function describeSchema(sqlite: Database) { * what the numbered migrations build on top of an older install. */ describe('initial_schema.sql snapshot', () => { + // Not openOperationalDatabase: it evolves the schema, which would run migrations over a snapshot + // that lags them. This database holds what initial_schema.sql alone creates. const sqlite = new Database(':memory:') applyInitialSchema(sqlite) afterAll(() => sqlite.close()) From a5ce4bf9e227481f918ad1addfb393b738bf544c Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Thu, 29 Oct 2026 22:19:54 -0700 Subject: [PATCH 27/30] refactor(web): drop the scrollIntoView guard in CollectionSelectOrCreate now that the test setup stubs it. Co-Authored-By: Claude Opus 5.5 --- packages/web/src/components/CollectionSelectOrCreate.tsx | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/web/src/components/CollectionSelectOrCreate.tsx b/packages/web/src/components/CollectionSelectOrCreate.tsx index e64a4ca4..08b4e794 100644 --- a/packages/web/src/components/CollectionSelectOrCreate.tsx +++ b/packages/web/src/components/CollectionSelectOrCreate.tsx @@ -103,9 +103,7 @@ export default function CollectionSelectOrCreate({ const el = containerRef.current?.querySelector( `[data-suggestion-index="${highlightIndex}"]` ) - if (el && typeof (el as HTMLElement).scrollIntoView === 'function') { - (el as HTMLElement).scrollIntoView({ block: 'nearest' }) - } + el?.scrollIntoView({ block: 'nearest' }) }, [showDropdown, highlightIndex]) useClickOutside(containerRef, () => setDropdownOpen(false), dropdownOpen) From be6cad4db664375c1ac29e03b64fa13d7a17049a Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Thu, 29 Oct 2026 22:20:53 -0700 Subject: [PATCH 28/30] refactor(web): pick the default scanner configuration through one defaultScannerConfiguration helper. Co-Authored-By: Claude Opus 5.5 --- .../lib/__tests__/scanner-configurations.test.ts | 16 ++++++++++++++++ packages/web/src/lib/scanner-configurations.ts | 4 ++++ packages/web/src/pages/ContainerMoveMicronix.tsx | 8 ++++---- packages/web/src/pages/PlateScanValidation.tsx | 7 ++++--- packages/web/src/pages/QpcrExperimentDetail.tsx | 9 ++++----- 5 files changed, 32 insertions(+), 12 deletions(-) create mode 100644 packages/web/src/lib/__tests__/scanner-configurations.test.ts create mode 100644 packages/web/src/lib/scanner-configurations.ts diff --git a/packages/web/src/lib/__tests__/scanner-configurations.test.ts b/packages/web/src/lib/__tests__/scanner-configurations.test.ts new file mode 100644 index 00000000..24286ed5 --- /dev/null +++ b/packages/web/src/lib/__tests__/scanner-configurations.test.ts @@ -0,0 +1,16 @@ +import { describe, it, expect } from 'vitest' +import { defaultScannerConfiguration } from '../scanner-configurations' + +describe('defaultScannerConfiguration', () => { + it('picks the configuration marked default', () => { + expect(defaultScannerConfiguration([{ id: 'a' }, { id: 'b', isDefault: true }])?.id).toBe('b') + }) + + it('falls back to the first configuration', () => { + expect(defaultScannerConfiguration([{ id: 'a', isDefault: false }, { id: 'b' }])?.id).toBe('a') + }) + + it('returns undefined for no configurations', () => { + expect(defaultScannerConfiguration([])).toBeUndefined() + }) +}) diff --git a/packages/web/src/lib/scanner-configurations.ts b/packages/web/src/lib/scanner-configurations.ts new file mode 100644 index 00000000..0137bff0 --- /dev/null +++ b/packages/web/src/lib/scanner-configurations.ts @@ -0,0 +1,4 @@ +/** The configuration marked default, else the first one; undefined for an empty list. */ +export function defaultScannerConfiguration(configurations: readonly T[]): T | undefined { + return configurations.find((c) => c.isDefault === true) ?? configurations[0] +} diff --git a/packages/web/src/pages/ContainerMoveMicronix.tsx b/packages/web/src/pages/ContainerMoveMicronix.tsx index e09085a9..a9a8abe2 100644 --- a/packages/web/src/pages/ContainerMoveMicronix.tsx +++ b/packages/web/src/pages/ContainerMoveMicronix.tsx @@ -11,6 +11,7 @@ import ScanMovePageShell from '../components/scan-move/ScanMovePageShell' import { MICRONIX_SCAN_MOVE_COPY } from '../components/scan-move/copy' import MicronixMoveInstructions from '../components/scan-move/MicronixMoveInstructions' import MicronixScannerConfigPanel from '../components/scan-move/MicronixScannerConfigPanel' +import { defaultScannerConfiguration } from '../lib/scanner-configurations' import '../styles/storage.css' import { EMPTY_LIST } from '../lib/empty-list' @@ -40,10 +41,9 @@ export default function ContainerMoveMicronix() { const fileInputRef = useRef(null) useEffect(() => { - if (scannerConfigurations.length === 0 || selectedConfigId !== null) return - const defaultConfig = - scannerConfigurations.find((c) => c.isDefault === true) ?? scannerConfigurations[0] - setSelectedConfigId(defaultConfig.id) + if (selectedConfigId !== null) return + const defaultConfig = defaultScannerConfiguration(scannerConfigurations) + if (defaultConfig) setSelectedConfigId(defaultConfig.id) }, [scannerConfigurations, selectedConfigId]) if (!canWrite) { diff --git a/packages/web/src/pages/PlateScanValidation.tsx b/packages/web/src/pages/PlateScanValidation.tsx index 8012e6a7..b0ac4793 100644 --- a/packages/web/src/pages/PlateScanValidation.tsx +++ b/packages/web/src/pages/PlateScanValidation.tsx @@ -3,6 +3,7 @@ import { Link } from 'react-router-dom' import { collectionsApi } from '../lib/api/collections'; import type { ValidatePlateScanResult, InferenceReport } from '../lib/api/collections'; import type { ScannerConfiguration } from '../lib/api/settings'; +import { defaultScannerConfiguration } from '../lib/scanner-configurations' import { usePlateScanBootstrap } from '../hooks/useExportWorkflow' import { PageError } from '../ui' import { extractPlateStemFromFilename, findPlateCandidatesFromStem } from '../lib/plate-filename-match' @@ -205,9 +206,9 @@ export default function PlateScanValidation() { const [error, setError] = useState(null) useEffect(() => { - if (selectedConfigId != null || scannerConfigurations.length === 0) return - const defaultConfig = scannerConfigurations.find((c) => c.isDefault === true) - setSelectedConfigId((defaultConfig ?? scannerConfigurations[0]).id) + if (selectedConfigId != null) return + const defaultConfig = defaultScannerConfiguration(scannerConfigurations) + if (defaultConfig) setSelectedConfigId(defaultConfig.id) }, [scannerConfigurations, selectedConfigId]) useEffect(() => { diff --git a/packages/web/src/pages/QpcrExperimentDetail.tsx b/packages/web/src/pages/QpcrExperimentDetail.tsx index 467a9c6f..2133f025 100644 --- a/packages/web/src/pages/QpcrExperimentDetail.tsx +++ b/packages/web/src/pages/QpcrExperimentDetail.tsx @@ -18,6 +18,7 @@ import { useUser } from '../contexts/UserContext' import { useToast } from '../contexts/ToastContext' import '../styles/qpcr.css' import { bytesToBase64 } from '../lib/base64' +import { defaultScannerConfiguration } from '../lib/scanner-configurations' import { EMPTY_LIST } from '../lib/empty-list' const PLATE_ROWS = ['A', 'B', 'C', 'D', 'E', 'F', 'G', 'H'] as const @@ -168,11 +169,9 @@ export default function QpcrExperimentDetail() { }, [experimentQuery.data]) useEffect(() => { - if (scannerConfigs.length === 0) return - const defaultConfig = scannerConfigs.find((c) => c.isDefault === true) ?? scannerConfigs[0] - if (selectedConfigId === null) { - setSelectedConfigId(defaultConfig.id) - } + if (selectedConfigId !== null) return + const defaultConfig = defaultScannerConfiguration(scannerConfigs) + if (defaultConfig) setSelectedConfigId(defaultConfig.id) }, [scannerConfigs, selectedConfigId]) // Sync form state from data.experiment when it changes (during render to avoid extra pass) From b32b4ac10b582fd3a3af1f3c1fbc007af18facff Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Thu, 29 Oct 2026 22:21:52 -0700 Subject: [PATCH 29/30] refactor(web): gate CollectionMove and ControlBatchWizard behind one WriteGuard. CollectionMove now redirects read-only users with rather than an effect that called navigate, as ControlBatchWizard already did. Co-Authored-By: Claude Opus 5.5 --- packages/web/src/components/WriteGuard.tsx | 13 ++++++ .../components/__tests__/WriteGuard.test.tsx | 41 +++++++++++++++++++ packages/web/src/pages/CollectionMove.tsx | 25 ++++------- packages/web/src/pages/ControlBatchWizard.tsx | 15 ++++--- 4 files changed, 68 insertions(+), 26 deletions(-) create mode 100644 packages/web/src/components/WriteGuard.tsx create mode 100644 packages/web/src/components/__tests__/WriteGuard.test.tsx diff --git a/packages/web/src/components/WriteGuard.tsx b/packages/web/src/components/WriteGuard.tsx new file mode 100644 index 00000000..30e352ae --- /dev/null +++ b/packages/web/src/components/WriteGuard.tsx @@ -0,0 +1,13 @@ +import type { ReactNode } from 'react' +import { Navigate } from 'react-router-dom' +import { useUser } from '../contexts/UserContext' + +/** + * Renders children only for a user who can write, and sends anyone else to `redirectTo`. + * Keeping the check outside the children lets their hooks run on every render. + */ +export default function WriteGuard({ redirectTo, children }: { redirectTo: string; children: ReactNode }) { + const { canWrite } = useUser() + if (!canWrite) return + return <>{children} +} diff --git a/packages/web/src/components/__tests__/WriteGuard.test.tsx b/packages/web/src/components/__tests__/WriteGuard.test.tsx new file mode 100644 index 00000000..836a81c0 --- /dev/null +++ b/packages/web/src/components/__tests__/WriteGuard.test.tsx @@ -0,0 +1,41 @@ +import { describe, it, expect, vi } from 'vitest' +import { Route, Routes } from 'react-router-dom' +import { render, screen } from '../../__tests__/helpers/render' +import WriteGuard from '../WriteGuard' + +const mockUseUser = vi.fn() +vi.mock('../../contexts/UserContext', async () => { + const actual = await vi.importActual('../../contexts/UserContext') + return { ...actual, useUser: () => mockUseUser() } +}) + +async function renderAt(canWrite: boolean) { + mockUseUser.mockReturnValue({ canWrite }) + await render( + + Home} /> + + Protected content + + } + /> + , + { initialEntries: ['/move'] }, + ) +} + +describe('WriteGuard', () => { + it('renders children for a user who can write', async () => { + await renderAt(true) + expect(screen.getByText('Protected content')).toBeInTheDocument() + }) + + it('redirects a read-only user without rendering children', async () => { + await renderAt(false) + expect(screen.getByText('Home')).toBeInTheDocument() + expect(screen.queryByText('Protected content')).not.toBeInTheDocument() + }) +}) diff --git a/packages/web/src/pages/CollectionMove.tsx b/packages/web/src/pages/CollectionMove.tsx index d70f9328..af8cfb15 100644 --- a/packages/web/src/pages/CollectionMove.tsx +++ b/packages/web/src/pages/CollectionMove.tsx @@ -1,11 +1,10 @@ -import { useState, useEffect, useMemo } from 'react' -import { useNavigate } from 'react-router-dom' +import { useState, useMemo } from 'react' import { useQueryClient } from '@tanstack/react-query' import { invalidateAfterBulkWrite } from '../lib/query-client' import { collectionsApi } from '../lib/api/collections' import CollectionMoveTreePicker, { type Collection } from '../components/CollectionMoveTreePicker' import LocationTreePicker, { type LocationSelection } from '../components/LocationTreePicker' -import { useUser } from '../contexts/UserContext' +import WriteGuard from '../components/WriteGuard' import { useAllCollections } from '../hooks/useCollections' import { useLocationsList } from '../hooks/useLocations' import { PageError, fromQuery, getQueryErrorMessage } from '../ui' @@ -25,23 +24,13 @@ function fromKey(key: string): { type: CollectionType; id: number } { } export default function CollectionMove() { - const navigate = useNavigate() - const { canWrite } = useUser() - - // Redirect if user doesn't have write permissions - useEffect(() => { - if (!canWrite) { - navigate('/', { replace: true }) - } - }, [canWrite, navigate]) - - if (!canWrite) { - return null - } - return + return ( + + + + ) } -/** Split out so its hooks run on every render, whatever canWrite is. */ function CollectionMoveContent() { const queryClient = useQueryClient() const [currentStep, setCurrentStep] = useState('select-collections') diff --git a/packages/web/src/pages/ControlBatchWizard.tsx b/packages/web/src/pages/ControlBatchWizard.tsx index 9343a61c..aeb34527 100644 --- a/packages/web/src/pages/ControlBatchWizard.tsx +++ b/packages/web/src/pages/ControlBatchWizard.tsx @@ -1,5 +1,5 @@ import { useState, useEffect, useMemo } from 'react' -import { useNavigate, useParams, useSearchParams, Navigate } from 'react-router-dom' +import { useNavigate, useParams, useSearchParams } from 'react-router-dom' import { useQueryClient } from '@tanstack/react-query' import { invalidateAfterBulkWrite } from '../lib/query-client' import BatchInfoStep from '../components/wizards/BatchInfoStep' @@ -14,7 +14,7 @@ import { useControlBatchWizardBootstrap, useControlDefinitionWizardSeed, } from '../hooks/useControls' -import { useUser } from '../contexts/UserContext' +import WriteGuard from '../components/WriteGuard' import { PageError, fromQuery, getHttpStatus, getQueryErrorMessage } from '../ui' import '../styles/blood-controls.css' import { localToday } from '../lib/date-utils' @@ -105,14 +105,13 @@ export function canProceedToReview( } export default function ControlBatchWizard() { - const { canWrite } = useUser() - if (!canWrite) { - return - } - return + return ( + + + + ) } -/** Split out so its hooks run on every render, whatever canWrite is. */ function ControlBatchWizardContent() { const navigate = useNavigate() const queryClient = useQueryClient() From cacf56f8d94120efd3c6e0ea94f5c293ca49db63 Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Thu, 29 Oct 2026 22:57:11 -0700 Subject: [PATCH 30/30] test(lint): type-check each rule test case as its own code when CI=true. With CI=true typescript-eslint infers a single run and builds one program from the files on disk, so every case was checked as the placeholder fixture and the invalid cases reported nothing on GitHub Actions. Co-Authored-By: Claude Opus 5.5 --- .../__tests__/no-promise-transaction-callback.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/api/eslint-rules/__tests__/no-promise-transaction-callback.test.ts b/packages/api/eslint-rules/__tests__/no-promise-transaction-callback.test.ts index 2a108898..666b9906 100644 --- a/packages/api/eslint-rules/__tests__/no-promise-transaction-callback.test.ts +++ b/packages/api/eslint-rules/__tests__/no-promise-transaction-callback.test.ts @@ -14,7 +14,9 @@ const fixtures = join(dirname(fileURLToPath(import.meta.url)), 'fixtures') const ruleTester = new RuleTester({ languageOptions: { parser, - parserOptions: { project: './tsconfig.json', tsconfigRootDir: fixtures }, + // With CI=true, typescript-eslint builds one program from the files on disk, so every case + // would be type-checked as the placeholder fixtures/file.ts instead of its own code. + parserOptions: { project: './tsconfig.json', tsconfigRootDir: fixtures, disallowAutomaticSingleRunInference: true }, }, })