From 6de04c62cabd00219f26e8bc79777b8563057761 Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Fri, 25 Sep 2026 09:26:47 -0700 Subject: [PATCH 01/35] fix(api): return 409 for constraint conflicts and harden login lookup. UNIQUE and FOREIGN KEY violations (a duplicate study title, re-using a deleted account's email) returned 500 with the raw SQLite message. They now return 409 with a readable message; the SQLite text is only included outside production. Login matched email or username with one query and took the first row, so a username equal to someone's email could shadow them. Email is now matched first, and usernames cannot contain "@". Admin settings writes validate the target userId. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/api/src/lib/auth/auth-service.ts | 16 +++++----- packages/api/src/lib/error-handler.ts | 31 +++++++++++++++++++ .../api/src/routes/__tests__/auth.test.ts | 25 +++++++++++++++ .../api/src/routes/__tests__/studies.test.ts | 9 ++++++ packages/api/src/routes/auth.ts | 16 +++++++--- packages/api/src/routes/settings.ts | 3 ++ 6 files changed, 89 insertions(+), 11 deletions(-) diff --git a/packages/api/src/lib/auth/auth-service.ts b/packages/api/src/lib/auth/auth-service.ts index 0b2aa810..0dd9c09a 100644 --- a/packages/api/src/lib/auth/auth-service.ts +++ b/packages/api/src/lib/auth/auth-service.ts @@ -1,4 +1,4 @@ -import { eq, and, isNull, or } from 'drizzle-orm' +import { eq, and, isNull } from 'drizzle-orm' import bcrypt from 'bcryptjs' import { nanoid } from 'nanoid' import type { Database } from '../../db/client' @@ -29,16 +29,18 @@ export async function findActiveUserByEmailOrUsername( database: Database, emailOrUsername: string ): Promise { + // Email first: a username must never shadow another user's email address. + const byEmail = await database + .select() + .from(users) + .where(and(eq(users.email, emailOrUsername), isNull(users.deletedAt))) + .get() + if (byEmail) return byEmail return ( (await database .select() .from(users) - .where( - and( - or(eq(users.email, emailOrUsername), eq(users.username, emailOrUsername)), - isNull(users.deletedAt) - ) - ) + .where(and(eq(users.username, emailOrUsername), isNull(users.deletedAt))) .get()) ?? null ) } diff --git a/packages/api/src/lib/error-handler.ts b/packages/api/src/lib/error-handler.ts index d613a87b..5b0b6462 100644 --- a/packages/api/src/lib/error-handler.ts +++ b/packages/api/src/lib/error-handler.ts @@ -29,6 +29,25 @@ function isZodError(error: unknown): error is z.ZodError { return 'issues' in obj && Array.isArray(obj.issues) } +/** Readable message for a SQLite UNIQUE / FOREIGN KEY violation, or null for other errors. */ +function sqliteConstraintConflict(error: Error): string | null { + const code = (error as { code?: unknown }).code + if (code === 'SQLITE_CONSTRAINT_UNIQUE' || code === 'SQLITE_CONSTRAINT_PRIMARYKEY') { + // "UNIQUE constraint failed: study.title, study.x" -> "title, x" + const columns = error.message + .replace(/^.*constraint failed:\s*/i, '') + .split(',') + .map((col) => col.trim().split('.').pop()?.replace(/_/g, ' ')) + .filter(Boolean) + .join(', ') + return columns ? `A record with this ${columns} already exists.` : 'This record already exists.' + } + if (code === 'SQLITE_CONSTRAINT_FOREIGNKEY') { + return 'This record is still referenced by other data, or refers to a record that does not exist.' + } + return null +} + export function handleRouteError(error: unknown, c: Context): Response { const database = getRequestDatabase(c) @@ -131,6 +150,18 @@ export function handleRouteError(error: unknown, c: Context): Response { }, 400) } + // A UNIQUE or FOREIGN KEY violation is a conflict with existing data, not a server fault. + // Reply 409 with a readable message instead of the raw SQLite text. + const constraint = sqliteConstraintConflict(error) + if (constraint) { + logError(database, 'backend', 'warning', error.message, error, errorContext).catch(logPersistFailure) + return c.json({ + error: constraint, + errorCode: 'CONFLICT', + ...(isDevelopment && { details: error.message }), + }, 409) + } + // Log error asynchronously (non-blocking) logBackendError(database, error, errorContext).catch(logPersistFailure) return c.json({ diff --git a/packages/api/src/routes/__tests__/auth.test.ts b/packages/api/src/routes/__tests__/auth.test.ts index 6d097e8e..4f948666 100644 --- a/packages/api/src/routes/__tests__/auth.test.ts +++ b/packages/api/src/routes/__tests__/auth.test.ts @@ -388,6 +388,31 @@ describe('Auth API', () => { expect(cookies).toContain('session_id') }) + it('matches an email before a username that equals it', async () => { + await testDb.insert(users).values({ + email: 'squatter@example.com', + name: 'Squatter', + username: 'victim@example.com', + passwordHash: await bcrypt.hash('squatter123', 10), + role: 'member', + createdAt: utcNow(), + approvedAt: utcNow(), + }) + await createTestUser(testDb, { email: 'victim@example.com', name: 'Victim', password: 'password123' }) + + const res = await createTestClient(app).api.auth.login.$post({ + json: { emailOrUsername: 'victim@example.com', password: 'password123' }, + }) + expect(res.status).toBe(200) + }) + + it('rejects a username containing "@"', async () => { + const res = await createTestClient(app).api.auth['self-register'].$post({ + json: { email: 'new@example.com', name: 'New', username: 'other@example.com', password: 'password123' }, + }) + expect(res.status).toBe(400) + }) + it('should reject invalid username', async () => { const client = createTestClient(app) const res = await client.api.auth.login.$post({ diff --git a/packages/api/src/routes/__tests__/studies.test.ts b/packages/api/src/routes/__tests__/studies.test.ts index 2c878169..30e40ba8 100644 --- a/packages/api/src/routes/__tests__/studies.test.ts +++ b/packages/api/src/routes/__tests__/studies.test.ts @@ -90,6 +90,15 @@ describe('Studies API (list, get, create, update)', () => { expect(data.study.shortCode).toBe('NEW01') }) + it('returns 409, not 500, for a duplicate title', async () => { + const body = { title: 'Dup Study', shortCode: 'DUP01', isLongitudinal: false, leadPerson: 'Lead' } + await ctx.request('/api/studies', { method: 'POST', json: body }) + const res = await ctx.request('/api/studies', { method: 'POST', json: { ...body, shortCode: 'DUP02' } }) + expect(res.status).toBe(409) + const data = (await res.json()) as { error: string } + expect(data.error).toBe('A record with this title already exists.') + }) + it('returns 401 when not authenticated', async () => { const res = await authenticatedRequest(ctx.createRequestApp(), '/api/studies', { method: 'POST', diff --git a/packages/api/src/routes/auth.ts b/packages/api/src/routes/auth.ts index 99284490..5e8992ed 100644 --- a/packages/api/src/routes/auth.ts +++ b/packages/api/src/routes/auth.ts @@ -49,6 +49,14 @@ const loginSchema = z.object({ password: z.string().min(1), }) +/** Usernames cannot contain "@", so a username can never be confused with an email at login. */ +const usernameSchema = z + .string() + .min(1) + .refine((value) => !value.includes('@'), { message: 'Username cannot contain "@"' }) + .optional() + .nullable() + // Dynamic register schema - password min length will be set based on settings const createRegisterSchema = async () => { const passwordRequirements = await getPasswordRequirementsFromDb() @@ -59,7 +67,7 @@ const createRegisterSchema = async () => { return z.object({ email: z.string().email(), name: z.string().min(1), - username: z.string().min(1).optional().nullable(), + username: usernameSchema, password: z.string().min(minLength), role: z.enum(['admin', 'member', 'viewer']).default('member'), }) @@ -113,7 +121,7 @@ const createSelfRegisterSchema = async () => { return z.object({ email: z.string().email(), name: z.string().min(1), - username: z.string().min(1).optional().nullable(), + username: usernameSchema, password: z.string().min(minLength), }) } @@ -245,7 +253,7 @@ auth.patch('/me', authMiddleware, async (c) => { const updateSchema = z.object({ name: z.string().min(1).optional(), email: z.string().email().optional(), - username: z.string().min(1).optional().nullable(), + username: usernameSchema, }) const data = updateSchema.parse(body) @@ -527,7 +535,7 @@ auth.put('/users/:id', adminMiddleware, async (c) => { const updateSchema = z.object({ name: z.string().min(1).optional(), email: z.string().email().optional(), - username: z.string().min(1).optional().nullable(), + username: usernameSchema, role: z.enum(['admin', 'member', 'viewer']).optional(), }) const data = updateSchema.parse(body) diff --git a/packages/api/src/routes/settings.ts b/packages/api/src/routes/settings.ts index a5ed9265..0a3b1a30 100644 --- a/packages/api/src/routes/settings.ts +++ b/packages/api/src/routes/settings.ts @@ -259,6 +259,9 @@ const tableViewConfigurationsSchema = z.object({ // Admins can specify userId in body to set for specific user, or omit for system-wide // Non-admins can only set user-specific settings for themselves const targetUserId = body.userId !== undefined ? body.userId : (isAdmin ? null : userId) + if (targetUserId !== null && targetUserId !== undefined && !(Number.isInteger(targetUserId) && targetUserId > 0)) { + return c.json({ error: 'userId must be a positive integer or null' }, 400) + } const actualBody = { ...body } delete actualBody.userId // Remove userId from body before validation From 4246f3b841e5629324a8efa193a39fb28f368300 Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Fri, 25 Sep 2026 09:27:58 -0700 Subject: [PATCH 02/35] fix(export): quote the chosen delimiter and neutralize formula cells. escapeCsvCell only quoted cells containing a comma, so ";" and tab exports split any value containing the delimiter into extra columns. Data exports (API exports and the collection table download) now prefix text cells starting with =, +, @, tab, or CR with "'" so a spreadsheet does not run them. A leading "-" is treated the same way unless it starts a number or a phrase, so values like "-131" export unchanged. Import templates are not affected. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../src/lib/export/__tests__/format.test.ts | 9 ++++-- packages/api/src/lib/export/format.ts | 4 +-- packages/contract/src/__tests__/csv.test.ts | 21 +++++++++++++ packages/contract/src/csv.ts | 31 ++++++++++++++++--- .../components/CollectionTableWithExport.tsx | 2 +- 5 files changed, 57 insertions(+), 10 deletions(-) diff --git a/packages/api/src/lib/export/__tests__/format.test.ts b/packages/api/src/lib/export/__tests__/format.test.ts index d25f9a89..56ab1547 100644 --- a/packages/api/src/lib/export/__tests__/format.test.ts +++ b/packages/api/src/lib/export/__tests__/format.test.ts @@ -203,9 +203,14 @@ describe('formatSimpleCSV', () => { ]) }) - it('handles cells starting with equals sign as plain text', () => { + it('neutralizes cells a spreadsheet would run as a formula', () => { const result = formatSimpleCSV(['formula'], [['=SUM(A1:A10)']], lfNoBom) - expect(result).toBe('formula\n=SUM(A1:A10)') + expect(result).toBe("formula\n'=SUM(A1:A10)") + }) + + it('leaves negative numbers and "-" phrases unchanged', () => { + const result = formatSimpleCSV(['subject', 'comment'], [['-131', '-2 in SubjID']], lfNoBom) + expect(result).toBe('subject,comment\n-131,-2 in SubjID') }) }) }) diff --git a/packages/api/src/lib/export/format.ts b/packages/api/src/lib/export/format.ts index c0814cba..3536ccdc 100644 --- a/packages/api/src/lib/export/format.ts +++ b/packages/api/src/lib/export/format.ts @@ -22,7 +22,7 @@ export function formatSimpleCSV( } const formattedRows = rows.map((row) => formatExportRow(headers, row)) - return serializeCsv(headers, formattedRows, options) + return serializeCsv(headers, formattedRows, { ...options, neutralizeFormulas: true }) } export async function formatAsCSV( @@ -41,7 +41,7 @@ export async function formatAsCSV( columnKeys.map((header) => formatExportCellValue(header, (row as any)[header])) ) - return serializeCsv(columnKeys, rows, options) + return serializeCsv(columnKeys, rows, { ...options, neutralizeFormulas: true }) } export async function formatAsJSON( diff --git a/packages/contract/src/__tests__/csv.test.ts b/packages/contract/src/__tests__/csv.test.ts index 6bf732f0..25eee151 100644 --- a/packages/contract/src/__tests__/csv.test.ts +++ b/packages/contract/src/__tests__/csv.test.ts @@ -119,4 +119,25 @@ describe('serializeCsv + parseCsv round trip', () => { const serialized = serializeCsv(columns, rows) expect(parseCsv(serialized)).toEqual([columns, ...rows]) }) + + describe('delimiter and formula handling', () => { + it('quotes cells containing a non-comma delimiter so they round-trip', () => { + const csv = serializeCsv(['id', 'comment'], [[1, 'low; hemolyzed']], { delimiter: ';', bom: false, lineEnding: 'lf' }) + expect(csv).toBe('id;comment\n1;"low; hemolyzed"') + expect(parseCsv(csv, { delimiter: ';' })[1]).toEqual(['1', 'low; hemolyzed']) + }) + + it('neutralizes formula-like text cells only when asked', () => { + const rows = [['=SUM(A1)', '+cmd', '@x', '-A1+1', '-131', '-2 in SubjID', -5, 'ok']] + const neutralized = serializeCsv(['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h'], rows, { + bom: false, + lineEnding: 'lf', + neutralizeFormulas: true, + }) + expect(neutralized.split('\n')[1]).toBe("'=SUM(A1),'+cmd,'@x,'-A1+1,-131,-2 in SubjID,-5,ok") + + const plain = serializeCsv(['a'], [['=SUM(A1)']], { bom: false, lineEnding: 'lf' }) + expect(plain.split('\n')[1]).toBe('=SUM(A1)') + }) + }) }) diff --git a/packages/contract/src/csv.ts b/packages/contract/src/csv.ts index cc09bb72..088e31cf 100644 --- a/packages/contract/src/csv.ts +++ b/packages/contract/src/csv.ts @@ -4,6 +4,11 @@ export interface CSVExportOptions { delimiter?: string bom?: boolean lineEnding?: CsvLineEnding + /** + * Prefix text cells a spreadsheet would run as a formula with `'`. + * Use for exports of stored data; leave off for templates meant to be re-imported. + */ + neutralizeFormulas?: boolean } export type CsvCellValue = string | number | null | undefined @@ -12,10 +17,25 @@ const DEFAULT_DELIMITER = ',' const DEFAULT_BOM = true const DEFAULT_LINE_ENDING: CsvLineEnding = 'crlf' -export function escapeCsvCell(value: CsvCellValue): string { +/** + * Text a spreadsheet would evaluate: leading =, +, @, tab, or CR, and "-" unless it starts a + * number or a phrase ("-131", "-2 in SubjID") so stored negative values export unchanged. + */ +function looksLikeFormula(s: string): boolean { + return /^[=+@\t\r]/.test(s) || /^-[^\d\s.]/.test(s) +} + +export function escapeCsvCell( + value: CsvCellValue, + options: { delimiter?: string; neutralizeFormulas?: boolean } = {}, +): string { if (value === null || value === undefined) return '' - const s = String(value) - if (s.includes(',') || s.includes('"') || s.includes('\n') || s.includes('\r')) { + const delimiter = options.delimiter ?? DEFAULT_DELIMITER + let s = String(value) + if (options.neutralizeFormulas && typeof value === 'string' && looksLikeFormula(s)) { + s = `'${s}` + } + if (s.includes(delimiter) || s.includes('"') || s.includes('\n') || s.includes('\r')) { return `"${s.replace(/"/g, '""')}"` } return s @@ -31,10 +51,11 @@ export function serializeCsv( const lineEnding = options.lineEnding ?? DEFAULT_LINE_ENDING const eol = lineEnding === 'crlf' ? '\r\n' : '\n' + const cellOptions = { delimiter, neutralizeFormulas: options.neutralizeFormulas } const formatRow = (row: CsvCellValue[]) => - row.map((cell) => escapeCsvCell(cell)).join(delimiter) + row.map((cell) => escapeCsvCell(cell, cellOptions)).join(delimiter) - const header = formatRow(columns) + const header = columns.map((cell) => escapeCsvCell(cell, { delimiter })).join(delimiter) const body = rows.map(formatRow).join(eol) const content = rows.length ? `${header}${eol}${body}` : header return bom ? `\uFEFF${content}` : content diff --git a/packages/web/src/components/CollectionTableWithExport.tsx b/packages/web/src/components/CollectionTableWithExport.tsx index 89f7e6d2..da4ff810 100644 --- a/packages/web/src/components/CollectionTableWithExport.tsx +++ b/packages/web/src/components/CollectionTableWithExport.tsx @@ -21,7 +21,7 @@ export default function CollectionTableWithExport({ const handleExportCsv = () => { const header = columns.map((c) => c.label) const rowArrays = rows.map((row) => columns.map((col) => row[col.key] ?? '')) - const csv = buildCsv(header, rowArrays) + const csv = buildCsv(header, rowArrays, { neutralizeFormulas: true }) downloadCsv(csv, exportFilename) } From 71c19194ecd237f6fb8ec19fe5531172e2dcf086 Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Fri, 25 Sep 2026 09:30:09 -0700 Subject: [PATCH 03/35] fix(api): validate derivation CSV rows the way the import writes them. - Positions were compared unnormalized, so "A2" passed validation over a stored A02 and "A3"/"A03" were not flagged as duplicates; the import then failed and rolled back. Positions are normalized and checked with the shared occupancy rule (static wells included). - A blank reduce_parent_quantity cell overrode a shared "false" setting. - Quantities that are not numbers ("2,5", "3x") were silently dropped; they are now row errors. - Paper rows passed validation if any paper existed anywhere, then failed on import when the named box or bag was missing. - Headers match case-insensitively, and rows with no values (Excel's trailing ",,,") are skipped. - The source specimen type warning compares against the first row whose parent resolves, not only row 0. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../src/lib/__tests__/derivations-csv.test.ts | 107 ++++++++++++++++++ packages/api/src/lib/derivations-csv.ts | 102 ++++++++++++----- 2 files changed, 183 insertions(+), 26 deletions(-) diff --git a/packages/api/src/lib/__tests__/derivations-csv.test.ts b/packages/api/src/lib/__tests__/derivations-csv.test.ts index 6fd32f6a..1c28a946 100644 --- a/packages/api/src/lib/__tests__/derivations-csv.test.ts +++ b/packages/api/src/lib/__tests__/derivations-csv.test.ts @@ -78,6 +78,11 @@ describe('derivations-csv', () => { expect(rows[0].parent_container_id).toBe('1') }) + it('matches headers case-insensitively and drops rows with no values', () => { + const rows = parseCsv('Parent_Container_ID,Plate_Name\n1,P1\n,\n,,\n') + expect(rows).toEqual([{ parent_container_id: '1', plate_name: 'P1' }]) + }) + it('handles newlines inside quoted fields', () => { const text = 'parent_container_id,notes\n1,"line one\nline two"' const rows = parseCsv(text) @@ -283,4 +288,106 @@ describe('derivations-csv', () => { expect(child!.unitId).toBe(unit.id) }) }) + + describe('row values and positions', () => { + let testDb: Database + let sqlite: Awaited>['sqlite'] + + beforeEach(async () => { + const setup = await setupTestDatabase() + testDb = setup.db + sqlite = setup.sqlite + }) + + afterEach(() => { + if (sqlite) cleanupTestDatabase(sqlite) + }) + + const settings = { + derivationType: 'Extraction', + specimenTypeName: 'DNA', + containerType: 'micronix_tube' as const, + protocol: 'Standard', + derivationDate: '2025-01-15', + } + const header = 'parent_container_id,plate_name,position,container_barcode' + + /** Parent tube at PlateDef/A01 holding 10 uL, plus a tube already at A02. */ + async function seedParent() { + const now = utcNow() + const unit = await createTestUnit(testDb, { symbol: 'uL', name: 'microliter', category: 'volume' }) + await setContainerDefaults(testDb, { micronix_tube: { totalQuantity: 1, remainingQuantity: 1, defaultUnitSymbol: 'uL' } }) + await testDb.insert(containerTypeUnit).values({ containerType: 'micronix_tube', unitId: unit.id }) + const study = await createTestStudy(testDb, { title: 'Deriv Study', shortCode: 'DERIV' }) + const subject = await createTestStudySubject(testDb, { studyId: study.id, name: 'Subj1' }) + const bloodType = await createTestSpecimenType(testDb, { name: 'Blood' }) + const dnaType = await createTestSpecimenType(testDb, { name: 'DNA' }) + await testDb.insert(specimenTypeContainerType).values([ + { specimenTypeId: bloodType.id, containerType: 'micronix_tube', created: now }, + { specimenTypeId: dnaType.id, containerType: 'micronix_tube', created: now }, + ]) + const [parentSpecimen] = await testDb + .insert(specimen) + .values({ studySubjectId: subject.id, specimenTypeId: bloodType.id, collectionDate: '2025-01-01', created: now, lastUpdated: now }) + .returning() + const storageType = await createTestStorageType(testDb, { name: 'Freezer' }) + const loc = await createTestLocation(testDb, { name: 'Loc', storageTypeId: String(storageType.id), canContainCollections: true }) + const plate = await createTestMicronixPlate(testDb, { name: 'PlateDef', locationId: loc.id }) + const addTube = async (barcode: string, position: string, quantity: number) => { + const [sc] = await testDb + .insert(storageContainer) + .values({ specimenId: parentSpecimen.id, unitId: unit.id, totalQuantity: quantity, remainingQuantity: quantity, created: now, lastUpdated: now }) + .returning() + await testDb.insert(micronixTube).values({ id: sc.id, collectionId: plate.id, position, barcode }) + return sc + } + const parent = await addTube('MT-PARENT', 'A01', 10) + await addTube('MT-TAKEN', 'A02', 1) + return { parent } + } + + it('checks non-padded positions against stored ones and against each other', async () => { + const { parent } = await seedParent() + const csv = [header, `${parent.id},PlateDef,A2,C1`, `${parent.id},PlateDef,A3,C2`, `${parent.id},PlateDef,A03,C3`].join('\n') + + const result = await validateDerivationsCsv(testDb, csv, settings) + + expect(result.rows.map((r) => r.valid)).toEqual([false, true, false]) + expect(result.rows[0].error).toMatch(/already used in that plate/) + expect(result.rows[2].error).toMatch(/more than once in your file/) + }) + + it('reports a quantity that is not a number instead of dropping it', async () => { + const { parent } = await seedParent() + const csv = [`${header},quantity_used`, `${parent.id},PlateDef,B01,C1,3x`].join('\n') + + const result = await validateDerivationsCsv(testDb, csv, settings) + + expect(result.rows[0].valid).toBe(false) + expect(result.rows[0].error).toBe("quantity_used '3x' is not a number") + }) + + it('uses the shared reduce-parent setting when the cell is blank', async () => { + const { parent } = await seedParent() + const csv = [`${header},quantity_used,reduce_parent_quantity`, `${parent.id},PlateDef,B01,C1,3,`].join('\n') + + const result = await importDerivationsFromCsv(testDb, csv, { + settings: { ...settings, reduceParentQuantity: false }, + }) + + expect(result.rows[0].success).toBe(true) + const after = await testDb.select().from(storageContainer).where(eq(storageContainer.id, parent.id)).get() + expect(after?.remainingQuantity).toBe(10) + }) + + it('rejects paper rows whose box does not exist', async () => { + const { parent } = await seedParent() + const csv = ['parent_container_id,container_type,box_name,sheet_name,sublabel', `${parent.id},paper,No Such Box,Sheet-1,S1`].join('\n') + + const result = await validateDerivationsCsv(testDb, csv, { ...settings, containerType: '' }) + + expect(result.rows[0].valid).toBe(false) + expect(result.rows[0].error).toMatch(/Box 'No Such Box' not found/) + }) + }) }) diff --git a/packages/api/src/lib/derivations-csv.ts b/packages/api/src/lib/derivations-csv.ts index c39ad74d..78ad65b2 100644 --- a/packages/api/src/lib/derivations-csv.ts +++ b/packages/api/src/lib/derivations-csv.ts @@ -1,6 +1,8 @@ import type { Database } from '../db/client' import type { DatabaseOrTransaction } from './db-types' import { + bag, + box, containerDerivation, cryovialBox, cryovialTube, @@ -24,6 +26,8 @@ import { import { createDerivation, type CreateDerivationInput } from './derivations' import { withWriteTransaction } from '../db/write-transaction' import { resolveParentContainerId } from './derivations/parent-container-resolver' +import { normalizePosition } from './normalize-position' +import { checkGridPositionOccupancy } from './container-occupancy' /** * Map technical DB/constraint errors to messages that are clear for non-technical users. @@ -157,6 +161,23 @@ function parseBoolean(value?: string): boolean | undefined { return v === 'true' || v === '1' || v === 'yes' || v === 'y' } +const BOOLEAN_WORDS = new Set(['true', 'false', '1', '0', 'yes', 'no', 'y', 'n']) + +/** First problem with a row's typed cells, so a bad value is reported instead of silently dropped. */ +function rowValueError(row: DerivationCsvRow): string | null { + for (const column of ['quantity', 'quantity_used'] as const) { + const raw = row[column] + if (raw != null && raw.trim() !== '' && parseNumber(raw) === undefined) { + return `${column} '${raw}' is not a number` + } + } + const flag = row.reduce_parent_quantity + if (flag != null && flag.trim() !== '' && !BOOLEAN_WORDS.has(flag.trim().toLowerCase())) { + return `reduce_parent_quantity '${flag}' must be true or false` + } + return null +} + function parseNumber(value?: string): number | undefined { if (value == null || value.trim() === '') return undefined const n = Number(value) @@ -203,6 +224,11 @@ function buildChildSummary(row: DerivationCsvRow): string { return 'Child' } +/** Paper rows name their parent with box_name or bag_name. */ +function paperParentTypeFor(row: DerivationCsvRow): 'box' | 'bag' { + return row.box_name ? 'box' : row.bag_name ? 'bag' : 'box' +} + function getCollectionNameForType( row: DerivationCsvRow, containerType: DerivationCsvRow['container_type'] | 'micronix_tube', @@ -248,11 +274,13 @@ function derivationCsvRowToContainerWrite( * skipped, matching the previous line-based parser. */ export function parseCsv(text: string): DerivationCsvRow[] { + // Drop rows with no values at all (blank lines, or Excel's trailing ",,,," rows). const cells = parseCsvCells(text) - .filter(row => !(row.length === 1 && row[0].trim() === '')) + .filter(row => row.some(cell => cell.trim() !== '')) if (cells.length === 0) return [] - const headers = cells[0].map(h => h.trim()) + // Headers are matched case-insensitively ("Plate_Name" -> plate_name). + const headers = cells[0].map(h => h.trim().toLowerCase()) const rows: DerivationCsvRow[] = [] for (let i = 1; i < cells.length; i++) { @@ -272,6 +300,7 @@ async function resolveCollectionId( containerType: DerivationCsvRow['container_type'], collectionName?: string, collectionBarcode?: string, + paperParentType: 'box' | 'bag' = 'box', ): Promise<{ id?: number; status: 'existing' | 'will_be_created' }> { if (!collectionName && !collectionBarcode) { return { status: 'will_be_created' } @@ -318,12 +347,16 @@ async function resolveCollectionId( } if (containerType === 'paper') { - const sheetRec = await database - .select({ id: paper.sheetId }) - .from(paper) - .limit(1) + // Paper goes on a sheet in the named box or bag; the sheet is created if missing, + // but the box or bag itself must already exist. + if (!collectionName) return { status: 'will_be_created' } + const parentTable = paperParentType === 'bag' ? bag : box + const parent = await database + .select({ id: parentTable.id }) + .from(parentTable) + .where(eq(parentTable.name, collectionName.trim())) .get() - return { id: sheetRec?.id, status: sheetRec?.id ? 'existing' : 'will_be_created' } + return { id: parent?.id, status: parent ? 'existing' : 'will_be_created' } } return { status: 'will_be_created' } @@ -388,6 +421,14 @@ export async function validateDerivationsCsv( } } + const valueError = rowValueError(row) + if (valueError) { + validationRow.error = valueError + validationRows.push(validationRow) + invalidCount++ + continue + } + const containerType = row.container_type || settings?.containerType || 'micronix_tube' const collectionName = getCollectionNameForType(row, containerType) if ((containerType === 'micronix_tube' || containerType === 'cryovial_tube') && !collectionName && !row.collection_barcode) { @@ -429,10 +470,10 @@ export async function validateDerivationsCsv( if (parentSpecimen) { currentParentSpecimenTypeId = parentSpecimen.specimenTypeId - // Store first row's specimen type for comparison - if (i === 0) { + // Compare against the first row whose parent resolved + if (firstParentSpecimenTypeId === null) { firstParentSpecimenTypeId = currentParentSpecimenTypeId - } else if (firstParentSpecimenTypeId !== null && currentParentSpecimenTypeId !== firstParentSpecimenTypeId) { + } else if (currentParentSpecimenTypeId !== firstParentSpecimenTypeId) { validationRow.warnings = validationRow.warnings || [] validationRow.warnings.push('Source specimen type does not match other rows') warningCount++ @@ -471,12 +512,22 @@ export async function validateDerivationsCsv( containerType, collectionName, row.collection_barcode, + paperParentTypeFor(row), ) validationRow.collectionStatus = collectionInfo.status + if (containerType === 'paper' && collectionInfo.status !== 'existing') { + const kind = paperParentTypeFor(row) + validationRow.error = `${kind === 'bag' ? 'Bag' : 'Box'} '${collectionName ?? ''}' not found. Create it before importing paper derivations.` + validationRows.push(validationRow) + invalidCount++ + continue + } + // For tube types, validate position and check for duplicate (existing collection or within CSV) if (containerType === 'micronix_tube' || containerType === 'cryovial_tube') { - const position = (row.position ?? '').toString().trim() + // Normalize ("A2" -> "A02") so checks match what the import stores. + const position = normalizePosition(row.position) ?? '' if (!position) { validationRow.error = 'position is required for each row when deriving to a plate or box' validationRows.push(validationRow) @@ -488,25 +539,21 @@ export async function validateDerivationsCsv( // Check if position is already used in an existing collection if (collectionInfo.id !== undefined) { + // Shared check: on a plate, tubes and static wells both occupy cells. + const occupancy = await checkGridPositionOccupancy(database as Database, { + collectionKind: containerType === 'micronix_tube' ? 'micronix_plate' : 'cryovial_box', + collectionId: collectionInfo.id, + position, + }) if (containerType === 'micronix_tube') { - const existing = await database - .select({ id: micronixTube.id }) - .from(micronixTube) - .where(and(eq(micronixTube.collectionId, collectionInfo.id), eq(micronixTube.position, position))) - .get() - if (existing) { + if (occupancy.occupied) { validationRow.error = 'That position is already used in that plate. Each position in a plate can only be used once. Use a different position or a different plate.' validationRows.push(validationRow) invalidCount++ continue } } else { - const existing = await database - .select({ id: cryovialTube.id }) - .from(cryovialTube) - .where(and(eq(cryovialTube.collectionId, collectionInfo.id), eq(cryovialTube.position, position))) - .get() - if (existing) { + if (occupancy.occupied) { validationRow.error = 'That position is already used in that box. Each position in a box can only be used once. Use a different position or a different box.' validationRows.push(validationRow) invalidCount++ @@ -649,6 +696,8 @@ export async function importDerivationsFromCsv( for (let i = 0; i < rows.length; i++) { const row = rows[i] try { + const valueError = rowValueError(row) + if (valueError) throw new Error(valueError) const parentContainerId = await resolveParentContainerId(tx, row) const childContainerType = (settings?.containerType || row.container_type!) as 'micronix_tube' | 'cryovial_tube' | 'paper' const containerParsed = derivationCsvRowToContainerWrite(row, childContainerType) @@ -660,6 +709,7 @@ export async function importDerivationsFromCsv( childContainerType, getCollectionNameForType(row, childContainerType), row.collection_barcode, + paperParentTypeFor(row), ) const input: CreateDerivationInput = { @@ -670,9 +720,8 @@ export async function importDerivationsFromCsv( quantity: row.quantity ? parseNumber(row.quantity) : settings?.quantity, unitSymbol: row.unit_symbol || settings?.unitSymbol, quantityUsed: row.quantity_used ? parseNumber(row.quantity_used) : settings?.quantityUsed, - reduceParentQuantity: row.reduce_parent_quantity !== undefined - ? parseBoolean(row.reduce_parent_quantity) - : settings?.reduceParentQuantity, + // A blank cell falls back to the shared setting rather than the library default. + reduceParentQuantity: parseBoolean(row.reduce_parent_quantity) ?? settings?.reduceParentQuantity, derivationDate: settings?.derivationDate || row.derivation_date!, protocol: settings?.protocol || row.protocol!, notes: row.notes, @@ -752,6 +801,7 @@ export async function importDerivationsFromCsv( containerType, getCollectionNameForType(row, containerType), row.collection_barcode, + paperParentTypeFor(row), ) results.push({ From 8541cc42be407d55dfa09433c9365aca03e5c84d Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Fri, 25 Sep 2026 09:30:40 -0700 Subject: [PATCH 04/35] fix(api): parse quoted plate scanner CSVs. parsePlateCSV split lines on raw commas, so a scanner export with quoted fields produced headers like "Position" (with quotes) that never matched the scanner configuration, and every row was dropped. It now uses the shared contract CSV parser. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/api/src/lib/__tests__/plate-csv.test.ts | 9 +++++++++ packages/api/src/lib/plate-csv.ts | 9 +++++---- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/packages/api/src/lib/__tests__/plate-csv.test.ts b/packages/api/src/lib/__tests__/plate-csv.test.ts index 6fa8a640..cf261f9e 100644 --- a/packages/api/src/lib/__tests__/plate-csv.test.ts +++ b/packages/api/src/lib/__tests__/plate-csv.test.ts @@ -131,5 +131,14 @@ describe('plate-csv', () => { const result = parsePlateCSV(csv, singleColumnConfig) expect(result[0]).toEqual({ wellPosition: 'A01', barcode: 'MT001' }) }) + + it('reads quoted scanner exports', () => { + const csv = '"Well","Barcode"\r\n"A01","FR123"\r\n"B2","FR, 456"' + const result = parsePlateCSV(csv, singleColumnConfig) + expect(result).toEqual([ + { wellPosition: 'A01', barcode: 'FR123' }, + { wellPosition: 'B02', barcode: 'FR, 456' }, + ]) + }) }) }) diff --git a/packages/api/src/lib/plate-csv.ts b/packages/api/src/lib/plate-csv.ts index 0c094dfb..feaf90fd 100644 --- a/packages/api/src/lib/plate-csv.ts +++ b/packages/api/src/lib/plate-csv.ts @@ -1,3 +1,4 @@ +import { parseCsv } from '@sampledb/contract' import type { ScannerConfiguration } from './settings' /** Normalize well position to A01 style (row + 2-digit column) */ @@ -30,13 +31,13 @@ export function parsePlateCSV( csvText: string, config: ScannerConfiguration ): { wellPosition: string; barcode: string }[] { - const lines = csvText.split('\n').map((l) => l.trim()).filter(Boolean) + // Shared parser: handles quoted fields ("A01","FR123"), BOM, and CRLF. + const lines = parseCsv(csvText).filter((cells) => cells.some((cell) => cell.trim() !== '')) if (lines.length <= config.skipRows) return [] - const headerLine = lines[config.skipRows] - const headers = headerLine.split(',').map((h) => h.trim()) + const headers = lines[config.skipRows].map((h) => h.trim()) const rows: { wellPosition: string; barcode: string }[] = [] for (let i = config.skipRows + 1; i < lines.length; i++) { - const values = lines[i].split(',').map((v) => v.trim()) + const values = lines[i].map((v) => v.trim()) const row: Record = {} headers.forEach((h, j) => { row[h] = values[j] ?? '' From 4d01f1519bee08aeb51272d4854ef95258c30317 Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Fri, 25 Sep 2026 09:33:00 -0700 Subject: [PATCH 05/35] fix(stats): correct study, location, and date filters. - An unknown study code returned totals for the whole database. - The location filter only counted micronix and cryovial tubes; static wells and papers on sheets in boxes or bags were left out (a DBS location reported 0 of its 2,873 papers). - Location and tag filters sent their full container id lists as IN parameters inside every chunk, which could exceed SQLite's limit and made a large location take about 60 s. They are now matched with subqueries and in-memory sets (under 1 s on the production copy). - A bare created_to date (YYYY-MM-DD) compared "<=" against timestamps and left out everything created on that day, in stats, exports, and the specimen list. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/api/src/lib/export/query.ts | 3 +- .../api/src/lib/specimens/specimen-read.ts | 3 +- .../__tests__/dashboard-stats.test.ts | 24 ++ .../__tests__/location-filter.test.ts | 27 ++- .../api/src/lib/statistics/dashboard-stats.ts | 212 +++++++----------- packages/api/src/lib/statistics/helpers.ts | 15 +- .../api/src/lib/statistics/location-filter.ts | 74 +++--- 7 files changed, 179 insertions(+), 179 deletions(-) diff --git a/packages/api/src/lib/export/query.ts b/packages/api/src/lib/export/query.ts index 8b40f5e2..d6db9c93 100644 --- a/packages/api/src/lib/export/query.ts +++ b/packages/api/src/lib/export/query.ts @@ -20,6 +20,7 @@ import type { StudyRecord, } from './types' import { buildExportSummary, validateStudyCodes } from './validate' +import { dateToUpperBound } from '../statistics/helpers' export async function buildContainerQuery(database: Database, filters: ExportFilters) { // First, get the study and its subjects @@ -158,7 +159,7 @@ export async function buildContainerQuery(database: Database, filters: ExportFil } if (filters.created_to) { - containerConditions.push(lte(storageContainer.created, filters.created_to)) + containerConditions.push(dateToUpperBound(storageContainer.created, filters.created_to)) } // Get matching containers diff --git a/packages/api/src/lib/specimens/specimen-read.ts b/packages/api/src/lib/specimens/specimen-read.ts index 74f7ecb6..cced0197 100644 --- a/packages/api/src/lib/specimens/specimen-read.ts +++ b/packages/api/src/lib/specimens/specimen-read.ts @@ -3,6 +3,7 @@ import { specimen, storageContainer, studySubject, study, specimenType, controlB import { eq, and, like, or, sql } from 'drizzle-orm' import { validatePage, validateLimit } from '../constants' import { resolveContainerByBarcode } from '../identifier-resolution' +import { dateToUpperBound } from '../statistics/helpers' export type ListSpecimensQuery = { sourceType?: string @@ -150,7 +151,7 @@ export async function listSpecimens(database: Database, query: ListSpecimensQuer conditions.push(sql`${specimen.created} >= ${createdFrom}`) } if (createdTo) { - conditions.push(sql`${specimen.created} <= ${createdTo}`) + conditions.push(dateToUpperBound(specimen.created, createdTo)) } if (barcode) { diff --git a/packages/api/src/lib/statistics/__tests__/dashboard-stats.test.ts b/packages/api/src/lib/statistics/__tests__/dashboard-stats.test.ts index 200ed625..cd8fa067 100644 --- a/packages/api/src/lib/statistics/__tests__/dashboard-stats.test.ts +++ b/packages/api/src/lib/statistics/__tests__/dashboard-stats.test.ts @@ -7,7 +7,10 @@ import { createTestMicronixPlate, createTestStorageContainer, createTestStorageType, + createTestSpecimen, + createTestSpecimenType, } from '../../../__tests__/helpers/factories' +import { utcNow } from '../../datetime' import { micronixTube } from '../../../db/schema' import { getDashboardStatistics } from '../dashboard-stats' @@ -60,6 +63,27 @@ describe('dashboard-stats', () => { expect(data.storage._summary).toBeUndefined() }) + it('does not report whole-database totals for an unknown study', async () => { + const type = await createTestSpecimenType(testDb, { name: 'Blood' }) + await createTestSpecimen(testDb, type.id) + + const all = await getDashboardStatistics(testDb, sqlite as SQLiteDatabase, {}) + const unknown = await getDashboardStatistics(testDb, sqlite as SQLiteDatabase, { study: 'NONEXISTENT' }) + + expect(all.specimens.total).toBe(1) + expect(unknown.specimens.total).toBe(0) + }) + + it('includes records created on the created_to date', async () => { + const type = await createTestSpecimenType(testDb, { name: 'Blood' }) + await createTestSpecimen(testDb, type.id) + const today = utcNow().slice(0, 10) + + const data = await getDashboardStatistics(testDb, sqlite as SQLiteDatabase, { created_to: today }) + + expect(data.specimens.total).toBe(1) + }) + it('includes containers at a filtered location', async () => { const storageType = await createTestStorageType(testDb, { name: 'Stats Storage' }) const loc = await createTestLocation(testDb, { diff --git a/packages/api/src/lib/statistics/__tests__/location-filter.test.ts b/packages/api/src/lib/statistics/__tests__/location-filter.test.ts index 7001300c..037adde7 100644 --- a/packages/api/src/lib/statistics/__tests__/location-filter.test.ts +++ b/packages/api/src/lib/statistics/__tests__/location-filter.test.ts @@ -8,7 +8,8 @@ import { createTestStorageContainer, createTestStorageType, } from '../../../__tests__/helpers/factories' -import { micronixTube } from '../../../db/schema' +import { micronixTube, staticWell, box, bag, sheet, paper } from '../../../db/schema' +import { utcNow } from '../../datetime' import { resolveContainerIdsAtLocations, resolveStatisticsLocationFilter, @@ -93,4 +94,28 @@ describe('location-filter', () => { expect(ids).toEqual([container.id]) }) + + it('resolveContainerIdsAtLocations includes static wells and papers in boxes and bags', async () => { + const storageType = await createTestStorageType(testDb, { name: 'Freezer' }) + const loc = await createTestLocation(testDb, { name: 'Freezer A', storageTypeId: String(storageType.id) }) + const now = utcNow() + const plate = await createTestMicronixPlate(testDb, { name: 'P1', locationId: loc.id }) + const well = await createTestStorageContainer(testDb) + await testDb.insert(staticWell).values({ id: well.id, collectionId: plate.id, position: 'A01' }) + const [boxRow] = await testDb.insert(box).values({ name: 'Box1', locationId: loc.id, created: now, lastUpdated: now }).returning() + const [bagRow] = await testDb.insert(bag).values({ name: 'Bag1', locationId: loc.id, created: now, lastUpdated: now }).returning() + const [boxSheet] = await testDb.insert(sheet).values({ name: 'S1', boxId: boxRow.id }).returning() + const [bagSheet] = await testDb.insert(sheet).values({ name: 'S1', bagId: bagRow.id }).returning() + const reuse = { specimenId: well.specimenId, unitId: well.unitId! } + const paperInBox = await createTestStorageContainer(testDb, reuse) + const paperInBag = await createTestStorageContainer(testDb, reuse) + await testDb.insert(paper).values([ + { id: paperInBox.id, sheetId: boxSheet.id }, + { id: paperInBag.id, sheetId: bagSheet.id }, + ]) + + const ids = await resolveContainerIdsAtLocations(testDb, [loc.id]) + + expect(ids.sort((a, b) => a - b)).toEqual([well.id, paperInBox.id, paperInBag.id].sort((a, b) => a - b)) + }) }) diff --git a/packages/api/src/lib/statistics/dashboard-stats.ts b/packages/api/src/lib/statistics/dashboard-stats.ts index 4924e109..9f087135 100644 --- a/packages/api/src/lib/statistics/dashboard-stats.ts +++ b/packages/api/src/lib/statistics/dashboard-stats.ts @@ -22,6 +22,30 @@ import { computeStorageStatistics } from './storage-aggregates' import type { DashboardStatistics, StatisticsFilters } from './types' /** Filtered dashboard statistics for specimens, containers, and storage. */ +function emptyDashboardStatistics(): DashboardStatistics { + return { + specimens: { + total: 0, + bySourceType: {}, + bySpecimenType: {}, + byStudy: {}, + collectionTimeline: [], + creationTimeline: [], + }, + containers: { + total: 0, + byType: {}, + byTags: {}, + byStatus: {}, + averagePerSpecimen: 0, + }, + storage: { + byLocation: [], + byRootLocation: {}, + }, + } +} + export async function getDashboardStatistics( database: Database, sqliteDatabase: SQLiteDatabase, @@ -52,46 +76,28 @@ export async function getDashboardStatistics( .where(eq(study.shortCode, studyCode)) .get() - if (studyRecord) { - const subjects = await database - .select({ id: studySubject.id }) - .from(studySubject) - .where(eq(studySubject.studyId, studyRecord.id)) - subjectIds = subjects.map(s => s.id) - if (subjectIds.length > 0) { - if (subjectIds.length === 1) { - specimenConditions.push( - eq(specimen.studySubjectId, subjectIds[0]) - ) - } else { - specimenConditions.push( - inArray(specimen.studySubjectId, subjectIds) - ) - } + // An unknown study matches nothing; it must not fall through to whole-database totals. + if (!studyRecord) { + return emptyDashboardStatistics() + } + const subjects = await database + .select({ id: studySubject.id }) + .from(studySubject) + .where(eq(studySubject.studyId, studyRecord.id)) + subjectIds = subjects.map(s => s.id) + if (subjectIds.length > 0) { + if (subjectIds.length === 1) { + specimenConditions.push( + eq(specimen.studySubjectId, subjectIds[0]) + ) } else { - // No subjects in study, return empty results - return { - specimens: { - total: 0, - bySourceType: {}, - bySpecimenType: {}, - byStudy: {}, - collectionTimeline: [], - creationTimeline: [], - }, - containers: { - total: 0, - byType: {}, - byTags: {}, - byStatus: {}, - averagePerSpecimen: 0, - }, - storage: { - byLocation: [], - byRootLocation: {}, - }, - } + specimenConditions.push( + inArray(specimen.studySubjectId, subjectIds) + ) } + } else { + // No subjects in study, return empty results + return emptyDashboardStatistics() } } @@ -134,27 +140,7 @@ export async function getDashboardStatistics( let filteredLocationIds: number[] = [] if (locationFilter.kind === 'not_found') { - return { - specimens: { - total: 0, - bySourceType: {}, - bySpecimenType: {}, - byStudy: {}, - collectionTimeline: [], - creationTimeline: [], - }, - containers: { - total: 0, - byType: {}, - byTags: {}, - byStatus: {}, - averagePerSpecimen: 0, - }, - storage: { - byLocation: [], - byRootLocation: {}, - }, - } + return emptyDashboardStatistics() } if (locationFilter.kind === 'resolved') { @@ -191,27 +177,7 @@ export async function getDashboardStatistics( if (tagFilteredContainerIds.length === 0) { // No containers match all the tags, return empty results - return { - specimens: { - total: 0, - bySourceType: {}, - bySpecimenType: {}, - byStudy: {}, - collectionTimeline: [], - creationTimeline: [], - }, - containers: { - total: 0, - byType: {}, - byTags: {}, - byStatus: {}, - averagePerSpecimen: 0, - }, - storage: { - byLocation: [], - byRootLocation: {}, - }, - } + return emptyDashboardStatistics() } } @@ -230,28 +196,26 @@ export async function getDashboardStatistics( ) } + const locationFilteredIdSet = locationFilteredContainerIds ? new Set(locationFilteredContainerIds) : null + const tagFilteredIdSet = tagFilteredContainerIds ? new Set(tagFilteredContainerIds) : null + // Batch query if too many specimen IDs to avoid SQLite variable limit // Use Promise.all to parallelize queries for better performance const specimenChunks = chunkArray(specimenIds, 500) const chunkResults = await Promise.all( specimenChunks.map(async (specimenChunk, chunkIndex) => { - let containerQuery = database.select().from(storageContainer) - const containerConditions = [inArray(storageContainer.specimenId, specimenChunk)] - - // Apply location filter if provided - if (locationFilteredContainerIds) { - containerConditions.push(inArray(storageContainer.id, locationFilteredContainerIds)) - } - - // Apply tag filter if provided - if (tagFilteredContainerIds) { - containerConditions.push(inArray(storageContainer.id, tagFilteredContainerIds)) - } - - containerQuery = containerQuery.where(and(...containerConditions) as any) as any - const containers = await containerQuery - return containers + const containers = await database + .select() + .from(storageContainer) + .where(inArray(storageContainer.specimenId, specimenChunk)) + // Location and tag filters are applied in memory: as IN lists they can exceed + // SQLite's parameter limit on large locations. + return containers.filter( + (c) => + (!locationFilteredIdSet || locationFilteredIdSet.has(c.id)) && + (!tagFilteredIdSet || tagFilteredIdSet.has(c.id)), + ) }) ) @@ -269,27 +233,7 @@ export async function getDashboardStatistics( }) } else if (filteredSpecimens.length === 0) { // No specimens match, so no containers - return { - specimens: { - total: 0, - bySourceType: {}, - bySpecimenType: {}, - byStudy: {}, - collectionTimeline: [], - creationTimeline: [], - }, - containers: { - total: 0, - byType: {}, - byTags: {}, - byStatus: {}, - averagePerSpecimen: 0, - }, - storage: { - byLocation: [], - byRootLocation: {}, - }, - } + return emptyDashboardStatistics() } else { // No specimen filter, but we might have state/status/location filters if (hasLocationFilter && filteredLocationIds.length > 0) { @@ -307,34 +251,28 @@ export async function getDashboardStatistics( const containerIdChunks = chunkArray(locationFilteredContainerIds, 500) const chunkResults = await Promise.all( containerIdChunks.map(async (containerIdChunk) => { - let containerQuery = database.select().from(storageContainer) - const containerConditions = [inArray(storageContainer.id, containerIdChunk)] - - // Apply tag filter if provided - if (tagFilteredContainerIds) { - containerConditions.push(inArray(storageContainer.id, tagFilteredContainerIds)) - } - - containerQuery = containerQuery.where(and(...containerConditions) as any) as any - return await containerQuery + // Tag filter is already intersected into locationFilteredContainerIds. + return await database + .select() + .from(storageContainer) + .where(inArray(storageContainer.id, containerIdChunk)) }) ) filteredContainers = chunkResults.flat() } } else { // No location filter - let containerQuery = database.select().from(storageContainer) - const containerConditions = [] - - // Apply tag filter if provided if (tagFilteredContainerIds) { - containerConditions.push(inArray(storageContainer.id, tagFilteredContainerIds)) - } - - if (containerConditions.length > 0) { - containerQuery = containerQuery.where(and(...containerConditions) as any) as any + const tagIdChunks = chunkArray(tagFilteredContainerIds, 500) + const chunkResults = await Promise.all( + tagIdChunks.map((idChunk) => + database.select().from(storageContainer).where(inArray(storageContainer.id, idChunk)), + ), + ) + filteredContainers = chunkResults.flat() + } else { + filteredContainers = await database.select().from(storageContainer) } - filteredContainers = await containerQuery } } diff --git a/packages/api/src/lib/statistics/helpers.ts b/packages/api/src/lib/statistics/helpers.ts index 7f814bdf..68574ecd 100644 --- a/packages/api/src/lib/statistics/helpers.ts +++ b/packages/api/src/lib/statistics/helpers.ts @@ -1,4 +1,4 @@ -import { gte, lte } from 'drizzle-orm' +import { gte, lt, lte } from 'drizzle-orm' import type { SQLiteColumn } from 'drizzle-orm/sqlite-core' /** Build date filter conditions for specimen date columns. */ @@ -8,11 +8,22 @@ export function buildDateFilter(column: SQLiteColumn, dateFrom?: string, dateTo? conditions.push(gte(column, dateFrom)) } if (dateTo) { - conditions.push(lte(column, dateTo)) + conditions.push(dateToUpperBound(column, dateTo)) } return conditions } +/** + * Upper bound for a "to" date filter. A bare YYYY-MM-DD includes that whole day, so a + * timestamp column ('2026-05-26 22:28:26') is compared with "< next day", not "<= date". + */ +export function dateToUpperBound(column: SQLiteColumn, dateTo: string) { + const match = /^(\d{4})-(\d{2})-(\d{2})$/.exec(dateTo) + if (!match) return lte(column, dateTo) + const next = new Date(Date.UTC(Number(match[1]), Number(match[2]) - 1, Number(match[3]) + 1)) + return lt(column, next.toISOString().slice(0, 10)) +} + /** Batch an array into chunks for SQLite variable limits. */ export function chunkArray(array: T[], chunkSize: number): T[][] { const chunks: T[][] = [] diff --git a/packages/api/src/lib/statistics/location-filter.ts b/packages/api/src/lib/statistics/location-filter.ts index c80e8ca3..9a83f43d 100644 --- a/packages/api/src/lib/statistics/location-filter.ts +++ b/packages/api/src/lib/statistics/location-filter.ts @@ -4,10 +4,15 @@ import { location, micronixPlate, micronixTube, + staticWell, cryovialBox, cryovialTube, + box, + bag, + sheet, + paper, } from '../../db/schema' -import { eq, inArray } from 'drizzle-orm' +import { eq, inArray, or } from 'drizzle-orm' export type StatisticsLocationFilterResult = | { kind: 'none' } @@ -43,50 +48,45 @@ export async function resolveStatisticsLocationFilter( } } -/** Resolve storage container ids at micronix plates / cryovial boxes in the given locations. */ +/** + * Resolve storage container ids stored in the given locations: tubes and static wells on + * micronix plates, tubes in cryovial boxes, and papers on sheets in boxes or bags. + * Collections are matched with subqueries so large locations never build huge IN lists. + */ export async function resolveContainerIdsAtLocations( database: Database, filteredLocationIds: number[], tagFilteredContainerIds?: number[] | null, ): Promise { - const [matchingPlates, matchingBoxes] = await Promise.all([ - database - .select({ id: micronixPlate.id }) - .from(micronixPlate) - .where(inArray(micronixPlate.locationId, filteredLocationIds)), - database - .select({ id: cryovialBox.id }) - .from(cryovialBox) - .where(inArray(cryovialBox.locationId, filteredLocationIds)), - ]) - - const plateIds = matchingPlates.map((p) => p.id) - const boxIds = matchingBoxes.map((b) => b.id) + const platesHere = database + .select({ id: micronixPlate.id }) + .from(micronixPlate) + .where(inArray(micronixPlate.locationId, filteredLocationIds)) + const cryovialBoxesHere = database + .select({ id: cryovialBox.id }) + .from(cryovialBox) + .where(inArray(cryovialBox.locationId, filteredLocationIds)) + const sheetsHere = database + .select({ id: sheet.id }) + .from(sheet) + .where( + or( + inArray(sheet.boxId, database.select({ id: box.id }).from(box).where(inArray(box.locationId, filteredLocationIds))), + inArray(sheet.bagId, database.select({ id: bag.id }).from(bag).where(inArray(bag.locationId, filteredLocationIds))), + ), + ) - const [micronixContainerIds, cryovialContainerIds] = await Promise.all([ - plateIds.length > 0 - ? database - .select({ id: micronixTube.id }) - .from(micronixTube) - .where(inArray(micronixTube.collectionId, plateIds)) - : Promise.resolve([]), - boxIds.length > 0 - ? database - .select({ id: cryovialTube.id }) - .from(cryovialTube) - .where(inArray(cryovialTube.collectionId, boxIds)) - : Promise.resolve([]), + const [micronix, wells, cryovial, papers] = await Promise.all([ + database.select({ id: micronixTube.id }).from(micronixTube).where(inArray(micronixTube.collectionId, platesHere)), + database.select({ id: staticWell.id }).from(staticWell).where(inArray(staticWell.collectionId, platesHere)), + database.select({ id: cryovialTube.id }).from(cryovialTube).where(inArray(cryovialTube.collectionId, cryovialBoxesHere)), + database.select({ id: paper.id }).from(paper).where(inArray(paper.sheetId, sheetsHere)), ]) - let locationFilteredContainerIds = [ - ...new Set([...micronixContainerIds.map((r) => r.id), ...cryovialContainerIds.map((r) => r.id)]), - ] - + const ids = new Set([...micronix, ...wells, ...cryovial, ...papers].map((r) => r.id)) if (tagFilteredContainerIds) { - locationFilteredContainerIds = locationFilteredContainerIds.filter((id) => - tagFilteredContainerIds.includes(id), - ) + const tagged = new Set(tagFilteredContainerIds) + return [...ids].filter((id) => tagged.has(id)) } - - return locationFilteredContainerIds + return [...ids] } From e01440810261401e7f49c8cd8dd505d549c48f65 Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Fri, 25 Sep 2026 09:35:31 -0700 Subject: [PATCH 06/35] fix(export): keep every listed visit and undated subjects. Multi-study and single-study exports stored one date filter per subject, so a manifest listing a subject for two visits exported only the last one. Once any subject in a study had a date, subjects listed without a date matched nothing. Each subject now keeps a list of date filters, and subjects listed without a date match all of their dates (still honoring the global date range). Exact-date tolerance is computed in UTC, and the single-study container query chunks specimen ids to stay under SQLite's parameter limit. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../src/lib/export/__tests__/query.test.ts | 60 +++++++- packages/api/src/lib/export/query.ts | 129 ++++++++++-------- packages/contract/src/export.ts | 10 +- packages/contract/src/index.ts | 1 + .../useSingleStudyExportWorkflow.test.tsx | 15 +- .../src/hooks/useSingleStudyExportWorkflow.ts | 27 ++-- packages/web/src/lib/api/export.ts | 8 +- 7 files changed, 174 insertions(+), 76 deletions(-) diff --git a/packages/api/src/lib/export/__tests__/query.test.ts b/packages/api/src/lib/export/__tests__/query.test.ts index 680bb5a2..45425920 100644 --- a/packages/api/src/lib/export/__tests__/query.test.ts +++ b/packages/api/src/lib/export/__tests__/query.test.ts @@ -10,8 +10,14 @@ import { createTestUnit, } from '../../../__tests__/helpers/factories' import type { Database } from '../../../db/client' -import { storageContainerTag } from '../../../db/schema' -import { buildContainerQuery, resolveMicronixBarcodesToContainers, buildContainerQueryByMicronixBarcodes } from '../query' +import { storageContainerTag, specimen as specimenTable } from '../../../db/schema' +import { utcNow } from '../../datetime' +import { + buildContainerQuery, + buildMultiStudyContainerQuery, + resolveMicronixBarcodesToContainers, + buildContainerQueryByMicronixBarcodes, +} from '../query' describe('buildContainerQuery', () => { let testDb: Database @@ -170,3 +176,53 @@ describe('buildContainerQueryByMicronixBarcodes', () => { }) }) + +describe('buildMultiStudyContainerQuery subject dates', () => { + let testDb: Database + let sqlite: Awaited>['sqlite'] + + beforeEach(async () => { + const setup = await setupTestDatabase() + testDb = setup.db + sqlite = setup.sqlite + }) + + afterEach(() => { + if (sqlite) cleanupTestDatabase(sqlite) + }) + + it('keeps every listed visit per subject and includes subjects listed without a date', async () => { + const study = await createTestStudy(testDb, { title: 'Visits', shortCode: 'VIS', leadPerson: 'L' }) + const s01 = await createTestStudySubject(testDb, { studyId: study.id, name: 'S01' }) + const s02 = await createTestStudySubject(testDb, { studyId: study.id, name: 'S02' }) + const type = await createTestSpecimenType(testDb, { name: 'Blood' }) + const unit = await createTestUnit(testDb, { symbol: 'uL', name: 'microliter', category: 'volume' }) + const now = utcNow() + const addSpecimen = async (subjectId: number, collectionDate: string) => { + const [row] = await testDb + .insert(specimenTable) + .values({ studySubjectId: subjectId, specimenTypeId: type.id, collectionDate, created: now, lastUpdated: now }) + .returning() + await createTestStorageContainer(testDb, { specimenId: row.id, unitId: unit.id }) + return row + } + const jan = await addSpecimen(s01.id, '2024-01-01') + const feb = await addSpecimen(s01.id, '2024-02-01') + await addSpecimen(s01.id, '2024-03-01') + const may = await addSpecimen(s02.id, '2024-05-05') + + const result = await buildMultiStudyContainerQuery( + testDb, + [ + { study_short_code: 'VIS', subject_name: 'S01', collection_date: '2024-01-01' }, + { study_short_code: 'VIS', subject_name: 'S01', collection_date: '2024-02-01' }, + { study_short_code: 'VIS', subject_name: 'S02' }, + ], + {}, + ) + + expect(result.containers.map((c) => c.specimen_id).sort((a, b) => a - b)).toEqual( + [jan.id, feb.id, may.id].sort((a, b) => a - b), + ) + }) +}) diff --git a/packages/api/src/lib/export/query.ts b/packages/api/src/lib/export/query.ts index d6db9c93..4864b295 100644 --- a/packages/api/src/lib/export/query.ts +++ b/packages/api/src/lib/export/query.ts @@ -11,6 +11,7 @@ import { resolveSubjectsByStudyGrouped } from '../identifier-resolution' import { resolveContainerIdsWithAllTags } from '../container-tag-filter' import { enrichContainerData } from './enrich' import { filterContainerIdsByType } from './filter' +import type { SubjectDateFilter } from '@sampledb/contract' import type { ContainerExportData, ExportFilters, @@ -20,7 +21,7 @@ import type { StudyRecord, } from './types' import { buildExportSummary, validateStudyCodes } from './validate' -import { dateToUpperBound } from '../statistics/helpers' +import { chunkArray, dateToUpperBound } from '../statistics/helpers' export async function buildContainerQuery(database: Database, filters: ExportFilters) { // First, get the study and its subjects @@ -77,46 +78,35 @@ export async function buildContainerQuery(database: Database, filters: ExportFil // Handle per-subject date filtering if (filters.subject_dates && Object.keys(filters.subject_dates).length > 0) { const tolerance = filters.date_tolerance || 0 + const subjectDates = filters.subject_dates const subjectDateConditions: any[] = [] - - // Build conditions for each subject with date filters - for (const [subjectIdStr, dateFilter] of Object.entries(filters.subject_dates)) { - const subjectId = parseInt(subjectIdStr) - if (isNaN(subjectId) || !filteredSubjectIds.includes(subjectId)) continue - - const subjectConditions: any[] = [eq(specimen.studySubjectId, subjectId)] - - if ('exact' in dateFilter) { - // Convert exact date with tolerance to range - const exactDate = dateFilter.exact - const fromDate = new Date(exactDate) - fromDate.setDate(fromDate.getDate() - tolerance) - const toDate = new Date(exactDate) - toDate.setDate(toDate.getDate() + tolerance) - - subjectConditions.push( - and( - gte(specimen.collectionDate, fromDate.toISOString().split('T')[0]), - lte(specimen.collectionDate, toDate.toISOString().split('T')[0]) - ) as any - ) - } else if ('from' in dateFilter || 'to' in dateFilter) { - // Date range - if (dateFilter.from) { - subjectConditions.push(gte(specimen.collectionDate, dateFilter.from)) - } - if (dateFilter.to) { - subjectConditions.push(lte(specimen.collectionDate, dateFilter.to)) - } + const undatedSubjectIds: number[] = [] + + for (const subjectId of filteredSubjectIds) { + const entry = subjectDates[subjectId] as SubjectDateFilter | SubjectDateFilter[] | undefined + const dateConditions = (entry == null ? [] : Array.isArray(entry) ? entry : [entry]) + .map((dateFilter) => subjectDateCondition(dateFilter, tolerance)) + .filter((condition) => condition != null) + if (dateConditions.length === 0) { + // Listed without a date: every collection date for this subject. + undatedSubjectIds.push(subjectId) + continue } - - subjectDateConditions.push(and(...subjectConditions) as any) + // Several entries for one subject (one per visit) are alternatives. + subjectDateConditions.push(and(eq(specimen.studySubjectId, subjectId), or(...dateConditions))) } - - // If we have per-subject date conditions, use them (OR for different subjects) - if (subjectDateConditions.length > 0) { - specimenConditions.push(or(...subjectDateConditions) as any) + + if (undatedSubjectIds.length > 0) { + subjectDateConditions.push( + and( + inArray(specimen.studySubjectId, undatedSubjectIds), + ...(filters.date_from ? [gte(specimen.collectionDate, filters.date_from)] : []), + ...(filters.date_to ? [lte(specimen.collectionDate, filters.date_to)] : []), + ), + ) } + + specimenConditions.push(or(...subjectDateConditions) as any) } else { // Use global date filters if no per-subject dates if (filters.date_from) { @@ -152,7 +142,6 @@ export async function buildContainerQuery(database: Database, filters: ExportFil // Build conditions for container query const containerConditions: any[] = [] - containerConditions.push(inArray(storageContainer.specimenId, specimenIds)) if (filters.created_from) { containerConditions.push(gte(storageContainer.created, filters.created_from)) @@ -162,11 +151,16 @@ export async function buildContainerQuery(database: Database, filters: ExportFil containerConditions.push(dateToUpperBound(storageContainer.created, filters.created_to)) } - // Get matching containers - let containers = await database - .select() - .from(storageContainer) - .where(and(...containerConditions) as any) + // Get matching containers, chunking specimen ids to stay under SQLite's parameter limit + let containers: Array = [] + for (const idChunk of chunkArray(specimenIds, 5000)) { + containers.push( + ...(await database + .select() + .from(storageContainer) + .where(and(inArray(storageContainer.specimenId, idChunk), ...containerConditions) as any)), + ) + } if (filters.tag_ids && filters.tag_ids.length > 0) { const matchingIds = new Set(await resolveContainerIdsWithAllTags(database, filters.tag_ids)) @@ -283,6 +277,25 @@ export async function buildContainerQueryByMicronixBarcodes( return { containers, specimens, studies, subjectToStudyMap } } +/** Collection date condition for one per-subject filter (exact date +/- tolerance days, or a range). */ +function subjectDateCondition(dateFilter: SubjectDateFilter, toleranceDays: number) { + if ('exact' in dateFilter) { + const match = /^(\d{4})-(\d{2})-(\d{2})/.exec(dateFilter.exact) + if (!match) return eq(specimen.collectionDate, dateFilter.exact) + const shift = (days: number) => + new Date(Date.UTC(Number(match[1]), Number(match[2]) - 1, Number(match[3]) + days)).toISOString().slice(0, 10) + return and( + gte(specimen.collectionDate, shift(-toleranceDays)), + lte(specimen.collectionDate, shift(toleranceDays)), + ) + } + const bounds = [ + ...(dateFilter.from ? [gte(specimen.collectionDate, dateFilter.from)] : []), + ...(dateFilter.to ? [lte(specimen.collectionDate, dateFilter.to)] : []), + ] + return bounds.length > 0 ? and(...bounds) : null +} + export async function buildMultiStudyContainerQuery( database: Database, entries: MultiStudyExportEntry[], @@ -330,20 +343,22 @@ export async function buildMultiStudyContainerQuery( const subjectsByStudy = await resolveSubjectsByStudyGrouped(database, subjectResolutionEntries) // Build subject dates map (by study and subject name) - const subjectDatesByStudy = new Map>() + // A subject may appear on several rows (one per visit); keep every date. A row without + // a date means all of that subject's dates, so it wins over dated rows. + const subjectDatesByStudy = new Map>() for (const [studyId, studyEntries] of entriesByStudy.entries()) { - const datesMap = new Map() + const datesMap = new Map() for (const entry of studyEntries) { - if (entry.collection_date) { - datesMap.set(entry.subject_name, { exact: entry.collection_date }) - } else if (entry.date_from || entry.date_to) { - datesMap.set(entry.subject_name, { - from: entry.date_from, - to: entry.date_to, - }) - } + const existing = datesMap.get(entry.subject_name) + if (existing === 'all') continue + const dateFilter: SubjectDateFilter | null = entry.collection_date + ? { exact: entry.collection_date } + : entry.date_from || entry.date_to + ? { from: entry.date_from, to: entry.date_to } + : null + datesMap.set(entry.subject_name, dateFilter ? [...(existing ?? []), dateFilter] : 'all') } - if (datesMap.size > 0) { + if ([...datesMap.values()].some((v) => v !== 'all')) { subjectDatesByStudy.set(studyId, datesMap) } } @@ -384,10 +399,10 @@ export async function buildMultiStudyContainerQuery( const studySubjectDates = subjectDatesByStudy.get(studyId) if (studySubjectDates && studySubjectDates.size > 0) { studyFilters.subject_dates = {} - for (const [subjectName, dateFilter] of studySubjectDates.entries()) { + for (const [subjectName, dateFilters] of studySubjectDates.entries()) { const subjectId = subjectMap.get(subjectName) - if (subjectId) { - studyFilters.subject_dates[subjectId] = dateFilter + if (subjectId && dateFilters !== 'all') { + studyFilters.subject_dates[subjectId] = dateFilters } } } diff --git a/packages/contract/src/export.ts b/packages/contract/src/export.ts index fb9d251b..6eab6c42 100644 --- a/packages/contract/src/export.ts +++ b/packages/contract/src/export.ts @@ -16,13 +16,17 @@ export interface ExportFilters { subject_ids?: number[] /** Global tolerance for exact subject collection dates (defaults to 0). */ date_tolerance?: number + /** + * Per-subject collection date filters. A subject may list several (one per visit); + * listed subjects without an entry match all their dates. + */ subject_dates?: { - [subjectId: number]: - | { exact: string } - | { from?: string; to?: string } + [subjectId: number]: SubjectDateFilter | SubjectDateFilter[] } } +export type SubjectDateFilter = { exact: string } | { from?: string; to?: string } + export interface ContainerExportData { container_id: number container_type: string diff --git a/packages/contract/src/index.ts b/packages/contract/src/index.ts index e50a1ec7..a25ff946 100644 --- a/packages/contract/src/index.ts +++ b/packages/contract/src/index.ts @@ -9,6 +9,7 @@ export { type ContainerExportData, type ExportFilters, + type SubjectDateFilter, } from './export' export { DEFAULT_EXPORT_COLUMN_KEYS, diff --git a/packages/web/src/hooks/__tests__/useSingleStudyExportWorkflow.test.tsx b/packages/web/src/hooks/__tests__/useSingleStudyExportWorkflow.test.tsx index ae05f955..dc95fa12 100644 --- a/packages/web/src/hooks/__tests__/useSingleStudyExportWorkflow.test.tsx +++ b/packages/web/src/hooks/__tests__/useSingleStudyExportWorkflow.test.tsx @@ -1,7 +1,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest' import { renderHook, waitFor, act } from '@testing-library/react' import { exportApi } from '../../lib/api/export' -import { useSingleStudyExportWorkflow } from '../useSingleStudyExportWorkflow' +import { buildSubjectDatesFromCsv, useSingleStudyExportWorkflow } from '../useSingleStudyExportWorkflow' vi.mock('../../lib/export-filter-csv', () => ({ parseExportModalCsv: vi.fn(), @@ -148,3 +148,16 @@ describe('useSingleStudyExportWorkflow', () => { expect(exportApi.downloadEnvelope).toHaveBeenCalled() }) }) + +describe('buildSubjectDatesFromCsv', () => { + it('keeps every visit per subject and drops subjects that have an undated row', () => { + expect( + buildSubjectDatesFromCsv([ + { subject_name: 'S01', collection_date: '2024-01-01' }, + { subject_name: 'S01', collection_date: '2024-02-01' }, + { subject_name: 'S02', collection_date: '2024-03-01' }, + { subject_name: 'S02' }, + ]), + ).toEqual({ S01: [{ exact: '2024-01-01' }, { exact: '2024-02-01' }] }) + }) +}) diff --git a/packages/web/src/hooks/useSingleStudyExportWorkflow.ts b/packages/web/src/hooks/useSingleStudyExportWorkflow.ts index bddc0bda..9b655e37 100644 --- a/packages/web/src/hooks/useSingleStudyExportWorkflow.ts +++ b/packages/web/src/hooks/useSingleStudyExportWorkflow.ts @@ -1,5 +1,6 @@ import { useCallback, useEffect, useRef, useState } from 'react' import { exportApi, type ExportFilters } from '../lib/api/export' +import type { SubjectDateFilter } from '@sampledb/contract' import { parseExportModalCsv, type SingleStudyExportCsvRow } from '../lib/export-filter-csv' import { formatLocalDateTime } from '../lib/date-utils' import { toggleArrayFilterValue } from '../lib/filter-array-toggle' @@ -16,18 +17,26 @@ export type SingleStudyExportSubmitParams = { columns: string[] } -function buildSubjectDatesFromCsv(csvData: SingleStudyExportCsvRow[]) { - const subjectDates: { [key: string]: { exact?: string; from?: string; to?: string } } = {} +/** + * Per-subject date filters from the uploaded CSV. A subject on several rows keeps every + * date (one per visit); a row without a date means all of that subject's dates. + */ +export function buildSubjectDatesFromCsv(csvData: SingleStudyExportCsvRow[]) { + const subjectDates: { [key: string]: SubjectDateFilter[] } = {} + const allDates = new Set() for (const row of csvData) { - if (row.collection_date) { - subjectDates[row.subject_name] = { exact: row.collection_date } - } else if (row.date_from || row.date_to) { - subjectDates[row.subject_name] = { - from: row.date_from, - to: row.date_to, - } + const dateFilter: SubjectDateFilter | null = row.collection_date + ? { exact: row.collection_date } + : row.date_from || row.date_to + ? { from: row.date_from, to: row.date_to } + : null + if (!dateFilter) { + allDates.add(row.subject_name) + continue } + ;(subjectDates[row.subject_name] ??= []).push(dateFilter) } + for (const name of allDates) delete subjectDates[name] return Object.keys(subjectDates).length > 0 ? subjectDates : undefined } diff --git a/packages/web/src/lib/api/export.ts b/packages/web/src/lib/api/export.ts index 971ef90b..84b6ad5e 100644 --- a/packages/web/src/lib/api/export.ts +++ b/packages/web/src/lib/api/export.ts @@ -1,4 +1,4 @@ -import type { ContainerExportData, CSVExportOptions, ExportFilters } from '@sampledb/contract' +import type { ContainerExportData, CSVExportOptions, ExportFilters, SubjectDateFilter } from '@sampledb/contract' import { api } from './client' import { downloadExportFile, @@ -107,7 +107,7 @@ export const exportApi = { containersByNames: (params: { study: string subject_names: string[] - subject_dates?: { [subjectName: string]: { exact?: string; from?: string; to?: string } } + subject_dates?: { [subjectName: string]: SubjectDateFilter | SubjectDateFilter[] } date_tolerance?: number format?: 'csv' | 'xlsx' | 'json' columns?: string[] @@ -138,7 +138,7 @@ export const exportApi = { containersCountByNames: (params: { study: string subject_names: string[] - subject_dates?: { [subjectName: string]: { exact?: string; from?: string; to?: string } } + subject_dates?: { [subjectName: string]: SubjectDateFilter | SubjectDateFilter[] } date_tolerance?: number specimen_type_ids?: number[] container_types?: string[] @@ -176,7 +176,7 @@ export const exportApi = { date_from?: string date_to?: string }> - subject_dates?: { [subjectName: string]: { exact?: string; from?: string; to?: string } } + subject_dates?: { [subjectName: string]: SubjectDateFilter | SubjectDateFilter[] } date_tolerance?: number format?: 'csv' | 'xlsx' | 'json' columns?: string[] From c00dd985f5316ca70ec6555ac7e6f9463fea1e00 Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Fri, 25 Sep 2026 09:37:43 -0700 Subject: [PATCH 07/35] fix(api): guard qPCR targets, control definition edits, and activity limit. - Replacing qPCR targets deleted the old ones before inserting the new ones outside a transaction; two targets with the same name (two blank names both become varATS) hit the unique index and left the experiment with no targets. Duplicates are now rejected up front and the replace runs in one transaction. - Control definition PATCH accepted controlType, which could move a definition out of every blood route, and skipped the name and composition uniqueness checks that create applies. - /activity/recent took any limit, including -1 (no limit in SQLite), with several queries per row. It is now clamped to 1-50. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../api/src/routes/__tests__/controls.test.ts | 32 +++++++++++++++++ .../routes/__tests__/qpcr-experiments.test.ts | 21 +++++++++++ packages/api/src/routes/activity.ts | 8 ++++- packages/api/src/routes/controls.ts | 33 ++++++++++++++++- packages/api/src/routes/qpcr-experiments.ts | 36 +++++++++++++------ 5 files changed, 117 insertions(+), 13 deletions(-) diff --git a/packages/api/src/routes/__tests__/controls.test.ts b/packages/api/src/routes/__tests__/controls.test.ts index 46cbaca6..38c8d3cb 100644 --- a/packages/api/src/routes/__tests__/controls.test.ts +++ b/packages/api/src/routes/__tests__/controls.test.ts @@ -387,6 +387,38 @@ describe('Controls API', () => { }) }) + describe(`PATCH ${BASE}/:id`, () => { + it('ignores controlType so a definition cannot leave the blood routes', async () => { + const def = await createTestControlDefinition(ctx.db, { name: 'Blood A', controlType: 'blood' }) + + const res = await ctx.request(`${BASE}/${def.id}`, { method: 'PATCH', json: { controlType: 'antibody' } }) + + expect(res.status).toBe(200) + const data = (await res.json()) as { control: { controlType: string } } + expect(data.control.controlType).toBe('blood') + }) + + it('returns 409 when renaming to an existing definition name', async () => { + await createTestControlDefinition(ctx.db, { name: 'Taken' }) + const def = await createTestControlDefinition(ctx.db, { name: 'Mine' }) + + const res = await ctx.request(`${BASE}/${def.id}`, { method: 'PATCH', json: { name: 'Taken' } }) + + expect(res.status).toBe(409) + }) + + it('returns 409 when changing density to match another definition', async () => { + const strain = await createTestStrain(ctx.db, { name: '3D7' }) + const strains = [{ id: strain.id, name: '3D7', percentage: 100 }] + await createTestControlDefinition(ctx.db, { name: 'D100', properties: { strains, targetDensity: 100 } }) + const def = await createTestControlDefinition(ctx.db, { name: 'D200', properties: { strains, targetDensity: 200 } }) + + const res = await ctx.request(`${BASE}/${def.id}`, { method: 'PATCH', json: { targetDensity: 100 } }) + + expect(res.status).toBe(409) + }) + }) + describe(`PATCH ${BASE}/batches/:id`, () => { it('returns 200 and updates batch name', async () => { const strain = await createTestStrain(ctx.db, { name: 'Strain Patch' }) diff --git a/packages/api/src/routes/__tests__/qpcr-experiments.test.ts b/packages/api/src/routes/__tests__/qpcr-experiments.test.ts index 294fb448..485de105 100644 --- a/packages/api/src/routes/__tests__/qpcr-experiments.test.ts +++ b/packages/api/src/routes/__tests__/qpcr-experiments.test.ts @@ -47,6 +47,27 @@ describe('qPCR Experiments Template', () => { }) }) + describe('PATCH / targets', () => { + it('rejects duplicate target names and keeps the existing targets', async () => { + const createRes = await ctx.request('/api/qpcr-experiments', { + method: 'POST', + json: { name: 'Dup targets', templateFormat: 'biorad' }, + }) + const created = (await createRes.json()) as { id: number } + + const res = await ctx.request(`/api/qpcr-experiments/${created.id}`, { + method: 'PATCH', + json: { targets: [{ targetName: '' }, { targetName: 'varATS' }] }, + }) + + expect(res.status).toBe(400) + const detail = (await (await ctx.request(`/api/qpcr-experiments/${created.id}`)).json()) as { + experiment: { targets: Array<{ targetName: string }> } + } + expect(detail.experiment.targets.map((t) => t.targetName)).toEqual(['varATS']) + }) + }) + describe('GET /:id/template', () => { it('includes only rows for wells that have a tube; skips positions with no tube', async () => { const [exp] = await ctx.db diff --git a/packages/api/src/routes/activity.ts b/packages/api/src/routes/activity.ts index 8258ec19..90954168 100644 --- a/packages/api/src/routes/activity.ts +++ b/packages/api/src/routes/activity.ts @@ -21,6 +21,9 @@ import { handleRouteError } from '../lib/error-handler' * Create activity routes with database injection * @param database - Database instance (required) */ +/** Upper bound for GET /activity/recent?limit=. */ +const MAX_RECENT_ACTIVITY = 50 + export function createActivityRoutes(database: Database): Hono { const activity = new Hono() const authMiddleware = createAuthMiddleware(database) @@ -28,7 +31,10 @@ export function createActivityRoutes(database: Database): Hono { // Get recent activity across all entity types activity.get('/recent', authMiddleware, async (c) => { try { - const limit = parseInt(c.req.query('limit') || '10') + // Each row is enriched with several queries, so keep the feed short (and never unbounded: + // SQLite treats LIMIT -1 as no limit). + const requested = parseInt(c.req.query('limit') || '10') + const limit = Number.isFinite(requested) ? Math.min(Math.max(requested, 1), MAX_RECENT_ACTIVITY) : 10 // Get recent specimens with enriched data const recentSpecimens = await database diff --git a/packages/api/src/routes/controls.ts b/packages/api/src/routes/controls.ts index 0cd43021..f8a88bc3 100644 --- a/packages/api/src/routes/controls.ts +++ b/packages/api/src/routes/controls.ts @@ -548,9 +548,9 @@ controls.patch('/:id', memberMiddleware, async (c) => { } const body = await c.req.json() + // controlType is fixed at creation: these routes only serve blood definitions. const schema = z.object({ name: z.string().min(1).optional(), - controlType: z.enum(['blood', 'plasma_positive', 'plasma_negative', 'antibody', 'extraction', 'negative']).optional(), targetDensity: z.number().optional(), targetDensityUnitId: z.number().int().optional(), strains: z.array(strainCompositionEntrySchema).optional(), @@ -585,6 +585,37 @@ controls.patch('/:id', memberMiddleware, async (c) => { } } + if (baseData.name !== undefined && baseData.name !== existing.name) { + const existingByName = await dbInstance + .select({ id: controlDefinition.id }) + .from(controlDefinition) + .where(eq(controlDefinition.name, baseData.name)) + .get() + if (existingByName) { + return c.json({ error: 'A control definition with this name already exists' }, 409) + } + } + + // Same rule as create: no two definitions with the same density and strain mix. + if (strains !== undefined || targetDensity !== undefined || targetDensityUnitId !== undefined) { + const storedStrains = (newProps.strains as Array<{ id: number; percentage: number }> | undefined) ?? [] + const others = await dbInstance + .select() + .from(controlDefinition) + .where(and(eq(controlDefinition.controlType, 'blood'), sql`${controlDefinition.id} != ${id}`)) + const duplicate = findMatchingDefinitionInList(others, { + strains: storedStrains.map((strain) => ({ strainId: strain.id, percentage: strain.percentage })), + targetDensity: newProps.targetDensity as number | undefined, + targetDensityUnitId: newProps.targetDensityUnitId as number | undefined, + }) + if (duplicate) { + return c.json({ + error: 'A control definition with this combination of density and strains already exists', + existingDefinition: duplicate.definition, + }, 409) + } + } + // Update control definition const user = c.get('user') const [updatedControl] = await dbInstance diff --git a/packages/api/src/routes/qpcr-experiments.ts b/packages/api/src/routes/qpcr-experiments.ts index c09a8a3a..d1e1e526 100644 --- a/packages/api/src/routes/qpcr-experiments.ts +++ b/packages/api/src/routes/qpcr-experiments.ts @@ -27,6 +27,7 @@ import { resolveMicronixBarcodesToContainers } from '../lib/export/query' import { normalizeWellPosition, parsePlateCSV, validateWellPosition } from '../lib/plate-csv' import { utcNow } from '../lib/datetime' import { requireParam } from '../lib/common-validators' +import { withWriteTransaction } from '../db/write-transaction' /** * Attach the Source (subject vs control provenance) to each well, resolving @@ -289,25 +290,38 @@ export function createQpcrExperimentsRoutes(database: Database): Hono { if (data.standardLayout !== undefined) updates.standardLayout = data.standardLayout if (data.status !== undefined) updates.status = data.status if (data.instrumentType !== undefined) updates.instrumentType = data.instrumentType - if (data.targets !== undefined && (exp.status === 'setup' || exp.status === 'in_progress')) { - await database.delete(qpcrExperimentTarget).where(eq(qpcrExperimentTarget.qpcrExperimentId, id)) - if (data.targets.length > 0) { - await database.insert(qpcrExperimentTarget).values( - data.targets.map((t, i) => ({ + const newTargets = + data.targets !== undefined && (exp.status === 'setup' || exp.status === 'in_progress') + ? data.targets.map((t, i) => ({ qpcrExperimentId: id, targetName: t.targetName.trim() || 'varATS', fluorophore: t.fluorophore ?? null, reporter: t.reporter ?? null, sortOrder: i, })) - ) + : null + if (newTargets) { + const names = newTargets.map((t) => t.targetName) + const duplicate = names.find((name, i) => names.indexOf(name) !== i) + if (duplicate) { + return c.json({ error: `Target '${duplicate}' is listed more than once.` }, 400) } } - const [updated] = await database - .update(qpcrExperiment) - .set(updates) - .where(eq(qpcrExperiment.id, id)) - .returning() + // Replace targets and update the experiment together, so a failure keeps the old targets. + const updated = await withWriteTransaction(database, async (tx) => { + if (newTargets) { + await tx.delete(qpcrExperimentTarget).where(eq(qpcrExperimentTarget.qpcrExperimentId, id)) + if (newTargets.length > 0) { + await tx.insert(qpcrExperimentTarget).values(newTargets) + } + } + const [row] = await tx + .update(qpcrExperiment) + .set(updates) + .where(eq(qpcrExperiment.id, id)) + .returning() + return row + }) const targets = await database .select() .from(qpcrExperimentTarget) From 938057f1f0a8f3fde492e1f161e3c50f07147710 Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Fri, 25 Sep 2026 09:38:58 -0700 Subject: [PATCH 08/35] fix(scripts): make the integrity check read-only and complete. check-integrity opened the file through openOperationalDatabase, which runs schema migrations, so pointing it at a backup upgraded the backup. It now opens the file read-only (DATABASE_PATH or --db=). GROUP_CONCAT(.id) failed on storage_container_tag, which has no id column, and the error was skipped, so orphaned tag links were never reported. Ids now come from rowid. qPCR well references are checked too. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/api/src/bun-sqlite.d.ts | 3 ++- packages/api/src/db/open.ts | 17 +++++++-------- packages/api/src/scripts/check-integrity.ts | 23 +++++++++++++++------ 3 files changed, 27 insertions(+), 16 deletions(-) diff --git a/packages/api/src/bun-sqlite.d.ts b/packages/api/src/bun-sqlite.d.ts index 881ba4b4..35eee7f9 100644 --- a/packages/api/src/bun-sqlite.d.ts +++ b/packages/api/src/bun-sqlite.d.ts @@ -1,6 +1,7 @@ declare module 'bun:sqlite' { export class Database { - constructor(path: string | ':memory:') + constructor(path: string | ':memory:', options?: { readonly?: boolean; create?: boolean; readwrite?: boolean; strict?: boolean }) + readonly filename: string exec(sql: string): void prepare(sql: string): Statement query(sql: string): Statement diff --git a/packages/api/src/db/open.ts b/packages/api/src/db/open.ts index a1e922a3..6e79c029 100644 --- a/packages/api/src/db/open.ts +++ b/packages/api/src/db/open.ts @@ -80,6 +80,13 @@ function resolveDatabaseFilePath(relativeOrAbsolute: string): string { return isAbsolute(relativeOrAbsolute) ? relativeOrAbsolute : resolve(getMonorepoRoot(), relativeOrAbsolute) } +/** Path openOperationalDatabase would use: explicit path, then DATABASE_PATH, then the dev default. */ +export function resolveOperationalDatabasePath(dbPath?: string): string { + if (dbPath) return resolveDatabaseFilePath(dbPath) + if (process.env.DATABASE_PATH) return resolveDatabaseFilePath(process.env.DATABASE_PATH) + return resolve(getMonorepoRoot(), 'sampledb_dev.sqlite') +} + export type OperationalDatabase = ReturnType> /** @@ -89,15 +96,7 @@ export function openOperationalDatabase(dbPath?: string): { db: OperationalDatabase sqlite: SQLiteDatabase } { - let resolvedPath: string - - if (dbPath) { - resolvedPath = resolveDatabaseFilePath(dbPath) - } else if (process.env.DATABASE_PATH) { - resolvedPath = resolveDatabaseFilePath(process.env.DATABASE_PATH) - } else { - resolvedPath = resolve(getMonorepoRoot(), 'sampledb_dev.sqlite') - } + const resolvedPath = resolveOperationalDatabasePath(dbPath) if (process.env.NODE_ENV !== 'production') { console.log(`📁 Monorepo root: ${getMonorepoRoot()}`) diff --git a/packages/api/src/scripts/check-integrity.ts b/packages/api/src/scripts/check-integrity.ts index d731fd41..1bc4eca0 100644 --- a/packages/api/src/scripts/check-integrity.ts +++ b/packages/api/src/scripts/check-integrity.ts @@ -12,7 +12,8 @@ * DATABASE_PATH=path/to/database.sqlite bun src/scripts/check-integrity.ts --export-json */ -import { openOperationalDatabase } from '../db/client' +import { Database } from 'bun:sqlite' +import { resolveOperationalDatabasePath } from '../db/open' import { writeFileSync } from 'fs' import { join } from 'path' @@ -59,7 +60,7 @@ function checkForeignKey( let query: string if (isSelfReference) { query = ` - SELECT COUNT(*) as count, GROUP_CONCAT(t1.id) as ids + SELECT COUNT(*) as count, GROUP_CONCAT(t1.rowid) as ids FROM ${table} t1 LEFT JOIN ${referencedTable} t2 ON t1.${column} = t2.${referencedColumn} WHERE t1.${column} IS NOT NULL @@ -67,7 +68,7 @@ function checkForeignKey( ` } else { query = ` - SELECT COUNT(*) as count, GROUP_CONCAT(${table}.id) as ids + SELECT COUNT(*) as count, GROUP_CONCAT(${table}.rowid) as ids FROM ${table} LEFT JOIN ${referencedTable} ON ${table}.${column} = ${referencedTable}.${referencedColumn} WHERE ${table}.${column} IS NOT NULL @@ -257,6 +258,15 @@ function checkDatabaseIntegrity(sqlite: any): IntegrityReport { ] issues.push(...derivationIssues.filter((issue): issue is IntegrityIssue => issue !== null)) + // qPCR checks + console.log('Checking qPCR relationships...') + const qpcrIssues = [ + checkForeignKey(sqlite, 'qpcr_experiment_well', 'qpcr_experiment_id', 'qpcr_experiment', 'id', 'qPCR wells with non-existent qpcr_experiment_id'), + checkForeignKey(sqlite, 'qpcr_experiment_well', 'storage_container_id', 'storage_container', 'id', 'qPCR wells with non-existent storage_container_id'), + checkForeignKey(sqlite, 'qpcr_experiment_well', 'specimen_id', 'specimen', 'id', 'qPCR wells with non-existent specimen_id'), + ] + issues.push(...qpcrIssues.filter((issue): issue is IntegrityIssue => issue !== null)) + // Other checks console.log('Checking other relationships...') const otherIssues = [ @@ -277,7 +287,7 @@ function checkDatabaseIntegrity(sqlite: any): IntegrityReport { entry.orphanedRecords += issue.orphanedCount } - const databasePath = process.env.DATABASE_PATH || 'sampledb_dev.sqlite' + const databasePath = sqlite.filename return { timestamp: startTime, @@ -343,8 +353,9 @@ function main() { const exportJson = args.includes('--export-json') try { - // Connect to database - const { sqlite } = openOperationalDatabase() + // Open read-only and without schema evolution: checking a backup must not migrate it. + const dbArg = args.find((arg) => arg.startsWith('--db='))?.slice('--db='.length) + const sqlite = new Database(resolveOperationalDatabasePath(dbArg), { readonly: true }) // Run integrity checks const report = checkDatabaseIntegrity(sqlite) From 4d4ce518f214e62735d8f9f97b81b3d49d4bf9ec Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Fri, 25 Sep 2026 09:42:37 -0700 Subject: [PATCH 09/35] fix(api): resolve sheets by box or bag and keep names unique per box. Paper moves resolved a sheet by name alone and took the first match anywhere, but default names like "Sheet-1" repeat in every box: a paper in box BB's Sheet-1 failed with "No target collection mapping", or moved into another box's sheet. Source sheets now match every sheet with that name (optionally narrowed by fromParentName), and the target must be a single sheet (narrowed by toParentName), otherwise the move reports the ambiguity. UNIQUE(name, box_id, bag_id) never fired because one of box_id / bag_id is always NULL. Migration 006 adds a unique (box_id, name) index; bags are exempt because legacy DBS bags hold many same-named sheets. Moving sheets into a box that already has a sheet with that name returns 409. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/api/initial_schema.sql | 3 +- packages/api/src/db/__tests__/client.test.ts | 1 + .../src/db/__tests__/schema-evolution.test.ts | 41 +++++++++++++++++-- .../migrations/006_box_sheet_name_unique.sql | 15 +++++++ packages/api/src/db/schema-evolution.ts | 2 +- packages/api/src/db/schema.ts | 3 +- .../src/lib/__tests__/container-move.test.ts | 37 ++++++++++++++++- .../src/lib/__tests__/write-atomicity.test.ts | 13 +++++- .../src/lib/collections/collection-resolve.ts | 24 +++++++++++ .../api/src/lib/collections/sheet-move.ts | 23 ++++++++++- packages/api/src/lib/container-move.ts | 22 +++++++++- packages/api/src/routes/collections.ts | 10 ++++- 12 files changed, 182 insertions(+), 12 deletions(-) create mode 100644 packages/api/src/db/migrations/006_box_sheet_name_unique.sql diff --git a/packages/api/initial_schema.sql b/packages/api/initial_schema.sql index 334ed82f..0f000bbb 100644 --- a/packages/api/initial_schema.sql +++ b/packages/api/initial_schema.sql @@ -259,6 +259,7 @@ CREATE TABLE IF NOT EXISTS sheet ( (box_id IS NULL AND bag_id IS NOT NULL) OR (box_id IS NULL AND bag_id IS NULL)) );--> statement-breakpoint +CREATE UNIQUE INDEX IF NOT EXISTS sheet_box_name_idx ON sheet(box_id, name) WHERE box_id IS NOT 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), @@ -283,7 +284,7 @@ CREATE TABLE IF NOT EXISTS storage_type ( CREATE TABLE IF NOT EXISTS schema_version ( version INTEGER NOT NULL );--> statement-breakpoint -INSERT INTO schema_version (version) VALUES (5);--> statement-breakpoint +INSERT INTO schema_version (version) VALUES (6);--> statement-breakpoint CREATE TABLE IF NOT EXISTS settings ( key TEXT NOT NULL, user_id INTEGER REFERENCES users(id), diff --git a/packages/api/src/db/__tests__/client.test.ts b/packages/api/src/db/__tests__/client.test.ts index 53b82116..c08ee650 100644 --- a/packages/api/src/db/__tests__/client.test.ts +++ b/packages/api/src/db/__tests__/client.test.ts @@ -33,6 +33,7 @@ describe('Database Client', () => { db.exec(`CREATE TABLE cryovial_tube (id INTEGER PRIMARY KEY, collection_id INTEGER NOT NULL, barcode TEXT, position TEXT)`) db.exec(`CREATE TABLE static_well (id INTEGER PRIMARY KEY, collection_id INTEGER NOT NULL, position TEXT)`) db.exec(`CREATE TABLE study_subject (id INTEGER PRIMARY KEY, study_id INTEGER NOT NULL, name TEXT NOT NULL)`) + db.exec(`CREATE TABLE sheet (id INTEGER PRIMARY KEY, name TEXT NOT NULL, box_id INTEGER, bag_id INTEGER)`) // Intentionally omit paper — migration 003 preflight creates legacy stub // Intentionally do NOT create error_logs const before = db.prepare("SELECT name FROM sqlite_master WHERE type='table' AND name='error_logs'").get() diff --git a/packages/api/src/db/__tests__/schema-evolution.test.ts b/packages/api/src/db/__tests__/schema-evolution.test.ts index a685b192..cd591ad3 100644 --- a/packages/api/src/db/__tests__/schema-evolution.test.ts +++ b/packages/api/src/db/__tests__/schema-evolution.test.ts @@ -36,6 +36,12 @@ function createTablesForLaterMigrations(sqlite: Database): void { study_id INTEGER NOT NULL, name TEXT NOT NULL )`) + sqlite.exec(`CREATE TABLE IF NOT EXISTS sheet ( + id INTEGER PRIMARY KEY, + name TEXT NOT NULL, + box_id INTEGER, + bag_id INTEGER + )`) } describe('schema evolution', () => { @@ -98,7 +104,7 @@ describe('schema evolution', () => { 'CREATE TABLE settings (key TEXT, user_id INTEGER, value TEXT, PRIMARY KEY (key, user_id))', ) sqlite.exec('CREATE TABLE storage_container (id INTEGER PRIMARY KEY)') - sqlite.exec('CREATE TABLE sheet (id INTEGER PRIMARY KEY, name TEXT)') + sqlite.exec('CREATE TABLE sheet (id INTEGER PRIMARY KEY, name TEXT, box_id INTEGER, bag_id INTEGER)') sqlite.exec(`CREATE TABLE paper ( id INTEGER PRIMARY KEY REFERENCES storage_container(id), sheet_id INTEGER NOT NULL REFERENCES sheet(id), @@ -123,7 +129,7 @@ describe('schema evolution', () => { sqlite.exec('INSERT INTO schema_version (version) VALUES (1)') sqlite.exec('CREATE TABLE study (id INTEGER PRIMARY KEY)') sqlite.exec('CREATE TABLE storage_container (id INTEGER PRIMARY KEY)') - sqlite.exec('CREATE TABLE sheet (id INTEGER PRIMARY KEY, name TEXT)') + sqlite.exec('CREATE TABLE sheet (id INTEGER PRIMARY KEY, name TEXT, box_id INTEGER, bag_id INTEGER)') sqlite.exec(`CREATE TABLE paper ( id INTEGER PRIMARY KEY REFERENCES storage_container(id), sheet_id INTEGER NOT NULL REFERENCES sheet(id), @@ -141,7 +147,7 @@ describe('schema evolution', () => { sqlite.exec('CREATE TABLE schema_version (version INTEGER NOT NULL)') sqlite.exec('INSERT INTO schema_version (version) VALUES (2)') sqlite.exec(`CREATE TABLE storage_container (id INTEGER PRIMARY KEY)`) - sqlite.exec(`CREATE TABLE sheet (id INTEGER PRIMARY KEY, name TEXT)`) + sqlite.exec(`CREATE TABLE sheet (id INTEGER PRIMARY KEY, name TEXT, box_id INTEGER, bag_id INTEGER)`) sqlite.exec(`CREATE TABLE paper ( id INTEGER PRIMARY KEY REFERENCES storage_container(id), sheet_id INTEGER NOT NULL REFERENCES sheet(id), @@ -181,7 +187,7 @@ describe('schema evolution', () => { sqlite.exec('CREATE TABLE schema_version (version INTEGER NOT NULL)') sqlite.exec('INSERT INTO schema_version (version) VALUES (2)') sqlite.exec(`CREATE TABLE storage_container (id INTEGER PRIMARY KEY)`) - sqlite.exec(`CREATE TABLE sheet (id INTEGER PRIMARY KEY, name TEXT)`) + sqlite.exec(`CREATE TABLE sheet (id INTEGER PRIMARY KEY, name TEXT, box_id INTEGER, bag_id INTEGER)`) sqlite.exec(`CREATE TABLE paper ( id INTEGER PRIMARY KEY REFERENCES storage_container(id), sheet_id INTEGER NOT NULL REFERENCES sheet(id), @@ -260,6 +266,33 @@ describe('schema evolution', () => { sqlite.close() }) + it('migration 006 makes sheet names unique per box but not per bag', () => { + const sqlite = new Database(testDbPath) + sqlite.exec('CREATE TABLE schema_version (version INTEGER NOT NULL)') + sqlite.exec('INSERT INTO schema_version (version) VALUES (5)') + createTablesForLaterMigrations(sqlite) + sqlite.exec(`INSERT INTO sheet (id, name, box_id, bag_id) VALUES (1, 'S1', 1, NULL), (2, 'S1', NULL, 7), (3, 'S1', NULL, 7)`) + + evolveOperationalSchema(sqlite) + + expect(getRecordedSchemaVersion(sqlite)).toBe(CURRENT_SCHEMA_VERSION) + expect(() => sqlite.exec(`INSERT INTO sheet (id, name, box_id) VALUES (4, 'S1', 1)`)).toThrow(/UNIQUE/) + sqlite.exec(`INSERT INTO sheet (id, name, bag_id) VALUES (5, 'S1', 7)`) + sqlite.close() + }) + + it('migration 006 aborts when a box has two sheets with the same name', () => { + const sqlite = new Database(testDbPath) + sqlite.exec('CREATE TABLE schema_version (version INTEGER NOT NULL)') + sqlite.exec('INSERT INTO schema_version (version) VALUES (5)') + createTablesForLaterMigrations(sqlite) + sqlite.exec(`INSERT INTO sheet (id, name, box_id) VALUES (1, 'S1', 1), (2, 'S1', 1)`) + + expect(() => evolveOperationalSchema(sqlite)).toThrow(/Rename one of them first/) + expect(getRecordedSchemaVersion(sqlite)).toBe(5) + sqlite.close() + }) + it('fail-hard: invalid migration leaves schema_version unchanged', () => { const sqlite = new Database(testDbPath) sqlite.exec('CREATE TABLE schema_version (version INTEGER NOT NULL)') diff --git a/packages/api/src/db/migrations/006_box_sheet_name_unique.sql b/packages/api/src/db/migrations/006_box_sheet_name_unique.sql new file mode 100644 index 00000000..139a7116 --- /dev/null +++ b/packages/api/src/db/migrations/006_box_sheet_name_unique.sql @@ -0,0 +1,15 @@ +-- Migration 006: sheet names are unique within a box. Bags are exempt: legacy DBS bags hold +-- many single-paper sheets that share one name. (UNIQUE(name, box_id, bag_id) never fired +-- because one of box_id / bag_id is always NULL.) +CREATE TEMP TABLE IF NOT EXISTS __migration_006_guard (id INTEGER PRIMARY KEY);--> statement-breakpoint +CREATE TRIGGER __migration_006_guard_tr BEFORE INSERT ON __migration_006_guard +BEGIN + SELECT RAISE(ABORT, 'Cannot add unique sheet name index: a box has two sheets with the same name. Rename one of them first.') + WHERE EXISTS ( + SELECT 1 FROM sheet WHERE box_id IS NOT NULL GROUP BY box_id, name HAVING COUNT(*) > 1 + ); +END;--> statement-breakpoint +INSERT INTO __migration_006_guard DEFAULT VALUES;--> statement-breakpoint +DROP TRIGGER __migration_006_guard_tr;--> statement-breakpoint +DROP TABLE __migration_006_guard;--> statement-breakpoint +CREATE UNIQUE INDEX IF NOT EXISTS sheet_box_name_idx ON sheet(box_id, name) WHERE box_id IS NOT NULL; diff --git a/packages/api/src/db/schema-evolution.ts b/packages/api/src/db/schema-evolution.ts index c0eeab90..b283e974 100644 --- a/packages/api/src/db/schema-evolution.ts +++ b/packages/api/src/db/schema-evolution.ts @@ -7,7 +7,7 @@ import { listNumberedMigrations, runSqlMigration } from './migration-runner' const PAPER_SUBLABEL_MIGRATION_VERSION = 3 /** Canonical schema level; bump when adding numbered deltas under migrations/. */ -export const CURRENT_SCHEMA_VERSION = 5 +export const CURRENT_SCHEMA_VERSION = 6 const SCHEMA_VERSION_TABLE = 'schema_version' const LEGACY_BASELINE_VERSION = 1 diff --git a/packages/api/src/db/schema.ts b/packages/api/src/db/schema.ts index 48c44717..8b339eab 100644 --- a/packages/api/src/db/schema.ts +++ b/packages/api/src/db/schema.ts @@ -356,7 +356,8 @@ export const sheet = sqliteTable('sheet', { (${t.boxId} IS NULL AND ${t.bagId} IS NOT NULL) OR (${t.boxId} IS NULL AND ${t.bagId} IS NULL) `), - unq: unique().on(t.name, t.boxId, t.bagId) + unq: unique().on(t.name, t.boxId, t.bagId), + boxNameUniq: uniqueIndex('sheet_box_name_idx').on(t.boxId, t.name).where(sql`${t.boxId} IS NOT NULL`), })) export const paper = sqliteTable('paper', { diff --git a/packages/api/src/lib/__tests__/container-move.test.ts b/packages/api/src/lib/__tests__/container-move.test.ts index 0aa75f46..02f77c28 100644 --- a/packages/api/src/lib/__tests__/container-move.test.ts +++ b/packages/api/src/lib/__tests__/container-move.test.ts @@ -17,7 +17,7 @@ import { type ContainerInfo, } from '../container-move' import { storageContainer } from '../../db/schema' -import { micronixTube, staticWell } from '../../db/schema' +import { micronixTube, staticWell, box, sheet, paper } from '../../db/schema' import type { Database } from '../../db/client' import { utcNow } from '../datetime' @@ -560,4 +560,39 @@ describe('container-move', () => { expect(info?.currentCollectionName).toBe(plate.name) }) }) + + describe('paper moves between same-named sheets', () => { + it('moves papers from Sheet-1 of one box to Sheet-1 of another using parent names', async () => { + const storageType = await createTestStorageType(testDb, { name: 'Freezer' }) + const location = await createTestLocation(testDb, { name: 'Loc', storageTypeId: String(storageType.id) }) + const specimenType = await createTestSpecimenType(testDb, { name: 'Blood' }) + const specimen = await createTestSpecimen(testDb, specimenType.id) + const unit = await createTestUnit(testDb, { symbol: 'uL', name: 'microliter', category: 'volume' }) + const now = utcNow() + const [boxA] = await testDb.insert(box).values({ name: 'BA', locationId: location.id, created: now, lastUpdated: now }).returning() + const [boxB] = await testDb.insert(box).values({ name: 'BB', locationId: location.id, created: now, lastUpdated: now }).returning() + const [sheetA] = await testDb.insert(sheet).values({ name: 'Sheet-1', boxId: boxA.id }).returning() + const [sheetB] = await testDb.insert(sheet).values({ name: 'Sheet-1', boxId: boxB.id }).returning() + const [container] = await testDb + .insert(storageContainer) + .values({ specimenId: specimen.id, unitId: unit.id, totalQuantity: 1, remainingQuantity: 1, created: now, lastUpdated: now }) + .returning() + await testDb.insert(paper).values({ id: container!.id, sheetId: sheetB.id }) + + const ambiguous = await executeMoves(testDb, { + mappings: [{ fromCollectionName: 'Sheet-1', toCollectionName: 'Sheet-1' }], + moves: [{ identifier: { type: 'container_id', containerId: container!.id }, targetPosition: '' }], + }) + expect(ambiguous.success).toBe(false) + expect(ambiguous.errors?.[0].error).toMatch(/set toParentName/) + + const result = await executeMoves(testDb, { + mappings: [{ fromCollectionName: 'Sheet-1', fromParentName: 'BB', toCollectionName: 'Sheet-1', toParentName: 'BA' }], + moves: [{ identifier: { type: 'container_id', containerId: container!.id }, targetPosition: '' }], + }) + expect(result.success).toBe(true) + const [moved] = await testDb.select().from(paper).all() + expect(moved.sheetId).toBe(sheetA.id) + }) + }) }) diff --git a/packages/api/src/lib/__tests__/write-atomicity.test.ts b/packages/api/src/lib/__tests__/write-atomicity.test.ts index 41710f55..97a55243 100644 --- a/packages/api/src/lib/__tests__/write-atomicity.test.ts +++ b/packages/api/src/lib/__tests__/write-atomicity.test.ts @@ -10,7 +10,7 @@ import { } from '../../__tests__/helpers/factories' import { setContainerDefaults } from '../settings' import { runBulkCombinedImport } from '../bulk-combined-import' -import { moveSheetsToCollection, SheetNotFoundError } from '../collections/sheet-move' +import { moveSheetsToCollection, SheetNameConflictError, SheetNotFoundError } from '../collections/sheet-move' import { specimenTypeContainerType, containerTypeUnit, @@ -116,4 +116,15 @@ describe('multi-step writes roll back on failure', () => { const [after] = await testDb.select().from(sheet).where(eq(sheet.id, s1.id)) expect(after.boxId).toBe(boxA.id) }) + + it('sheet move refuses a name that already exists in the target box', async () => { + const { loc } = await setupMicronixPlate() + const now = utcNow() + const [boxA] = await testDb.insert(box).values({ name: 'BoxA', locationId: loc.id, created: now, lastUpdated: now }).returning() + const [boxB] = await testDb.insert(box).values({ name: 'BoxB', locationId: loc.id, created: now, lastUpdated: now }).returning() + const [moving] = await testDb.insert(sheet).values({ name: 'Sheet-1', boxId: boxA.id }).returning() + await testDb.insert(sheet).values({ name: 'Sheet-1', boxId: boxB.id }) + + await expect(moveSheetsToCollection(testDb, [moving.id], boxB.id, 'box')).rejects.toBeInstanceOf(SheetNameConflictError) + }) }) diff --git a/packages/api/src/lib/collections/collection-resolve.ts b/packages/api/src/lib/collections/collection-resolve.ts index 510e7512..7cf45682 100644 --- a/packages/api/src/lib/collections/collection-resolve.ts +++ b/packages/api/src/lib/collections/collection-resolve.ts @@ -58,6 +58,27 @@ export async function resolveCollectionByName( } } +/** + * Sheets with this name, optionally only those in the box or bag named parentName. + * Sheet names are not unique (default names like "Sheet-1" repeat in every box, and legacy + * bags hold many same-named sheets), so callers must handle several matches. + */ +export async function resolveSheetsByName( + database: Database, + name: string, + parentName?: string, +): Promise { + const rows = await database + .select({ id: sheet.id, boxName: box.name, bagName: bag.name }) + .from(sheet) + .leftJoin(box, eq(sheet.boxId, box.id)) + .leftJoin(bag, eq(sheet.bagId, bag.id)) + .where(eq(sheet.name, name)) + return rows + .filter((row) => parentName === undefined || row.boxName === parentName || row.bagName === parentName) + .map((row) => row.id) +} + /** Resolve collection by barcode (micronix plates and cryovial boxes only). */ export async function resolveCollectionByBarcode( barcode: string, @@ -225,6 +246,9 @@ export const moveContainersBodySchema = z.object({ z.object({ fromCollectionName: z.string().min(1), toCollectionName: z.string().min(1), + /** For sheets: box or bag holding the source / target sheet (sheet names repeat). */ + fromParentName: z.string().min(1).optional(), + toParentName: z.string().min(1).optional(), }), ), moves: z.array( diff --git a/packages/api/src/lib/collections/sheet-move.ts b/packages/api/src/lib/collections/sheet-move.ts index 65efc6da..e169c94a 100644 --- a/packages/api/src/lib/collections/sheet-move.ts +++ b/packages/api/src/lib/collections/sheet-move.ts @@ -1,4 +1,4 @@ -import { eq, sql } from 'drizzle-orm' +import { eq, inArray, sql } from 'drizzle-orm' import type { Database } from '../../db/client' import { box, bag, sheet } from '../../db/schema' import { withWriteTransaction } from '../../db/write-transaction' @@ -17,6 +17,13 @@ export class SheetNotFoundError extends Error { } } +export class SheetNameConflictError extends Error { + constructor(name: string) { + super(`The target box already has a sheet named '${name}'. Rename one of them first.`) + this.name = 'SheetNameConflictError' + } +} + export async function moveSheetsToCollection( database: Database, sheetIds: number[], @@ -26,6 +33,20 @@ export async function moveSheetsToCollection( if (targetCollectionType === 'box') { const exists = await database.select().from(box).where(eq(box.id, targetCollectionId)).get() if (!exists) throw new SheetMoveTargetNotFoundError('box') + // Sheet names are unique within a box (bags still allow repeated legacy names). + const moving = sheetIds.length > 0 + ? await database.select({ id: sheet.id, name: sheet.name }).from(sheet).where(inArray(sheet.id, sheetIds)) + : [] + const staying = await database + .select({ id: sheet.id, name: sheet.name }) + .from(sheet) + .where(eq(sheet.boxId, targetCollectionId)) + const movingIds = new Set(moving.map((s) => s.id)) + const taken = new Set(staying.filter((s) => !movingIds.has(s.id)).map((s) => s.name)) + for (const s of moving) { + if (taken.has(s.name)) throw new SheetNameConflictError(s.name) + taken.add(s.name) + } } else { const exists = await database.select().from(bag).where(eq(bag.id, targetCollectionId)).get() if (!exists) throw new SheetMoveTargetNotFoundError('bag') diff --git a/packages/api/src/lib/container-move.ts b/packages/api/src/lib/container-move.ts index 410640fe..cb743ccd 100644 --- a/packages/api/src/lib/container-move.ts +++ b/packages/api/src/lib/container-move.ts @@ -11,7 +11,7 @@ import { sheet, } from '../db/schema' import { eq, and, isNull } from 'drizzle-orm' -import { resolveCollection, type CollectionType } from './collections/collection-resolve' +import { resolveCollection, resolveSheetsByName, type CollectionType } from './collections/collection-resolve' import { checkGridPositionOccupancy } from './container-occupancy' import { normalizePosition } from './normalize-position' @@ -42,6 +42,9 @@ export interface MoveOperation { export interface CollectionMapping { fromCollectionName: string toCollectionName: string + /** For sheets: box or bag holding the source / target sheet (sheet names repeat). */ + fromParentName?: string + toParentName?: string } export interface BatchMoveRequest { @@ -378,6 +381,23 @@ export async function executeMoves(database: Database, request: BatchMoveRequest const mappingMap = new Map() const mappingErrors: string[] = [] for (const m of mappings) { + if (collectionType === 'sheet') { + // Every sheet with the source name (in fromParentName, if given) maps to the one + // target sheet; the target must be unambiguous. + const fromIds = await resolveSheetsByName(database, m.fromCollectionName, m.fromParentName) + const toIds = await resolveSheetsByName(database, m.toCollectionName, m.toParentName) + if (fromIds.length === 0) mappingErrors.push(`Source collection "${m.fromCollectionName}" not found`) + if (toIds.length === 0) mappingErrors.push(`Target collection "${m.toCollectionName}" not found`) + if (toIds.length > 1) { + mappingErrors.push( + `Target sheet "${m.toCollectionName}" matches ${toIds.length} sheets; set toParentName to the box or bag that holds it`, + ) + } + if (fromIds.length > 0 && toIds.length === 1) { + for (const fromId of fromIds) mappingMap.set(fromId, toIds[0]) + } + continue + } const fromId = await resolveCollection(m.fromCollectionName, collectionType, database) const toId = await resolveCollection(m.toCollectionName, collectionType, database) if (fromId && toId) { diff --git a/packages/api/src/routes/collections.ts b/packages/api/src/routes/collections.ts index f07707b6..96f82c3a 100644 --- a/packages/api/src/routes/collections.ts +++ b/packages/api/src/routes/collections.ts @@ -41,7 +41,12 @@ import { deleteWithContentsBodySchema, } from '../lib/collections/collection-resolve' import { collectionDeletePreflightSchema } from '@sampledb/contract' -import { moveSheetsToCollection, SheetMoveTargetNotFoundError, SheetNotFoundError } from '../lib/collections/sheet-move' +import { + moveSheetsToCollection, + SheetMoveTargetNotFoundError, + SheetNameConflictError, + SheetNotFoundError, +} from '../lib/collections/sheet-move' import type { CollectionType } from '../lib/collections/types' function mapCreateCollectionError(error: unknown, c: { json: (body: unknown, status?: number) => Response }) { @@ -287,6 +292,9 @@ export function createCollectionsRoutes(database: Database): Hono { if (error instanceof SheetMoveTargetNotFoundError || error instanceof SheetNotFoundError) { return c.json({ error: error.message }, 404) } + if (error instanceof SheetNameConflictError) { + return c.json({ error: error.message }, 409) + } return handleRouteError(error, c) } }) From e3fdf2f9506fb56bafbd78be16c7edb87b31042e Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Fri, 25 Sep 2026 09:43:35 -0700 Subject: [PATCH 10/35] fix(api): set a location's storage type correctly on create and move. Creating a root location without an explicit parentId stored a NULL storage type (the check was parentId === null), and moving a root under a parent without sending storageTypeId kept it; both violated the location CHECK and returned 500. Moving a child to the root now requires a storage type up front. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../src/routes/__tests__/locations.test.ts | 34 +++++++++++++++++++ packages/api/src/routes/locations.ts | 11 ++++-- 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/packages/api/src/routes/__tests__/locations.test.ts b/packages/api/src/routes/__tests__/locations.test.ts index 927da0ad..b5970793 100644 --- a/packages/api/src/routes/__tests__/locations.test.ts +++ b/packages/api/src/routes/__tests__/locations.test.ts @@ -6,6 +6,8 @@ import { } from '../../__tests__/helpers/authenticated-route-test' import { createLocationsRoutes } from '../locations' import { createTestStorageType, createTestLocation } from '../../__tests__/helpers/factories' +import { location } from '../../db/schema' +import { eq } from 'drizzle-orm' describe('Locations API', () => { let ctx: AuthenticatedRouteTestContext @@ -18,6 +20,9 @@ describe('Locations API', () => { password: 'password123', role: 'member', }, + additionalUsers: [ + { key: 'admin', email: 'admin@test.com', name: 'Admin', password: 'password123', role: 'admin' }, + ], settings: { pagination: true }, mount: (app, { db, sqlite }) => { app.route('/api/locations', createLocationsRoutes(db, sqlite)) @@ -79,4 +84,33 @@ describe('Locations API', () => { expect(res.status).toBe(401) }) }) + + describe('storage type on root and child locations', () => { + it('creates a root location when parentId is omitted', async () => { + const storageType = await createTestStorageType(ctx.db, { name: 'Freezer' }) + const res = await ctx.request('/api/locations', { + method: 'POST', + cookie: ctx.cookies.admin, + json: { name: 'Root A', storageTypeId: String(storageType.id), canContainCollections: false }, + }) + expect(res.status).toBe(201) + }) + + it('moves a root location under a parent without sending storageTypeId', async () => { + const storageType = await createTestStorageType(ctx.db, { name: 'Freezer' }) + const parent = await createTestLocation(ctx.db, { name: 'Parent', storageTypeId: String(storageType.id) }) + const root = await createTestLocation(ctx.db, { name: 'Other Root', storageTypeId: String(storageType.id) }) + + const res = await ctx.request(`/api/locations/${root.id}`, { + method: 'PUT', + cookie: ctx.cookies.admin, + json: { parentId: parent.id }, + }) + + expect(res.status).toBe(200) + const [row] = await ctx.db.select().from(location).where(eq(location.id, root.id)) + expect(row.parentId).toBe(parent.id) + expect(row.storageTypeId).toBeNull() + }) + }) }) diff --git a/packages/api/src/routes/locations.ts b/packages/api/src/routes/locations.ts index f8491146..75ddd273 100644 --- a/packages/api/src/routes/locations.ts +++ b/packages/api/src/routes/locations.ts @@ -325,7 +325,8 @@ locations.post('/', adminMiddleware, async (c) => { .values({ parentId: data.parentId ?? null, name: data.name, - storageTypeId: data.parentId === null ? data.storageTypeId : null, // Only set for root locations + // Only set for root locations; an omitted parentId also means root. + storageTypeId: data.parentId == null ? data.storageTypeId : null, description: data.description, canContainCollections: data.canContainCollections, created: now, @@ -430,6 +431,10 @@ locations.put('/:id', adminMiddleware, async (c) => { return c.json({ error: 'Storage type ID must be null for child locations' }, 400) } } + // A child moved to the root needs a storage type (the table CHECK requires it). + if (isRoot && existing.parentId !== null && !data.storageTypeId) { + return c.json({ error: 'Storage type ID is required when moving a location to the root' }, 400) + } // Check for duplicate name if name or parent is being changed if (data.name !== undefined || data.parentId !== undefined) { @@ -459,8 +464,8 @@ locations.put('/:id', adminMiddleware, async (c) => { } if (data.parentId !== undefined) updateData.parentId = data.parentId ?? null if (data.name !== undefined) updateData.name = data.name - // Only set storageTypeId for root locations - if (data.storageTypeId !== undefined) { + // Only root locations have a storage type; moving a root under a parent clears it. + if (data.storageTypeId !== undefined || !isRoot) { updateData.storageTypeId = isRoot ? data.storageTypeId : null } if (data.description !== undefined) updateData.description = data.description ?? null From c5327394ef44581027aca8f704a3b8508fcded52 Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Fri, 25 Sep 2026 09:46:25 -0700 Subject: [PATCH 11/35] fix(controls): create multi-density batches all-or-nothing. The batch wizard created one batch per density with separate requests. If the third of four failed, the first two stayed, and a retry created them again under new suggested names. The error shown was Axios's "Request failed with status code 400" rather than the server's message. A new POST /blood-controls/batches/create-many-with-specimens creates every batch in one transaction; the wizard sends all densities in one request and shows the server's error. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../__tests__/batch-with-specimens.test.ts | 36 ++++++++++- .../api/src/lib/controls/batch-schemas.ts | 5 ++ .../src/lib/controls/batch-with-specimens.ts | 22 +++++++ packages/api/src/routes/controls.ts | 14 +++++ .../web/src/components/wizards/ReviewStep.tsx | 20 ++++--- .../wizards/__tests__/ReviewStep.test.tsx | 60 +++++++++++++++---- packages/web/src/lib/api/controls.ts | 38 ++++++++---- 7 files changed, 159 insertions(+), 36 deletions(-) diff --git a/packages/api/src/lib/controls/__tests__/batch-with-specimens.test.ts b/packages/api/src/lib/controls/__tests__/batch-with-specimens.test.ts index 82debf71..6d8157fc 100644 --- a/packages/api/src/lib/controls/__tests__/batch-with-specimens.test.ts +++ b/packages/api/src/lib/controls/__tests__/batch-with-specimens.test.ts @@ -10,8 +10,8 @@ import { } from '../../../__tests__/helpers/factories' import { setContainerDefaults, clearSettingsCache } from '../../settings' import { clearDefaultsCache } from '../../defaults' -import { createBatchWithSpecimens } from '../batch-with-specimens' -import { specimenTypeContainerType, containerTypeUnit, storageContainer, box } from '../../../db/schema' +import { createBatchWithSpecimens, createBatchesWithSpecimens } from '../batch-with-specimens' +import { specimenTypeContainerType, containerTypeUnit, storageContainer, box, controlBatch } from '../../../db/schema' import { eq } from 'drizzle-orm' import type { Database } from '../../../db/client' import { utcNow } from '../../datetime' @@ -372,4 +372,36 @@ describe('batch-with-specimens', () => { expect(created.unitId).toBe(unit.id) }) }) + + describe('createBatchesWithSpecimens', () => { + it('creates no batches when a later batch fails', async () => { + const unit = await createTestUnit(testDb, { symbol: 'uL', name: 'microliter', category: 'volume' }) + await setContainerDefaults(testDb, { + micronix_tube: { totalQuantity: 1, remainingQuantity: 1, defaultUnitSymbol: 'uL' }, + }) + await testDb.insert(containerTypeUnit).values({ containerType: 'micronix_tube', unitId: unit.id }) + const def100 = await createTestControlDefinition(testDb, { name: 'D100' }) + const def200 = await createTestControlDefinition(testDb, { name: 'D200' }) + const specimenType = await createTestSpecimenType(testDb, { name: 'DNA' }) + await testDb.insert(specimenTypeContainerType).values({ specimenTypeId: specimenType.id, containerType: 'micronix_tube', created: utcNow() }) + const storageType = await createTestStorageType(testDb, { name: 'Freezer' }) + const location = await createTestLocation(testDb, { name: 'Loc', storageTypeId: String(storageType.id) }) + const plate = await createTestMicronixPlate(testDb, { name: 'Plate1', locationId: location.id }) + const tube = (barcode: string, position: string) => ({ + containerType: 'micronix_tube' as const, + barcode, + collection: { type: 'micronix_plate' as const, id: plate.id, position }, + }) + + await expect( + createBatchesWithSpecimens(testDb, [ + { batch: { controlDefinitionId: def100.id, name: 'B100' }, specimens: [{ specimenTypeName: 'DNA', containers: [tube('MT1', 'A01')] }] }, + { batch: { controlDefinitionId: def200.id, name: 'B200' }, specimens: [{ specimenTypeName: 'Nope', containers: [tube('MT2', 'A02')] }] }, + ]), + ).rejects.toThrow(/Batch 2 \('B200'\).*No batches were created/) + + expect(await testDb.select().from(controlBatch).all()).toHaveLength(0) + expect(await testDb.select().from(storageContainer).all()).toHaveLength(0) + }) + }) }) diff --git a/packages/api/src/lib/controls/batch-schemas.ts b/packages/api/src/lib/controls/batch-schemas.ts index 11601c6c..e21f3e81 100644 --- a/packages/api/src/lib/controls/batch-schemas.ts +++ b/packages/api/src/lib/controls/batch-schemas.ts @@ -73,6 +73,11 @@ export const createBatchWithSpecimensSchema = z.object({ specimens: z.array(batchSpecimenInputSchema).min(1), }) +/** Several batches (e.g. one per density from a composition CSV), created all-or-nothing. */ +export const createBatchesWithSpecimensSchema = z.object({ + batches: z.array(createBatchWithSpecimensSchema).min(1), +}) + export const addSpecimensToBatchSchema = z.object({ specimens: z.array(batchSpecimenInputSchema).min(1), }) diff --git a/packages/api/src/lib/controls/batch-with-specimens.ts b/packages/api/src/lib/controls/batch-with-specimens.ts index e1ace72a..eb329278 100644 --- a/packages/api/src/lib/controls/batch-with-specimens.ts +++ b/packages/api/src/lib/controls/batch-with-specimens.ts @@ -379,3 +379,25 @@ export async function addSpecimensToBatch( } }) } + +/** + * Create several batches with their specimens in one transaction, so a failure on any + * batch leaves none of them behind (retrying would otherwise duplicate the earlier ones). + */ +export async function createBatchesWithSpecimens( + database: Database, + batches: CreateBatchWithSpecimensRequest[], +): Promise>>> { + return withWriteTransaction(database, async (tx) => { + const results: Array>> = [] + for (const [index, data] of batches.entries()) { + try { + results.push(await createBatchWithSpecimens(tx, data)) + } catch (error: unknown) { + const message = error instanceof Error ? error.message : String(error) + throw new ValidationError(`Batch ${index + 1} ('${data.batch.name}'): ${message}. No batches were created.`) + } + } + return results + }) +} diff --git a/packages/api/src/routes/controls.ts b/packages/api/src/routes/controls.ts index f8a88bc3..075e418b 100644 --- a/packages/api/src/routes/controls.ts +++ b/packages/api/src/routes/controls.ts @@ -21,11 +21,13 @@ import { validateControlBatchCsv } from '../lib/controls/batch-csv-validate' import { createBloodControlBatchSchema, createBatchWithSpecimensSchema, + createBatchesWithSpecimensSchema, addSpecimensToBatchSchema, validateControlBatchCsvSchema, } from '../lib/controls/batch-schemas' import { createBatchWithSpecimens, + createBatchesWithSpecimens, addSpecimensToBatch, } from '../lib/controls/batch-with-specimens' import { getBloodControlBatchSummary } from '../lib/controls/batch-summary' @@ -717,6 +719,18 @@ controls.post('/:id/batches', memberMiddleware, async (c) => { }) // Create batch with specimens +// Create several batches (one per density) all-or-nothing +controls.post('/batches/create-many-with-specimens', memberMiddleware, async (c) => { + try { + const body = await c.req.json() + const data = createBatchesWithSpecimensSchema.parse(body) + const results = await createBatchesWithSpecimens(dbInstance, data.batches) + return c.json({ batches: results }, 201) + } catch (error) { + return handleRouteError(error, c) + } +}) + controls.post('/batches/create-with-specimens', memberMiddleware, async (c) => { try { const body = await c.req.json() diff --git a/packages/web/src/components/wizards/ReviewStep.tsx b/packages/web/src/components/wizards/ReviewStep.tsx index d9a99450..34e428fe 100644 --- a/packages/web/src/components/wizards/ReviewStep.tsx +++ b/packages/web/src/components/wizards/ReviewStep.tsx @@ -5,7 +5,8 @@ import { type FlatControlBatchContainer, } from '../../lib/control-batch-payload' import { settingsApi } from '../../lib/api/settings'; -import type { ControlDefinition } from '../../lib/api/controls'; +import { getQueryErrorMessage } from '../../ui' +import type { ControlDefinition, CreateBatchWithSpecimensRequest } from '../../lib/api/controls'; import { normalizePosition, groupRowsByDensity } from '../../lib/control-batch-csv' import type { BatchInfo, SpecimenTypeConfig, CSVFileData, CompositionStrains } from '../../pages/ControlBatchWizard' @@ -147,7 +148,7 @@ export default function ReviewStep({ setSubmitting(true) try { - let lastBatchId: number | null = null + const batchPayloads: CreateBatchWithSpecimensRequest[] = [] for (const batchRow of multiBatchRows) { const definitionId = batchRow.candidates.length === 1 ? batchRow.candidates[0]!.id @@ -196,7 +197,7 @@ export default function ReviewStep({ specimenTypeName, containers, })) - const res = await controlsApi.createBatchWithSpecimens({ + batchPayloads.push({ batch: { controlDefinitionId: definitionId, name: batchName, @@ -207,16 +208,19 @@ export default function ReviewStep({ containers: containers.map(flatControlBatchContainerToWriteInput), })), }) - const batchId = res.batch?.id - if (batchId != null) lastBatchId = batchId } - if (lastBatchId != null) onSuccess(lastBatchId) - else { + if (batchPayloads.length === 0) { setError('No batches were created') + return } + // One request: the server creates every batch or none, so a retry cannot duplicate. + const res = await controlsApi.createBatchesWithSpecimens({ batches: batchPayloads }) + const created = res.batches + if (created.length > 0) onSuccess(created[created.length - 1].batch.id) + else setError('No batches were created') } catch (err: unknown) { console.error('Multi-batch submit failed:', err) - setError(err instanceof Error ? err.message : 'Failed to create batches') + setError(getQueryErrorMessage(err, 'Failed to create batches')) } finally { setSubmitting(false) } diff --git a/packages/web/src/components/wizards/__tests__/ReviewStep.test.tsx b/packages/web/src/components/wizards/__tests__/ReviewStep.test.tsx index 051b536c..74da5e26 100644 --- a/packages/web/src/components/wizards/__tests__/ReviewStep.test.tsx +++ b/packages/web/src/components/wizards/__tests__/ReviewStep.test.tsx @@ -5,6 +5,7 @@ import type { BatchInfo, CSVFileData, CompositionStrains } from '../../../pages/ import type { ControlDefinition } from '../../../lib/api/controls' const mockCreateBatchWithSpecimens = vi.fn() +const mockCreateBatchesWithSpecimens = vi.fn() const mockSuggestBatchName = vi.fn() vi.mock('../../../lib/api/controls', async () => { @@ -12,6 +13,7 @@ vi.mock('../../../lib/api/controls', async () => { return createMockedDomainModule('controls', { controlsApi: { createBatchWithSpecimens: (...args: unknown[]) => mockCreateBatchWithSpecimens(...args), + createBatchesWithSpecimens: (...args: unknown[]) => mockCreateBatchesWithSpecimens(...args), suggestBatchName: (...args: unknown[]) => mockSuggestBatchName(...args), }, }) @@ -65,7 +67,7 @@ describe('ReviewStep multi-batch CSV', () => { beforeEach(() => { vi.clearAllMocks() mockSuggestBatchName.mockResolvedValue({ name: 'Suggested Batch' }) - mockCreateBatchWithSpecimens.mockResolvedValue({ batch: { id: 101 }, specimens: [], createdCollections: [] }) + mockCreateBatchesWithSpecimens.mockResolvedValue({ batches: [{ batch: { id: 101 }, specimens: [], createdCollections: [] }] }) }) it('does not call createBatchWithSpecimens when a density has no matching definition', async () => { @@ -107,7 +109,7 @@ describe('ReviewStep multi-batch CSV', () => { expect(submitBtn).toBeDisabled() fireEvent.click(submitBtn) - expect(mockCreateBatchWithSpecimens).not.toHaveBeenCalled() + expect(mockCreateBatchesWithSpecimens).not.toHaveBeenCalled() }) it('calls createBatchWithSpecimens with correct definition IDs when all densities match', async () => { @@ -158,11 +160,13 @@ describe('ReviewStep multi-batch CSV', () => { fireEvent.click(submitBtn) await waitFor(() => { - expect(mockCreateBatchWithSpecimens).toHaveBeenCalledTimes(2) + // Both densities go in one all-or-nothing request. + expect(mockCreateBatchesWithSpecimens).toHaveBeenCalledTimes(1) }) expect(mockSuggestBatchName).toHaveBeenCalled() - const createCalls = mockCreateBatchWithSpecimens.mock.calls - const definitionIds = createCalls.map((c) => c[0].batch.controlDefinitionId) + const batches = mockCreateBatchesWithSpecimens.mock.calls[0][0].batches as Array<{ batch: { controlDefinitionId: number } }> + expect(batches).toHaveLength(2) + const definitionIds = batches.map((b) => b.batch.controlDefinitionId) expect(definitionIds).toContain(10) expect(definitionIds).toContain(11) }) @@ -217,13 +221,43 @@ describe('ReviewStep multi-batch CSV', () => { fireEvent.click(submitBtn) await waitFor(() => { - expect(mockCreateBatchWithSpecimens).toHaveBeenCalledTimes(1) + expect(mockCreateBatchesWithSpecimens).toHaveBeenCalledTimes(1) + }) + expect(mockCreateBatchesWithSpecimens).toHaveBeenCalledWith({ + batches: [expect.objectContaining({ batch: expect.objectContaining({ controlDefinitionId: 21 }) })], }) - expect(mockCreateBatchWithSpecimens).toHaveBeenCalledWith( - expect.objectContaining({ - batch: expect.objectContaining({ controlDefinitionId: 21 }), - }) + }) +}) + +describe('ReviewStep multi-batch CSV errors', () => { + it("shows the server's error instead of the HTTP status text", async () => { + vi.clearAllMocks() + mockSuggestBatchName.mockResolvedValue({ name: 'Suggested Batch' }) + mockCreateBatchesWithSpecimens.mockRejectedValue( + Object.assign(new Error('Request failed with status code 400'), { + response: { data: { error: "Batch 2 ('B'): Specimen type not found. No batches were created." } }, + }), + ) + const def5000: ControlDefinition = { + id: 10, name: 'Def 5000', controlType: 'blood', created: '', lastUpdated: '', targetDensity: 5000, unitSymbol: 'µL', + } + render( + {}} + onCancel={() => {}} + onSuccess={() => {}} + isAddMode={false} + /> ) + + fireEvent.click(screen.getByRole('button', { name: /Create Batch/i })) + + expect(await screen.findByText(/No batches were created/)).toBeInTheDocument() }) }) @@ -233,7 +267,7 @@ describe('ReviewStep multi-batch CSV nested collection placement', () => { beforeEach(() => { vi.clearAllMocks() mockSuggestBatchName.mockResolvedValue({ name: 'Suggested Batch' }) - mockCreateBatchWithSpecimens.mockResolvedValue({ batch: { id: 103 }, specimens: [], createdCollections: [] }) + mockCreateBatchesWithSpecimens.mockResolvedValue({ batches: [{ batch: { id: 103 }, specimens: [], createdCollections: [] }] }) }) it('nests collection.parent when CSV file has collectionName but no collectionId', async () => { @@ -281,9 +315,9 @@ describe('ReviewStep multi-batch CSV nested collection placement', () => { fireEvent.click(submitBtn) await waitFor(() => { - expect(mockCreateBatchWithSpecimens).toHaveBeenCalledTimes(1) + expect(mockCreateBatchesWithSpecimens).toHaveBeenCalledTimes(1) }) - const payload = mockCreateBatchWithSpecimens.mock.calls[0][0] + const payload = mockCreateBatchesWithSpecimens.mock.calls[0][0].batches[0] expect(payload).not.toHaveProperty('createCollections') expect(payload.specimens[0].containers[0]).toEqual({ containerType: 'paper', diff --git a/packages/web/src/lib/api/controls.ts b/packages/web/src/lib/api/controls.ts index ba6522fb..5ca103e9 100644 --- a/packages/web/src/lib/api/controls.ts +++ b/packages/web/src/lib/api/controls.ts @@ -122,6 +122,26 @@ export interface ControlDefinitionSummaryResponse { } } +export type CreateBatchWithSpecimensRequest = { + batch: { + controlDefinitionId: number + name: string + productionDate?: string + properties?: ControlBatchProperties + } + specimens: Array<{ + specimenTypeName: string + collectionDate?: string + containers: ControlBatchContainerWriteInput[] + }> +} + +export type CreateBatchWithSpecimensResponse = { + batch: ControlBatch + specimens: Array<{ id: number; specimenTypeName: string; containerCount: number; containerIds: number[] }> + createdCollections: Array<{ type: string; id: number; name: string }> +} + export const controlsApi = { list: (type?: string) => api.get<{ controls: ControlDefinition[] }>('/blood-controls', { params: { type } }), get: (id: number) => api.get<{ control: ControlDefinition }>(`/blood-controls/${id}`), @@ -143,19 +163,11 @@ export const controlsApi = { api.post<{ name: string }>('/blood-controls/batches/suggest-name', { definitionId, productionDate }), getBatch: (id: number) => api.get<{ batch: ControlBatch }>(`/blood-controls/batches/${id}`), getBatchSummary: (id: number) => api.get(`/blood-controls/batches/${id}/summary`), - createBatchWithSpecimens: (data: { - batch: { - controlDefinitionId: number - name: string - productionDate?: string - properties?: ControlBatchProperties - } - specimens: Array<{ - specimenTypeName: string - collectionDate?: string - containers: ControlBatchContainerWriteInput[] - }> - }) => api.post<{ batch: ControlBatch; specimens: Array<{ id: number; specimenTypeName: string; containerCount: number; containerIds: number[] }>; createdCollections: Array<{ type: string; id: number; name: string }> }>('/blood-controls/batches/create-with-specimens', data), + createBatchWithSpecimens: (data: CreateBatchWithSpecimensRequest) => + api.post('/blood-controls/batches/create-with-specimens', data), + /** Create several batches (one per density) all-or-nothing. */ + createBatchesWithSpecimens: (data: { batches: CreateBatchWithSpecimensRequest[] }) => + api.post<{ batches: CreateBatchWithSpecimensResponse[] }>('/blood-controls/batches/create-many-with-specimens', data), addSpecimensToBatch: (batchId: number, data: { specimens: Array<{ specimenTypeName: string From af9f940b95c2888eea3e3682ef15d112113a03ec Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Fri, 25 Sep 2026 09:47:45 -0700 Subject: [PATCH 12/35] fix(web): sign out on an expired session and drop the cache on user change. When a session expired, every request returned 401 but the user stayed set, so AuthGuard never redirected and each page showed "Could not load" with a Retry that kept failing. A 401 from any non-auth endpoint now clears the user, which sends them to login. The query cache also survived logout, login, and user switch, so the next user briefly saw the previous user's personal export configurations and settings. It is cleared whenever the user changes. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/web/src/contexts/UserContext.tsx | 26 ++++++++++-- .../web/src/lib/api/__tests__/client.test.ts | 42 ++++++++++++++++++- packages/web/src/lib/api/client.ts | 20 +++++++++ 3 files changed, 83 insertions(+), 5 deletions(-) diff --git a/packages/web/src/contexts/UserContext.tsx b/packages/web/src/contexts/UserContext.tsx index 7946c5f3..016a21b7 100644 --- a/packages/web/src/contexts/UserContext.tsx +++ b/packages/web/src/contexts/UserContext.tsx @@ -1,5 +1,7 @@ -import { useContext, useState, useEffect, useMemo, useCallback, ReactNode } from 'react' +import { useContext, useState, useEffect, useMemo, useCallback, useRef, ReactNode } from 'react' +import { useQueryClient } from '@tanstack/react-query' import { authApi, type User } from '../lib/api/auth' +import { setSessionExpiredHandler } from '../lib/api/client' import { addRecentUser } from '../lib/localUserHistory' import { UserContext } from './user-context-instance' @@ -14,12 +16,28 @@ export function UserProvider({ children }: { children: ReactNode }) { const [loading, setLoading] = useState(true) const [error, setError] = useState(null) + const queryClient = useQueryClient() + const currentUserId = useRef(null) + const setUser = useCallback((u: User | null) => { + // Cached queries include per-user data (personal export configs, settings); never show + // one user's cache to the next after a logout, login, or switch. + const nextId = u?.id ?? null + if (currentUserId.current !== null && currentUserId.current !== nextId) { + queryClient.clear() + } + currentUserId.current = nextId setUserState(u) userCachedUser = u userCachedLoading = false userCachedError = null - }, []) + }, [queryClient]) + + // A 401 from a data request means the session is gone: sign out so AuthGuard redirects. + useEffect(() => { + setSessionExpiredHandler(() => setUser(null)) + return () => setSessionExpiredHandler(null) + }, [setUser]) const refreshUser = useCallback(async () => { setLoading(true) @@ -53,7 +71,7 @@ export function UserProvider({ children }: { children: ReactNode }) { setLoading(false) userCachedLoading = false } - }, []) + }, [setUser]) const switchUser = useCallback(async (userId: number, password: string) => { try { @@ -101,7 +119,7 @@ export function UserProvider({ children }: { children: ReactNode }) { // Calling refreshUser() could fail and log the user out even though they're still logged in throw new Error(errorMessage) } - }, []) + }, [setUser]) useEffect(() => { if (!userDidInit) { diff --git a/packages/web/src/lib/api/__tests__/client.test.ts b/packages/web/src/lib/api/__tests__/client.test.ts index 764a9f5c..e3878ce3 100644 --- a/packages/web/src/lib/api/__tests__/client.test.ts +++ b/packages/web/src/lib/api/__tests__/client.test.ts @@ -1,10 +1,11 @@ -import { describe, it, expect, beforeEach, afterEach } from 'vitest' +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest' import type { InternalAxiosRequestConfig } from 'axios' import { axiosApi, getLastResponseRequestId, resetRequestIdStateForTesting, REQUEST_ID_HEADER, + setSessionExpiredHandler, } from '../client' describe('axiosApi request correlation', () => { @@ -89,3 +90,42 @@ function readHeader(headers: unknown, name: string): string | undefined { const record = headers as Record return record[name] ?? record[name.toLowerCase()] } + +describe('session expiry', () => { + const originalAdapter = axiosApi.defaults.adapter + + afterEach(() => { + axiosApi.defaults.adapter = originalAdapter + setSessionExpiredHandler(null) + }) + + function respond401() { + axiosApi.defaults.adapter = async (config: InternalAxiosRequestConfig) => { + const error = Object.assign(new Error('Unauthorized'), { + isAxiosError: true, + config, + response: { status: 401, headers: {}, config, data: { error: 'Invalid or expired session' } }, + }) + throw error + } + } + + it('calls the handler when a data request gets 401', async () => { + const handler = vi.fn() + setSessionExpiredHandler(handler) + respond401() + + await expect(axiosApi.get('/studies')).rejects.toThrow() + expect(handler).toHaveBeenCalledTimes(1) + }) + + it('does not call the handler for auth endpoints (wrong password, not signed in)', async () => { + const handler = vi.fn() + setSessionExpiredHandler(handler) + respond401() + + await expect(axiosApi.post('/auth/login', {})).rejects.toThrow() + await expect(axiosApi.get('/auth/me')).rejects.toThrow() + expect(handler).not.toHaveBeenCalled() + }) +}) diff --git a/packages/web/src/lib/api/client.ts b/packages/web/src/lib/api/client.ts index 27d1ae09..16610488 100644 --- a/packages/web/src/lib/api/client.ts +++ b/packages/web/src/lib/api/client.ts @@ -31,6 +31,22 @@ function readRequestIdHeader(headers: unknown): string | undefined { return undefined } +type SessionExpiredHandler = () => void +let sessionExpiredHandler: SessionExpiredHandler | null = null + +/** + * Called when a non-auth request gets 401 (session expired or revoked). The user context + * registers a handler that signs the user out, so the app redirects to login instead of + * showing "Could not load" everywhere. + */ +export function setSessionExpiredHandler(handler: SessionExpiredHandler | null): void { + sessionExpiredHandler = handler +} + +function isAuthEndpoint(url: string | undefined): boolean { + return !!url && /^\/?auth\//.test(url.replace(/^\/api/, '')) +} + function attachRequestIdInterceptor(instance: AxiosInstance): void { instance.interceptors.request.use((config) => { const headers = config.headers ?? {} @@ -52,6 +68,10 @@ function attachRequestIdInterceptor(instance: AxiosInstance): void { if (axios.isAxiosError(error)) { const id = readRequestIdHeader(error.response?.headers) if (id) lastResponseRequestId = id + // Auth endpoints report their own 401s (wrong password, not logged in yet). + if (error.response?.status === 401 && !isAuthEndpoint(error.config?.url)) { + sessionExpiredHandler?.() + } } return Promise.reject(error) }, From 5a3ceb11e8f7ba15c19ae3ad99a883d64c631a2b Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Fri, 25 Sep 2026 09:49:11 -0700 Subject: [PATCH 13/35] fix(web): refresh after settings and qPCR edits; drop stale responses. - Saving settings only invalidated the settings query, so the Export, qPCR, and move pages kept old export and scanner configurations for up to 5 minutes. A save now marks every query stale. - qPCR well content-type edits updated only local state; revisiting the page within 5 minutes showed the old wells. - Clicking a qPCR well and quickly another could show the first well's container in the second well's panel. - A slow Locations collection search response could overwrite newer results or reopen the dropdown after the box was cleared. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/web/src/pages/Locations.tsx | 10 +++++++++- packages/web/src/pages/QpcrExperimentDetail.tsx | 17 ++++++++++++++++- packages/web/src/pages/Settings.tsx | 6 ++++-- 3 files changed, 29 insertions(+), 4 deletions(-) diff --git a/packages/web/src/pages/Locations.tsx b/packages/web/src/pages/Locations.tsx index 9354ebc5..64da1ed6 100644 --- a/packages/web/src/pages/Locations.tsx +++ b/packages/web/src/pages/Locations.tsx @@ -169,7 +169,12 @@ export default function Locations() { useClickOutside(searchRef, () => setIsSearchOpen(false), isSearchOpen) + // Latest search text; responses for older text are dropped so a slow "PL-1" reply + // cannot overwrite "PL-12" results or reopen the dropdown after the box is cleared. + const latestSearchRef = useRef('') + useEffect(() => { + latestSearchRef.current = search if (search.length >= 1) { const timeoutId = setTimeout(() => { performCollectionSearch(search) @@ -179,6 +184,7 @@ export default function Locations() { } else { setCollectionResults([]) setIsSearchOpen(false) + setSearchLoading(false) } }, [search]) @@ -186,6 +192,7 @@ export default function Locations() { try { setSearchLoading(true) const response = await searchApi.search(searchQuery, 'collection') + if (searchQuery !== latestSearchRef.current) return // Filter to only collection types (micronix_plate, cryovial_box, box, bag) const collectionResults = response.results.filter( (r): r is CollectionSearchResult => @@ -194,10 +201,11 @@ export default function Locations() { setCollectionResults(collectionResults) setIsSearchOpen(true) } catch (error) { + if (searchQuery !== latestSearchRef.current) return console.error('Collection search failed:', error) setCollectionResults([]) } finally { - setSearchLoading(false) + if (searchQuery === latestSearchRef.current) setSearchLoading(false) } } diff --git a/packages/web/src/pages/QpcrExperimentDetail.tsx b/packages/web/src/pages/QpcrExperimentDetail.tsx index d538a8cb..cf1d1c2f 100644 --- a/packages/web/src/pages/QpcrExperimentDetail.tsx +++ b/packages/web/src/pages/QpcrExperimentDetail.tsx @@ -204,24 +204,33 @@ export default function QpcrExperimentDetail() { const well = data.wells.find((w) => w.wellPosition === selectedWellPosition) const containerId = well?.storageContainerId setWellDetailsError(null) + // Ignore a response that arrives after the user picked another well. + let ignore = false if (containerId != null) { setWellDetailsLoading(true) setWellDetails(null) containersApi .get(containerId) .then((res) => { + if (ignore) return setWellDetails(res) setWellDetailsError(null) }) .catch((err: { response?: { data?: { error?: string } } }) => { + if (ignore) return setWellDetailsError(err.response?.data?.error ?? 'Failed to load container details') setWellDetails(null) }) - .finally(() => setWellDetailsLoading(false)) + .finally(() => { + if (!ignore) setWellDetailsLoading(false) + }) } else { setWellDetails(null) setWellDetailsLoading(false) } + return () => { + ignore = true + } }, [selectedWellPosition, data?.wells]) if (!id || Number.isNaN(experimentId)) { @@ -432,6 +441,8 @@ export default function QpcrExperimentDetail() { try { const res = await qpcrExperimentsApi.updateWells(parseInt(id), { wellPosition: selectedWellPosition, contentType }) setData((prev) => (prev ? { ...prev, wells: res.wells } : null)) + // Keep the cached experiment in step, or revisiting the page shows the old wells. + invalidateQpcrExperimentQueries(queryClient) showSuccess(contentType === 'negative' ? 'Well set as NTC' : 'Well set as empty') } catch (err: unknown) { const msg = err && typeof err === 'object' && 'response' in err && typeof (err as { response?: { data?: { error?: string } } }).response?.data?.error === 'string' @@ -449,6 +460,8 @@ export default function QpcrExperimentDetail() { try { const res = await qpcrExperimentsApi.updateWells(parseInt(id), { positions: emptyWellPositions, contentType: 'negative' }) setData((prev) => (prev ? { ...prev, wells: res.wells } : null)) + // Keep the cached experiment in step, or revisiting the page shows the old wells. + invalidateQpcrExperimentQueries(queryClient) showSuccess(`Set ${emptyWellPositions.length} well(s) to NTC`) } catch (err: unknown) { const msg = err && typeof err === 'object' && 'response' in err && typeof (err as { response?: { data?: { error?: string } } }).response?.data?.error === 'string' @@ -466,6 +479,8 @@ export default function QpcrExperimentDetail() { try { const res = await qpcrExperimentsApi.updateWells(parseInt(id), { positions: ntcWellPositions, contentType: 'empty' }) setData((prev) => (prev ? { ...prev, wells: res.wells } : null)) + // Keep the cached experiment in step, or revisiting the page shows the old wells. + invalidateQpcrExperimentQueries(queryClient) showSuccess(`Set ${ntcWellPositions.length} well(s) to empty`) } catch (err: unknown) { const msg = err && typeof err === 'object' && 'response' in err && typeof (err as { response?: { data?: { error?: string } } }).response?.data?.error === 'string' diff --git a/packages/web/src/pages/Settings.tsx b/packages/web/src/pages/Settings.tsx index e481abb7..fe255116 100644 --- a/packages/web/src/pages/Settings.tsx +++ b/packages/web/src/pages/Settings.tsx @@ -3,7 +3,7 @@ import { useSearchParams } from 'react-router-dom' import { useQueryClient } from '@tanstack/react-query' import type { AllSettings } from '../lib/api/settings' import { useUser } from '../contexts/UserContext' -import { settingsKeys, useAllSettings } from '../hooks/useSettings' +import { useAllSettings } from '../hooks/useSettings' import { PageError, fromQuery, getQueryErrorMessage } from '../ui' import { useTheme, THEME_IDS, THEME_LABELS } from '../contexts/ThemeContext' import InfoTooltip from '../components/InfoTooltip' @@ -246,8 +246,10 @@ export default function Settings() { const [mutationError, setMutationError] = useState(null) const [success, setSuccess] = useState(null) + // Settings feed many other queries (export and scanner configurations, move page + // bootstraps, container defaults, table views), so a save marks every query stale. const refreshSettings = () => { - void queryClient.invalidateQueries({ queryKey: settingsKeys.allSettings() }) + void queryClient.invalidateQueries() } const handleSave = async () => { From dd18f03c7e282a5b9c7c4b7f96a953f81fa12058 Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Fri, 25 Sep 2026 09:50:07 -0700 Subject: [PATCH 14/35] fix(web): parse bulk import and scanner CSVs with quotes. The bulk import parser, the import preview, and the scanner plate parser split lines on raw commas, so a quoted comment like "low vol, hemolyzed" shifted every later column, and quoted scanner exports kept the quote characters. They now use the contract's RFC 4180 parser, like the derivation and control batch imports. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/web/src/components/BulkImportFlow.tsx | 7 ++++--- .../web/src/lib/__tests__/bulk-import-validation.test.ts | 5 +++++ packages/web/src/lib/__tests__/scanner-plate-csv.test.ts | 5 +++++ packages/web/src/lib/bulk-import-validation.ts | 9 +++++---- packages/web/src/lib/scanner-plate-csv.ts | 8 ++++---- 5 files changed, 23 insertions(+), 11 deletions(-) diff --git a/packages/web/src/components/BulkImportFlow.tsx b/packages/web/src/components/BulkImportFlow.tsx index 6a51cd45..8bbcd382 100644 --- a/packages/web/src/components/BulkImportFlow.tsx +++ b/packages/web/src/components/BulkImportFlow.tsx @@ -30,6 +30,7 @@ import { } from '../lib/bulk-import-success-message' import LocationPicker from './LocationPicker' import '../styles/storage.css' +import { parseCsv } from '@sampledb/contract' export type ImportType = LibImportType type Step = 'upload' | 'collections' | 'import' @@ -136,14 +137,14 @@ export default function BulkImportFlow({ fixedStudyShortCode, backLink }: BulkIm const reader = new FileReader() reader.onload = (event) => { const text = event.target?.result as string - const lines = text.split('\n').filter(line => line.trim()) + const lines = parseCsv(text).filter((cells) => cells.some((cell) => cell.trim() !== '')) if (lines.length === 0) return - const headers = lines[0].split(',').map(h => h.trim()) + const headers = lines[0].map(h => h.trim()) const previewRows: CSVRow[] = [] for (let i = 1; i < Math.min(6, lines.length); i++) { - const values = lines[i].split(',') + const values = lines[i] const row: CSVRow = {} headers.forEach((header, j) => { row[header] = values[j]?.trim() || '' diff --git a/packages/web/src/lib/__tests__/bulk-import-validation.test.ts b/packages/web/src/lib/__tests__/bulk-import-validation.test.ts index 01e8c5a7..94110996 100644 --- a/packages/web/src/lib/__tests__/bulk-import-validation.test.ts +++ b/packages/web/src/lib/__tests__/bulk-import-validation.test.ts @@ -75,6 +75,11 @@ describe('bulk-import-validation', () => { expect(rows[0].position).toBe('A01') expect(rows[0].study_short_code).toBe('ST1') }) + it('keeps quoted commas inside a cell and handles CRLF', () => { + const csv = 'subject_name,comment,position\r\nS1,"low vol, hemolyzed",A01\r\n' + const rows = parseBulkImportCSV(csv) + expect(rows).toEqual([{ subject_name: 'S1', comment: 'low vol, hemolyzed', position: 'A01' }]) + }) }) describe('mapBulkImportRowsToPayload', () => { diff --git a/packages/web/src/lib/__tests__/scanner-plate-csv.test.ts b/packages/web/src/lib/__tests__/scanner-plate-csv.test.ts index 90654966..e558f79f 100644 --- a/packages/web/src/lib/__tests__/scanner-plate-csv.test.ts +++ b/packages/web/src/lib/__tests__/scanner-plate-csv.test.ts @@ -42,6 +42,11 @@ describe('parseScannerPlateCsv', () => { expect(rows[1]).toMatchObject({ container_barcode: '', target_position: 'B02' }) }) + it('reads quoted scanner exports', () => { + const rows = parseScannerPlateCsv('"container_barcode","target_position"\r\n"MTX1","A01"', singleConfig) + expect(rows[0]).toMatchObject({ container_barcode: 'MTX1', target_position: 'A01' }) + }) + it('returns no rows for an empty or header-only file', () => { expect(parseScannerPlateCsv('', singleConfig)).toEqual([]) expect(parseScannerPlateCsv('container_barcode,target_position', singleConfig)).toEqual([]) diff --git a/packages/web/src/lib/bulk-import-validation.ts b/packages/web/src/lib/bulk-import-validation.ts index cb496e8c..4fed1cbc 100644 --- a/packages/web/src/lib/bulk-import-validation.ts +++ b/packages/web/src/lib/bulk-import-validation.ts @@ -5,6 +5,7 @@ import type { ContainerType } from './container-types' import { getCollectionNameColumn } from './container-columns' import type { FlatBulkImportContainer } from './bulk-import-payload' +import { parseCsv } from '@sampledb/contract' export interface CSVRow { [key: string]: string @@ -81,15 +82,15 @@ function normalizeHeader(header: string): string { } export function parseBulkImportCSV(text: string): CSVRow[] { - const lines = text.split('\n').filter((line) => line.trim()) + // Shared RFC 4180 parser: quoted commas ("low vol, hemolyzed"), quotes, CRLF, and BOM. + const lines = parseCsv(text).filter((cells) => cells.some((cell) => cell.trim() !== '')) if (lines.length < 2) return [] - const rawHeaders = lines[0].split(',').map((h) => h.trim()) - const headers = rawHeaders.map(normalizeHeader) + const headers = lines[0].map((h) => normalizeHeader(h.trim())) const rows: CSVRow[] = [] for (let i = 1; i < lines.length; i++) { - const values = lines[i].split(',') + const values = lines[i] const row: CSVRow = {} headers.forEach((header, j) => { row[header] = values[j]?.trim() ?? '' diff --git a/packages/web/src/lib/scanner-plate-csv.ts b/packages/web/src/lib/scanner-plate-csv.ts index f6dcefa5..90750852 100644 --- a/packages/web/src/lib/scanner-plate-csv.ts +++ b/packages/web/src/lib/scanner-plate-csv.ts @@ -1,5 +1,6 @@ import type { ScannerConfiguration } from './api/settings' import { normalizeWellPosition, validateFullPlatePositions } from './micronix-plate-positions' +import { parseCsv } from '@sampledb/contract' export type ScannerPlateCsvRow = Record @@ -17,15 +18,14 @@ function buildPosition(config: ScannerConfiguration, row: ScannerPlateCsvRow): s * Parse a scanner plate CSV using column mapping from scanner configuration. */ export function parseScannerPlateCsv(text: string, config: ScannerConfiguration): ScannerPlateCsvRow[] { - const lines = text.split('\n').filter((line) => line.trim()) + const lines = parseCsv(text).filter((cells) => cells.some((cell) => cell.trim() !== '')) if (lines.length < 2 + config.skipRows) return [] - const headerLine = lines[config.skipRows] - const headers = headerLine.split(',').map((h) => h.trim()) + const headers = lines[config.skipRows].map((h) => h.trim()) const rows: ScannerPlateCsvRow[] = [] for (let i = config.skipRows + 1; i < lines.length; i++) { - const values = lines[i].split(',') + const values = lines[i] const row: ScannerPlateCsvRow = {} headers.forEach((header, j) => { row[header] = values[j]?.trim() || '' From c98a782b07585833a4d99a221a1d0146e4069eeb Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Fri, 25 Sep 2026 09:51:37 -0700 Subject: [PATCH 15/35] fix(web): show and default calendar dates in local time. new Date('2024-01-15') is UTC midnight, so collection and derivation dates rendered a day early in the Americas. "Today" defaults used toISOString(), which is tomorrow's date after about 5pm Pacific, and that date was saved as the derivation or production date. Bare YYYY-MM-DD values are now parsed as local calendar dates, and today's date comes from the local clock. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../components/ContainerDerivationModal.tsx | 3 ++- .../src/components/DerivationChainView.tsx | 3 ++- packages/web/src/components/SpecimenCard.tsx | 3 ++- packages/web/src/components/StudyCard.tsx | 3 ++- .../src/components/wizards/BatchInfoStep.tsx | 3 ++- .../web/src/components/wizards/ReviewStep.tsx | 3 ++- packages/web/src/hooks/useControls.ts | 3 ++- .../web/src/lib/__tests__/date-utils.test.ts | 19 +++++++++++++ packages/web/src/lib/date-utils.ts | 27 +++++++++++++++++-- packages/web/src/lib/template-generator.ts | 3 ++- packages/web/src/pages/ControlBatchWizard.tsx | 5 ++-- packages/web/src/pages/Statistics.tsx | 3 ++- 12 files changed, 65 insertions(+), 13 deletions(-) diff --git a/packages/web/src/components/ContainerDerivationModal.tsx b/packages/web/src/components/ContainerDerivationModal.tsx index f3fe94e1..c44236a0 100644 --- a/packages/web/src/components/ContainerDerivationModal.tsx +++ b/packages/web/src/components/ContainerDerivationModal.tsx @@ -13,6 +13,7 @@ import CollectionSelectOrCreate from './CollectionSelectOrCreate' import { Modal } from '../ui' import { DERIVATION_TYPES } from '../lib/derivation-types' import '../styles/storage.css' +import { localToday } from '../lib/date-utils' interface ContainerDerivationModalProps { isOpen: boolean @@ -67,7 +68,7 @@ function ContainerDerivationModalContent({ unitSymbol: '', quantityUsed: undefined as number | undefined, reduceParentQuantity: true, - derivationDate: new Date().toISOString().split('T')[0], + derivationDate: localToday(), protocol: '', notes: '', properties: undefined as Record | undefined, diff --git a/packages/web/src/components/DerivationChainView.tsx b/packages/web/src/components/DerivationChainView.tsx index f814cdf5..ebade47b 100644 --- a/packages/web/src/components/DerivationChainView.tsx +++ b/packages/web/src/components/DerivationChainView.tsx @@ -6,6 +6,7 @@ import type { Derivation } from '../lib/api/derivations' import { containerDisplayIdentifier } from '../lib/container-display' import { formatDerivationType } from '../lib/derivation-types' import { getContainerTypeIcon, getContainerTypeName } from '../lib/icons' +import { parseDisplayDate } from '../lib/date-utils' interface DerivationChainViewProps { containerId: number @@ -190,7 +191,7 @@ export default function DerivationChainView({ containerId, onClose }: Derivation {renderContainerIdentifier(item.container)} {item.derivation.derivationDate && (
- {new Date(item.derivation.derivationDate).toLocaleDateString()} + {parseDisplayDate(item.derivation.derivationDate).toLocaleDateString()}
)} diff --git a/packages/web/src/components/SpecimenCard.tsx b/packages/web/src/components/SpecimenCard.tsx index 96968404..421295c1 100644 --- a/packages/web/src/components/SpecimenCard.tsx +++ b/packages/web/src/components/SpecimenCard.tsx @@ -1,5 +1,6 @@ import { Link } from 'react-router-dom' import { getSpecimenTypeIcon, getContainerTypeIcon, getContainerTypeName } from '../lib/icons' +import { parseDisplayDate } from '../lib/date-utils' import type { SubjectSummarySpecimen } from '../lib/api/subjects';interface SpecimenCardProps { specimen: SubjectSummarySpecimen } @@ -7,7 +8,7 @@ import type { SubjectSummarySpecimen } from '../lib/api/subjects';interface Spec export default function SpecimenCard({ specimen }: SpecimenCardProps) { const formatDate = (dateString?: string) => { if (!dateString) return null - return new Date(dateString).toLocaleDateString() + return parseDisplayDate(dateString).toLocaleDateString() } const formatContainerBreakdown = () => { diff --git a/packages/web/src/components/StudyCard.tsx b/packages/web/src/components/StudyCard.tsx index c365e8ea..3dbdc5d0 100644 --- a/packages/web/src/components/StudyCard.tsx +++ b/packages/web/src/components/StudyCard.tsx @@ -1,5 +1,6 @@ import { useState } from 'react' import { Link } from 'react-router-dom' +import { parseDisplayDate } from '../lib/date-utils' import type { Study, StudySummaryBasic } from '../lib/api/studies';interface StudySummaryData extends StudySummaryBasic { averageSpecimensPerSubject?: number studyDurationDays?: number | null @@ -59,7 +60,7 @@ export default function StudyCard({ study, summary, loading, onLoadSummary, vari const [expanded, setExpanded] = useState(false) const formatDate = (dateString: string) => { - return new Date(dateString).toLocaleDateString('en-US', { + return parseDisplayDate(dateString).toLocaleDateString('en-US', { month: 'short', day: 'numeric', year: 'numeric' diff --git a/packages/web/src/components/wizards/BatchInfoStep.tsx b/packages/web/src/components/wizards/BatchInfoStep.tsx index 916abfcc..13556866 100644 --- a/packages/web/src/components/wizards/BatchInfoStep.tsx +++ b/packages/web/src/components/wizards/BatchInfoStep.tsx @@ -8,6 +8,7 @@ import { useStrains } from '../../hooks/useReferenceData' import { PageError, SectionMessage, getQueryErrorMessage } from '../../ui' import { Modal } from '../../ui' import ControlDefinitionForm from '../forms/ControlDefinitionForm' +import { localToday } from '../../lib/date-utils' const STRAIN_BAR_COLORS = [ 'rgb(var(--app-accent))', @@ -133,7 +134,7 @@ export default function BatchInfoStep({ suggestedName = response.name setNameSuggestion(suggestedName) } catch { - const date = batchInfo.productionDate || new Date().toISOString().split('T')[0] + const date = batchInfo.productionDate || localToday() suggestedName = `${definition.name} ${date}` setNameSuggestion(null) } diff --git a/packages/web/src/components/wizards/ReviewStep.tsx b/packages/web/src/components/wizards/ReviewStep.tsx index 34e428fe..177f3656 100644 --- a/packages/web/src/components/wizards/ReviewStep.tsx +++ b/packages/web/src/components/wizards/ReviewStep.tsx @@ -9,6 +9,7 @@ import { getQueryErrorMessage } from '../../ui' import type { ControlDefinition, CreateBatchWithSpecimensRequest } from '../../lib/api/controls'; import { normalizePosition, groupRowsByDensity } from '../../lib/control-batch-csv' import type { BatchInfo, SpecimenTypeConfig, CSVFileData, CompositionStrains } from '../../pages/ControlBatchWizard' +import { localToday } from '../../lib/date-utils' /** Row key for batch definition selection: fileIndex + densityKey (numeric density for matching). */ function batchRowKey(fileIndex: number, densityKey: number | undefined): string { @@ -122,7 +123,7 @@ export default function ReviewStep({ const handleMultiBatchSubmit = async () => { setError(null) - const productionDate = batchInfo.productionDate || new Date().toISOString().split('T')[0] + const productionDate = batchInfo.productionDate || localToday() for (const file of csvFiles) { const hasCollection = diff --git a/packages/web/src/hooks/useControls.ts b/packages/web/src/hooks/useControls.ts index cfde3518..080ea730 100644 --- a/packages/web/src/hooks/useControls.ts +++ b/packages/web/src/hooks/useControls.ts @@ -3,6 +3,7 @@ import { controlsApi, type ControlBatch, type ControlDefinition } from '../lib/a 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' export const controlKeys = { all: ['blood-controls'] as const, @@ -113,7 +114,7 @@ export function useControlDefinitionWizardSeed(definitionId: number | undefined) return useQuery({ queryKey: [...controlKeys.all, 'wizard-definition-seed', definitionId] as const, queryFn: async () => { - const today = new Date().toISOString().split('T')[0] + const today = localToday() const [summaryResponse, nameResponse] = await Promise.all([ controlsApi.getDefinitionSummary(definitionId!), controlsApi.suggestBatchName(definitionId!, today), diff --git a/packages/web/src/lib/__tests__/date-utils.test.ts b/packages/web/src/lib/__tests__/date-utils.test.ts index 127da2f4..19f7742d 100644 --- a/packages/web/src/lib/__tests__/date-utils.test.ts +++ b/packages/web/src/lib/__tests__/date-utils.test.ts @@ -72,3 +72,22 @@ describe('date-utils', () => { }) }) }) + +describe('calendar dates in local time', () => { + it('parseDisplayDate keeps a bare YYYY-MM-DD on its calendar day', async () => { + const { parseDisplayDate } = await import('../date-utils') + const d = parseDisplayDate('2024-01-15') + expect([d.getFullYear(), d.getMonth(), d.getDate()]).toEqual([2024, 0, 15]) + }) + + it('formatDate shows a collection date on its own day', async () => { + const { formatDate } = await import('../date-utils') + expect(formatDate('2024-01-15')).toBe('Jan 15, 2024') + }) + + it('toLocalDateString uses the local date, not the UTC date', async () => { + const { toLocalDateString } = await import('../date-utils') + // 11pm local on Jan 15 is already Jan 16 in UTC for any zone west of UTC. + expect(toLocalDateString(new Date(2024, 0, 15, 23, 30))).toBe('2024-01-15') + }) +}) diff --git a/packages/web/src/lib/date-utils.ts b/packages/web/src/lib/date-utils.ts index 854947e0..5783f089 100644 --- a/packages/web/src/lib/date-utils.ts +++ b/packages/web/src/lib/date-utils.ts @@ -1,3 +1,26 @@ +/** + * Parse a date for display. A bare YYYY-MM-DD is a calendar date, so it is built in local + * time: new Date('2024-01-15') is UTC midnight, which renders as Jan 14 in the Americas. + */ +export function parseDisplayDate(value: string): Date { + const match = /^(\d{4})-(\d{2})-(\d{2})$/.exec(value) + if (match) return new Date(Number(match[1]), Number(match[2]) - 1, Number(match[3])) + return new Date(value) +} + +/** A Date as a local YYYY-MM-DD calendar date (toISOString() gives the UTC date). */ +export function toLocalDateString(date: Date): string { + const year = date.getFullYear() + const month = String(date.getMonth() + 1).padStart(2, '0') + const day = String(date.getDate()).padStart(2, '0') + return `${year}-${month}-${day}` +} + +/** Today's local calendar date as YYYY-MM-DD, for date input defaults. */ +export function localToday(): string { + return toLocalDateString(new Date()) +} + /** * Formats a date string with relative time for recent dates and absolute dates for older items * @param dateString - ISO date string @@ -6,7 +29,7 @@ export function formatDateWithRelativeTime(dateString: string | null | undefined): string { if (!dateString) return 'N/A' - const date = new Date(dateString) + const date = parseDisplayDate(dateString) const now = new Date() const diffMs = now.getTime() - date.getTime() const diffSeconds = Math.floor(diffMs / 1000) @@ -40,7 +63,7 @@ export function formatDateWithRelativeTime(dateString: string | null | undefined export function formatDate(dateString: string | null | undefined): string { if (!dateString) return 'N/A' - const date = new Date(dateString) + const date = parseDisplayDate(dateString) return date.toLocaleDateString('en-US', { month: 'short', day: 'numeric', diff --git a/packages/web/src/lib/template-generator.ts b/packages/web/src/lib/template-generator.ts index 4ba5e527..de6b8669 100644 --- a/packages/web/src/lib/template-generator.ts +++ b/packages/web/src/lib/template-generator.ts @@ -1,6 +1,7 @@ import type { BulkDerivationSettings } from './api/derivations' import { buildCsv } from './csv' import { getCollectionNameColumn } from './container-columns' +import { localToday } from './date-utils' export interface TemplateOptions { parentType: 'barcode' | 'control_batch' | 'study_subject' | 'cryovial_position' @@ -83,7 +84,7 @@ export function generateDerivationsTemplate(options: TemplateOptions): string { if (parentColumns.includes('specimen_type_name')) defaultDerivationPerRow.specimen_type_name = derivedExampleSpecimenType if (parentColumns.includes('container_type')) defaultDerivationPerRow.container_type = settings.containerType || 'micronix_tube' if (parentColumns.includes('protocol')) defaultDerivationPerRow.protocol = exampleProtocolVal - if (parentColumns.includes('derivation_date')) defaultDerivationPerRow.derivation_date = new Date().toISOString().split('T')[0] + if (parentColumns.includes('derivation_date')) defaultDerivationPerRow.derivation_date = localToday() if (parentColumns.includes('quantity')) defaultDerivationPerRow.quantity = '1' if (parentColumns.includes('quantity_used')) defaultDerivationPerRow.quantity_used = '1' if (parentColumns.includes('reduce_parent_quantity')) defaultDerivationPerRow.reduce_parent_quantity = 'true' diff --git a/packages/web/src/pages/ControlBatchWizard.tsx b/packages/web/src/pages/ControlBatchWizard.tsx index 97198282..dfd475fc 100644 --- a/packages/web/src/pages/ControlBatchWizard.tsx +++ b/packages/web/src/pages/ControlBatchWizard.tsx @@ -17,6 +17,7 @@ import { import { useUser } from '../contexts/UserContext' import { PageError, fromQuery, getQueryErrorMessage } from '../ui' import '../styles/blood-controls.css' +import { localToday } from '../lib/date-utils' /** Strains for composition (used for multi-batch CSV flow). */ export type CompositionStrains = Array<{ id: number; percentage?: number }> @@ -136,7 +137,7 @@ export default function ControlBatchWizard() { controlDefinitionId: null, controlDefinition: null, name: '', - productionDate: new Date().toISOString().split('T')[0], + productionDate: localToday(), }) const [compositionStrains, setCompositionStrains] = useState(null) @@ -165,7 +166,7 @@ export default function ControlBatchWizard() { controlDefinitionId: data.batch.controlDefinitionId, controlDefinition: data.controlDefinition, name: data.batch.name, - productionDate: data.batch.productionDate || new Date().toISOString().split('T')[0], + productionDate: data.batch.productionDate || localToday(), properties: data.batch.properties, }) }, [batchBootstrapQuery.data]) diff --git a/packages/web/src/pages/Statistics.tsx b/packages/web/src/pages/Statistics.tsx index f4608750..dff8ca35 100644 --- a/packages/web/src/pages/Statistics.tsx +++ b/packages/web/src/pages/Statistics.tsx @@ -8,6 +8,7 @@ import { SkeletonCard } from '../ui' import { useStatistics } from '../hooks/useStatistics' import { PageError, fromQuery, getQueryErrorMessage } from '../ui' import '../styles/statistics.css' +import { toLocalDateString } from '../lib/date-utils' type BinSize = 'day' | 'week' | 'month' | 'quarter' | 'year' @@ -25,7 +26,7 @@ const TIME_PRESETS = [ function formatDateOffset(daysAgo: number): string { const d = new Date() d.setDate(d.getDate() - daysAgo) - return d.toISOString().slice(0, 10) + return toLocalDateString(d) } /** Build human-readable filter chip labels from applied filters. */ From e0047beb47364b16ec96b4c7e8e21145e391e1f2 Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Fri, 25 Sep 2026 09:52:37 -0700 Subject: [PATCH 16/35] fix(containers): save a cleared container comment. ContainerEditModal sent `formData.comment || undefined`, so an emptied comment was dropped from the JSON body, the server changed nothing, and the modal closed as if it had saved. It now sends '', which the API stores as NULL. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../src/routes/__tests__/containers.test.ts | 25 +++++++++++++++++++ packages/api/src/routes/containers.ts | 4 ++- .../web/src/components/ContainerEditModal.tsx | 3 ++- 3 files changed, 30 insertions(+), 2 deletions(-) diff --git a/packages/api/src/routes/__tests__/containers.test.ts b/packages/api/src/routes/__tests__/containers.test.ts index 9b71d64e..b9bf89dd 100644 --- a/packages/api/src/routes/__tests__/containers.test.ts +++ b/packages/api/src/routes/__tests__/containers.test.ts @@ -201,6 +201,31 @@ describe('Containers API', () => { expect(row?.barcode).toBe('NEW-CRYO-1') }) + it('clears the comment when it is emptied', async () => { + const now = utcNow() + const [boxRecord] = await ctx.db + .insert(cryovialBox) + .values({ name: `BOX-CMT-${Date.now()}`, locationId: testLocation.id, created: now, lastUpdated: now }) + .returning() + const testStudy = await createTestStudy(ctx.db, { title: 'C Study', shortCode: 'CST' + Date.now() }) + const subj = await createTestStudySubject(ctx.db, { studyId: testStudy.id, name: 'S1' }) + const [spec] = await ctx.db + .insert(specimen) + .values({ studySubjectId: subj.id, specimenTypeId: testSpecimenType.id, created: now, lastUpdated: now }) + .returning() + const [container] = await ctx.db + .insert(storageContainer) + .values({ specimenId: spec.id, unitId: testUnit.id, totalQuantity: 1, remainingQuantity: 1, comment: 'hemolyzed', created: now, lastUpdated: now }) + .returning() + await ctx.db.insert(cryovialTube).values({ id: container.id, collectionId: boxRecord.id, barcode: 'CMT-1', position: 'A01' }) + + const res = await ctx.request(`/api/containers/${container.id}`, { method: 'PATCH', json: { comment: '' } }) + + expect(res.status).toBe(200) + const row = await ctx.db.select().from(storageContainer).where(eq(storageContainer.id, container.id)).get() + expect(row?.comment).toBeNull() + }) + it('returns 400 when cryovial_tube barcode is already in use', async () => { const now = utcNow() const [boxRecord] = await ctx.db diff --git a/packages/api/src/routes/containers.ts b/packages/api/src/routes/containers.ts index bbeb7787..b20a905b 100644 --- a/packages/api/src/routes/containers.ts +++ b/packages/api/src/routes/containers.ts @@ -160,7 +160,9 @@ containers.patch('/:id', memberMiddleware, async (c) => { const data = schema.parse(body) const { tagIds, unitId, barcode: rawBarcode, sublabel: rawSublabel, ...restData } = data - const updateData: { comment?: string; remainingQuantity?: number; unitId?: number } = { ...restData } + const updateData: { comment?: string | null; remainingQuantity?: number; unitId?: number } = { ...restData } + // An emptied comment clears it. + if (restData.comment !== undefined && restData.comment.trim() === '') updateData.comment = null const [container, micronixInfo, cryovialInfo, paperInfo, staticWellInfo] = await Promise.all([ database.select().from(storageContainer).where(eq(storageContainer.id, id)).get(), diff --git a/packages/web/src/components/ContainerEditModal.tsx b/packages/web/src/components/ContainerEditModal.tsx index 81958f17..d600d54f 100644 --- a/packages/web/src/components/ContainerEditModal.tsx +++ b/packages/web/src/components/ContainerEditModal.tsx @@ -105,7 +105,8 @@ function ContainerEditModalForm({ // Only include fields that have changed if (formData.comment !== (container.comment || '')) { - updateData.comment = formData.comment || undefined + // Send '' rather than undefined, which JSON drops, so clearing the comment is saved. + updateData.comment = formData.comment } // Check if remainingQuantity changed From 00025187be145e7c95cdb8416b6fa97e19a5bbeb Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Fri, 25 Sep 2026 09:53:44 -0700 Subject: [PATCH 17/35] fix(web): keep a scanner Enter from submitting the collection entry grid. In Add Specimens to Collection, a barcode scanner's trailing Enter submitted the form, rows filled so far were created, half-filled rows were skipped silently, and a 400 showed only "Validation failed". Enter in a field no longer submits, partly filled positions are listed as errors, and per-row server errors are shown against their positions. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../components/CollectionSpecimenEntry.tsx | 37 +++++++++++- .../CollectionSpecimenEntry.test.tsx | 57 +++++++++++++++++++ 2 files changed, 91 insertions(+), 3 deletions(-) create mode 100644 packages/web/src/components/__tests__/CollectionSpecimenEntry.test.tsx diff --git a/packages/web/src/components/CollectionSpecimenEntry.tsx b/packages/web/src/components/CollectionSpecimenEntry.tsx index a5fc5e39..e883a7dd 100644 --- a/packages/web/src/components/CollectionSpecimenEntry.tsx +++ b/packages/web/src/components/CollectionSpecimenEntry.tsx @@ -109,13 +109,25 @@ export default function CollectionSpecimenEntry({ try { const validEntries: Array = [] - + const incompletePositions: string[] = [] + for (const [position, entry] of entries.entries()) { if (entry.studyShortCode && entry.subjectName && entry.specimenTypeName) { validEntries.push({ ...entry, position }) + } else if (entry.subjectName || entry.barcode || entry.collectionDate) { + // Started but missing study, subject, or specimen type: say so instead of skipping it. + incompletePositions.push(position) } } + if (incompletePositions.length > 0) { + setWorkflowError( + `Complete or clear these positions (study, subject, and specimen type are required): ${incompletePositions.join(', ')}`, + ) + setSubmitting(false) + return + } + if (validEntries.length === 0) { setWorkflowError('Please fill in at least one specimen entry') setSubmitting(false) @@ -144,7 +156,19 @@ export default function CollectionSpecimenEntry({ onSuccess?.() } } catch (err: unknown) { - setWorkflowError(getQueryErrorMessage(err, 'Failed to create specimens')) + // A 400 carries per-row errors; show them against their positions. + const rowErrors = (err as { response?: { data?: { errors?: Array<{ index: number; error: string }> } } }) + .response?.data?.errors + if (rowErrors && rowErrors.length > 0) { + const positionsByIndex = [...entries.entries()] + .filter(([, entry]) => entry.studyShortCode && entry.subjectName && entry.specimenTypeName) + .map(([position]) => position) + setWorkflowError( + rowErrors.map((e) => `${positionsByIndex[e.index] ?? `Row ${e.index + 1}`}: ${e.error}`).join('; '), + ) + } else { + setWorkflowError(getQueryErrorMessage(err, 'Failed to create specimens')) + } } finally { setSubmitting(false) } @@ -178,7 +202,14 @@ export default function CollectionSpecimenEntry({ )} -
+ { + if (e.key === 'Enter' && e.target instanceof HTMLInputElement) e.preventDefault() + }} + className="space-y-4" + >
diff --git a/packages/web/src/components/__tests__/CollectionSpecimenEntry.test.tsx b/packages/web/src/components/__tests__/CollectionSpecimenEntry.test.tsx new file mode 100644 index 00000000..0ebf44b1 --- /dev/null +++ b/packages/web/src/components/__tests__/CollectionSpecimenEntry.test.tsx @@ -0,0 +1,57 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest' +import { render, screen, fireEvent } from '@testing-library/react' +import CollectionSpecimenEntry from '../CollectionSpecimenEntry' +import { specimensApi } from '../../lib/api/specimens' + +vi.mock('../../hooks/useStudies', () => ({ + useStudies: () => ({ + data: { studies: [{ id: 1, shortCode: 'ST1', title: 'Study 1' }] }, + isLoading: false, + isError: false, + }), +})) +vi.mock('../../hooks/useReferenceData', () => ({ + useSpecimenTypes: () => ({ data: [{ id: 1, name: 'DNA' }], isLoading: false, isError: false }), +})) +vi.mock('../../lib/api/specimens', () => ({ + specimensApi: { createBulk: vi.fn() }, +})) + +function renderEntry() { + return render( + , + ) +} + +describe('CollectionSpecimenEntry', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it('does not submit when a barcode scanner sends Enter', () => { + renderEntry() + const barcode = screen.getAllByPlaceholderText('Barcode')[0] + fireEvent.change(barcode, { target: { value: 'MT001' } }) + + const notPrevented = fireEvent.keyDown(barcode, { key: 'Enter' }) + + expect(notPrevented).toBe(false) + expect(specimensApi.createBulk).not.toHaveBeenCalled() + }) + + it('reports partly filled positions instead of skipping them', async () => { + renderEntry() + fireEvent.change(screen.getAllByPlaceholderText('Subject name')[1], { target: { value: 'S2' } }) + + fireEvent.click(screen.getByRole('button', { name: /create|add|save/i })) + + expect(await screen.findByText(/Complete or clear these positions.*A02/)).toBeInTheDocument() + expect(specimensApi.createBulk).not.toHaveBeenCalled() + }) +}) From dd4ff424585ce0b37872a570b81a559af9aba2ce Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Fri, 25 Sep 2026 09:55:37 -0700 Subject: [PATCH 18/35] fix(web): URL-driven specimen filters, Backspace in modals, large uploads. - /specimens read its filters from the URL only on mount, so "Clear Filters" (and back/forward) changed the URL but not the table. Filters are now derived from the URL. - The Backspace "back to specimens" hotkey fired while the Add Container modal was open if focus was on a button. - qPCR result uploads encoded the file with String.fromCharCode(...bytes), which throws on files over a few hundred KB; encoding is now chunked. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/web/src/lib/__tests__/base64.test.ts | 16 +++++++++++++++ packages/web/src/lib/base64.ts | 12 +++++++++++ .../web/src/pages/QpcrExperimentDetail.tsx | 3 ++- packages/web/src/pages/SpecimenDetail.tsx | 3 ++- packages/web/src/pages/Specimens.tsx | 20 ++++++++++++------- 5 files changed, 45 insertions(+), 9 deletions(-) create mode 100644 packages/web/src/lib/__tests__/base64.test.ts create mode 100644 packages/web/src/lib/base64.ts diff --git a/packages/web/src/lib/__tests__/base64.test.ts b/packages/web/src/lib/__tests__/base64.test.ts new file mode 100644 index 00000000..f788ca29 --- /dev/null +++ b/packages/web/src/lib/__tests__/base64.test.ts @@ -0,0 +1,16 @@ +import { describe, it, expect } from 'vitest' +import { bytesToBase64 } from '../base64' + +describe('bytesToBase64', () => { + it('matches btoa for small input', () => { + const bytes = new TextEncoder().encode('Well,Cq\nA1,22.1') + expect(bytesToBase64(bytes)).toBe(btoa('Well,Cq\nA1,22.1')) + }) + + it('encodes files larger than the argument limit', () => { + const bytes = new Uint8Array(2_000_000).map((_, i) => i % 256) + const encoded = bytesToBase64(bytes) + expect(encoded.length).toBe(Math.ceil(bytes.length / 3) * 4) + expect(atob(encoded).charCodeAt(1_999_999)).toBe(1_999_999 % 256) + }) +}) diff --git a/packages/web/src/lib/base64.ts b/packages/web/src/lib/base64.ts new file mode 100644 index 00000000..000d0417 --- /dev/null +++ b/packages/web/src/lib/base64.ts @@ -0,0 +1,12 @@ +/** + * Base64-encode bytes in chunks. `String.fromCharCode(...bytes)` on a whole file passes every + * byte as a function argument and throws a RangeError once the file is a few hundred KB. + */ +export function bytesToBase64(bytes: Uint8Array): string { + const CHUNK = 0x8000 + let binary = '' + for (let i = 0; i < bytes.length; i += CHUNK) { + binary += String.fromCharCode(...bytes.subarray(i, i + CHUNK)) + } + return btoa(binary) +} diff --git a/packages/web/src/pages/QpcrExperimentDetail.tsx b/packages/web/src/pages/QpcrExperimentDetail.tsx index cf1d1c2f..bcdc590d 100644 --- a/packages/web/src/pages/QpcrExperimentDetail.tsx +++ b/packages/web/src/pages/QpcrExperimentDetail.tsx @@ -17,6 +17,7 @@ import { getContainerTypeIcon, getContainerTypeName, getSpecimenTypeIcon } from import { useUser } from '../contexts/UserContext' import { useToast } from '../contexts/ToastContext' import '../styles/qpcr.css' +import { bytesToBase64 } from '../lib/base64' const PLATE_ROWS = ['A', 'B', 'C', 'D', 'E', 'F', 'G', 'H'] as const const PLATE_COLS = 12 @@ -978,7 +979,7 @@ export default function QpcrExperimentDetail() { setResultsUploading(true) try { const buf = await file.arrayBuffer() - const base64 = btoa(String.fromCharCode(...new Uint8Array(buf))) + const base64 = bytesToBase64(new Uint8Array(buf)) const resultInstrument = (instrumentSelectRef.current?.value as 'Biorad_CFX' | 'QuantStudio' | undefined) ?? 'Biorad_CFX' await qpcrExperimentsApi.uploadResults(parseInt(id), { fileContent: base64, fileName: file.name, instrumentType: resultInstrument }) invalidateQpcrExperimentQueries(queryClient) diff --git a/packages/web/src/pages/SpecimenDetail.tsx b/packages/web/src/pages/SpecimenDetail.tsx index 40d7ebca..34a57e5e 100644 --- a/packages/web/src/pages/SpecimenDetail.tsx +++ b/packages/web/src/pages/SpecimenDetail.tsx @@ -96,7 +96,8 @@ export default function SpecimenDetail() { } navigate('/specimens') }, - { preventDefault: true } + // Not while the Add Container modal is open (focus may be on a button inside it). + { preventDefault: true, enabled: !addContainerModalOpen } ) useModifierHotkey( diff --git a/packages/web/src/pages/Specimens.tsx b/packages/web/src/pages/Specimens.tsx index aa93a411..efbc8d95 100644 --- a/packages/web/src/pages/Specimens.tsx +++ b/packages/web/src/pages/Specimens.tsx @@ -1,4 +1,4 @@ -import { useMemo, useState } from 'react' +import { useMemo, useState, useEffect } from 'react' import DataTable, { Column } from '../components/DataTable' import { Link, useSearchParams, useNavigate } from 'react-router-dom' import SpecimenFilter, { type SpecimenFilters } from '../components/SpecimenFilter' @@ -15,6 +15,7 @@ import { getQueryErrorMessage, } from '../ui' import '../styles/subject-specimen.css' +import { parseDisplayDate } from '../lib/date-utils' type SpecimenListRow = Specimen & { study?: { shortCode?: string } @@ -55,7 +56,12 @@ export default function Specimens() { const [page, setPage] = useState(1) const pageSize = 50 - const [filters, setFilters] = useState(() => filtersFromSearchParams(searchParams)) + // The URL is the source of truth, so "Clear Filters" and back/forward update the table. + const filters = useMemo(() => filtersFromSearchParams(searchParams), [searchParams]) + // Back to page 1 whenever the filters change, including from outside (Clear Filters). + useEffect(() => { + setPage(1) + }, [searchParams]) const listParams = useMemo(() => filtersToParams(filters), [filters]) const specimensQuery = useSpecimens(listParams) @@ -65,8 +71,7 @@ export default function Specimens() { isEmpty: specimensQuery.isSuccess && specimens.length === 0, }) - const handleFilterChange = (newFilters: SpecimenFilters) => { - setFilters(newFilters) + const handleFilterChange = (newFilters: SpecimenFilters, options?: { replace?: boolean }) => { setPage(1) const params: Record = {} @@ -79,12 +84,12 @@ export default function Specimens() { if (newFilters.createdTo) params.created_to = newFilters.createdTo if (newFilters.search) params.search = newFilters.search - setSearchParams(params) + setSearchParams(params, { replace: options?.replace }) } const formatDate = (dateString?: string) => { if (!dateString) return '—' - return new Date(dateString).toLocaleDateString() + return parseDisplayDate(dateString).toLocaleDateString() } const columns: Column[] = [ @@ -172,7 +177,8 @@ export default function Specimens() {
handleFilterChange(next, { replace: true })} onSubmit={handleFilterChange} isLoading={specimensQuery.isPending} /> From e1372e4cfeb0319b6a6c31383024af486adc6cda Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Fri, 25 Sep 2026 10:26:52 -0700 Subject: [PATCH 19/35] fix(studies): query summary and delete with subqueries, not id lists. The study summary and study delete passed every subject, specimen, and container id as bound parameters, which fails once a study passes SQLite's variable limit (about 65k here). They now select through subqueries on study_id. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../api/src/routes/__tests__/studies.test.ts | 34 +++++++++- packages/api/src/routes/studies.ts | 66 +++++++++---------- 2 files changed, 63 insertions(+), 37 deletions(-) diff --git a/packages/api/src/routes/__tests__/studies.test.ts b/packages/api/src/routes/__tests__/studies.test.ts index 30e40ba8..7591f8a1 100644 --- a/packages/api/src/routes/__tests__/studies.test.ts +++ b/packages/api/src/routes/__tests__/studies.test.ts @@ -5,7 +5,7 @@ import { type AuthenticatedRouteTestContext, } from '../../__tests__/helpers/authenticated-route-test' import { createStudiesRoutes } from '../studies' -import { createTestStudy } from '../../__tests__/helpers/factories' +import { createTestStudy, createTestSpecimenType, createTestUnit } from '../../__tests__/helpers/factories' describe('Studies API (list, get, create, update)', () => { let ctx: AuthenticatedRouteTestContext @@ -148,4 +148,36 @@ describe('Studies API (list, get, create, update)', () => { expect(res.status).toBe(401) }) }) + + describe('GET /api/studies/:id/summary', () => { + it('handles studies with more specimens than SQLite allows as bound parameters', async () => { + const studyRecord = await createTestStudy(ctx.db, { title: 'Big Study', shortCode: 'BIG01' }) + const type = await createTestSpecimenType(ctx.db, { name: 'DBS' }) + const unit = await createTestUnit(ctx.db, { symbol: 'spots', name: 'spots', category: 'count' }) + const count = 70_000 + ctx.sqlite.exec('BEGIN') + const addSubject = ctx.sqlite.prepare( + "INSERT INTO study_subject (study_id, name, created, last_updated) VALUES (?, ?, '2024-01-01', '2024-01-01')", + ) + const addSpecimen = ctx.sqlite.prepare( + "INSERT INTO specimen (study_subject_id, specimen_type_id, collection_date, created, last_updated) VALUES (?, ?, '2024-01-01', '2024-01-01', '2024-01-01')", + ) + const addContainer = ctx.sqlite.prepare( + "INSERT INTO storage_container (specimen_id, unit_id, total_quantity, remaining_quantity, created, last_updated) VALUES (?, ?, 1, 1, '2024-01-01', '2024-01-01')", + ) + for (let i = 0; i < count; i++) { + const subject = addSubject.run(studyRecord.id, `S${i}`) + const spec = addSpecimen.run(subject.lastInsertRowid, type.id) + addContainer.run(spec.lastInsertRowid, unit.id) + } + ctx.sqlite.exec('COMMIT') + + const res = await ctx.request(`/api/studies/${studyRecord.id}/summary`) + + expect(res.status).toBe(200) + const data = (await res.json()) as { summary: { totalSpecimens: number; totalContainers: number } } + expect(data.summary.totalSpecimens).toBe(count) + expect(data.summary.totalContainers).toBe(count) + }, 60_000) + }) }) diff --git a/packages/api/src/routes/studies.ts b/packages/api/src/routes/studies.ts index 4641f3d6..d0e8ac7d 100644 --- a/packages/api/src/routes/studies.ts +++ b/packages/api/src/routes/studies.ts @@ -247,8 +247,8 @@ studies.get('/:id/summary', authMiddleware, async (c) => { }) } - // Get all specimens for these subjects - const placeholders = subjectIds.map(() => '?').join(',') + // Get all specimens for these subjects. Subqueries (not id lists) keep large studies + // under SQLite's bound-parameter limit. const specimensQuery = ` SELECT s.id, @@ -258,10 +258,10 @@ studies.get('/:id/summary', authMiddleware, async (c) => { s.created, s.last_updated as lastUpdated FROM specimen s - WHERE s.study_subject_id IN (${placeholders}) + WHERE s.study_subject_id IN (SELECT id FROM study_subject WHERE study_id = ?) ` const stmt = sqliteDatabase.prepare(specimensQuery) - const specimens = stmt.all(...subjectIds) as Array<{ + const specimens = stmt.all(id) as Array<{ id: number studySubjectId: number specimenTypeId: number @@ -307,7 +307,6 @@ studies.get('/:id/summary', authMiddleware, async (c) => { }) } - const specimenIds = specimens.map(s => s.id) const specimenTypeIds = [...new Set(specimens.map(s => s.specimenTypeId))] // Get specimen types @@ -319,29 +318,30 @@ studies.get('/:id/summary', authMiddleware, async (c) => { const specimenTypeMap = new Map(specimenTypes.map(st => [st.id, st.name])) // Get all containers for these specimens + const studySpecimenIds = database + .select({ id: specimen.id }) + .from(specimen) + .where(inArray(specimen.studySubjectId, database.select({ id: studySubject.id }).from(studySubject).where(eq(studySubject.studyId, id)))) const containers = await database .select() .from(storageContainer) - .where(inArray(storageContainer.specimenId, specimenIds)) + .where(inArray(storageContainer.specimenId, studySpecimenIds)) const totalContainers = containers.length - const containerIds = containers.map(c => c.id) + const studyContainerIds = database + .select({ id: storageContainer.id }) + .from(storageContainer) + .where(inArray(storageContainer.specimenId, studySpecimenIds)) // Get container type information - const [micronixTubes, cryovialTubes, papers, staticWells] = await Promise.all([ - containerIds.length > 0 - ? database.select({ id: micronixTube.id }).from(micronixTube).where(inArray(micronixTube.id, containerIds)) - : [], - containerIds.length > 0 - ? database.select({ id: cryovialTube.id }).from(cryovialTube).where(inArray(cryovialTube.id, containerIds)) - : [], - containerIds.length > 0 - ? database.select({ id: paper.id }).from(paper).where(inArray(paper.id, containerIds)) - : [], - containerIds.length > 0 - ? database.select({ id: staticWell.id }).from(staticWell).where(inArray(staticWell.id, containerIds)) - : [], - ]) + const [micronixTubes, cryovialTubes, papers, staticWells] = totalContainers > 0 + ? await Promise.all([ + database.select({ id: micronixTube.id }).from(micronixTube).where(inArray(micronixTube.id, studyContainerIds)), + database.select({ id: cryovialTube.id }).from(cryovialTube).where(inArray(cryovialTube.id, studyContainerIds)), + database.select({ id: paper.id }).from(paper).where(inArray(paper.id, studyContainerIds)), + database.select({ id: staticWell.id }).from(staticWell).where(inArray(staticWell.id, studyContainerIds)), + ]) + : [[], [], [], []] // Create container type map const containerTypeMap = new Map() @@ -720,23 +720,17 @@ studies.delete('/:id', memberMiddleware, async (c) => { .where(eq(studySubject.studyId, id)) const subjectIds = subjects.map((s) => s.id) - let specimenIds: number[] = [] - if (subjectIds.length > 0) { - const specimens = await database - .select({ id: specimen.id }) - .from(specimen) - .where(inArray(specimen.studySubjectId, subjectIds)) - specimenIds = specimens.map((s) => s.id) - } - - let containerIds: number[] = [] - if (specimenIds.length > 0) { - const containers = await database + // Subqueries, not id lists, so large studies stay under SQLite's parameter limit. + const studySubjectIds = database.select({ id: studySubject.id }).from(studySubject).where(eq(studySubject.studyId, id)) + const specimenIds = ( + await database.select({ id: specimen.id }).from(specimen).where(inArray(specimen.studySubjectId, studySubjectIds)) + ).map((s) => s.id) + const containerIds = ( + await database .select({ id: storageContainer.id }) .from(storageContainer) - .where(inArray(storageContainer.specimenId, specimenIds)) - containerIds = containers.map((c) => c.id) - } + .where(inArray(storageContainer.specimenId, database.select({ id: specimen.id }).from(specimen).where(inArray(specimen.studySubjectId, studySubjectIds)))) + ).map((c) => c.id) await assertNotUsedInQpcr(database, { containerIds, specimenIds }, `study '${existingStudy.shortCode}'`) From 30ceb3e644549bde44dfc607b81f35adb033ae59 Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Fri, 25 Sep 2026 10:27:42 -0700 Subject: [PATCH 20/35] fix(auth): take as long to reject a missing account as a wrong password. Login and /auth/switch skipped bcrypt when the account did not exist, so response time revealed which emails, usernames, and user ids are real. Both now compare against a throwaway hash in that case. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../src/lib/__tests__/password-timing.test.ts | 21 +++++++++++++++++++ packages/api/src/lib/auth/auth-service.ts | 20 +++++++++++++----- packages/api/src/routes/auth.ts | 3 ++- 3 files changed, 38 insertions(+), 6 deletions(-) create mode 100644 packages/api/src/lib/__tests__/password-timing.test.ts diff --git a/packages/api/src/lib/__tests__/password-timing.test.ts b/packages/api/src/lib/__tests__/password-timing.test.ts new file mode 100644 index 00000000..38ee385e --- /dev/null +++ b/packages/api/src/lib/__tests__/password-timing.test.ts @@ -0,0 +1,21 @@ +import { describe, it, expect, vi } from 'vitest' +import bcrypt from 'bcryptjs' +import { verifyPasswordConstantTime } from '../auth/auth-service' + +describe('verifyPasswordConstantTime', () => { + it('still runs a bcrypt comparison when there is no such user', async () => { + const compare = vi.spyOn(bcrypt, 'compare') + + const valid = await verifyPasswordConstantTime('guess', undefined) + + expect(valid).toBe(false) + expect(compare).toHaveBeenCalledTimes(1) + compare.mockRestore() + }) + + it('checks the real hash when the user exists', async () => { + const hash = await bcrypt.hash('right', 4) + expect(await verifyPasswordConstantTime('right', hash)).toBe(true) + expect(await verifyPasswordConstantTime('wrong', hash)).toBe(false) + }) +}) diff --git a/packages/api/src/lib/auth/auth-service.ts b/packages/api/src/lib/auth/auth-service.ts index 0dd9c09a..118bc779 100644 --- a/packages/api/src/lib/auth/auth-service.ts +++ b/packages/api/src/lib/auth/auth-service.ts @@ -45,17 +45,27 @@ export async function findActiveUserByEmailOrUsername( ) } +let dummyPasswordHash: string | null = null + +/** + * Check a password against a user's hash, or against a throwaway hash when there is no such + * user, so a missing account takes as long to reject as a wrong password. + */ +export async function verifyPasswordConstantTime(password: string, passwordHash: string | undefined): Promise { + if (passwordHash) return bcrypt.compare(password, passwordHash) + dummyPasswordHash ??= await bcrypt.hash('no-such-user', 10) + await bcrypt.compare(password, dummyPasswordHash) + return false +} + export async function verifyLoginCredentials( database: Database, emailOrUsername: string, password: string ): Promise { const user = await findActiveUserByEmailOrUsername(database, emailOrUsername) - if (!user) { - throw new UnauthorizedError('Invalid credentials') - } - const valid = await bcrypt.compare(password, user.passwordHash) - if (!valid) { + const valid = await verifyPasswordConstantTime(password, user?.passwordHash) + if (!user || !valid) { throw new UnauthorizedError('Invalid credentials') } if (!user.approvedAt) { diff --git a/packages/api/src/routes/auth.ts b/packages/api/src/routes/auth.ts index 5e8992ed..c53df0cd 100644 --- a/packages/api/src/routes/auth.ts +++ b/packages/api/src/routes/auth.ts @@ -13,6 +13,7 @@ import { handleRouteError } from '../lib/error-handler' import { toPublicUser, verifyLoginCredentials, + verifyPasswordConstantTime, createUserSession, registerApprovedUser, } from '../lib/auth/auth-service' @@ -460,7 +461,7 @@ auth.post('/switch', rateLimit(10, 60 * 1000), authMiddleware, async (c) => { // Same response for a missing user and a wrong password, so the endpoint does not reveal // which accounts exist; approval status is only reported after the password checks out. - const valid = targetUser ? await bcrypt.compare(password, targetUser.passwordHash) : false + const valid = await verifyPasswordConstantTime(password, targetUser?.passwordHash) if (!targetUser || !valid) { return c.json({ error: 'Invalid password' }, 401) } From 6b72c15ed1b847df9d2ff127fa3834904caa0d57 Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Fri, 25 Sep 2026 10:28:45 -0700 Subject: [PATCH 21/35] fix(search): match % and _ in search text literally. Search queries built LIKE '%%' from raw user input, so "_" and "%" acted as wildcards: searching "3D7_0" also matched "3D7X0.05". Search, specimen list, location, and error-log filters now escape the text with ESCAPE '\'. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../src/lib/search/__tests__/subject-search.test.ts | 10 ++++++++++ packages/api/src/lib/search/collection-search.ts | 13 +++++++------ packages/api/src/lib/search/container-search.ts | 7 ++++--- packages/api/src/lib/search/study-search.ts | 5 +++-- packages/api/src/lib/search/subject-search.ts | 5 +++-- packages/api/src/lib/specimens/specimen-read.ts | 9 +++++---- packages/api/src/lib/sql-like.ts | 12 ++++++++++++ packages/api/src/routes/error-logs.ts | 7 ++++--- packages/api/src/routes/locations.ts | 12 ++++++------ 9 files changed, 54 insertions(+), 26 deletions(-) create mode 100644 packages/api/src/lib/sql-like.ts diff --git a/packages/api/src/lib/search/__tests__/subject-search.test.ts b/packages/api/src/lib/search/__tests__/subject-search.test.ts index b30ce07a..a93f08ce 100644 --- a/packages/api/src/lib/search/__tests__/subject-search.test.ts +++ b/packages/api/src/lib/search/__tests__/subject-search.test.ts @@ -41,4 +41,14 @@ describe('subject-search', () => { url: `/subjects/${subject.id}`, }) }) + + it('treats _ and % in the query literally', async () => { + const studyRecord = await createTestStudy(testDb, { title: 'Controls', shortCode: 'CTL' }) + await createTestStudySubject(testDb, { studyId: studyRecord.id, name: '3D7_0.05' }) + await createTestStudySubject(testDb, { studyId: studyRecord.id, name: '3D7X0.05' }) + await createTestStudySubject(testDb, { studyId: studyRecord.id, name: '100% pure' }) + + expect((await searchSubjects(testDb, '3D7_0')).map((r) => r.title)).toEqual(['3D7_0.05']) + expect((await searchSubjects(testDb, '0%')).map((r) => r.title)).toEqual(['100% pure']) + }) }) diff --git a/packages/api/src/lib/search/collection-search.ts b/packages/api/src/lib/search/collection-search.ts index cd800a56..6c4392af 100644 --- a/packages/api/src/lib/search/collection-search.ts +++ b/packages/api/src/lib/search/collection-search.ts @@ -8,9 +8,10 @@ import { controlBatch, controlDefinition, } from '../../db/schema' -import { eq, or, like } from 'drizzle-orm' +import { eq, or } from 'drizzle-orm' import { formatLocationPath } from '../container-enrichment' import type { SearchResult } from './types' +import { likeContains } from '../sql-like' /** Search collections and control batches by name, barcode, or numeric id. */ export async function searchCollections(database: Database, query: string): Promise { @@ -32,7 +33,7 @@ export async function searchCollections(database: Database, query: string): Prom .where( isNumeric ? eq(micronixPlate.id, queryNum) - : or(like(micronixPlate.name, `%${query}%`), like(micronixPlate.barcode, `%${query}%`))!, + : or(likeContains(micronixPlate.name, query), likeContains(micronixPlate.barcode, query))!, ) .limit(10) @@ -68,7 +69,7 @@ export async function searchCollections(database: Database, query: string): Prom .where( isNumeric ? eq(cryovialBox.id, queryNum) - : or(like(cryovialBox.name, `%${query}%`), like(cryovialBox.barcode, `%${query}%`))!, + : or(likeContains(cryovialBox.name, query), likeContains(cryovialBox.barcode, query))!, ) .limit(10) @@ -100,7 +101,7 @@ export async function searchCollections(database: Database, query: string): Prom }) .from(box) .leftJoin(location, eq(box.locationId, location.id)) - .where(isNumeric ? eq(box.id, queryNum) : like(box.name, `%${query}%`)) + .where(isNumeric ? eq(box.id, queryNum) : likeContains(box.name, query)) .limit(10) for (const paperBox of boxes) { @@ -126,7 +127,7 @@ export async function searchCollections(database: Database, query: string): Prom }) .from(bag) .leftJoin(location, eq(bag.locationId, location.id)) - .where(isNumeric ? eq(bag.id, queryNum) : like(bag.name, `%${query}%`)) + .where(isNumeric ? eq(bag.id, queryNum) : likeContains(bag.name, query)) .limit(10) for (const paperBag of bags) { @@ -153,7 +154,7 @@ export async function searchCollections(database: Database, query: string): Prom .where( isNumeric ? eq(controlBatch.id, queryNum) - : or(like(controlBatch.name, `%${query}%`), like(controlDefinition.name, `%${query}%`))!, + : or(likeContains(controlBatch.name, query), likeContains(controlDefinition.name, query))!, ) .limit(10) diff --git a/packages/api/src/lib/search/container-search.ts b/packages/api/src/lib/search/container-search.ts index 07041022..4323d93d 100644 --- a/packages/api/src/lib/search/container-search.ts +++ b/packages/api/src/lib/search/container-search.ts @@ -1,12 +1,13 @@ import type { Database } from '../../db/client' import { micronixTube, cryovialTube, micronixPlate, cryovialBox } from '../../db/schema' -import { eq, like, sql } from 'drizzle-orm' +import { eq, sql } from 'drizzle-orm' import { resolveContainerPlacements, type ContainerPlacement, type KnownContainerPlacement, } from '../container-placement' import type { SearchResult } from './types' +import { likeContains } from '../sql-like' type ContainerSearchRow = { id: number @@ -88,7 +89,7 @@ export async function searchContainers(database: Database, query: string): Promi }) .from(micronixTube) .leftJoin(micronixPlate, eq(micronixTube.collectionId, micronixPlate.id)) - .where(like(micronixTube.barcode, `%${query}%`)) + .where(likeContains(micronixTube.barcode, query)) .limit(10) for (const tube of micronixTubes) { @@ -115,7 +116,7 @@ export async function searchContainers(database: Database, query: string): Promi }) .from(cryovialTube) .leftJoin(cryovialBox, eq(cryovialTube.collectionId, cryovialBox.id)) - .where(like(cryovialTube.barcode, `%${query}%`)) + .where(likeContains(cryovialTube.barcode, query)) .limit(10) for (const tube of cryovialTubes) { diff --git a/packages/api/src/lib/search/study-search.ts b/packages/api/src/lib/search/study-search.ts index 5879372e..0a6a8778 100644 --- a/packages/api/src/lib/search/study-search.ts +++ b/packages/api/src/lib/search/study-search.ts @@ -1,14 +1,15 @@ import type { Database } from '../../db/client' import { study } from '../../db/schema' -import { like, or } from 'drizzle-orm' +import { or } from 'drizzle-orm' import type { SearchResult } from './types' +import { likeContains } from '../sql-like' /** Search studies by short code or title. */ export async function searchStudies(database: Database, query: string): Promise { const studies = await database .select() .from(study) - .where(or(like(study.shortCode, `%${query}%`), like(study.title, `%${query}%`))!) + .where(or(likeContains(study.shortCode, query), likeContains(study.title, query))!) .limit(10) return studies.map((studyRecord) => ({ diff --git a/packages/api/src/lib/search/subject-search.ts b/packages/api/src/lib/search/subject-search.ts index 479552f9..c488b1fc 100644 --- a/packages/api/src/lib/search/subject-search.ts +++ b/packages/api/src/lib/search/subject-search.ts @@ -1,7 +1,8 @@ import type { Database } from '../../db/client' import { study, studySubject } from '../../db/schema' -import { eq, like } from 'drizzle-orm' +import { eq } from 'drizzle-orm' import type { SearchResult } from './types' +import { likeContains } from '../sql-like' /** Search subjects by name. */ export async function searchSubjects(database: Database, query: string): Promise { @@ -14,7 +15,7 @@ export async function searchSubjects(database: Database, query: string): Promise }) .from(studySubject) .leftJoin(study, eq(studySubject.studyId, study.id)) - .where(like(studySubject.name, `%${query}%`)) + .where(likeContains(studySubject.name, query)) .limit(10) return subjects.map((subject) => ({ diff --git a/packages/api/src/lib/specimens/specimen-read.ts b/packages/api/src/lib/specimens/specimen-read.ts index cced0197..9ca373ad 100644 --- a/packages/api/src/lib/specimens/specimen-read.ts +++ b/packages/api/src/lib/specimens/specimen-read.ts @@ -1,9 +1,10 @@ import type { Database } from '../../db/client' import { specimen, storageContainer, studySubject, study, specimenType, controlBatch } from '../../db/schema' -import { eq, and, like, or, sql } from 'drizzle-orm' +import { eq, and, or, sql } from 'drizzle-orm' import { validatePage, validateLimit } from '../constants' import { resolveContainerByBarcode } from '../identifier-resolution' import { dateToUpperBound } from '../statistics/helpers' +import { likeContains } from '../sql-like' export type ListSpecimensQuery = { sourceType?: string @@ -174,9 +175,9 @@ export async function listSpecimens(database: Database, query: ListSpecimensQuer if (search) { conditions.push(or( - like(studySubject.name, `%${search}%`), - like(controlBatch.name, `%${search}%`), - like(specimenType.name, `%${search}%`), + likeContains(studySubject.name, search), + likeContains(controlBatch.name, search), + likeContains(specimenType.name, search), )) } diff --git a/packages/api/src/lib/sql-like.ts b/packages/api/src/lib/sql-like.ts new file mode 100644 index 00000000..248517a2 --- /dev/null +++ b/packages/api/src/lib/sql-like.ts @@ -0,0 +1,12 @@ +import { sql, type SQL } from 'drizzle-orm' +import type { SQLiteColumn } from 'drizzle-orm/sqlite-core' + +/** Escape LIKE wildcards so "%" and "_" in user text match themselves. */ +export function escapeLikePattern(text: string): string { + return text.replace(/[\\%_]/g, (ch) => `\\${ch}`) +} + +/** `column LIKE '%text%'` with the text taken literally (a "_" in "3D7_0.05" is not a wildcard). */ +export function likeContains(column: SQLiteColumn | SQL, text: string): SQL { + return sql`${column} LIKE ${`%${escapeLikePattern(text)}%`} ESCAPE '\\'` +} diff --git a/packages/api/src/routes/error-logs.ts b/packages/api/src/routes/error-logs.ts index d7b0e12e..9ebdfd8f 100644 --- a/packages/api/src/routes/error-logs.ts +++ b/packages/api/src/routes/error-logs.ts @@ -4,11 +4,12 @@ import type { Database } from '../db/client' import { errorLogs } from '../db/schema' import { logFrontendError, cleanupOldErrorLogs, type ErrorLogContext } from '../lib/error-logger' import { handleRouteError } from '../lib/error-handler' -import { eq, and, desc, sql, like, or } from 'drizzle-orm' +import { eq, and, desc, sql, or } from 'drizzle-orm' import { createAdminMiddleware, createAuthMiddleware, createOptionalAuthMiddleware } from '../middleware/auth' import { rateLimit } from '../middleware/rate-limit' import { utcNow } from '../lib/datetime' import { requireParam } from '../lib/common-validators' +import { likeContains } from '../lib/sql-like' // Schema for frontend error submission const frontendErrorSchema = z.object({ @@ -134,8 +135,8 @@ export function createErrorLogsRoutes(database: Database): Hono { if (search) { conditions.push( or( - like(errorLogs.message, `%${search}%`), - like(errorLogs.errorCode, `%${search}%`) + likeContains(errorLogs.message, search), + likeContains(errorLogs.errorCode, search) )! ) } diff --git a/packages/api/src/routes/locations.ts b/packages/api/src/routes/locations.ts index 75ddd273..f4de95f0 100644 --- a/packages/api/src/routes/locations.ts +++ b/packages/api/src/routes/locations.ts @@ -2,7 +2,7 @@ import { Hono } from 'hono' import type { Database } from '../db/client' import type { Database as SQLiteDatabase } from 'bun:sqlite' import { location, micronixPlate, cryovialBox, box, bag, storageType } from '../db/schema' -import { eq, and, sql, or, like, desc, isNull, inArray } from 'drizzle-orm' +import { eq, and, sql, or, desc, isNull, inArray } from 'drizzle-orm' import { validatePage, validateLimit } from '../lib/constants' import { z } from 'zod' import { @@ -19,6 +19,7 @@ import { handleRouteError, NotFoundError, ValidationError } from '../lib/error-h import { createAdminMiddleware, createAuthMiddleware } from '../middleware/auth' import { utcNow } from '../lib/datetime' import { requireParam } from '../lib/common-validators' +import { likeContains } from '../lib/sql-like' /** * Create locations routes with database injection @@ -51,12 +52,11 @@ locations.get('/', authMiddleware, async (c) => { // Build conditions for each word - each word must match at least one field // Note: storageTypeId is only on root locations, so we search it but it may be null const wordConditions = searchWords.map(word => { - const pattern = `%${word}%` return or( - like(location.name, pattern), - like(location.path, pattern), - sql`${location.storageTypeId} LIKE ${pattern}`, // Handle nullable storageTypeId - like(location.description, pattern) + likeContains(location.name, word), + likeContains(location.path, word), + likeContains(location.storageTypeId, word), // storageTypeId is null on child locations + likeContains(location.description, word) )! }) From 643f0060851df3e0c9d873956639c646fe56742e Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Fri, 25 Sep 2026 10:29:41 -0700 Subject: [PATCH 22/35] perf(activity): build the recent feed with joins instead of per-row queries. Each recent specimen ran up to 3 follow-up queries and each container up to 7. The feed now uses three joined queries in total (specimens, studies, containers) and produces the same labels, which a new test pins. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../api/src/routes/__tests__/activity.test.ts | 31 ++ packages/api/src/routes/activity.ts | 327 +++++++----------- 2 files changed, 147 insertions(+), 211 deletions(-) diff --git a/packages/api/src/routes/__tests__/activity.test.ts b/packages/api/src/routes/__tests__/activity.test.ts index e6b8cdb6..ccf8b4a8 100644 --- a/packages/api/src/routes/__tests__/activity.test.ts +++ b/packages/api/src/routes/__tests__/activity.test.ts @@ -9,7 +9,14 @@ import { createTestControlBatch, createTestSpecimenType, createTestSpecimen, + createTestStudy, + createTestStudySubject, + createTestStorageContainer, + createTestStorageType, + createTestLocation, + createTestMicronixPlate, } from '../../__tests__/helpers/factories' +import { micronixTube } from '../../db/schema' import { createActivityRoutes } from '../activity' describe('Activity API', () => { @@ -75,5 +82,29 @@ describe('Activity API', () => { expect(specimenActivity!.label).toContain('HPV18 DBS Control') expect(specimenActivity!.label).toContain('HPV18-2024-001') }) + + it('labels subject specimens and their containers with subject and study', async () => { + const studyRecord = await createTestStudy(ctx.db, { title: 'Malaria Cohort', shortCode: 'MAL' }) + const subject = await createTestStudySubject(ctx.db, { studyId: studyRecord.id, name: 'P-001' }) + const type = await createTestSpecimenType(ctx.db, { name: 'Whole Blood' }) + const spec = await createTestSpecimen(ctx.db, type.id, { studySubjectId: subject.id }) + const container = await createTestStorageContainer(ctx.db, { specimenId: spec.id }) + const storageType = await createTestStorageType(ctx.db, { name: 'Freezer' }) + const loc = await createTestLocation(ctx.db, { name: 'Loc', storageTypeId: String(storageType.id) }) + const plate = await createTestMicronixPlate(ctx.db, { name: 'P1', locationId: loc.id }) + await ctx.db.insert(micronixTube).values({ id: container.id, collectionId: plate.id, barcode: 'MT-9', position: 'A01' }) + + const res = await ctx.request('/api/activity/recent', { method: 'GET' }) + const data = (await res.json()) as { activity: Array<{ id: number; type: string; label: string; context?: string }> } + + expect(data.activity.find((a) => a.type === 'specimen' && a.id === spec.id)).toMatchObject({ + label: 'Whole Blood • P-001 (MAL)', + context: 'Malaria Cohort', + }) + expect(data.activity.find((a) => a.type === 'container' && a.id === container.id)).toMatchObject({ + label: 'Micronix Tube (MT-9)', + context: 'Whole Blood • P-001 (MAL)', + }) + }) }) }) diff --git a/packages/api/src/routes/activity.ts b/packages/api/src/routes/activity.ts index 90954168..c8d7bb03 100644 --- a/packages/api/src/routes/activity.ts +++ b/packages/api/src/routes/activity.ts @@ -31,103 +31,66 @@ export function createActivityRoutes(database: Database): Hono { // Get recent activity across all entity types activity.get('/recent', authMiddleware, async (c) => { try { - // Each row is enriched with several queries, so keep the feed short (and never unbounded: - // SQLite treats LIMIT -1 as no limit). + // Keep the feed short, and never unbounded (SQLite treats LIMIT -1 as no limit). const requested = parseInt(c.req.query('limit') || '10') const limit = Number.isFinite(requested) ? Math.min(Math.max(requested, 1), MAX_RECENT_ACTIVITY) : 10 - // Get recent specimens with enriched data + // Recent specimens, joined to their subject/study or batch/definition in one query + // (never expose DB IDs in labels) const recentSpecimens = await database .select({ id: specimen.id, - type: sql`'specimen'`.as('type'), created: specimen.created, lastUpdated: specimen.lastUpdated, specimenTypeName: specimenType.name, collectionDate: specimen.collectionDate, - studySubjectId: specimen.studySubjectId, controlBatchId: specimen.controlBatchId, + studySubjectId: specimen.studySubjectId, + batchName: controlBatch.name, + productionDate: controlBatch.productionDate, + definitionName: controlDefinition.name, + subjectName: studySubject.name, + studyTitle: study.title, + studyShortCode: study.shortCode, }) .from(specimen) .leftJoin(specimenType, eq(specimen.specimenTypeId, specimenType.id)) + .leftJoin(controlBatch, eq(specimen.controlBatchId, controlBatch.id)) + .leftJoin(controlDefinition, eq(controlBatch.controlDefinitionId, controlDefinition.id)) + .leftJoin(studySubject, eq(specimen.studySubjectId, studySubject.id)) + .leftJoin(study, eq(studySubject.studyId, study.id)) .orderBy(sql`COALESCE(${specimen.lastUpdated}, ${specimen.created}) DESC`) .limit(limit) - - // Enrich specimens with subject/study or batch/definition info (never expose DB IDs in labels) - const enrichedSpecimens = await Promise.all( - recentSpecimens.map(async (spec) => { - const specimenTypeName = spec.specimenTypeName || 'Specimen' - let label: string - let context: string | undefined = undefined - - if (spec.controlBatchId) { - try { - const batchInfo = await database - .select({ - batchName: controlBatch.name, - productionDate: controlBatch.productionDate, - definitionName: controlDefinition.name, - }) - .from(controlBatch) - .innerJoin(controlDefinition, eq(controlBatch.controlDefinitionId, controlDefinition.id)) - .where(eq(controlBatch.id, spec.controlBatchId)) - .get() - - if (batchInfo) { - label = `${specimenTypeName} • ${batchInfo.definitionName} (${batchInfo.batchName})` - context = spec.collectionDate || batchInfo.productionDate || undefined - } else { - label = `${specimenTypeName} • Control batch` - } - } catch (e) { - label = `${specimenTypeName} • Control batch` - } - } else if (spec.studySubjectId) { - try { - const subject = await database - .select({ - name: studySubject.name, - studyId: studySubject.studyId, - }) - .from(studySubject) - .where(eq(studySubject.id, spec.studySubjectId)) - .get() - - if (subject) { - const studyRecord = await database - .select({ - title: study.title, - shortCode: study.shortCode, - }) - .from(study) - .where(eq(study.id, subject.studyId)) - .get() - - if (studyRecord) { - label = `${specimenTypeName} • ${subject.name} (${studyRecord.shortCode})` - context = studyRecord.title - } else { - label = `${specimenTypeName} • ${subject.name}` - } - } else { - label = specimenTypeName - } - } catch (e) { - label = specimenTypeName - } + + const enrichedSpecimens = recentSpecimens.map((spec) => { + const specimenTypeName = spec.specimenTypeName || 'Specimen' + let label: string = specimenTypeName + let context: string | undefined = undefined + + if (spec.controlBatchId) { + if (spec.batchName && spec.definitionName) { + label = `${specimenTypeName} • ${spec.definitionName} (${spec.batchName})` + context = spec.collectionDate || spec.productionDate || undefined } else { - label = specimenTypeName + label = `${specimenTypeName} • Control batch` } - - return { - id: spec.id, - type: 'specimen' as const, - timestamp: spec.lastUpdated || spec.created || '', - label, - context, + } else if (spec.studySubjectId && spec.subjectName) { + if (spec.studyShortCode) { + label = `${specimenTypeName} • ${spec.subjectName} (${spec.studyShortCode})` + context = spec.studyTitle ?? undefined + } else { + label = `${specimenTypeName} • ${spec.subjectName}` } - }) - ) + } + + return { + id: spec.id, + type: 'specimen' as const, + timestamp: spec.lastUpdated || spec.created || '', + label, + context, + } + }) // Get recent studies with enriched data const recentStudies = await database @@ -152,148 +115,90 @@ export function createActivityRoutes(database: Database): Hono { })) ) - // Get recent containers with enriched data + // Recent containers, joined to their subtype row and specimen/subject/study in one query const recentContainers = await database .select({ id: storageContainer.id, created: storageContainer.created, lastUpdated: storageContainer.lastUpdated, - specimenId: storageContainer.specimenId, + micronixId: micronixTube.id, + micronixBarcode: micronixTube.barcode, + micronixPosition: micronixTube.position, + cryovialId: cryovialTube.id, + cryovialBarcode: cryovialTube.barcode, + cryovialPosition: cryovialTube.position, + paperId: paper.id, + paperSublabel: paper.sublabel, + wellId: staticWell.id, + wellPosition: staticWell.position, + specimenTypeName: specimenType.name, + subjectName: studySubject.name, + studyShortCode: study.shortCode, }) .from(storageContainer) + .leftJoin(micronixTube, eq(micronixTube.id, storageContainer.id)) + .leftJoin(cryovialTube, eq(cryovialTube.id, storageContainer.id)) + .leftJoin(paper, eq(paper.id, storageContainer.id)) + .leftJoin(staticWell, eq(staticWell.id, storageContainer.id)) + .leftJoin(specimen, eq(storageContainer.specimenId, specimen.id)) + .leftJoin(specimenType, eq(specimen.specimenTypeId, specimenType.id)) + .leftJoin(studySubject, eq(specimen.studySubjectId, studySubject.id)) + .leftJoin(study, eq(studySubject.studyId, study.id)) .orderBy(sql`COALESCE(${storageContainer.lastUpdated}, ${storageContainer.created}) DESC`) .limit(limit) - - // Enrich containers with type, barcode, and specimen info - const enrichedContainers = await Promise.all( - recentContainers.map(async (container) => { - // Determine container type by checking subtype tables - const [micronixInfo, cryovialInfo, paperInfo, staticWellInfo] = await Promise.all([ - database.select({ barcode: micronixTube.barcode, position: micronixTube.position }) - .from(micronixTube) - .where(eq(micronixTube.id, container.id)) - .get(), - database.select({ barcode: cryovialTube.barcode, position: cryovialTube.position }) - .from(cryovialTube) - .where(eq(cryovialTube.id, container.id)) - .get(), - database.select({ sublabel: paper.sublabel }) - .from(paper) - .where(eq(paper.id, container.id)) - .get(), - database.select({ position: staticWell.position }) - .from(staticWell) - .where(eq(staticWell.id, container.id)) - .get(), - ]) - - let containerType = 'container' - let barcode: string | null = null - let position: string | null = null - - if (micronixInfo) { - containerType = 'micronix_tube' - barcode = micronixInfo.barcode || null - position = micronixInfo.position || null - } else if (cryovialInfo) { - containerType = 'cryovial_tube' - barcode = cryovialInfo.barcode || null - position = cryovialInfo.position || null - } else if (paperInfo) { - containerType = 'paper' - barcode = paperInfo.sublabel || null - position = null - } else if (staticWellInfo) { - containerType = 'static_well' - position = staticWellInfo.position || null - } - - // Format container type name (never expose DB IDs) - const containerTypeName = containerType - .split('_') - .map(word => word.charAt(0).toUpperCase() + word.slice(1)) - .join(' ') - - let label: string - if (barcode) { - label = `${containerTypeName} (${barcode})` - } else if (position) { - label = `${containerTypeName} at ${position}` - } else { - label = `Unnamed ${containerTypeName.toLowerCase()}` - } - - let context: string | undefined = undefined - - // Get specimen info for context - if (container.specimenId) { - try { - const spec = await database - .select({ - id: specimen.id, - specimenTypeId: specimen.specimenTypeId, - studySubjectId: specimen.studySubjectId, - }) - .from(specimen) - .where(eq(specimen.id, container.specimenId)) - .get() - - if (spec) { - const specType = await database - .select({ name: specimenType.name }) - .from(specimenType) - .where(eq(specimenType.id, spec.specimenTypeId)) - .get() - - if (specType) { - context = specType.name - } - - if (spec.studySubjectId) { - try { - const subject = await database - .select({ - name: studySubject.name, - studyId: studySubject.studyId, - }) - .from(studySubject) - .where(eq(studySubject.id, spec.studySubjectId)) - .get() - - if (subject) { - const studyRecord = await database - .select({ - shortCode: study.shortCode, - }) - .from(study) - .where(eq(study.id, subject.studyId)) - .get() - - if (studyRecord) { - context = `${specType?.name || ''} • ${subject.name} (${studyRecord.shortCode})`.trim() - } else { - context = `${specType?.name || ''} • ${subject.name}`.trim() - } - } - } catch (e) { - // Silently fail - } - } - } - } catch (e) { - // Silently fail - context is optional - } - } - - return { - id: container.id, - type: 'container' as const, - timestamp: container.lastUpdated || container.created || '', - label, - context, - } - }) - ) + + const enrichedContainers = recentContainers.map((container) => { + let containerType = 'container' + let barcode: string | null = null + let position: string | null = null + + if (container.micronixId != null) { + containerType = 'micronix_tube' + barcode = container.micronixBarcode || null + position = container.micronixPosition || null + } else if (container.cryovialId != null) { + containerType = 'cryovial_tube' + barcode = container.cryovialBarcode || null + position = container.cryovialPosition || null + } else if (container.paperId != null) { + containerType = 'paper' + barcode = container.paperSublabel || null + } else if (container.wellId != null) { + containerType = 'static_well' + position = container.wellPosition || null + } + + // Format container type name (never expose DB IDs) + const containerTypeName = containerType + .split('_') + .map(word => word.charAt(0).toUpperCase() + word.slice(1)) + .join(' ') + + let label: string + if (barcode) { + label = `${containerTypeName} (${barcode})` + } else if (position) { + label = `${containerTypeName} at ${position}` + } else { + label = `Unnamed ${containerTypeName.toLowerCase()}` + } + + let context: string | undefined = container.specimenTypeName ?? undefined + if (container.subjectName) { + const subject = container.studyShortCode + ? `${container.subjectName} (${container.studyShortCode})` + : container.subjectName + context = `${container.specimenTypeName || ''} • ${subject}`.trim() + } + + return { + id: container.id, + type: 'container' as const, + timestamp: container.lastUpdated || container.created || '', + label, + context, + } + }) // Combine and sort by timestamp const allActivity = [ From c89f89e36694ed85da1150935f55f86edc851854 Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Fri, 25 Sep 2026 10:31:05 -0700 Subject: [PATCH 23/35] fix(web): keep /studies search, sort, and filters in the URL. The Studies page kept search, sort, and type/lead filters in component state, so the "Clear Filters" command (which clears the URL) did nothing, filtered views could not be linked, and back/forward lost them. They now live in the URL (q, sort, dir, type, lead), with defaults left out; each control updates the URL in a single change. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/web/src/pages/Studies.tsx | 67 +++++++++++++------ .../web/src/pages/__tests__/Studies.test.tsx | 15 ++++- 2 files changed, 62 insertions(+), 20 deletions(-) diff --git a/packages/web/src/pages/Studies.tsx b/packages/web/src/pages/Studies.tsx index f0447a86..6c6b47df 100644 --- a/packages/web/src/pages/Studies.tsx +++ b/packages/web/src/pages/Studies.tsx @@ -33,19 +33,55 @@ interface StudyWithSummary extends Study { const PAGE_LIMIT = 50 +const SORT_OPTIONS: SortOption[] = ['title', 'date', 'subjects', 'specimens', 'containers', 'lead'] + +/** Names sort A-Z by default; dates and counts sort largest first. */ +function defaultSortDirection(sortBy: SortOption): 'asc' | 'desc' { + return sortBy === 'title' || sortBy === 'lead' ? 'asc' : 'desc' +} + export default function Studies() { const [searchParams, setSearchParams] = useSearchParams() const { canWrite } = useUser() const showDeletedMessage = searchParams.get('deleted') === '1' - const [search, setSearch] = useState('') + // Search, sort, and filters live in the URL so links, back/forward, and the + // "Clear Filters" command all work. Defaults are left out of the URL. + const search = searchParams.get('q') ?? '' + const sortParam = searchParams.get('sort') + const sortBy: SortOption = SORT_OPTIONS.includes(sortParam as SortOption) ? (sortParam as SortOption) : 'date' + const sortDirection: 'asc' | 'desc' = + searchParams.get('dir') === 'asc' || searchParams.get('dir') === 'desc' + ? (searchParams.get('dir') as 'asc' | 'desc') + : defaultSortDirection(sortBy) + const typeParam = searchParams.get('type') + const filterType: FilterType = typeParam === 'longitudinal' || typeParam === 'cross-sectional' ? typeParam : 'all' + const filterLead = searchParams.get('lead') ?? '' + + /** Apply several URL changes in one update ('' or null removes a key). */ + const updateListParams = useCallback( + (changes: Record) => { + setSearchParams( + (prev) => { + const next = new URLSearchParams(prev) + for (const [key, value] of Object.entries(changes)) { + if (value == null || value === '') next.delete(key) + else next.set(key, value) + } + return next + }, + { replace: true }, + ) + }, + [setSearchParams], + ) + const setSearch = (value: string) => updateListParams({ q: value }) + const setFilterType = (value: FilterType) => updateListParams({ type: value === 'all' ? null : value }) + const setFilterLead = (value: string) => updateListParams({ lead: value }) + const [viewMode, setViewMode] = useState(() => { const saved = localStorage.getItem('studies-view-mode') return saved === 'grid' || saved === 'list' ? saved : 'grid' }) - const [sortBy, setSortBy] = useState('date') - const [sortDirection, setSortDirection] = useState<'asc' | 'desc'>('desc') - const [filterType, setFilterType] = useState('all') - const [filterLead, setFilterLead] = useState('') const { prefetch: prefetchSummaries, getCardState } = useStudySummaryCards() const summaryObserverRef = useRef(null) const loadMoreRef = useRef(null) @@ -399,14 +435,8 @@ export default function Studies() { value={sortBy} onChange={(e) => { const newSort = e.target.value as SortOption - setSortBy(newSort) - if (newSort === 'date') { - setSortDirection('desc') - } else if (newSort === 'title' || newSort === 'lead') { - setSortDirection('asc') - } else { - setSortDirection('desc') - } + // New sort resets to its natural direction (dir omitted = default). + updateListParams({ sort: newSort === 'date' ? null : newSort, dir: null }) }} className="form-select text-sm rounded-lg border px-3 py-2" style={{ borderColor: 'rgb(var(--app-border))' }} @@ -420,7 +450,10 @@ export default function Studies() { +} + describe('Studies', () => { beforeEach(() => { vi.clearAllMocks() @@ -58,4 +64,22 @@ describe('Studies', () => { await waitFor(() => expect(searchBox).toHaveValue('')) }) + + it('undoes a filter change with Back', async () => { + const { default: Studies } = await import('../Studies') + await render( + <> + + + , + { initialEntries: ['/studies'] }, + ) + const typeFilter = await screen.findByDisplayValue(/all types/i) + + fireEvent.change(typeFilter, { target: { value: 'longitudinal' } }) + await waitFor(() => expect(typeFilter).toHaveValue('longitudinal')) + + fireEvent.click(screen.getByRole('button', { name: 'Back' })) + await waitFor(() => expect(typeFilter).toHaveValue('all')) + }) }) From 5ae933eadab58015762dc4aacc413f3afca740cb Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Mon, 5 Oct 2026 22:02:48 -0700 Subject: [PATCH 30/35] fix(controls): keep wizard conflicts a 409 and say "control batch". createBatchesWithSpecimens wrapped every failure in a ValidationError, so a UNIQUE violation or ConflictError came back 400. Those now stay 409 with the readable conflict message; other failures stay 400. The message reads "Control batch N (...): ... No control batches were created.", per CONTEXT.md, as do the wizard's own fallbacks. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../__tests__/batch-with-specimens.test.ts | 34 ++++++++++++++++++- .../src/lib/controls/batch-with-specimens.ts | 11 ++++-- packages/api/src/lib/error-handler.ts | 2 +- .../web/src/components/wizards/ReviewStep.tsx | 4 +-- .../wizards/__tests__/ReviewStep.test.tsx | 4 +-- 5 files changed, 46 insertions(+), 9 deletions(-) diff --git a/packages/api/src/lib/controls/__tests__/batch-with-specimens.test.ts b/packages/api/src/lib/controls/__tests__/batch-with-specimens.test.ts index 6d8157fc..d0265e3c 100644 --- a/packages/api/src/lib/controls/__tests__/batch-with-specimens.test.ts +++ b/packages/api/src/lib/controls/__tests__/batch-with-specimens.test.ts @@ -1,3 +1,4 @@ +import { ConflictError } from '../../error-handler' import { describe, it, expect, beforeEach, afterEach } from 'vitest' import { setupTestDatabase, cleanupTestDatabase } from '../../../__tests__/helpers/db-setup' import { @@ -398,10 +399,41 @@ describe('batch-with-specimens', () => { { batch: { controlDefinitionId: def100.id, name: 'B100' }, specimens: [{ specimenTypeName: 'DNA', containers: [tube('MT1', 'A01')] }] }, { batch: { controlDefinitionId: def200.id, name: 'B200' }, specimens: [{ specimenTypeName: 'Nope', containers: [tube('MT2', 'A02')] }] }, ]), - ).rejects.toThrow(/Batch 2 \('B200'\).*No batches were created/) + ).rejects.toThrow(/^Control batch 2 \('B200'\): .*\. No control batches were created\.$/) expect(await testDb.select().from(controlBatch).all()).toHaveLength(0) expect(await testDb.select().from(storageContainer).all()).toHaveLength(0) }) + + it('reports a UNIQUE violation as a conflict', async () => { + const unit = await createTestUnit(testDb, { symbol: 'uL', name: 'microliter', category: 'volume' }) + await setContainerDefaults(testDb, { + micronix_tube: { totalQuantity: 1, remainingQuantity: 1, defaultUnitSymbol: 'uL' }, + }) + await testDb.insert(containerTypeUnit).values({ containerType: 'micronix_tube', unitId: unit.id }) + const def100 = await createTestControlDefinition(testDb, { name: 'D100' }) + const def200 = await createTestControlDefinition(testDb, { name: 'D200' }) + const specimenType = await createTestSpecimenType(testDb, { name: 'DNA' }) + await testDb.insert(specimenTypeContainerType).values({ specimenTypeId: specimenType.id, containerType: 'micronix_tube', created: utcNow() }) + const storageType = await createTestStorageType(testDb, { name: 'Freezer' }) + const location = await createTestLocation(testDb, { name: 'Loc', storageTypeId: String(storageType.id) }) + const plate = await createTestMicronixPlate(testDb, { name: 'Plate1', locationId: location.id }) + const tube = (barcode: string, position: string) => ({ + containerType: 'micronix_tube' as const, + barcode, + collection: { type: 'micronix_plate' as const, id: plate.id, position }, + }) + + // Stand-in for any constraint the app doesn't check first: one specimen per type. + sqlite.exec('CREATE UNIQUE INDEX test_one_specimen_per_type ON specimen(specimen_type_id)') + + const attempt = createBatchesWithSpecimens(testDb, [ + { batch: { controlDefinitionId: def100.id, name: 'B100' }, specimens: [{ specimenTypeName: 'DNA', containers: [tube('MT1', 'A01')] }] }, + { batch: { controlDefinitionId: def200.id, name: 'B200' }, specimens: [{ specimenTypeName: 'DNA', containers: [tube('MT2', 'A02')] }] }, + ]) + await expect(attempt).rejects.toBeInstanceOf(ConflictError) + await expect(attempt).rejects.toThrow(/^Control batch 2 \('B200'\): .*No control batches were created\.$/) + expect(await testDb.select().from(controlBatch).all()).toHaveLength(0) + }) }) }) diff --git a/packages/api/src/lib/controls/batch-with-specimens.ts b/packages/api/src/lib/controls/batch-with-specimens.ts index eb329278..7b03b5f3 100644 --- a/packages/api/src/lib/controls/batch-with-specimens.ts +++ b/packages/api/src/lib/controls/batch-with-specimens.ts @@ -26,7 +26,7 @@ import { import type { BatchContainerInput } from './batch-schemas' import { withWriteTransaction } from '../../db/write-transaction' import { validateCollectionDate } from '../validation' -import { ValidationError } from '../error-handler' +import { ConflictError, ValidationError, sqliteConstraintConflict } from '../error-handler' export interface CreateBatchWithSpecimensRequest { batch: { @@ -394,8 +394,13 @@ export async function createBatchesWithSpecimens( try { results.push(await createBatchWithSpecimens(tx, data)) } catch (error: unknown) { - const message = error instanceof Error ? error.message : String(error) - throw new ValidationError(`Batch ${index + 1} ('${data.batch.name}'): ${message}. No batches were created.`) + // Keep a conflict a 409 (ConflictError or a UNIQUE/FK violation); the rest are bad input. + const conflict = error instanceof Error ? sqliteConstraintConflict(error) : null + const reason = conflict ?? (error instanceof Error ? error.message : String(error)) + const sentence = /[.!?]$/.test(reason) ? reason : `${reason}.` + const message = `Control batch ${index + 1} ('${data.batch.name}'): ${sentence} No control batches were created.` + if (conflict || error instanceof ConflictError) throw new ConflictError(message) + throw new ValidationError(message) } } return results diff --git a/packages/api/src/lib/error-handler.ts b/packages/api/src/lib/error-handler.ts index 5b0b6462..6afbdd78 100644 --- a/packages/api/src/lib/error-handler.ts +++ b/packages/api/src/lib/error-handler.ts @@ -30,7 +30,7 @@ function isZodError(error: unknown): error is z.ZodError { } /** Readable message for a SQLite UNIQUE / FOREIGN KEY violation, or null for other errors. */ -function sqliteConstraintConflict(error: Error): string | null { +export function sqliteConstraintConflict(error: Error): string | null { const code = (error as { code?: unknown }).code if (code === 'SQLITE_CONSTRAINT_UNIQUE' || code === 'SQLITE_CONSTRAINT_PRIMARYKEY') { // "UNIQUE constraint failed: study.title, study.x" -> "title, x" diff --git a/packages/web/src/components/wizards/ReviewStep.tsx b/packages/web/src/components/wizards/ReviewStep.tsx index 177f3656..0420277f 100644 --- a/packages/web/src/components/wizards/ReviewStep.tsx +++ b/packages/web/src/components/wizards/ReviewStep.tsx @@ -211,14 +211,14 @@ export default function ReviewStep({ }) } if (batchPayloads.length === 0) { - setError('No batches were created') + setError('No control batches were created') return } // One request: the server creates every batch or none, so a retry cannot duplicate. const res = await controlsApi.createBatchesWithSpecimens({ batches: batchPayloads }) const created = res.batches if (created.length > 0) onSuccess(created[created.length - 1].batch.id) - else setError('No batches were created') + else setError('No control batches were created') } catch (err: unknown) { console.error('Multi-batch submit failed:', err) setError(getQueryErrorMessage(err, 'Failed to create batches')) diff --git a/packages/web/src/components/wizards/__tests__/ReviewStep.test.tsx b/packages/web/src/components/wizards/__tests__/ReviewStep.test.tsx index 74da5e26..2c141214 100644 --- a/packages/web/src/components/wizards/__tests__/ReviewStep.test.tsx +++ b/packages/web/src/components/wizards/__tests__/ReviewStep.test.tsx @@ -235,7 +235,7 @@ describe('ReviewStep multi-batch CSV errors', () => { mockSuggestBatchName.mockResolvedValue({ name: 'Suggested Batch' }) mockCreateBatchesWithSpecimens.mockRejectedValue( Object.assign(new Error('Request failed with status code 400'), { - response: { data: { error: "Batch 2 ('B'): Specimen type not found. No batches were created." } }, + response: { data: { error: "Control batch 2 ('B'): Specimen type not found. No control batches were created." } }, }), ) const def5000: ControlDefinition = { @@ -257,7 +257,7 @@ describe('ReviewStep multi-batch CSV errors', () => { fireEvent.click(screen.getByRole('button', { name: /Create Batch/i })) - expect(await screen.findByText(/No batches were created/)).toBeInTheDocument() + expect(await screen.findByText(/No control batches were created/)).toBeInTheDocument() }) }) From 9b37827b02634bd70a686dba6b1ee873233da836 Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Mon, 5 Oct 2026 22:05:11 -0700 Subject: [PATCH 31/35] test(web): cover the qPCR well and Locations search races. 5a3ceb1 fixed both without tests. Each new test fails with the stale-response guard removed: a slow reply for well A01 must not replace well B01's container, and a slow "PL-1" collection search must not replace "PL-12" results. The three qPCR well edits share one applyUpdatedWells helper instead of three copies. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../web/src/pages/QpcrExperimentDetail.tsx | 18 ++++---- .../src/pages/__tests__/Locations.test.tsx | 27 ++++++++++++ .../__tests__/QpcrExperimentDetail.test.tsx | 41 +++++++++++++++++++ 3 files changed, 77 insertions(+), 9 deletions(-) diff --git a/packages/web/src/pages/QpcrExperimentDetail.tsx b/packages/web/src/pages/QpcrExperimentDetail.tsx index bcdc590d..665c5437 100644 --- a/packages/web/src/pages/QpcrExperimentDetail.tsx +++ b/packages/web/src/pages/QpcrExperimentDetail.tsx @@ -436,14 +436,18 @@ export default function QpcrExperimentDetail() { return w != null && w.contentType === 'negative' }) + /** Show the server's wells and refresh the cached experiment, or revisiting the page shows the old wells. */ + const applyUpdatedWells = (wells: QpcrExperimentWell[]) => { + setData((prev) => (prev ? { ...prev, wells } : null)) + invalidateQpcrExperimentQueries(queryClient) + } + const handleSetWellContentType = async (contentType: 'empty' | 'negative') => { if (!id || !selectedWellPosition) return setWellsUpdating(true) try { const res = await qpcrExperimentsApi.updateWells(parseInt(id), { wellPosition: selectedWellPosition, contentType }) - setData((prev) => (prev ? { ...prev, wells: res.wells } : null)) - // Keep the cached experiment in step, or revisiting the page shows the old wells. - invalidateQpcrExperimentQueries(queryClient) + applyUpdatedWells(res.wells) showSuccess(contentType === 'negative' ? 'Well set as NTC' : 'Well set as empty') } catch (err: unknown) { const msg = err && typeof err === 'object' && 'response' in err && typeof (err as { response?: { data?: { error?: string } } }).response?.data?.error === 'string' @@ -460,9 +464,7 @@ export default function QpcrExperimentDetail() { setWellsUpdating(true) try { const res = await qpcrExperimentsApi.updateWells(parseInt(id), { positions: emptyWellPositions, contentType: 'negative' }) - setData((prev) => (prev ? { ...prev, wells: res.wells } : null)) - // Keep the cached experiment in step, or revisiting the page shows the old wells. - invalidateQpcrExperimentQueries(queryClient) + applyUpdatedWells(res.wells) showSuccess(`Set ${emptyWellPositions.length} well(s) to NTC`) } catch (err: unknown) { const msg = err && typeof err === 'object' && 'response' in err && typeof (err as { response?: { data?: { error?: string } } }).response?.data?.error === 'string' @@ -479,9 +481,7 @@ export default function QpcrExperimentDetail() { setWellsUpdating(true) try { const res = await qpcrExperimentsApi.updateWells(parseInt(id), { positions: ntcWellPositions, contentType: 'empty' }) - setData((prev) => (prev ? { ...prev, wells: res.wells } : null)) - // Keep the cached experiment in step, or revisiting the page shows the old wells. - invalidateQpcrExperimentQueries(queryClient) + applyUpdatedWells(res.wells) showSuccess(`Set ${ntcWellPositions.length} well(s) to empty`) } catch (err: unknown) { const msg = err && typeof err === 'object' && 'response' in err && typeof (err as { response?: { data?: { error?: string } } }).response?.data?.error === 'string' diff --git a/packages/web/src/pages/__tests__/Locations.test.tsx b/packages/web/src/pages/__tests__/Locations.test.tsx index 816eab27..9f74ee83 100644 --- a/packages/web/src/pages/__tests__/Locations.test.tsx +++ b/packages/web/src/pages/__tests__/Locations.test.tsx @@ -1,4 +1,5 @@ import { describe, it, expect, vi, beforeEach } from 'vitest' +import userEvent from '@testing-library/user-event' import { render, screen, waitFor } from '../../__tests__/helpers/render' import Locations from '../Locations' @@ -27,6 +28,7 @@ vi.mock('../../contexts/UserContext', async () => { }) import { locationsApi } from '../../lib/api/locations' +import { searchApi, type SearchResponse, type SearchResult } from '../../lib/api/search' describe('Locations page', () => { beforeEach(() => { @@ -75,4 +77,29 @@ describe('Locations page', () => { expect(screen.getByRole('button', { name: /try again/i })).toBeInTheDocument() }) }) + + it('ignores a collection search reply that arrives after a newer one', async () => { + const plate = (id: number, title: string): SearchResult => ({ + type: 'micronix_plate', id, title, subtitle: '', url: '', data: null, name: title, + }) + const replies: Array<(value: SearchResponse) => void> = [] + vi.mocked(searchApi.search).mockImplementation( + () => new Promise((resolve) => { replies.push(resolve) }) as ReturnType, + ) + const user = userEvent.setup() + await render() + const box = await screen.findByPlaceholderText(/Search collections/) + + await user.type(box, 'PL-1') + await waitFor(() => expect(replies).toHaveLength(1)) + await user.type(box, '2') + await waitFor(() => expect(replies).toHaveLength(2)) + + replies[1]({ query: 'PL-12', count: 1, results: [plate(12, 'PL-12 plate')] }) + expect(await screen.findByText('PL-12 plate')).toBeInTheDocument() + replies[0]({ query: 'PL-1', count: 1, results: [plate(1, 'PL-1 plate')] }) + await new Promise((r) => setTimeout(r, 50)) + expect(screen.queryByText('PL-1 plate')).not.toBeInTheDocument() + expect(screen.getByText('PL-12 plate')).toBeInTheDocument() + }) }) diff --git a/packages/web/src/pages/__tests__/QpcrExperimentDetail.test.tsx b/packages/web/src/pages/__tests__/QpcrExperimentDetail.test.tsx index 409b2793..d69387fc 100644 --- a/packages/web/src/pages/__tests__/QpcrExperimentDetail.test.tsx +++ b/packages/web/src/pages/__tests__/QpcrExperimentDetail.test.tsx @@ -1,8 +1,10 @@ import { describe, it, expect, vi, beforeEach } from 'vitest' +import userEvent from '@testing-library/user-event' import { render, screen, waitFor } from '../../__tests__/helpers/render' import QpcrExperimentDetail from '../QpcrExperimentDetail' import { qpcrExperimentsApi } from '../../lib/api/qpcr' import { settingsApi } from '../../lib/api/settings' +import { containersApi } from '../../lib/api/containers' vi.mock('react-router-dom', async () => { const actual = await vi.importActual('react-router-dom') @@ -36,6 +38,11 @@ vi.mock('../../lib/api/settings', async () => { }) }) +vi.mock('../../lib/api/containers', async () => { + const { createMockedDomainModule } = await import('../../__tests__/helpers/mock-api') + return createMockedDomainModule('containers', { containersApi: { get: vi.fn() } }) +}) + vi.mock('../../contexts/UserContext', async () => { const actual = await vi.importActual( '../../contexts/UserContext' @@ -79,4 +86,38 @@ describe('QpcrExperimentDetail', () => { expect(screen.getByRole('button', { name: /try again/i })).toBeInTheDocument() }) }) + + it("keeps the selected well's container when an earlier well's reply arrives late", async () => { + const well = (id: number, wellPosition: string, storageContainerId: number) => ({ + id, qpcrExperimentId: 1, wellPosition, barcode: `MT-${wellPosition}`, storageContainerId, specimenId: null, + contentType: 'unknown', standardDensity: null, cq: null, startingQuantity: null, cqMean: null, rawSampleName: null, + }) + vi.mocked(qpcrExperimentsApi.get).mockResolvedValue({ + ...mockDetail, + wells: [well(1, 'A01', 101), well(2, 'B01', 102)], + } as never) + const replies = new Map void>() + vi.mocked(containersApi.get).mockImplementation( + (containerId: number) => new Promise((resolve) => { replies.set(containerId, resolve) }) as never, + ) + const container = (id: number, plateName: string) => ({ + container: { id, containerType: 'micronix_tube', barcode: `MT-${id}`, collection: { id: 1, name: plateName, position: 'A01' } }, + specimen: null, + source: null, + }) + const user = userEvent.setup() + await render() + + await user.click(await screen.findByRole('button', { name: /^Well A01/ })) + await waitFor(() => expect(replies.has(101)).toBe(true)) + await user.click(screen.getByRole('button', { name: /^Well B01/ })) + await waitFor(() => expect(replies.has(102)).toBe(true)) + + replies.get(102)!(container(102, 'Plate B')) + expect(await screen.findByText(/Plate B/)).toBeInTheDocument() + replies.get(101)!(container(101, 'Plate A')) + await new Promise((r) => setTimeout(r, 50)) + expect(screen.queryByText(/Plate A/)).not.toBeInTheDocument() + expect(screen.getByText(/Plate B/)).toBeInTheDocument() + }) }) From 92b0882bc0048cc92ce716268e9c58e36807632e Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Mon, 5 Oct 2026 22:06:27 -0700 Subject: [PATCH 32/35] test(api): cover the integrity script's tag/qPCR checks and settings userId. 938057f and 6de04c6 shipped without tests. The integrity test plants an orphaned tag link (no id column) and qPCR well, and fails if ids come from .id instead of rowid; checkDatabaseIntegrity is exported for it. The settings test sends userId "abc", -1, and 1.5, each now a 400. .gitignore's blanket scripts/ rule also hid packages/api/src/scripts (its tracked files were force-added), so it is now an exception like the other script folders. Co-Authored-By: Claude Opus 5.5 (1M context) --- .gitignore | 1 + .../api/src/routes/__tests__/settings.test.ts | 12 ++++++ .../scripts/__tests__/check-integrity.test.ts | 40 +++++++++++++++++++ packages/api/src/scripts/check-integrity.ts | 2 +- 4 files changed, 54 insertions(+), 1 deletion(-) create mode 100644 packages/api/src/scripts/__tests__/check-integrity.test.ts diff --git a/.gitignore b/.gitignore index 5caa4c1a..9ea36f71 100644 --- a/.gitignore +++ b/.gitignore @@ -73,6 +73,7 @@ packages/e2e/sampledb_e2e.sqlite scripts/ !packages/api/scripts/ !packages/api/scripts/seed-table-view-config.ts +!packages/api/src/scripts/ !packages/docs/scripts/ test-csvs/ diff --git a/packages/api/src/routes/__tests__/settings.test.ts b/packages/api/src/routes/__tests__/settings.test.ts index 66c9638d..b967436b 100644 --- a/packages/api/src/routes/__tests__/settings.test.ts +++ b/packages/api/src/routes/__tests__/settings.test.ts @@ -46,4 +46,16 @@ describe('Settings API', () => { }) }) + describe('PUT /api/settings/:key', () => { + for (const userId of ['abc', -1, 1.5]) { + it(`rejects userId ${JSON.stringify(userId)} with 400`, async () => { + const res = await ctx.request('/api/settings/session_settings', { + method: 'PUT', + json: { userId, timeoutMinutes: 60 }, + }) + expect(res.status).toBe(400) + expect(((await res.json()) as { error: string }).error).toMatch(/userId must be a positive integer or null/) + }) + } + }) }) diff --git a/packages/api/src/scripts/__tests__/check-integrity.test.ts b/packages/api/src/scripts/__tests__/check-integrity.test.ts new file mode 100644 index 00000000..4c21a39e --- /dev/null +++ b/packages/api/src/scripts/__tests__/check-integrity.test.ts @@ -0,0 +1,40 @@ +import { describe, it, expect, beforeEach, afterEach } from 'vitest' +import type { Database } from 'bun:sqlite' +import { setupTestDatabase, cleanupTestDatabase } from '../../__tests__/helpers/db-setup' +import { checkDatabaseIntegrity } from '../check-integrity' + +describe('checkDatabaseIntegrity', () => { + let sqlite: Database + + beforeEach(async () => { + sqlite = (await setupTestDatabase()).sqlite + }) + + afterEach(() => { + cleanupTestDatabase(sqlite) + }) + + it('reports a clean database as clean', () => { + expect(checkDatabaseIntegrity(sqlite).totalIssues).toBe(0) + }) + + it('reports orphaned tag links, which have no id column', () => { + sqlite.exec('PRAGMA foreign_keys = OFF') + sqlite.exec('INSERT INTO storage_container_tag (storage_container_id, tag_id) VALUES (999, 998)') + sqlite.exec('PRAGMA foreign_keys = ON') + + const tables = checkDatabaseIntegrity(sqlite).issues.map((issue) => `${issue.table}.${issue.column}`) + expect(tables).toContain('storage_container_tag.storage_container_id') + expect(tables).toContain('storage_container_tag.tag_id') + }) + + it('reports qPCR wells pointing at missing containers', () => { + sqlite.exec('PRAGMA foreign_keys = OFF') + sqlite.exec(`INSERT INTO qpcr_experiment_well (qpcr_experiment_id, well_position, storage_container_id) VALUES (999, 'A01', 998)`) + sqlite.exec('PRAGMA foreign_keys = ON') + + const tables = checkDatabaseIntegrity(sqlite).issues.map((issue) => `${issue.table}.${issue.column}`) + expect(tables).toContain('qpcr_experiment_well.qpcr_experiment_id') + expect(tables).toContain('qpcr_experiment_well.storage_container_id') + }) +}) diff --git a/packages/api/src/scripts/check-integrity.ts b/packages/api/src/scripts/check-integrity.ts index 1bc4eca0..52b76861 100644 --- a/packages/api/src/scripts/check-integrity.ts +++ b/packages/api/src/scripts/check-integrity.ts @@ -154,7 +154,7 @@ function getOrphanedIds( /** * Main integrity check function */ -function checkDatabaseIntegrity(sqlite: any): IntegrityReport { +export function checkDatabaseIntegrity(sqlite: any): IntegrityReport { const issues: IntegrityIssue[] = [] const startTime = new Date().toISOString() From 538485ec439813cbe895e512624e066eaf9fd540 Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Mon, 5 Oct 2026 22:07:00 -0700 Subject: [PATCH 33/35] test(web): write CollectionSpecimenEntry tests the way the test README asks. Use the render helper, createMockedDomainModule, and user-event. The scanner test now checks that Enter starts no submit (no half-filled-row error), and still fails with the Enter guard removed. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../CollectionSpecimenEntry.test.tsx | 33 ++++++++++--------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/packages/web/src/components/__tests__/CollectionSpecimenEntry.test.tsx b/packages/web/src/components/__tests__/CollectionSpecimenEntry.test.tsx index 0ebf44b1..eaffa6c0 100644 --- a/packages/web/src/components/__tests__/CollectionSpecimenEntry.test.tsx +++ b/packages/web/src/components/__tests__/CollectionSpecimenEntry.test.tsx @@ -1,5 +1,6 @@ import { describe, it, expect, vi, beforeEach } from 'vitest' -import { render, screen, fireEvent } from '@testing-library/react' +import userEvent from '@testing-library/user-event' +import { render, screen } from '../../__tests__/helpers/render' import CollectionSpecimenEntry from '../CollectionSpecimenEntry' import { specimensApi } from '../../lib/api/specimens' @@ -13,12 +14,13 @@ vi.mock('../../hooks/useStudies', () => ({ vi.mock('../../hooks/useReferenceData', () => ({ useSpecimenTypes: () => ({ data: [{ id: 1, name: 'DNA' }], isLoading: false, isError: false }), })) -vi.mock('../../lib/api/specimens', () => ({ - specimensApi: { createBulk: vi.fn() }, -})) +vi.mock('../../lib/api/specimens', async () => { + const { createMockedDomainModule } = await import('../../__tests__/helpers/mock-api') + return createMockedDomainModule('specimens', { specimensApi: { createBulk: vi.fn() } }) +}) -function renderEntry() { - return render( +async function renderEntry() { + await render( { vi.clearAllMocks() }) - it('does not submit when a barcode scanner sends Enter', () => { - renderEntry() - const barcode = screen.getAllByPlaceholderText('Barcode')[0] - fireEvent.change(barcode, { target: { value: 'MT001' } }) + it('does not submit when a barcode scanner sends Enter', async () => { + const user = userEvent.setup() + await renderEntry() - const notPrevented = fireEvent.keyDown(barcode, { key: 'Enter' }) + await user.type(screen.getAllByPlaceholderText('Barcode')[0], 'MT001{Enter}') - expect(notPrevented).toBe(false) + // A submit would validate the half-filled row and show this message. + expect(screen.queryByText(/Complete or clear these positions/)).not.toBeInTheDocument() expect(specimensApi.createBulk).not.toHaveBeenCalled() }) it('reports partly filled positions instead of skipping them', async () => { - renderEntry() - fireEvent.change(screen.getAllByPlaceholderText('Subject name')[1], { target: { value: 'S2' } }) + const user = userEvent.setup() + await renderEntry() + await user.type(screen.getAllByPlaceholderText('Subject name')[1], 'S2') - fireEvent.click(screen.getByRole('button', { name: /create|add|save/i })) + await user.click(screen.getByRole('button', { name: /create|add|save/i })) expect(await screen.findByText(/Complete or clear these positions.*A02/)).toBeInTheDocument() expect(specimensApi.createBulk).not.toHaveBeenCalled() From ff7dd54a501844585dfff4ca1ba769124b0ffec0 Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Mon, 5 Oct 2026 22:10:33 -0700 Subject: [PATCH 34/35] refactor: share the blank-row CSV filter and calendar-date shift. - parseCsv takes skipBlankRows instead of four callers pasting the same filter. - shiftCalendarDate (lib/datetime.ts) replaces the two inline Date.UTC day shifts in the stats upper bound and export tolerance window. - The "Create batch with specimens" comment is back on its route. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../api/src/lib/__tests__/datetime.test.ts | 18 ++++++++++++++++++ packages/api/src/lib/datetime.ts | 10 ++++++++++ packages/api/src/lib/export/query.ts | 12 ++++++------ packages/api/src/lib/plate-csv.ts | 2 +- packages/api/src/lib/statistics/helpers.ts | 7 +++---- packages/api/src/routes/controls.ts | 2 +- packages/contract/src/__tests__/csv.test.ts | 6 ++++++ packages/contract/src/csv.ts | 9 ++++++++- packages/web/src/components/BulkImportFlow.tsx | 2 +- packages/web/src/lib/bulk-import-validation.ts | 2 +- packages/web/src/lib/scanner-plate-csv.ts | 2 +- 11 files changed, 56 insertions(+), 16 deletions(-) create mode 100644 packages/api/src/lib/__tests__/datetime.test.ts diff --git a/packages/api/src/lib/__tests__/datetime.test.ts b/packages/api/src/lib/__tests__/datetime.test.ts new file mode 100644 index 00000000..48eb498e --- /dev/null +++ b/packages/api/src/lib/__tests__/datetime.test.ts @@ -0,0 +1,18 @@ +import { describe, it, expect } from 'vitest' +import { shiftCalendarDate } from '../datetime' + +describe('shiftCalendarDate', () => { + it('rolls over months and years', () => { + expect(shiftCalendarDate('2024-01-31', 1)).toBe('2024-02-01') + expect(shiftCalendarDate('2024-03-01', -1)).toBe('2024-02-29') + expect(shiftCalendarDate('2023-12-31', 1)).toBe('2024-01-01') + }) + + it('reads the date part of a timestamp', () => { + expect(shiftCalendarDate('2024-05-26 22:28:26', 0)).toBe('2024-05-26') + }) + + it('returns null for text that does not start with a date', () => { + expect(shiftCalendarDate('1/5/2024', 1)).toBeNull() + }) +}) diff --git a/packages/api/src/lib/datetime.ts b/packages/api/src/lib/datetime.ts index ae8e4d05..538aba43 100644 --- a/packages/api/src/lib/datetime.ts +++ b/packages/api/src/lib/datetime.ts @@ -2,3 +2,13 @@ export function utcNow(): string { return new Date().toISOString().replace('T', ' ').replace('Z', '').split('.')[0] } + +/** + * The calendar date `days` after a value that starts with YYYY-MM-DD (negative goes back), or + * null when it doesn't start with one. Month and year rollovers follow the calendar. + */ +export function shiftCalendarDate(date: string, days: number): string | null { + const match = /^(\d{4})-(\d{2})-(\d{2})/.exec(date) + if (!match) return null + return new Date(Date.UTC(Number(match[1]), Number(match[2]) - 1, Number(match[3]) + days)).toISOString().slice(0, 10) +} diff --git a/packages/api/src/lib/export/query.ts b/packages/api/src/lib/export/query.ts index 4864b295..c7c36d90 100644 --- a/packages/api/src/lib/export/query.ts +++ b/packages/api/src/lib/export/query.ts @@ -22,6 +22,7 @@ import type { } from './types' import { buildExportSummary, validateStudyCodes } from './validate' import { chunkArray, dateToUpperBound } from '../statistics/helpers' +import { shiftCalendarDate } from '../datetime' export async function buildContainerQuery(database: Database, filters: ExportFilters) { // First, get the study and its subjects @@ -280,13 +281,12 @@ export async function buildContainerQueryByMicronixBarcodes( /** Collection date condition for one per-subject filter (exact date +/- tolerance days, or a range). */ function subjectDateCondition(dateFilter: SubjectDateFilter, toleranceDays: number) { if ('exact' in dateFilter) { - const match = /^(\d{4})-(\d{2})-(\d{2})/.exec(dateFilter.exact) - if (!match) return eq(specimen.collectionDate, dateFilter.exact) - const shift = (days: number) => - new Date(Date.UTC(Number(match[1]), Number(match[2]) - 1, Number(match[3]) + days)).toISOString().slice(0, 10) + const from = shiftCalendarDate(dateFilter.exact, -toleranceDays) + const to = shiftCalendarDate(dateFilter.exact, toleranceDays) + if (!from || !to) return eq(specimen.collectionDate, dateFilter.exact) return and( - gte(specimen.collectionDate, shift(-toleranceDays)), - lte(specimen.collectionDate, shift(toleranceDays)), + gte(specimen.collectionDate, from), + lte(specimen.collectionDate, to), ) } const bounds = [ diff --git a/packages/api/src/lib/plate-csv.ts b/packages/api/src/lib/plate-csv.ts index feaf90fd..0b46dbcb 100644 --- a/packages/api/src/lib/plate-csv.ts +++ b/packages/api/src/lib/plate-csv.ts @@ -32,7 +32,7 @@ export function parsePlateCSV( config: ScannerConfiguration ): { wellPosition: string; barcode: string }[] { // Shared parser: handles quoted fields ("A01","FR123"), BOM, and CRLF. - const lines = parseCsv(csvText).filter((cells) => cells.some((cell) => cell.trim() !== '')) + const lines = parseCsv(csvText, { skipBlankRows: true }) if (lines.length <= config.skipRows) return [] const headers = lines[config.skipRows].map((h) => h.trim()) const rows: { wellPosition: string; barcode: string }[] = [] diff --git a/packages/api/src/lib/statistics/helpers.ts b/packages/api/src/lib/statistics/helpers.ts index 68574ecd..464f4a8d 100644 --- a/packages/api/src/lib/statistics/helpers.ts +++ b/packages/api/src/lib/statistics/helpers.ts @@ -1,5 +1,6 @@ import { gte, lt, lte } from 'drizzle-orm' import type { SQLiteColumn } from 'drizzle-orm/sqlite-core' +import { shiftCalendarDate } from '../datetime' /** Build date filter conditions for specimen date columns. */ export function buildDateFilter(column: SQLiteColumn, dateFrom?: string, dateTo?: string) { @@ -18,10 +19,8 @@ export function buildDateFilter(column: SQLiteColumn, dateFrom?: string, dateTo? * timestamp column ('2026-05-26 22:28:26') is compared with "< next day", not "<= date". */ export function dateToUpperBound(column: SQLiteColumn, dateTo: string) { - const match = /^(\d{4})-(\d{2})-(\d{2})$/.exec(dateTo) - if (!match) return lte(column, dateTo) - const next = new Date(Date.UTC(Number(match[1]), Number(match[2]) - 1, Number(match[3]) + 1)) - return lt(column, next.toISOString().slice(0, 10)) + const next = /^\d{4}-\d{2}-\d{2}$/.test(dateTo) ? shiftCalendarDate(dateTo, 1) : null + return next ? lt(column, next) : lte(column, dateTo) } /** Batch an array into chunks for SQLite variable limits. */ diff --git a/packages/api/src/routes/controls.ts b/packages/api/src/routes/controls.ts index 075e418b..f322de26 100644 --- a/packages/api/src/routes/controls.ts +++ b/packages/api/src/routes/controls.ts @@ -718,7 +718,6 @@ controls.post('/:id/batches', memberMiddleware, async (c) => { } }) -// Create batch with specimens // Create several batches (one per density) all-or-nothing controls.post('/batches/create-many-with-specimens', memberMiddleware, async (c) => { try { @@ -731,6 +730,7 @@ controls.post('/batches/create-many-with-specimens', memberMiddleware, async (c) } }) +// Create batch with specimens controls.post('/batches/create-with-specimens', memberMiddleware, async (c) => { try { const body = await c.req.json() diff --git a/packages/contract/src/__tests__/csv.test.ts b/packages/contract/src/__tests__/csv.test.ts index 7028459f..a74192b8 100644 --- a/packages/contract/src/__tests__/csv.test.ts +++ b/packages/contract/src/__tests__/csv.test.ts @@ -127,6 +127,12 @@ describe('serializeCsv + parseCsv round trip', () => { expect(parseCsv(csv, { delimiter: ';' })[1]).toEqual(['1', 'low; hemolyzed']) }) + it('drops blank and comma-only rows when asked', () => { + const text = 'a,b\r\n1,2\r\n,\r\n , \r\n3,4\r\n' + expect(parseCsv(text, { skipBlankRows: true })).toEqual([['a', 'b'], ['1', '2'], ['3', '4']]) + expect(parseCsv(text)).toHaveLength(5) + }) + it('neutralizes formula-like text cells only when asked', () => { const rows = [['=SUM(A1)', '+cmd', '@x', '-A1+1', "-1+cmd|'/C calc'!A0", '-131', '-0.5', '-2 in SubjID', -5, 'ok']] const neutralized = serializeCsv(['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j'], rows, { diff --git a/packages/contract/src/csv.ts b/packages/contract/src/csv.ts index 986a507d..765fd69a 100644 --- a/packages/contract/src/csv.ts +++ b/packages/contract/src/csv.ts @@ -64,7 +64,14 @@ export function serializeCsv( return bom ? `\uFEFF${content}` : content } -export function parseCsv(input: string, options: { delimiter?: string } = {}): string[][] { +/** Parse CSV text into rows of cells. skipBlankRows drops rows whose cells are all empty or whitespace. */ +export function parseCsv( + input: string, + options: { delimiter?: string; skipBlankRows?: boolean } = {}, +): string[][] { + if (options.skipBlankRows) { + return parseCsv(input, { delimiter: options.delimiter }).filter((cells) => cells.some((cell) => cell.trim() !== '')) + } const delimiter = options.delimiter ?? DEFAULT_DELIMITER let text = input if (text.charCodeAt(0) === 0xfeff) { diff --git a/packages/web/src/components/BulkImportFlow.tsx b/packages/web/src/components/BulkImportFlow.tsx index 8bbcd382..a247dd62 100644 --- a/packages/web/src/components/BulkImportFlow.tsx +++ b/packages/web/src/components/BulkImportFlow.tsx @@ -137,7 +137,7 @@ export default function BulkImportFlow({ fixedStudyShortCode, backLink }: BulkIm const reader = new FileReader() reader.onload = (event) => { const text = event.target?.result as string - const lines = parseCsv(text).filter((cells) => cells.some((cell) => cell.trim() !== '')) + const lines = parseCsv(text, { skipBlankRows: true }) if (lines.length === 0) return const headers = lines[0].map(h => h.trim()) diff --git a/packages/web/src/lib/bulk-import-validation.ts b/packages/web/src/lib/bulk-import-validation.ts index 4fed1cbc..1027be77 100644 --- a/packages/web/src/lib/bulk-import-validation.ts +++ b/packages/web/src/lib/bulk-import-validation.ts @@ -83,7 +83,7 @@ function normalizeHeader(header: string): string { export function parseBulkImportCSV(text: string): CSVRow[] { // Shared RFC 4180 parser: quoted commas ("low vol, hemolyzed"), quotes, CRLF, and BOM. - const lines = parseCsv(text).filter((cells) => cells.some((cell) => cell.trim() !== '')) + const lines = parseCsv(text, { skipBlankRows: true }) if (lines.length < 2) return [] const headers = lines[0].map((h) => normalizeHeader(h.trim())) diff --git a/packages/web/src/lib/scanner-plate-csv.ts b/packages/web/src/lib/scanner-plate-csv.ts index 90750852..206ee904 100644 --- a/packages/web/src/lib/scanner-plate-csv.ts +++ b/packages/web/src/lib/scanner-plate-csv.ts @@ -18,7 +18,7 @@ function buildPosition(config: ScannerConfiguration, row: ScannerPlateCsvRow): s * Parse a scanner plate CSV using column mapping from scanner configuration. */ export function parseScannerPlateCsv(text: string, config: ScannerConfiguration): ScannerPlateCsvRow[] { - const lines = parseCsv(text).filter((cells) => cells.some((cell) => cell.trim() !== '')) + const lines = parseCsv(text, { skipBlankRows: true }) if (lines.length < 2 + config.skipRows) return [] const headers = lines[config.skipRows].map((h) => h.trim()) From c03663f3d2af51e9817976f175cbeb21b82249f7 Mon Sep 17 00:00:00 2001 From: Maxwell Murphy Date: Mon, 5 Oct 2026 22:11:56 -0700 Subject: [PATCH 35/35] refactor(api): reuse the composition finder and study specimen subquery. - Control definition PATCH calls findBloodControlDefinitionByComposition with a new excludeId option instead of re-creating its query; a test checks a definition doesn't conflict with itself. - Study delete names the specimen subquery once instead of inlining it twice. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../src/lib/controls/strain-composition.ts | 7 ++++--- .../api/src/routes/__tests__/controls.test.ts | 10 ++++++++++ packages/api/src/routes/controls.ts | 20 +++++++++---------- packages/api/src/routes/studies.ts | 7 +++---- 4 files changed, 27 insertions(+), 17 deletions(-) diff --git a/packages/api/src/lib/controls/strain-composition.ts b/packages/api/src/lib/controls/strain-composition.ts index c3c6118a..69956545 100644 --- a/packages/api/src/lib/controls/strain-composition.ts +++ b/packages/api/src/lib/controls/strain-composition.ts @@ -1,5 +1,5 @@ import { z } from 'zod' -import { eq, inArray } from 'drizzle-orm' +import { and, eq, inArray, ne } from 'drizzle-orm' import type { Database } from '../../db/client' import { controlDefinition, strain } from '../../db/schema' @@ -175,12 +175,13 @@ export async function resolveStrainComposition( export async function findBloodControlDefinitionByComposition( database: Database, criteria: CompositionCriteria, - options?: CompositionMatchOptions, + options?: CompositionMatchOptions & { /** Skip this definition, e.g. the one being edited. */ excludeId?: number }, ): Promise<(typeof controlDefinition.$inferSelect) | null> { + const isBlood = eq(controlDefinition.controlType, 'blood') const definitions = await database .select() .from(controlDefinition) - .where(eq(controlDefinition.controlType, 'blood')) + .where(options?.excludeId === undefined ? isBlood : and(isBlood, ne(controlDefinition.id, options.excludeId))) for (const definition of definitions) { if (definitionCompositionMatches(definition.properties, criteria, options)) { diff --git a/packages/api/src/routes/__tests__/controls.test.ts b/packages/api/src/routes/__tests__/controls.test.ts index 38c8d3cb..284ac411 100644 --- a/packages/api/src/routes/__tests__/controls.test.ts +++ b/packages/api/src/routes/__tests__/controls.test.ts @@ -417,6 +417,16 @@ describe('Controls API', () => { expect(res.status).toBe(409) }) + + it('does not count the definition being edited as a duplicate', async () => { + const strain = await createTestStrain(ctx.db, { name: '3D7' }) + const strains = [{ id: strain.id, name: '3D7', percentage: 100 }] + const def = await createTestControlDefinition(ctx.db, { name: 'D200', properties: { strains, targetDensity: 200 } }) + + const res = await ctx.request(`${BASE}/${def.id}`, { method: 'PATCH', json: { targetDensity: 200 } }) + + expect(res.status).toBe(200) + }) }) describe(`PATCH ${BASE}/batches/:id`, () => { diff --git a/packages/api/src/routes/controls.ts b/packages/api/src/routes/controls.ts index f322de26..7328f40c 100644 --- a/packages/api/src/routes/controls.ts +++ b/packages/api/src/routes/controls.ts @@ -601,19 +601,19 @@ controls.patch('/:id', memberMiddleware, async (c) => { // Same rule as create: no two definitions with the same density and strain mix. if (strains !== undefined || targetDensity !== undefined || targetDensityUnitId !== undefined) { const storedStrains = (newProps.strains as Array<{ id: number; percentage: number }> | undefined) ?? [] - const others = await dbInstance - .select() - .from(controlDefinition) - .where(and(eq(controlDefinition.controlType, 'blood'), sql`${controlDefinition.id} != ${id}`)) - const duplicate = findMatchingDefinitionInList(others, { - strains: storedStrains.map((strain) => ({ strainId: strain.id, percentage: strain.percentage })), - targetDensity: newProps.targetDensity as number | undefined, - targetDensityUnitId: newProps.targetDensityUnitId as number | undefined, - }) + const duplicate = await findBloodControlDefinitionByComposition( + dbInstance, + { + strains: storedStrains.map((strain) => ({ strainId: strain.id, percentage: strain.percentage })), + targetDensity: newProps.targetDensity as number | undefined, + targetDensityUnitId: newProps.targetDensityUnitId as number | undefined, + }, + { excludeId: id }, + ) if (duplicate) { return c.json({ error: 'A control definition with this combination of density and strains already exists', - existingDefinition: duplicate.definition, + existingDefinition: duplicate, }, 409) } } diff --git a/packages/api/src/routes/studies.ts b/packages/api/src/routes/studies.ts index d0e8ac7d..e006ccd8 100644 --- a/packages/api/src/routes/studies.ts +++ b/packages/api/src/routes/studies.ts @@ -722,14 +722,13 @@ studies.delete('/:id', memberMiddleware, async (c) => { const subjectIds = subjects.map((s) => s.id) // Subqueries, not id lists, so large studies stay under SQLite's parameter limit. const studySubjectIds = database.select({ id: studySubject.id }).from(studySubject).where(eq(studySubject.studyId, id)) - const specimenIds = ( - await database.select({ id: specimen.id }).from(specimen).where(inArray(specimen.studySubjectId, studySubjectIds)) - ).map((s) => s.id) + const studySpecimenIds = database.select({ id: specimen.id }).from(specimen).where(inArray(specimen.studySubjectId, studySubjectIds)) + const specimenIds = (await studySpecimenIds).map((s) => s.id) const containerIds = ( await database .select({ id: storageContainer.id }) .from(storageContainer) - .where(inArray(storageContainer.specimenId, database.select({ id: specimen.id }).from(specimen).where(inArray(specimen.studySubjectId, studySubjectIds)))) + .where(inArray(storageContainer.specimenId, studySpecimenIds)) ).map((c) => c.id) await assertNotUsedInQpcr(database, { containerIds, specimenIds }, `study '${existingStudy.shortCode}'`)