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

[gdevenyi]

Addresses item 1 of #1412.

The bug

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 entirely and reads cell.row.original.id — the same value for every cell in the row. So the row's id is emitted once per rendered column, and removeSubjectIdScope runs once per column too.

That array was then the right-hand side of an includes inside a filter over the export, making the match a linear scan per exported row, against a list several times longer than it needed to be.

Measured, not estimated — and the issue understated it

I rendered the real table and counted. It has three data columns plus the row-actions column libui injects, which #1412 didn't account for:

cellsPerRow=4  rows=5000
oldArrayLength=20000  newSetSize=5000  factor=4.0

So the duplication is 4×, not the "at least three" I wrote in the issue.

Filtering 200,000 export rows against 5,000 listed subjects, identical results both ways (167,000 matched):

before	3,070 ms
after	17 ms

That is ~180×, and it is conservative: a 25,000-record group with 30 measures per record produces roughly 750,000 export rows, ~3.75× the size benchmarked here.

The change

const getListedSubjectIds = (table: TanstackTable.Table<Subject>): Set<string> => {
  return new Set(table.getPrePaginationRowModel().rows.map((row) => removeSubjectIdScope(row.original.id)));
};

and listedSubjects.has(...) at the call site. Extracted to a named helper so the reason it reads one id per row is documented where someone would otherwise reintroduce the cell loop.

Tests

Three, driving the real DataTable with the master table's shape (three columns + row actions) rather than a hand-built stand-in — so the duplication claim is verified rather than assumed:

