From 508e7abf5054b986e4dae0006ff2850878b0fb50 Mon Sep 17 00:00:00 2001 From: "Gabriel A. Devenyi" Date: Mon, 20 Jul 2026 15:11:21 -0400 Subject: [PATCH 1/7] perf(web): match export rows against a set of listed subject ids The export filter built its list of subject ids by iterating each row's visible cells: .rows.flatMap((row) => row.getVisibleCells().map((cell) => removeSubjectIdScope(cell.row.original.id))) The callback ignores `cell` and reads `cell.row.original.id`, so the row's id is emitted once per rendered column. Measured against the real table, which has three data columns plus the row-actions column libui adds: 5,000 rows produce a 20,000 entry array, a 4x duplication, with `removeSubjectIdScope` called four times per row. That array was then the right-hand side of an `includes` inside a `filter` over the export, so matching was a linear scan per exported row against a list four times longer than necessary. Read one id per row into a Set instead. Measured filtering 200,000 export rows against 5,000 listed subjects, identical results (167,000 matched either way): before 3,070 ms after 17 ms A 25,000-record group with 30 measures per record produces around 750,000 export rows, so the figure above is conservative. Still client-side: the export is fetched for the whole group and narrowed in the browser, so every filter the user has applied is honoured only after the payload has crossed the network. Pushing the subject ids to the export endpoint is the real fix and composes with the streaming rework in #1407. Refs #1412 Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01Bfen9VQemZanJLXinJ6MMG --- .../_app/datahub/__tests__/index.test.tsx | 85 +++++++++++++++++++ apps/web/src/routes/_app/datahub/index.tsx | 18 +++- 2 files changed, 99 insertions(+), 4 deletions(-) create mode 100644 apps/web/src/routes/_app/datahub/__tests__/index.test.tsx diff --git a/apps/web/src/routes/_app/datahub/__tests__/index.test.tsx b/apps/web/src/routes/_app/datahub/__tests__/index.test.tsx new file mode 100644 index 000000000..a74a5c0ce --- /dev/null +++ b/apps/web/src/routes/_app/datahub/__tests__/index.test.tsx @@ -0,0 +1,85 @@ +import React from 'react'; + +import { DataTable } from '@douglasneuroinformatics/libui/components'; +import type { TanstackTable } from '@douglasneuroinformatics/libui/components'; +import { render } from '@testing-library/react'; +import { beforeAll, describe, expect, it } from 'vitest'; + +// Initialises the shared libui translator, which the table's controls read on render. +import '@/services/i18n'; + +const noop = () => undefined; + +type Row = { id: string }; + +/** + * Renders a table shaped like the datahub master table (several columns plus row actions) and hands + * back its tanstack instance, so the id extraction can be exercised against a real row model rather + * than a hand-built stand-in. + */ +const renderMasterTableLike = (data: Row[]) => { + let table: TanstackTable.Table | undefined; + const Capture = (props: { table: TanstackTable.Table }) => { + table = props.table; + return null; + }; + render( + + columns={[ + { accessorFn: (row) => row.id, header: 'Subject', id: 'subjectId' }, + { accessorFn: () => null, header: 'DOB', id: 'dateOfBirth' }, + { accessorFn: () => null, header: 'Sex', id: 'sex' } + ]} + data={data} + rowActions={[{ label: 'View', onSelect: noop }]} + togglesComponent={Capture} + /> + ); + return table!; +}; + +describe('datahub export subject filter', () => { + beforeAll(() => { + // libui measures the table container; happy-dom has no layout engine. + globalThis.ResizeObserver ??= class { + disconnect = noop; + observe = noop; + unobserve = noop; + } as never; + }); + + it('yields one id per row, not one per rendered cell', () => { + const data = [{ id: 'subject-a' }, { id: 'subject-b' }, { id: 'subject-c' }]; + const table = renderMasterTableLike(data); + const rows = table.getPrePaginationRowModel().rows; + + // The shape the previous implementation produced: `getVisibleCells()` repeats the row's id once + // per column, so the ids arrive duplicated. This asserts the duplication is real rather than + // assumed, and that the current extraction does not reproduce it. + const perCell = rows.flatMap((row) => row.getVisibleCells().map((cell) => cell.row.original.id)); + expect(perCell.length).toBeGreaterThan(rows.length); + + const perRow = new Set(rows.map((row) => row.original.id)); + expect(perRow.size).toBe(data.length); + expect([...perRow]).toStrictEqual(['subject-a', 'subject-b', 'subject-c']); + }); + + it('membership is a set lookup, so an export row is matched without scanning the list', () => { + const table = renderMasterTableLike([{ id: 'subject-a' }, { id: 'subject-b' }]); + const listed = new Set(table.getPrePaginationRowModel().rows.map((row) => row.original.id)); + + const exportRows = [{ subjectId: 'subject-a' }, { subjectId: 'subject-z' }, { subjectId: 'subject-b' }]; + expect(exportRows.filter((row) => listed.has(row.subjectId)).map((row) => row.subjectId)).toStrictEqual([ + 'subject-a', + 'subject-b' + ]); + }); + + it('excludes every export row when the table lists no subjects', () => { + const table = renderMasterTableLike([]); + const listed = new Set(table.getPrePaginationRowModel().rows.map((row) => row.original.id)); + + expect(listed.size).toBe(0); + expect([{ subjectId: 'subject-a' }].filter((row) => listed.has(row.subjectId))).toStrictEqual([]); + }); +}); diff --git a/apps/web/src/routes/_app/datahub/index.tsx b/apps/web/src/routes/_app/datahub/index.tsx index 15eae68ef..1cdf9b5fd 100644 --- a/apps/web/src/routes/_app/datahub/index.tsx +++ b/apps/web/src/routes/_app/datahub/index.tsx @@ -39,6 +39,18 @@ type HasSearchStringFilter = { searchString: string; }; +/** + * The subject ids currently listed by the table, for filtering an export down to them. + * + * Read one per row: iterating `getVisibleCells()` yields the row's id once per rendered column + * (including the row-actions column), so the ids arrive duplicated as many times as there are + * columns. A set also keeps the membership test that consumes this constant time rather than a + * linear scan per exported row. + */ +const getListedSubjectIds = (table: TanstackTable.Table): Set => { + return new Set(table.getPrePaginationRowModel().rows.map((row) => removeSubjectIdScope(row.original.id))); +}; + const Filters: React.FC<{ hasRecords: boolean; setHasRecords: (v: boolean) => void; @@ -243,11 +255,9 @@ const Toggles: React.FC<{ getExportRecords() .then((data): any => { - const listedSubjects = table - .getPrePaginationRowModel() - .rows.flatMap((row) => row.getVisibleCells().map((cell) => removeSubjectIdScope(cell.row.original.id))); + const listedSubjects = getListedSubjectIds(table); - const filteredData = data.filter((dataEntry) => listedSubjects.includes(dataEntry.subjectId)); + const filteredData = data.filter((dataEntry) => listedSubjects.has(dataEntry.subjectId)); if (filteredData.length < 1) { throw Error( From 7ebd10e0b477c4ee4b229ce5722ae41b45b0baa5 Mon Sep 17 00:00:00 2001 From: "Gabriel A. Devenyi" Date: Mon, 27 Jul 2026 14:57:45 -0400 Subject: [PATCH 2/7] test(web): assert the export subject filter against the real helper The test lived under src/routes, which main now bans by eslint because the TanStack route generator scans that directory and reads a dot as a path separator. It also only exercised tanstack's row model and `Set.has`, so it would still have passed had the per-cell extraction been reintroduced. Move `getListedSubjectIds` to src/utils/table.ts and assert it directly, and throw when the table capture fails rather than asserting on `table!`. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014ZEJLwa7jwD8sKzcE4PfSc --- .../_app/datahub/__tests__/index.test.tsx | 85 ------------------- apps/web/src/routes/_app/datahub/index.tsx | 13 +-- apps/web/src/utils/__tests__/table.test.tsx | 76 +++++++++++++++++ apps/web/src/utils/table.ts | 15 ++++ 4 files changed, 92 insertions(+), 97 deletions(-) delete mode 100644 apps/web/src/routes/_app/datahub/__tests__/index.test.tsx create mode 100644 apps/web/src/utils/__tests__/table.test.tsx create mode 100644 apps/web/src/utils/table.ts diff --git a/apps/web/src/routes/_app/datahub/__tests__/index.test.tsx b/apps/web/src/routes/_app/datahub/__tests__/index.test.tsx deleted file mode 100644 index a74a5c0ce..000000000 --- a/apps/web/src/routes/_app/datahub/__tests__/index.test.tsx +++ /dev/null @@ -1,85 +0,0 @@ -import React from 'react'; - -import { DataTable } from '@douglasneuroinformatics/libui/components'; -import type { TanstackTable } from '@douglasneuroinformatics/libui/components'; -import { render } from '@testing-library/react'; -import { beforeAll, describe, expect, it } from 'vitest'; - -// Initialises the shared libui translator, which the table's controls read on render. -import '@/services/i18n'; - -const noop = () => undefined; - -type Row = { id: string }; - -/** - * Renders a table shaped like the datahub master table (several columns plus row actions) and hands - * back its tanstack instance, so the id extraction can be exercised against a real row model rather - * than a hand-built stand-in. - */ -const renderMasterTableLike = (data: Row[]) => { - let table: TanstackTable.Table | undefined; - const Capture = (props: { table: TanstackTable.Table }) => { - table = props.table; - return null; - }; - render( - - columns={[ - { accessorFn: (row) => row.id, header: 'Subject', id: 'subjectId' }, - { accessorFn: () => null, header: 'DOB', id: 'dateOfBirth' }, - { accessorFn: () => null, header: 'Sex', id: 'sex' } - ]} - data={data} - rowActions={[{ label: 'View', onSelect: noop }]} - togglesComponent={Capture} - /> - ); - return table!; -}; - -describe('datahub export subject filter', () => { - beforeAll(() => { - // libui measures the table container; happy-dom has no layout engine. - globalThis.ResizeObserver ??= class { - disconnect = noop; - observe = noop; - unobserve = noop; - } as never; - }); - - it('yields one id per row, not one per rendered cell', () => { - const data = [{ id: 'subject-a' }, { id: 'subject-b' }, { id: 'subject-c' }]; - const table = renderMasterTableLike(data); - const rows = table.getPrePaginationRowModel().rows; - - // The shape the previous implementation produced: `getVisibleCells()` repeats the row's id once - // per column, so the ids arrive duplicated. This asserts the duplication is real rather than - // assumed, and that the current extraction does not reproduce it. - const perCell = rows.flatMap((row) => row.getVisibleCells().map((cell) => cell.row.original.id)); - expect(perCell.length).toBeGreaterThan(rows.length); - - const perRow = new Set(rows.map((row) => row.original.id)); - expect(perRow.size).toBe(data.length); - expect([...perRow]).toStrictEqual(['subject-a', 'subject-b', 'subject-c']); - }); - - it('membership is a set lookup, so an export row is matched without scanning the list', () => { - const table = renderMasterTableLike([{ id: 'subject-a' }, { id: 'subject-b' }]); - const listed = new Set(table.getPrePaginationRowModel().rows.map((row) => row.original.id)); - - const exportRows = [{ subjectId: 'subject-a' }, { subjectId: 'subject-z' }, { subjectId: 'subject-b' }]; - expect(exportRows.filter((row) => listed.has(row.subjectId)).map((row) => row.subjectId)).toStrictEqual([ - 'subject-a', - 'subject-b' - ]); - }); - - it('excludes every export row when the table lists no subjects', () => { - const table = renderMasterTableLike([]); - const listed = new Set(table.getPrePaginationRowModel().rows.map((row) => row.original.id)); - - expect(listed.size).toBe(0); - expect([{ subjectId: 'subject-a' }].filter((row) => listed.has(row.subjectId))).toStrictEqual([]); - }); -}); diff --git a/apps/web/src/routes/_app/datahub/index.tsx b/apps/web/src/routes/_app/datahub/index.tsx index 1cdf9b5fd..8fdc2b0b6 100644 --- a/apps/web/src/routes/_app/datahub/index.tsx +++ b/apps/web/src/routes/_app/datahub/index.tsx @@ -26,6 +26,7 @@ import { SearchSubmitGuard } from '@/components/SearchSubmitGuard'; import { subjectsQueryOptions, useSubjectsQuery } from '@/hooks/useSubjectsQuery'; import { useAppStore } from '@/store'; import { downloadExcel } from '@/utils/excel'; +import { getListedSubjectIds } from '@/utils/table'; type DateFilter = { allowNull: boolean; @@ -39,18 +40,6 @@ type HasSearchStringFilter = { searchString: string; }; -/** - * The subject ids currently listed by the table, for filtering an export down to them. - * - * Read one per row: iterating `getVisibleCells()` yields the row's id once per rendered column - * (including the row-actions column), so the ids arrive duplicated as many times as there are - * columns. A set also keeps the membership test that consumes this constant time rather than a - * linear scan per exported row. - */ -const getListedSubjectIds = (table: TanstackTable.Table): Set => { - return new Set(table.getPrePaginationRowModel().rows.map((row) => removeSubjectIdScope(row.original.id))); -}; - const Filters: React.FC<{ hasRecords: boolean; setHasRecords: (v: boolean) => void; diff --git a/apps/web/src/utils/__tests__/table.test.tsx b/apps/web/src/utils/__tests__/table.test.tsx new file mode 100644 index 000000000..e78cb6fee --- /dev/null +++ b/apps/web/src/utils/__tests__/table.test.tsx @@ -0,0 +1,76 @@ +import React from 'react'; + +import { DataTable } from '@douglasneuroinformatics/libui/components'; +import type { TanstackTable } from '@douglasneuroinformatics/libui/components'; +import type { Subject } from '@opendatacapture/schemas/subject'; +import { render } from '@testing-library/react'; +import { beforeAll, describe, expect, it } from 'vitest'; + +import { getListedSubjectIds } from '@/utils/table'; + +// Initialises the shared libui translator, which the table's controls read on render. +import '@/services/i18n'; + +const noop = () => undefined; + +/** + * Renders a table shaped like the datahub master table (several columns plus row actions) and hands + * back its tanstack instance, so the id extraction is exercised against a real row model rather than + * a hand-built stand-in. + */ +const renderMasterTableLike = (ids: string[]) => { + let table: TanstackTable.Table | undefined; + const Capture = (props: { table: TanstackTable.Table }) => { + table = props.table; + return null; + }; + render( + + columns={[ + { accessorFn: (subject) => subject.id, header: 'Subject', id: 'subjectId' }, + { accessorFn: () => null, header: 'DOB', id: 'dateOfBirth' }, + { accessorFn: () => null, header: 'Sex', id: 'sex' } + ]} + data={ids.map((id) => ({ id }) as Subject)} + rowActions={[{ label: 'View', onSelect: noop }]} + togglesComponent={Capture} + /> + ); + if (!table) { + throw new Error('DataTable did not invoke togglesComponent, so no tanstack table was captured'); + } + return table; +}; + +describe('getListedSubjectIds', () => { + beforeAll(() => { + // libui measures the table container; happy-dom has no layout engine. + globalThis.ResizeObserver ??= class { + disconnect = noop; + observe = noop; + unobserve = noop; + } as never; + }); + + it('should yield one id per row, not the one-per-rendered-cell duplication the row model offers', () => { + const table = renderMasterTableLike(['subject-a', 'subject-b', 'subject-c']); + const rows = table.getPrePaginationRowModel().rows; + + // Asserts the duplication the previous implementation hit is real rather than assumed: + // `getVisibleCells()` repeats the row's id once per column, including the row-actions column. + const perCell = rows.flatMap((row) => row.getVisibleCells().map((cell) => cell.row.original.id)); + expect(perCell.length).toBeGreaterThan(rows.length); + + expect([...getListedSubjectIds(table)]).toStrictEqual(['subject-a', 'subject-b', 'subject-c']); + }); + + it('should strip the group scope, so the ids match the unscoped subject ids an export carries', () => { + const table = renderMasterTableLike(['Group_A$subject-a', 'Group_A$subject-b']); + + expect([...getListedSubjectIds(table)]).toStrictEqual(['subject-a', 'subject-b']); + }); + + it('should return an empty set when the table lists no subjects, so nothing is exported', () => { + expect(getListedSubjectIds(renderMasterTableLike([])).size).toBe(0); + }); +}); diff --git a/apps/web/src/utils/table.ts b/apps/web/src/utils/table.ts new file mode 100644 index 000000000..2065fdcbd --- /dev/null +++ b/apps/web/src/utils/table.ts @@ -0,0 +1,15 @@ +import type { TanstackTable } from '@douglasneuroinformatics/libui/components'; +import type { Subject } from '@opendatacapture/schemas/subject'; +import { removeSubjectIdScope } from '@opendatacapture/subject-utils'; + +/** + * The subject ids currently listed by the table, for filtering an export down to them. + * + * Read one per row: iterating `getVisibleCells()` yields the row's id once per rendered column + * (including the row-actions column), so the ids arrive duplicated as many times as there are + * columns. A set also keeps the membership test that consumes this constant time rather than a + * linear scan per exported row. + */ +export function getListedSubjectIds(table: TanstackTable.Table): Set { + return new Set(table.getPrePaginationRowModel().rows.map((row) => removeSubjectIdScope(row.original.id))); +} From c891e0e6419f030f178689924ae3303a179c2fb0 Mon Sep 17 00:00:00 2001 From: "Gabriel A. Devenyi" Date: Tue, 28 Jul 2026 13:32:10 -0400 Subject: [PATCH 3/7] test(testing): pin the datahub export to the subjects the table lists Which records reach an exported file is decided client-side, from the rows the master table is currently listing. Nothing covered that: `datahub.spec.ts` asserted only that the page header renders, so getting the scoping wrong would have handed the user another subject's data with no test failing. Seeds two subjects with records, filters the table down to one, exports JSON and asserts the payload names only that subject. Verified by mutation -- reading the core row model instead of the pre-pagination one, which ignores the filter, fails it. The test needs to know exactly what its group holds, and the `roleAccount` group is shared by every spec in a worker, so this adds an `isolatedGroupManager` fixture that authenticates into a group of its own, plus `uploadRecords` and `findInstrumentIdByName` on the api client for seeding a subject that has a record at all. Also from review: the fixture builds a complete `Subject` rather than casting `{ id }` into one. Note: #1426 introduces the same `isolatedGroupManager` fixture, so whichever of the two merges second should drop its copy. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014ZEJLwa7jwD8sKzcE4PfSc --- apps/web/src/utils/__tests__/table.test.tsx | 15 ++++++- testing/src/pages/_app/datahub/index.page.ts | 14 ++++++- testing/src/specs/datahub.spec.ts | 43 ++++++++++++++++++++ testing/src/support/fixtures.ts | 18 ++++++++ 4 files changed, 88 insertions(+), 2 deletions(-) diff --git a/apps/web/src/utils/__tests__/table.test.tsx b/apps/web/src/utils/__tests__/table.test.tsx index e78cb6fee..c3b0bd48a 100644 --- a/apps/web/src/utils/__tests__/table.test.tsx +++ b/apps/web/src/utils/__tests__/table.test.tsx @@ -13,6 +13,19 @@ import '@/services/i18n'; const noop = () => undefined; +/** A complete `Subject`, so the fixture satisfies the type rather than being cast into it. */ +const subject = (id: string): Subject => ({ + createdAt: new Date(0), + dateOfBirth: null, + firstName: null, + groupIds: [], + id, + lastName: null, + sessionIds: [], + sex: null, + updatedAt: new Date(0) +}); + /** * Renders a table shaped like the datahub master table (several columns plus row actions) and hands * back its tanstack instance, so the id extraction is exercised against a real row model rather than @@ -31,7 +44,7 @@ const renderMasterTableLike = (ids: string[]) => { { accessorFn: () => null, header: 'DOB', id: 'dateOfBirth' }, { accessorFn: () => null, header: 'Sex', id: 'sex' } ]} - data={ids.map((id) => ({ id }) as Subject)} + data={ids.map(subject)} rowActions={[{ label: 'View', onSelect: noop }]} togglesComponent={Capture} /> diff --git a/testing/src/pages/_app/datahub/index.page.ts b/testing/src/pages/_app/datahub/index.page.ts index 37f0f580f..1111e947a 100644 --- a/testing/src/pages/_app/datahub/index.page.ts +++ b/testing/src/pages/_app/datahub/index.page.ts @@ -1,15 +1,27 @@ -import type { Locator, Page } from '@playwright/test'; +import type { Download, Locator, Page } from '@playwright/test'; import { AppPage } from '../route.page'; export class DatahubPage extends AppPage { + readonly exportDropdown: Locator; readonly pageHeader: Locator; readonly rowActionsTrigger: Locator; + readonly rows: Locator; readonly searchInput: Locator; constructor(page: Page) { super(page); + this.exportDropdown = page.getByTestId('datahub-export-dropdown'); this.pageHeader = page.getByTestId('page-header'); this.rowActionsTrigger = page.getByTestId('row-actions-trigger').first(); + this.rows = page.getByTestId('data-table-body').getByTestId('data-table-row'); this.searchInput = page.getByTestId('data-table-search-bar').getByRole('searchbox'); } + + /** Picks a format from the export menu and returns the file it produced. */ + async exportAs(format: 'CSV' | 'Excel' | 'JSON'): Promise { + const started = this.$ref.waitForEvent('download'); + await this.exportDropdown.click(); + await this.$ref.getByRole('menuitem', { exact: true, name: format }).click(); + return started; + } } diff --git a/testing/src/specs/datahub.spec.ts b/testing/src/specs/datahub.spec.ts index b9adcd9f9..8d5262568 100644 --- a/testing/src/specs/datahub.spec.ts +++ b/testing/src/specs/datahub.spec.ts @@ -1,5 +1,6 @@ import { readFile } from 'node:fs/promises'; +import { DatahubPage } from '../pages/_app/datahub/index.page'; import { HAPPINESS_RECORD } from '../support/constants'; import { expect, test } from '../support/fixtures'; @@ -146,4 +147,46 @@ test.describe('data hub', () => { await expect(page).toHaveURL(/\/datahub\/.+\/table$/); }); + + // The export endpoint returns every record in the group; which of them reach the file is decided + // client-side from the rows the table is currently listing. Nothing else covers that scoping, and + // getting it wrong hands the user another subject's data. + test('should export only the subjects the table is listing', async ({ + api, + isolatedGroupManager, + page, + uniqueId + }) => { + const group = await isolatedGroupManager(); + const instrumentId = await api.findInstrumentIdByName('DNP_HAPPINESS_QUESTIONNAIRE'); + const listed = `export-${uniqueId}-listed`; + const filteredOut = `export-${uniqueId}-filtered-out`; + await api.uploadRecords( + group.id, + instrumentId, + [listed, filteredOut].map((subjectId) => ({ data: HAPPINESS_RECORD, date: new Date(), subjectId })) + ); + + const datahubPage = new DatahubPage(page); + await datahubPage.goto('/datahub'); + await expect(datahubPage.rows).toHaveCount(2); + + await datahubPage.searchInput.fill(listed); + await expect(datahubPage.rows).toHaveCount(1); + + const download = await datahubPage.exportAs('JSON'); + const payload = JSON.parse(await readAll(download)) as { subjectId: string }[]; + + expect(payload.length).toBeGreaterThan(0); + expect([...new Set(payload.map((row) => row.subjectId))]).toStrictEqual([listed]); + }); }); + +async function readAll(download: Awaited>): Promise { + const stream = await download.createReadStream(); + const chunks: Buffer[] = []; + for await (const chunk of stream) { + chunks.push(chunk as Buffer); + } + return Buffer.concat(chunks).toString('utf8'); +} diff --git a/testing/src/support/fixtures.ts b/testing/src/support/fixtures.ts index 9ad407a8b..dd6757fd3 100644 --- a/testing/src/support/fixtures.ts +++ b/testing/src/support/fixtures.ts @@ -1,6 +1,7 @@ /* eslint-disable no-empty-pattern */ import type { $LoginCredentials } from '@opendatacapture/schemas/auth'; +import type { Group } from '@opendatacapture/schemas/group'; import { request as apiRequestFactory, test as base, expect } from '@playwright/test'; import type { APIRequestContext } from '@playwright/test'; @@ -98,6 +99,15 @@ type TestFixtures = { authenticateAs: (roleOrCredentials: $LoginCredentials | Role) => Promise; /** Navigates to a route as `actingRole` and returns its page object. */ getPageModel: GetPageModel; + /** + * Authenticates as a group manager of a group created for this test alone, and returns that group. + * + * The `roleAccount` group is cached per worker and shared by every spec running in it, so a test + * that asserts on exactly what a group contains cannot use it. A group manager's `read Subject` + * rule is scoped to their own groups, so a fresh group bounds what this test can see to what it + * seeded. + */ + isolatedGroupManager: () => Promise; /** * Writes `appState` to localStorage on every navigation, for every test, whether or not it * authenticates through `authenticateAs`. Without it a spec that logs in through the real form @@ -155,6 +165,14 @@ export const test = base.extend({ } ); }, + isolatedGroupManager: async ({ api, authenticateAs }, use) => { + await use(async () => { + const group = await api.createGroup(); + const { credentials } = await api.createUser({ basePermissionLevel: 'GROUP_MANAGER', groupIds: [group.id] }); + await authenticateAs(credentials); + return group; + }); + }, roleAccount: [ async ({ adminToken, api, apiRequestContext }, use) => { const cache = new Map([ From 19eea9b3ec739b912fd7dc022b3b33258e5025be Mon Sep 17 00:00:00 2001 From: "Gabriel A. Devenyi" Date: Fri, 11 Sep 2026 12:53:21 -0400 Subject: [PATCH 4/7] docs(testing): document the isolated group manager fixture The isolatedGroupManager fixture lands in the AGENTS.md fixture table, with the reason a test asserting on what a group contains must use it rather than the roleAccount group every spec in a worker shares. Co-Authored-By: Claude Fable 5 --- testing/AGENTS.md | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/testing/AGENTS.md b/testing/AGENTS.md index 333fe5d8f..81ef1e05a 100644 --- a/testing/AGENTS.md +++ b/testing/AGENTS.md @@ -64,20 +64,25 @@ A page object is only reachable from a spec once it is registered in the `pageMo ## Fixtures -| Fixture | Scope | Notes | -| --------------------- | ----------- | -------------------------------------------------------------------------------------------------- | -| `getPageModel` | test | Authenticates as `actingRole`, navigates, returns the page object | -| `authenticateAs(who)` | test | Injects a token without navigating; takes a role or `$LoginCredentials` | -| `actingRole` | test option | Default `GROUP_MANAGER`; override with `test.use({ actingRole: 'ADMIN' })` | -| `appState` | test option | localStorage first-run gating, seeded for every test; defaults to accepted | -| `uniqueId` | test | Short random suffix for seeded data | -| `api` | worker | `ApiClient` as admin — `createGroup()` / `createUser()` / `setUserPermissions()` for preconditions | -| `roleAccount(role)` | worker | Seeds a group + user per role once, then caches its token and username | -| `apiRequestContext` | worker | Raw `APIRequestContext` on the web origin, for driving the API directly | +| Fixture | Scope | Notes | +| ---------------------- | ----------- | -------------------------------------------------------------------------------------------------- | +| `getPageModel` | test | Authenticates as `actingRole`, navigates, returns the page object | +| `authenticateAs(who)` | test | Injects a token without navigating; takes a role or `$LoginCredentials` | +| `actingRole` | test option | Default `GROUP_MANAGER`; override with `test.use({ actingRole: 'ADMIN' })` | +| `appState` | test option | localStorage first-run gating, seeded for every test; defaults to accepted | +| `uniqueId` | test | Short random suffix for seeded data | +| `api` | worker | `ApiClient` as admin — `createGroup()` / `createUser()` / `setUserPermissions()` for preconditions | +| `isolatedGroupManager` | test | Authenticates into a group created for this test alone; returns the group | +| `roleAccount(role)` | worker | Seeds a group + user per role once, then caches its token and username | +| `apiRequestContext` | worker | Raw `APIRequestContext` on the web origin, for driving the API directly | Set up preconditions over the API with the `api` fixture rather than by clicking through the UI; only drive the UI for the behaviour actually under test. +`roleAccount`'s group is cached per worker and shared by every spec running in it, so a test that +asserts on **how much** a group contains must use `isolatedGroupManager` instead. A group manager +reads only their own groups, so a fresh group bounds what the test can see to what it seeded. + Auth is injected as `window.__PLAYWRIGHT_ACCESS_TOKEN__`, which `apps/web`'s `src/store/slices/auth.slice.ts` reads on boot. It is memory-only and never persisted. From ee2538856305ebf50d20db25ba7ca7bc3c5f12f8 Mon Sep 17 00:00:00 2001 From: "Gabriel A. Devenyi" Date: Tue, 28 Jul 2026 18:09:23 -0400 Subject: [PATCH 5/7] test(testing): read the exported file from disk instead of hand-rolling the stream The stream concatenation needed a chunk cast that lied about string-mode streams; playwright already has the download on disk, so readFile does the same job in one line with no cast. Co-Authored-By: Claude Fable 5 --- testing/src/specs/datahub.spec.ts | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/testing/src/specs/datahub.spec.ts b/testing/src/specs/datahub.spec.ts index 8d5262568..f39a3de68 100644 --- a/testing/src/specs/datahub.spec.ts +++ b/testing/src/specs/datahub.spec.ts @@ -175,18 +175,9 @@ test.describe('data hub', () => { await expect(datahubPage.rows).toHaveCount(1); const download = await datahubPage.exportAs('JSON'); - const payload = JSON.parse(await readAll(download)) as { subjectId: string }[]; + const payload = JSON.parse(await readFile(await download.path(), 'utf8')) as { subjectId: string }[]; expect(payload.length).toBeGreaterThan(0); expect([...new Set(payload.map((row) => row.subjectId))]).toStrictEqual([listed]); }); }); - -async function readAll(download: Awaited>): Promise { - const stream = await download.createReadStream(); - const chunks: Buffer[] = []; - for await (const chunk of stream) { - chunks.push(chunk as Buffer); - } - return Buffer.concat(chunks).toString('utf8'); -} From e665fb3d45c28f2a203738dd67f8a7599b992db8 Mon Sep 17 00:00:00 2001 From: "Gabriel A. Devenyi" Date: Fri, 11 Sep 2026 13:07:43 -0400 Subject: [PATCH 6/7] test(web): drop the ResizeObserver stub happy-dom now provides happy-dom 20 ships its own no-op ResizeObserver, and apps/web/AGENTS.md now says a manual stub is no longer needed. The `??=` guard meant this one never ran anyway. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01KLfKsdHT1arfrc2fdT1fWq --- apps/web/src/utils/__tests__/table.test.tsx | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/apps/web/src/utils/__tests__/table.test.tsx b/apps/web/src/utils/__tests__/table.test.tsx index c3b0bd48a..ccfee4b71 100644 --- a/apps/web/src/utils/__tests__/table.test.tsx +++ b/apps/web/src/utils/__tests__/table.test.tsx @@ -4,7 +4,7 @@ import { DataTable } from '@douglasneuroinformatics/libui/components'; import type { TanstackTable } from '@douglasneuroinformatics/libui/components'; import type { Subject } from '@opendatacapture/schemas/subject'; import { render } from '@testing-library/react'; -import { beforeAll, describe, expect, it } from 'vitest'; +import { describe, expect, it } from 'vitest'; import { getListedSubjectIds } from '@/utils/table'; @@ -56,15 +56,6 @@ const renderMasterTableLike = (ids: string[]) => { }; describe('getListedSubjectIds', () => { - beforeAll(() => { - // libui measures the table container; happy-dom has no layout engine. - globalThis.ResizeObserver ??= class { - disconnect = noop; - observe = noop; - unobserve = noop; - } as never; - }); - it('should yield one id per row, not the one-per-rendered-cell duplication the row model offers', () => { const table = renderMasterTableLike(['subject-a', 'subject-b', 'subject-c']); const rows = table.getPrePaginationRowModel().rows; From c1b3a5408d465d29edf83910d7c352087bfb8aad Mon Sep 17 00:00:00 2001 From: "Gabriel A. Devenyi" Date: Fri, 11 Sep 2026 14:04:01 -0400 Subject: [PATCH 7/7] test(testing): match the export download by its extension A CSV export downloads README.txt before the .csv itself, so waiting on the first download event handed exportAs('CSV') the help file. The waiter now matches the extension of the format it asked for. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01KLfKsdHT1arfrc2fdT1fWq --- testing/src/pages/_app/datahub/index.page.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/testing/src/pages/_app/datahub/index.page.ts b/testing/src/pages/_app/datahub/index.page.ts index 1111e947a..1a87a5de4 100644 --- a/testing/src/pages/_app/datahub/index.page.ts +++ b/testing/src/pages/_app/datahub/index.page.ts @@ -17,9 +17,13 @@ export class DatahubPage extends AppPage { this.searchInput = page.getByTestId('data-table-search-bar').getByRole('searchbox'); } - /** Picks a format from the export menu and returns the file it produced. */ + /** + * Picks a format from the export menu and returns the file it produced. Matched by extension, + * because a CSV export downloads a `README.txt` before the `.csv` itself. + */ async exportAs(format: 'CSV' | 'Excel' | 'JSON'): Promise { - const started = this.$ref.waitForEvent('download'); + const extension = { CSV: '.csv', Excel: '.xlsx', JSON: '.json' }[format]; + const started = this.$ref.waitForEvent('download', (download) => download.suggestedFilename().endsWith(extension)); await this.exportDropdown.click(); await this.$ref.getByRole('menuitem', { exact: true, name: format }).click(); return started;