perf(api): group the has-record subject ids in the database

[gdevenyi]

Fixes #1413.

The "with records only" datahub filter resolved its subject list with:

findMany({ distinct: ['subjectId'], select: { subjectId: true }, where })

Prisma applies distinct in the query engine, not in MongoDB, so the driver receives one row per record and collapses them to one row per subject only after they have arrived. On a group holding 100k records that is 100k rows transferred to produce a list of at most a few thousand ids.

groupBy asks MongoDB for the distinct values directly.

Measured over HTTP, live instance, 200,300 records, median of 7

	main	this branch
GET /v1/subjects?hasRecord=true	366 ms	42 ms
GET /v1/subjects?groupId=<g>&hasRecord=true	67 ms	37 ms
GET /v1/subjects?groupId=<g> (filter off, for reference)	4 ms	3 ms

The unscoped case is where the difference shows, and it widens with the collection: groupBy returns one row per subject regardless of how many records back it, so its cost is flat in records-per-subject where the old form was linear.

Confirmed at the query level too — the same comparison run directly through the Prisma client:

findMany distinct (current impl), scoped        197ms  -> 55 ids
findMany distinct (current impl), unscoped     1229ms  -> 100 ids
groupBy by subjectId, scoped                     95ms  -> 55 ids
groupBy by subjectId, unscoped                  100ms  -> 100 ids

Note distinct going 197 → 1229 ms as the scope widens while groupBy stays at ~100 ms.

The issue's "better still" suggestion is wrong — please don't take it

#1413 floated expressing this as a relation filter on Subject:

{ instrumentRecords: { some: groupId ? { groupId } : {} } }

and said it was "the version worth trying first". I measured it on the same dataset and it takes 26,849 ms — 270× slower than the code being replaced, let alone the replacement:

subject.findMany({instrumentRecords:{some}})   26849ms  -> 55 subjects

Prisma compiles a relation filter on MongoDB into a $lookup over the whole record collection. That is the same construct that fails outright at scale in #1416 (fixed in #1423 by removing it), and it carries the same 100 MiB per-document ceiling. I've left a comment on the method saying so, and corrected the issue.

Also fixed: the record query now respects the caller's ability

querySubjectIdsWithRecords ignored ability entirely. The outer subject query already constrained the result, so this was not a data leak — but the inner query read records the caller had no permission to read, and was inconsistent with every other query in the service.

Tests

The three existing hasRecord tests are updated to the new call and still assert the same outcomes. Two added:

• the ids are grouped in the database, asserting groupBy is used and findMany is not — a guard against the distinct form coming back;
• the record query carries the caller's ability rather than being unconstrained.

Checks

• tsc --noEmit on apps/api — clean
• eslint src — clean
• prettier --check — clean
• vitest run (whole workspace) — 254 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.

---

Rebased onto main, review item addressed

Rebased onto 26ec89168; CI green.

Copilot: expect(call.where.AND[0]).not.toStrictEqual({}) did not prove anything. Correct, and
worse than it looks — an unconditional read rule legitimately produces {}, so that assertion would
have passed against a query that ignored the caller entirely. The test now builds a conditioned
ability and compares the clause against accessibleQuery(ability, 'read', 'InstrumentRecord')
directly, with accessibleQuery imported as suggested.

Verified by mutation: passing undefined in place of the ability now fails the test, where before it
passed.

---

Round 2: rebased onto 5bfe4b2dc, review items addressed

1. The 500 — confirmed and fixed. Reproduced exactly as you described: with a STANDARD ability,
accessibleQuery(ability, 'read', 'InstrumentRecord') throws
ForbiddenError: It's not allowed to run "read" on "InstrumentRecord" rather than returning {}.
querySubjectIdsWithRecords now returns an empty list for a caller holding no such rule, before
accessibleQuery is reached — the contract you settled on. The scoping itself stays.

2. A unit test for that caller. Added, asserting the empty list rather than a throw, with the
ability built through AbilityFactory with a STANDARD payload. Verified by mutation: removing the
guard fails it with that ForbiddenError.

3. End-to-end. datahub.spec.ts now toggles "With records only" and asserts the table empties,
plus an API-level case asserting a standard user gets an answer rather than a 500. Both verified by
mutation. This needed the datahub-filter-has-records and datahub-filters-trigger testids and
filter locators you flagged as missing.

The narrowing test has 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. #1425 introduces the same
fixture — whichever merges second should drop its copy.

4. The not.toHaveBeenCalled() assertion. Dropped, agreed — the groupBy assertion above it
already rules out the distinct form.

The comment recording why the relation filter was rejected is kept, as you asked. Group-visibility
consequence stays as decided.

🤖 Generated with Claude Code

https://claude.ai/code/session_014ZEJLwa7jwD8sKzcE4PfSc

[Copilot]

Pull request overview

This PR improves the performance of the API’s “with records only” subject filter by moving subject-id deduplication into MongoDB via Prisma groupBy, avoiding Prisma’s client-side distinct behavior that transfers one row per record over the wire. It also updates the query to respect the caller’s ability when resolving subject ids from instrument records.

Changes:

• Replace findMany({ distinct: [...] }) with instrumentRecord.groupBy({ by: ['subjectId'] }) when resolving “subjects with records”.
• Apply accessibleQuery(ability, 'read', 'InstrumentRecord') to the record-side query for consistency and correctness.
• Update and extend unit tests to guard against regressions (ensuring groupBy is used and ability is threaded through).

