Skip to content

perf(datahub): resolve record usernames server-side#1420

Open
gdevenyi wants to merge 2 commits into
mainfrom
fix/datahub-record-usernames
Open

perf(datahub): resolve record usernames server-side#1420
gdevenyi wants to merge 2 commits into
mainfrom
fix/datahub-record-usernames

Conversation

@gdevenyi

Copy link
Copy Markdown
Contributor

Fixes #1410.

Viewing one subject's records fetched every session in the current group, each with its subject document embedded, and joined them in the browser with sessions.find(...) inside records.map(...) — O(records × sessions) to label a few dozen rows with a username.

The username is now resolved on the server, keyed on the sessions the returned records actually reference.

Measured effect

Against a real MongoDB 8 replica set, database provisioned through the app's own POST /v1/setup, with 20,031 sessions in the group being viewed:

main this branch
GET /v1/instrument-records?subjectId=… 2,624 B 2,786 B
GET /v1/sessions?groupId=… 12,922,313 B / 950 ms not requested
total for one subject view ~12.9 MB 2.8 KB

~4,600× fewer bytes. The /v1/sessions request is gone from the datahub entirely, and the client-side join with it — useInstrumentVisualization now reads record.session?.user?.username directly.

An important correction to the approach the issue proposed

#1410 proposed doing this with an include on the relation:

include: {
  instrument: false,
  session: { select: { user: { select: { username: true } } } }
}

That does not work, and I only found out by testing it against a live instance. InstrumentRecord.session is declared required in the prisma schema (session Session @relation(...)), so if any single record's session has since been deleted, Prisma aborts the whole query:

Invalid `prisma.instrumentRecord.findMany()` invocation:
Inconsistent query result: Field session is required to return data, got `null` instead.

I reproduced this by deleting one session and re-querying: main returns 200, the include version returned 500. That would have turned a performance fix into an outage for any instance with an orphaned record — and MongoDB enforces no referential integrity, so nothing prevents that state.

The merged approach is a second scoped query instead:

const sessions = await this.sessionModel.findMany({
  select: { id: true, user: { select: { username: true } } },
  where: { id: { in: Array.from(new Set(records.map((record) => record.sessionId))) } }
});

