perf(datahub): match export rows against a set of listed subject ids

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;
@@ -243,11 +244,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(

apps/web/src/utils/__tests__/table.test.tsx

@@ -0,0 +1,80 @@
+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 { 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;
+
+/** 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
+ * a hand-built stand-in.
+ */
+const renderMasterTableLike = (ids: string[]) => {
+  let table: TanstackTable.Table<Subject> | undefined;
+  const Capture = (props: { table: TanstackTable.Table<Subject> }) => {
+    table = props.table;
+    return null;
+  };
+  render(
+    <DataTable<Subject>
+      columns={[
+        { accessorFn: (subject) => subject.id, header: 'Subject', id: 'subjectId' },
+        { accessorFn: () => null, header: 'DOB', id: 'dateOfBirth' },
+        { accessorFn: () => null, header: 'Sex', id: 'sex' }
+      ]}
+      data={ids.map(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', () => {
+  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);
+  });
+});

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<Subject>): Set<string> {
+  return new Set(table.getPrePaginationRowModel().rows.map((row) => removeSubjectIdScope(row.original.id)));
+}

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.
 

testing/src/pages/_app/datahub/index.page.ts

@@ -1,15 +1,31 @@
-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. Matched by extension,
+   * because a CSV export downloads a `README.txt` before the `.csv` itself.
+   */
+  async exportAs(format: 'CSV' | 'Excel' | 'JSON'): Promise<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;
+  }
 }

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,37 @@ 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 readFile(await download.path(), 'utf8')) as { subjectId: string }[];
+
+    expect(payload.length).toBeGreaterThan(0);
+    expect([...new Set(payload.map((row) => row.subjectId))]).toStrictEqual([listed]);
+  });
 });

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<void>;
   /** 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<Group>;
   /**
    * 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<TestFixtures, WorkerFixtures>({
       }
     );
   },
+  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<Role, { accessToken: string; username: string }>([