Reviewed changes

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

File	Description
apps/api/src/subjects/subjects.service.ts	Switch subject-id resolution to Prisma groupBy and apply ability to the instrument record query.
apps/api/src/subjects/tests/subjects.service.spec.ts	Update existing hasRecord tests for groupBy and add regression tests for groupBy usage and ability propagation.

---

💡 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 29771462892

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]

The perf work is right and the evidence for it is good — groupBy genuinely pushes the distinct into
MongoDB where distinct did not, and the comment recording why the instrumentRecords: { some: ... }
relation filter was rejected is worth more than the code change itself. Please keep it. The
accessibleQuery pin in the new test is also a real improvement over the not.toStrictEqual({})
version.

One blocking problem, in the other half of the change:

1. accessibleQuery(ability, 'read', 'InstrumentRecord') at apps/api/src/subjects/subjects.service.ts:178
   throws when the caller holds no read rule for InstrumentRecord. @casl/prisma's
   accessibleBy proxy raises ForbiddenError rather than returning {} in that case, and every
   STANDARD user is in that case — ability.factory.ts grants them create on InstrumentRecord but
   never read. This route's guard is @RouteAccess({ action: 'read', subject: 'Subject' }), which
   STANDARD users pass, so they reach the service and the CASL error escapes as a 500 (it is not
   an HttpException, so libnest's filter maps it to Internal Server Error). On main the same
   request returns a correct 200. I reproduced this by driving SubjectsService.find({ hasRecord: true })
   with an ability built from AbilityFactory.createForPayload({ basePermissionLevel: 'STANDARD', ... }).

   Nobody hits it through the web UI — useNavItems.ts:63 hides the datahub entirely without
   read InstrumentRecord — but GET /v1/subjects?hasRecord=true is a documented endpoint and the
   guard admits these callers. Every other accessibleQuery(..., 'InstrumentRecord') in the codebase
   sits behind a route guard naming the same subject, which is why this shape has not bitten before.

   Joshua has settled the contract: such a caller should get an empty list — the honest reading
   of "subjects whose records you can see" — not a 403 and not the pre-PR behaviour of ignoring
   record permissions here. So the no-rule case needs handling before accessibleQuery is reached,
   or the ForbiddenError catching, with an empty result returned either way.

2. Please add a unit test for that caller, asserting the empty list rather than a throw. The new
   ability test builds a permission set that can read InstrumentRecord, so it exercises the
   conditioned branch but never the no-rule branch — which is exactly the one that breaks.

3. There is no e2e test, which the root CLAUDE.md asks for. testing/src/specs/datahub.spec.ts
   only checks that the page header renders. Toggling "With records only" and asserting the table
   narrows would cover both the perf path and the permissions change. The checkbox at
   apps/web/src/routes/_app/datahub/index.tsx:169 has no data-testid and
   testing/src/pages/_app/datahub/index.page.ts exposes no filter locator, so both need adding.

4. Minor: expect(prismaClient.instrumentRecord.findMany).not.toHaveBeenCalled() in
   subjects.service.spec.ts:105 asserts an implementation detail. The groupBy assertion just above
   it already stops the distinct form coming back, so the negative one mostly adds a way for an
   unrelated future refactor to fail the test.

On the other consequence of the scoping — a group manager no longer seeing subjects whose records
live only in groups they cannot read — Joshua has confirmed that is intended, as a disclosure fix
consistent with how /v1/instrument-records already scopes. Nothing to change there.

If you hand this to Claude Code, Fable 5 is the right size — it is auth and group-scoping work,
and the fix has to be carried consistently through the service, its unit tests and a new e2e spec.

Reviewed at commit 53c8db8.

[joshunrau]

Thanks — everything from the last round landed, lint is clean across all 33 workspaces here and the api
suite passes 156/156. The scoping question this raised has been settled in favour of what you wrote:
ungrouped (admin-created) records should not count toward "with records only" for a group manager,
so no change to the filter is wanted.

Worth stating plainly, because it makes this more than a perf change: on main this subquery ran with
no ability filter at all (where: groupId ? { groupId } : {}), so a group manager's "has records"
answer was computed from every record in the collection. Adding accessibleQuery here fixes that.

One thing to add before merge:

1. apps/api/src/subjects/subjects.service.ts:182-187 — please note in the doc comment that the
   accessibleQuery clause drops records with a null groupId, since a group manager's rule matches
   groupId: { in: groupIds } and Prisma's in does not match null. It is the behaviour we want, but
   it is invisible at the call site and it is precisely why admin-created records disappear from a
   manager's filtered view. The block already explains the STANDARD short-circuit just above, so this
   is the same pattern in the same place.

No change needed for STANDARD users, by the way — I checked, and both datahub entries in
useNavItems.ts are already gated on read InstrumentRecord, so they have no UI path to the toggle.

On ordering: this one is planned to merge first, ahead of #1425 and #1427. Nothing for you to do about
the shared isolatedGroupManager fixture — the later PRs will drop their copies.

If you hand this to Claude Code, Sonnet 5 is the right size — a single comment in one file,
mirroring the explanatory comment already three lines above it.

Reviewed at commit 3bd9380.

[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 3a73274.