• one id per row, not one per cell (asserts getVisibleCells() genuinely over-counts, and that the current extraction doesn't);
• membership matches the right export rows;
• an empty table excludes everything, rather than accidentally matching.

Checks

• tsc --noEmit on apps/web — clean
• eslint src — clean
• prettier --check — clean
• vitest run (whole workspace) — 255 passed, 1 skipped, 1 failed

The one failure is instrument-records.service.spec.ts > upload > should create records with pending set. This branch is cut from main, so it carries that pre-existing failure; #1422 fixes it.

What is still open in #1412

The filtering is still client-side. The export is fetched for the entire group and narrowed in the browser, so the sex filter, the date-of-birth range, the search string and the "with records only" toggle are all honoured only after the payload has crossed the network. Pushing the subject ids (or the filter criteria) to /v1/instrument-records/export is the real fix, and it composes with the streaming rework proposed in #1407 — worth doing together rather than bolting a subjectIds parameter onto an endpoint that is about to be restructured.

Item 3 of #1412 — the same anti-pattern in the audit log download — is already handled in #1419, which replaces it with a server-side fetch.

---

Rebased onto main, review items addressed

Rebased onto 26ec89168; CI green.

Copilot: the tests did not exercise the production code. Correct — they asserted tanstack's row
model and Set.has, so reintroducing the per-cell extraction would not have failed them.
getListedSubjectIds moved to apps/web/src/utils/table.ts and is now asserted directly, including
that it strips the group scope from each id.

This also clears a lint failure the rebase would otherwise have introduced: main now bans tests
under apps/web/src/routes (08d36a093), because the TanStack route generator scans that directory
and reads a dot as a path separator — 36311a02c moved an existing test out for the same reason. The
test file this PR added sat exactly there. It now lives in src/utils/__tests__/, which is where
apps/web/AGENTS.md says helpers and their tests belong.

Copilot: bare table!. Replaced with an explicit throw naming the failure, so a DataTable API
change reports itself rather than surfacing as "cannot read property of undefined".

---

Round 2: rebased onto 5bfe4b2dc, review items addressed

1. End-to-end coverage. Added. It seeds two subjects that have records, filters the master table
down to one, exports JSON from datahub-export-dropdown, and asserts via waitForEvent('download')
that the payload names only the listed subject. Verified by mutation: reading the core row model
instead of the pre-pagination one — which ignores the filter — fails it.

Supporting pieces, as you anticipated: an export locator and a search helper on
testing/src/pages/_app/datahub/index.page.ts, and uploadRecords / findInstrumentIdByName on the
api client, since the export only carries subjects that actually hold a record.

One thing worth flagging: the test has to know exactly what its group holds, and the roleAccount
group is cached per worker and shared by every spec running in it. So this adds an
isolatedGroupManager fixture that authenticates into a group of its own. #1426 introduces the
same fixture for the same reason — whichever of the two merges second should drop its copy.

2. The as Subject cast. Replaced with a factory returning a complete Subject.

🤖 Generated with Claude Code

https://claude.ai/code/session_014ZEJLwa7jwD8sKzcE4PfSc

[Copilot]

Pull request overview

This PR improves Datahub export filtering performance by extracting one subject ID per listed table row (instead of per visible cell) and using a Set for constant-time membership checks when filtering exported rows client-side.

Changes:

• Replace per-cell subject ID collection (getVisibleCells().flatMap(...)) with a per-row extraction helper that deduplicates via Set.
• Update export filtering to use Set.has(...) instead of Array.includes(...).
• Add a new test file that renders a real DataTable shape (including row actions) to validate the underlying “per-cell duplicates IDs” behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
apps/web/src/routes/_app/datahub/index.tsx	Switch export filtering to per-row ID extraction and Set membership checks for large performance gains.
apps/web/src/routes/_app/datahub/tests/index.test.tsx	Add tests around table row-model behavior and basic set membership filtering.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[joshunrau]

CI is red on this branch, so I have not reviewed it yet.

Failing check: lint-and-test — run 29770998724

FAIL  api  src/instrument-records/__tests__/instrument-records.service.spec.ts
  > InstrumentRecordsService > upload > should create records with pending set,
    so they are not excluded from queries filtering on the field
AssertionError: expected "vi.fn()" to be called with arguments: [ { data: [ ObjectContaining{ pending: false } ] } ]

That test is not in your diff and no longer exists on main — it was removed in 722f0a0b0 ("test(api): update stale pending-on-create assertion for instrument records", 2026-07-20 22:39 EDT), which landed after this run. The result is stale.

Please merge or rebase onto current main and push to re-run CI. I'll pick the review back up once it's green.

[joshunrau]

Suggested model: Sonnet 5

Sizing the remaining work on this PR for whoever picks it up, human or agent. The branch is mergeable and the only blocker is stale CI, so remediation is merging main and pushing.

[joshunrau]

Requesting changes so this tracks as needing author action.

Blocking: stale CI only — the branch is mergeable. Details in my earlier comment: the failing test isn't in your diff and was removed from main in 722f0a0b0, after this run was created. Merge main and push to get a fresh result.

No content review has been done on this branch yet — it's gated on CI being green.

[joshunrau]

Thanks — this is the right fix, and the extraction to apps/web/src/utils/table.ts with the
explanatory comment is a good call. Lint is clean on the merge result and the new unit test passes
and genuinely exercises the helper, group-scope stripping included.

Two things before this can go in:

1. The repo requires an end-to-end test in testing/ alongside the unit test for every change, and
   there isn't one here. This matters more than usual for this code path: datahub.spec.ts today
   only asserts the page header renders, so nothing end-to-end covers which subjects' data ends up
   in an exported file. Please add a spec that seeds two subjects through the api fixture, filters
   the master table down to one, triggers the JSON export from the datahub-export-dropdown control,
   and asserts via page.waitForEvent('download') that the payload contains only the listed subject.
   testing/AGENTS.md and .agents/docs/playbooks/add-e2e-test.md cover the fixtures and page-object
   registration; you will probably want to add a locator for the export flow to
   testing/src/pages/_app/datahub/index.page.ts.

2. Minor, optional: apps/web/src/utils/__tests__/table.test.tsx:34 builds rows with
   { id } as Subject. The repo's typing rules discourage casts at call sites — a small factory
   returning a complete Subject would avoid it.

If you hand this to Claude Code, Opus 5 is the right size — the e2e touches the spec, a page
object and the fixtures, and needs judgment about download interception and seeding preconditions.

Reviewed at commit 38fae5c.

[joshunrau]

The e2e test lands this. Seeding two subjects, filtering to one and asserting the downloaded JSON
names only that subject is exactly the coverage this path was missing, and pulling
isolatedGroupManager out as a fixture (with the AGENTS.md entry) is the right call rather than
fighting the shared roleAccount group. Lint is green across all 33 tasks on the merge result, the
unit test passes, and CI including Playwright is green.

One thing left:

• testing/src/specs/datahub.spec.ts:52-59 — the readAll helper hand-rolls stream concatenation
  and needs chunk as Buffer on line 56 to type-check. The repo's typing rules disallow casts at
  call sites, and this one is a latent lie: a stream in string mode would yield strings and
  Buffer.concat would throw. await readFile(await download.path(), 'utf8') from
  node:fs/promises does the same job in one line with no cast, and the whole helper can go.

Also worth remembering: #1426 introduces the same isolatedGroupManager fixture, so whichever of the
two merges second should drop its copy — you already noted this, just flagging that it is still true.

If you hand this to Claude Code, Sonnet 5 is the right size — one mechanical edit inside a single
test file.

Reviewed at commit bb0cc3f.

[joshunrau]

This branch no longer merges cleanly into main — GitHub reports it as conflicting, so it can't be reviewed or merged as it stands. main has moved since the last pass (#1444 and the e2e-test work landed).

Please merge or rebase onto current main and push. Review picks up again once the branch is mergeable and CI is green — the new commits since the last review haven't been looked at yet.

Reviewed at commit 5b50df6.