joined with a Map. It is bounded by the records being returned (not by the group's session count), it degrades to a missing username rather than a failed request, and it costs one extra round trip that is skipped entirely when there are no records.

Verified on a live instance

All three cases, against the running API:

record 6a5e67b4 -> username: "perfadmin"   (session with a user)
record 6a5e67b4 -> username: "perfadmin"
record 6a5e67b4 -> username: null          (session with no user)
record 6a5e67b4 -> username: null          (session deleted — no crash)
HTTP 200

Tests

5 new server tests covering the username labelling, deduplication of session ids, skipping the lookup when there are no records, and — the one that matters most — that a record whose session no longer exists is still returned, with no username.

The 8 existing useInstrumentVisualization tests pass unchanged. They assert that Username,…testusername appears in the CSV, TSV, Excel and JSON exports; those assertions are untouched, so they demonstrate the username still reaches every export format, just sourced from the record instead of a separate fetch. Only the fixture moved.

Checks

  • tsc --noEmit on packages/schemas, apps/api, apps/web — clean
  • eslint src on all three — clean
  • prettier --check — clean
  • vitest run (whole workspace) — 257 passed, 1 skipped, 1 failed

The one failure is instrument-records.service.spec.ts > upload > should create records with pending set, which fails identically on pristine main and is unrelated.

Notes for review

  • useFindSessionQuery had exactly one caller and is removed, since this change orphaned it.
  • GET /v1/sessions is left in place. It is now unused by the web client but is still unbounded (no pagination, no subjectId filter, and it embeds the full subject per session). Removing it is a breaking API change I did not want to make unilaterally; scoping or removing it is worth a follow-up, and Performance: datahub downloads every session in the group to attach usernames, then joins O(n*m) in the browser #1410 remains open for that.
  • I could not do a manual browser pass — Chrome in this environment is snap-confined and cannot reach host ports. The API behaviour was verified directly against the live stack as shown above.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Bfen9VQemZanJLXinJ6MMG

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses #1410 by eliminating the client-side O(records × sessions) join and the large /v1/sessions fetch when viewing a subject’s instrument records in the datahub. Instead, the API resolves the session user’s username server-side using a scoped secondary lookup keyed only to session IDs referenced by returned records.

Changes:

  • Add an optional session.user.username shape to the shared InstrumentRecord schema so clients can read usernames directly from records.
  • Update the datahub visualization hook to stop fetching /v1/sessions and use record.session?.user?.username instead.
  • Add an API-side post-processing step to label records with session usernames using a deduplicated sessionId lookup, with new unit tests covering edge cases (including orphaned sessions).

Reviewed changes

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

Show a summary per file
File Description
packages/schemas/src/instrument-records/instrument-records.ts Extends InstrumentRecord schema to optionally carry session.user.username for server-resolved usernames.
apps/web/src/hooks/useInstrumentVisualization.ts Removes sessions fetch + client-side join; reads username directly from record.session.
apps/web/src/hooks/useFindSessionQuery.ts Deletes now-unused hook that queried /v1/sessions.
apps/web/src/hooks/tests/useInstrumentVisualization.test.ts Updates fixtures/mocks to provide username via record.session.user.username.
apps/api/src/instrument-records/instrument-records.service.ts Adds session-model lookup and mapping helper to attach usernames to records server-side.
apps/api/src/instrument-records/tests/instrument-records.service.spec.ts Adds tests for username labelling, deduped lookups, empty-record short-circuit, and orphaned-session behavior.

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

Comment thread apps/api/src/instrument-records/instrument-records.service.ts Outdated
* record's session has since been deleted. Looking the sessions up by id degrades to a missing
* username for that record instead of a failed request.
*/
private async withSessionUsernames(records: PrismaInstrumentRecord[]): Promise<InstrumentRecord[]> {
Comment thread apps/api/src/instrument-records/instrument-records.service.ts
@joshunrau

Copy link
Copy Markdown
Collaborator

@gdevenyi please rebase against main

Viewing one subject's records fetched every session in the current
group, each with its subject embedded, and joined them in the browser
with `sessions.find(...)` inside `records.map(...)` -- O(records x
sessions) to label a few dozen rows.

Resolve the username on the server instead, keyed on the sessions the
returned records actually reference. Measured against a real MongoDB
replica set with 20,031 sessions in the group being viewed:

  before   records 2,624 B + sessions 12,922,313 B / 950 ms
  after    records 2,786 B                        /  50 ms

The `/v1/sessions` request is gone from the datahub entirely, and the
client-side join with it.

This is a second scoped query rather than an `include` on the relation.
The first attempt used `include: { session: { select: { user: ... } } }`,
which is what the issue proposed, but `session` is declared required in
the prisma schema, so Prisma aborts the entire query with "Inconsistent
query result: Field session is required to return data, got `null`" if
any one record's session has since been deleted. Verified against a live
instance: main returns 200 for such a record, the `include` version
returned 500. Looking sessions up by id degrades to a missing username
for that record instead of a failed request, and is covered by a test.

`useFindSessionQuery` had no other caller and is removed. `GET
/v1/sessions` is left in place -- it is now unused by the web client and
is still unbounded, so it remains a candidate for scoping or removal.

Refs #1410

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bfen9VQemZanJLXinJ6MMG
@gdevenyi
gdevenyi force-pushed the fix/datahub-record-usernames branch from 6d95442 to c3e9199 Compare July 23, 2026 17:41

@joshunrau joshunrau left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to apply ability scoping. Right now, this does not apply access controls.

The server-side username resolution queried sessions by id alone, so a
readable record referencing a session outside the caller's read scope
(possible with inconsistent MongoDB data) could leak that session's
username. Thread the ability through and apply accessibleQuery.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Performance: datahub downloads every session in the group to attach usernames, then joins O(n*m) in the browser

3 participants