Skip to content
Closed
Show file tree
Hide file tree
Changes from 89 commits
Commits
Show all changes
93 commits
Select commit Hold shift + click to select a range
f94f1aa
feat: create useFindSession Hook
david-roper Oct 24, 2025
4c2f8ce
feat: make useSession hook return one session instead of array
david-roper Oct 27, 2025
afc75ec
feat: add hook to return list of user ids
david-roper Oct 27, 2025
67a2fec
feat: new api reqs for finding sessions
david-roper Oct 27, 2025
50b12da
feat: add query from nest
david-roper Nov 6, 2025
582d8ff
feat: add userinfo method
david-roper Nov 6, 2025
d9864a5
refactor: remove unused useFindSession hook
david-roper Nov 10, 2025
72a3659
feat: add userInfo call to useEffect to get userId from the session
david-roper Nov 10, 2025
57ed21f
chore: rename user id column
david-roper Nov 10, 2025
3dc66b2
feat: collect username with user api call
david-roper Nov 12, 2025
3106352
refactor: move session and user api methods to separate hook files
david-roper Nov 12, 2025
6aefd4f
feat: create mocks for findUser and findSession hooks
david-roper Nov 12, 2025
1cf8276
test: fix use tests with wait for methods
david-roper Nov 13, 2025
3a2e07c
refactor: make Username a standalone column in long export formats
david-roper Nov 14, 2025
7247e53
test: change tests to include username
david-roper Nov 14, 2025
784d576
chore: small changes to test
david-roper Nov 14, 2025
ebe30a4
fix: fix description in session controller
david-roper Nov 17, 2025
29a4895
fix: add ability to find sessions list
david-roper Nov 17, 2025
aaa9e19
fix: error msg in usefindsession
david-roper Nov 17, 2025
441218d
fix: throw and error to catch instead of returning null when error oc…
david-roper Nov 17, 2025
c5e9c6d
feat: use schema parsing to confirm contents instead of casting it
david-roper Nov 17, 2025
b58dd17
feat: adjust username variable to start as N/A, adjust tests
david-roper Nov 17, 2025
7c08a6b
fix: fix type exports
david-roper Nov 17, 2025
9ce8825
test: change positions of subjectId and username column to make linte…
david-roper Nov 17, 2025
e4fca74
refactor: remove unused api call
david-roper Nov 17, 2025
5248ffe
chore: linter fixes
david-roper Nov 18, 2025
78d4456
test: add resolved promise is session and userinfo mocked methods
david-roper Nov 18, 2025
b366c83
fix: remove extra append statement in excel download method
david-roper Nov 18, 2025
d42bac2
feat: fix not finding user id issue by making subject inclusion optional
david-roper Nov 18, 2025
ca014eb
chore: revert session schema and parse change
david-roper Nov 18, 2025
b534f9b
fix: make username column in wideRow method and its tests more consi…
david-roper Nov 18, 2025
85034fe
feat: remove redundant null return type from useFindSession
david-roper Nov 18, 2025
3dce520
feat: add error notification for useEffect
david-roper Nov 18, 2025
1f2a215
chore: and encodeUriComponent to ids
david-roper Nov 19, 2025
98b220d
feat: fetch sessions then users in parrallel and update test mock values
david-roper Nov 19, 2025
f7e7ab6
feat: add cancelled var to avoid race conditions in fetch records
david-roper Nov 19, 2025
7c4c807
feat: return null on errors userInfo issues
david-roper Nov 19, 2025
688e779
feat: add new findAllSessionsIncludeUsernames api call and todo comments
david-roper Nov 19, 2025
b6e51b3
feat: rename function to useFindSessionQuery
david-roper Nov 20, 2025
33f53b1
feat: update types of findAllIncludeUsernames
david-roper Nov 20, 2025
6e8bb64
feat: update query and type imports
david-roper Nov 20, 2025
945e6be
feat: changed how we find sessions to useFindSessionQuery instead, te…
david-roper Nov 20, 2025
490c373
feat: cleanup unused sessionInfo method, resolve prettier issues
david-roper Nov 21, 2025
2b4e691
test: update test mocks
david-roper Nov 21, 2025
abd0ddd
feat: user find method instead of filter to get 1 unique userSession
david-roper Nov 21, 2025
a5f0b71
fix: adding ! to currentSession subject mentions at they should alway…
david-roper Nov 21, 2025
ddf1ac2
feat: create useFindSession Hook
david-roper Oct 24, 2025
4e86bec
feat: make useSession hook return one session instead of array
david-roper Oct 27, 2025
12a49c4
feat: add hook to return list of user ids
david-roper Oct 27, 2025
0267288
feat: new api reqs for finding sessions
david-roper Oct 27, 2025
017cdf5
feat: add query from nest
david-roper Nov 6, 2025
e4a6175
feat: add userinfo method
david-roper Nov 6, 2025
9fda503
refactor: remove unused useFindSession hook
david-roper Nov 10, 2025
2d5cf94
feat: add userInfo call to useEffect to get userId from the session
david-roper Nov 10, 2025
b5664b1
chore: rename user id column
david-roper Nov 10, 2025
350964d
feat: collect username with user api call
david-roper Nov 12, 2025
9a505d1
refactor: move session and user api methods to separate hook files
david-roper Nov 12, 2025
8daaf56
feat: create mocks for findUser and findSession hooks
david-roper Nov 12, 2025
78717a1
test: fix use tests with wait for methods
david-roper Nov 13, 2025
2a3f220
refactor: make Username a standalone column in long export formats
david-roper Nov 14, 2025
db573d1
test: change tests to include username
david-roper Nov 14, 2025
fc31951
chore: small changes to test
david-roper Nov 14, 2025
02845cd
fix: fix description in session controller
david-roper Nov 17, 2025
e071da8
fix: add ability to find sessions list
david-roper Nov 17, 2025
d81b8c5
fix: error msg in usefindsession
david-roper Nov 17, 2025
0fc8f52
fix: throw and error to catch instead of returning null when error oc…
david-roper Nov 17, 2025
30dc81c
feat: use schema parsing to confirm contents instead of casting it
david-roper Nov 17, 2025
cb271da
feat: adjust username variable to start as N/A, adjust tests
david-roper Nov 17, 2025
a4584e9
fix: fix type exports
david-roper Nov 17, 2025
5392053
test: change positions of subjectId and username column to make linte…
david-roper Nov 17, 2025
14a2220
refactor: remove unused api call
david-roper Nov 17, 2025
e1f9a75
chore: linter fixes
david-roper Nov 18, 2025
e7161a8
test: add resolved promise is session and userinfo mocked methods
david-roper Nov 18, 2025
4890054
fix: remove extra append statement in excel download method
david-roper Nov 18, 2025
58407e1
feat: fix not finding user id issue by making subject inclusion optional
david-roper Nov 18, 2025
191127e
chore: revert session schema and parse change
david-roper Nov 18, 2025
6d8fcca
fix: make username column in wideRow method and its tests more consi…
david-roper Nov 18, 2025
31da08f
feat: remove redundant null return type from useFindSession
david-roper Nov 18, 2025
1c3d955
feat: add error notification for useEffect
david-roper Nov 18, 2025
85809da
chore: and encodeUriComponent to ids
david-roper Nov 19, 2025
066c5a9
feat: fetch sessions then users in parrallel and update test mock values
david-roper Nov 19, 2025
a954235
feat: add cancelled var to avoid race conditions in fetch records
david-roper Nov 19, 2025
856d824
feat: return null on errors userInfo issues
david-roper Nov 19, 2025
efe9d87
feat: add new findAllSessionsIncludeUsernames api call and todo comments
david-roper Nov 19, 2025
9c15607
feat: rename function to useFindSessionQuery
david-roper Nov 20, 2025
8349bd0
feat: update types of findAllIncludeUsernames
david-roper Nov 20, 2025
adf6df7
feat: update query and type imports
david-roper Nov 20, 2025
fad53a5
Merge branch 'add-username-to-export' into add-username-to-export
david-roper Nov 24, 2025
19515b8
Remove duplicate import for SessionWithUser type
david-roper Nov 25, 2025
895495b
fix: use zod error message for useFindSessionQuery
david-roper Nov 26, 2025
c070dec
fix: remove unused parts of tests, remove unused userInfo method
david-roper Nov 26, 2025
20c046c
chore: add linter format changes
david-roper Nov 26, 2025
7e71a32
chore: update parse to parseAsync in useFindSessionQuery
david-roper Nov 26, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion apps/api/src/sessions/sessions.controller.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { CurrentUser } from '@douglasneuroinformatics/libnest';
import { Body, Controller, Get, Param, Post } from '@nestjs/common';
import { Body, Controller, Get, Param, Post, Query } from '@nestjs/common';
import { ApiOperation } from '@nestjs/swagger';
import type { SessionWithUser } from '@opendatacapture/schemas/session';
import type { Session } from '@prisma/client';

import type { AppAbility } from '@/auth/auth.types';
Expand All @@ -20,6 +21,16 @@ export class SessionsController {
return this.sessionsService.create(data);
}

@ApiOperation({ description: 'Find all sessions and usernames attached to them' })
@Get()
@RouteAccess({ action: 'read', subject: 'Session' })
findAllIncludeUsernames(
@CurrentUser('ability') ability: AppAbility,
@Query('groupId') groupId?: string
): Promise<SessionWithUser[]> {
return this.sessionsService.findAllIncludeUsernames(groupId, { ability });
}

@ApiOperation({ description: 'Find Session by ID' })
@Get(':id')
@RouteAccess({ action: 'read', subject: 'Session' })
Expand Down
24 changes: 23 additions & 1 deletion apps/api/src/sessions/sessions.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,9 @@ export class SessionsService {
let group: Group | null = null;
if (groupId && !subject.groupIds.includes(groupId)) {
group = await this.groupsService.findById(groupId);
await this.subjectsService.addGroupForSubject(subject.id, group.id);
if (group) {
await this.subjectsService.addGroupForSubject(subject.id, group.id);
}
}

const { id } = await this.sessionModel.create({
Expand Down Expand Up @@ -94,6 +96,26 @@ export class SessionsService {
});
}

async findAllIncludeUsernames(groupId?: string, { ability }: EntityOperationOptions = {}) {
const sessionsWithUsers = await this.sessionModel.findMany({
include: {
user: {
select: {
username: true
}
},
subject: true
},
where: {
AND: [accessibleQuery(ability, 'read', 'Session'), { groupId }]
}
});
Comment on lines 99 to 112
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 20, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Fix optional groupId filtering logic.

When groupId is undefined, Prisma interprets { groupId: undefined } as "match rows where groupId IS NULL" rather than ignoring the filter. This will return only sessions without a group instead of all sessions.

Apply this diff to conditionally include the groupId filter:

 async findAllIncludeUsernames(groupId?: string, { ability }: EntityOperationOptions = {}) {
   const sessionsWithUsers = await this.sessionModel.findMany({
     include: {
       user: {
         select: {
           username: true
         }
       },
       subject: true
     },
     where: {
-      AND: [accessibleQuery(ability, 'read', 'Session'), { groupId }]
+      AND: [accessibleQuery(ability, 'read', 'Session'), ...(groupId ? [{ groupId }] : [])]
     }
   });
🧰 Tools
🪛 ESLint

[error] 104-104: Expected "subject" to come before "user".

(perfectionist/sort-objects)

🤖 Prompt for AI Agents
In apps/api/src/sessions/sessions.service.ts around lines 96 to 109, the current
where clause always includes { groupId } which causes Prisma to treat undefined
as IS NULL; change the where construction to only include the groupId filter
when groupId is not undefined (e.g. build the AND array or a where object, push
or spread { groupId } only if typeof groupId !== 'undefined'), leaving
accessibleQuery(ability, 'read', 'Session') always present so that when groupId
is omitted the query returns all groups instead of only nulls.

Copy link
Collaborator

Choose a reason for hiding this comment

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

coderabbit is wrong

Copy link
Contributor

Choose a reason for hiding this comment

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

@joshunrau You're correct. The current code is fine.

Prisma ignores undefined values in where clauses, so when groupId is undefined, the filter { groupId } is effectively ignored and the query returns all sessions (subject to ability checks). When groupId has a value, it filters correctly. No changes needed.


✏️ Learnings added
Learnt from: joshunrau
Repo: DouglasNeuroInformatics/OpenDataCapture PR: 1227
File: apps/api/src/sessions/sessions.service.ts:99-112
Timestamp: 2025-11-25T23:34:34.698Z
Learning: In Prisma where clauses, undefined values are ignored by the query builder. For example, `{ where: { groupId: undefined } }` will not filter by groupId at all, rather than matching NULL values. This means optional parameters can be safely passed directly without conditional spreading.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

if (!sessionsWithUsers) {
throw new NotFoundException(`Failed to find users`);
}
Comment on lines +113 to +115
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 20, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Remove incorrect null check and fix error message.

findMany returns an array, which is never null or undefined. An empty array [] is truthy, so this check never throws. If you want to error on zero results, check .length === 0. Additionally, the error message says "users" but should say "sessions."

Apply this diff:

-  if (!sessionsWithUsers) {
-    throw new NotFoundException(`Failed to find users`);
+  if (sessionsWithUsers.length === 0) {
+    throw new NotFoundException(`Failed to find sessions`);
   }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (!sessionsWithUsers) {
throw new NotFoundException(`Failed to find users`);
}
if (sessionsWithUsers.length === 0) {
throw new NotFoundException(`Failed to find sessions`);
}
🤖 Prompt for AI Agents
In apps/api/src/sessions/sessions.service.ts around lines 110 to 112, the
current null check on the result of findMany is incorrect and the error message
refers to "users" instead of "sessions"; replace the condition with an explicit
empty-array check (if (sessionsWithUsers.length === 0) ...) and update the
thrown NotFoundException message to say something like "Failed to find sessions"
so it triggers when no sessions are returned and reports the correct resource.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

return sessionsWithUsers;
}

async findById(id: string, { ability }: EntityOperationOptions = {}) {
const session = await this.sessionModel.findFirst({
where: { AND: [accessibleQuery(ability, 'read', 'Session')], id }
Expand Down
4 changes: 2 additions & 2 deletions apps/web/src/components/Sidebar/Sidebar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ export const Sidebar = () => {
>
<h5 className="text-sm font-medium">{t('common.sessionInProgress')}</h5>
<hr className="my-1.5 h-[1px] border-none bg-slate-700" />
{isSubjectWithPersonalInfo(currentSession.subject) ? (
{isSubjectWithPersonalInfo(currentSession.subject!) ? (
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the old PR superseded by this? If so, please close it.

<div data-testid="current-session-info">
<p>{`${t('core.fullName')}: ${currentSession.subject.firstName} ${currentSession.subject.lastName}`}</p>
<p>
Expand All @@ -100,7 +100,7 @@ export const Sidebar = () => {
</div>
) : (
<div data-testid="current-session-info">
<p>ID: {removeSubjectIdScope(currentSession.subject.id)}</p>
<p>ID: {removeSubjectIdScope(currentSession.subject!.id)}</p>
</div>
)}
</motion.div>
Expand Down
100 changes: 74 additions & 26 deletions apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { toBasicISOString } from '@douglasneuroinformatics/libjs';
import { act, renderHook } from '@testing-library/react';
import { act, renderHook, waitFor } from '@testing-library/react';
import { beforeEach, describe, expect, it, vi } from 'vitest';

import { useInstrumentVisualization } from '../useInstrumentVisualization';
Expand Down Expand Up @@ -32,11 +32,32 @@
{
computedMeasures: {},
data: { someValue: 'abc' },
date: FIXED_TEST_DATE
date: FIXED_TEST_DATE,
sessionId: '123'
}
]
};

const mockSessionWithUsername = {
data: [
{
id: '123',
user: {
username: 'testusername'
}
}
]
};

const mockSession = {

Check failure on line 52 in apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts

View workflow job for this annotation

GitHub Actions / lint-and-test

'mockSession' is assigned a value but never used. Allowed unused vars must match /^_/u
Copy link
Collaborator

Choose a reason for hiding this comment

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

make sure all linting passes

userId: '111'
};

const mockUser = {

Check failure on line 56 in apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts

View workflow job for this annotation

GitHub Actions / lint-and-test

'mockUser' is assigned a value but never used. Allowed unused vars must match /^_/u
id: '111',
username: 'testusername'
};
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Double‑check mock module path matches the hook’s import.

The hook imports sessionInfo from './useFindSessionQuery', but the test mocks '@/hooks/useFindSession'. Unless there’s a re‑export wiring those paths together, this mock won’t intercept the actual sessionInfo calls and tests could hit real network requests.

Confirm that @/hooks/useFindSession resolves to the same module used by useInstrumentVisualization, or adjust the mock path to match the actual import.

Also applies to: 76-82


🏁 Script executed:

#!/bin/bash
# Verify how sessionInfo is imported and where it's defined
rg -n "sessionInfo" apps/web/src/hooks -C3

# See which modules import '@/hooks/useFindSession' vs './useFindSessionQuery'
rg -n "@/hooks/useFindSession" -C2
rg -n "./useFindSessionQuery" apps/web/src/hooks -C2

Length of output: 3359


Fix mock module path to match actual import source.

The test mocks '@/hooks/useFindSession' (line 76), but the hook imports from './useFindSessionQuery'. No module at @/hooks/useFindSession exists—the actual module is useFindSessionQuery.ts. Change the mock to:

vi.mock('./useFindSessionQuery', () => ({
  sessionInfo: () => Promise.resolve(mockSession),
  useFindSessionQuery: () => ({ /* mock */ })
}));

Same issue at line 80 for '@/hooks/useFindUser'—verify the actual import path and adjust accordingly.

🤖 Prompt for AI Agents
In apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts around lines
41 to 48, the test mocks incorrect module paths (it mocks
'@/hooks/useFindSession' and '@/hooks/useFindUser') while the hook actually
imports from local files (e.g. './useFindSessionQuery' and likely
'./useFindUserQuery'); update the vi.mock calls to target the real module
filenames used by the hook (replace '@/hooks/useFindSession' with
'./useFindSessionQuery' and '@/hooks/useFindUser' with the correct
'./useFindUserQuery' or exact import path), and adapt the mocked exports to
match the actual exported names (export sessionInfo/useFindSessionQuery and the
corresponding user hooks) so the tests import the mocks correctly.


vi.mock('@/hooks/useInstrument', () => ({
useInstrument: mockUseInstrument
}));
Expand All @@ -63,78 +84,97 @@
useInstrumentRecords: () => mockInstrumentRecords
}));

vi.mock('@/hooks/useFindSessionQuery', () => ({
useFindSessionQuery: () => mockSessionWithUsername
}));

describe('useInstrumentVisualization', () => {
beforeEach(() => {
vi.clearAllMocks();
});

describe('CSV', () => {
it('Should download', () => {
it('Should download', async () => {
const { result } = renderHook(() => useInstrumentVisualization({ params: { subjectId: 'testId' } }));
const { records } = result.current;
await waitFor(() => {
expect(result.current.records.length).toBeGreaterThan(0);
});
act(() => result.current.dl('CSV'));
expect(records).toBeDefined();
expect(mockDownloadFn).toHaveBeenCalledTimes(1);
const [filename, getContentFn] = mockDownloadFn.mock.calls[0] ?? [];
expect(filename).toContain('.csv');
const csvContents = getContentFn();
expect(csvContents).toMatch(
`GroupID,subjectId,Date,someValue\r\ntestGroupId,testId,${toBasicISOString(FIXED_TEST_DATE)},abc`
`GroupID,subjectId,Date,Username,someValue\r\ntestGroupId,testId,${toBasicISOString(FIXED_TEST_DATE)},testusername,abc`
);
});
});
describe('TSV', () => {
it('Should download', () => {
it('Should download', async () => {
const { result } = renderHook(() => useInstrumentVisualization({ params: { subjectId: 'testId' } }));
const { dl, records } = result.current;
act(() => dl('TSV'));
const { records } = result.current;
await waitFor(() => {
expect(result.current.records.length).toBeGreaterThan(0);
});
act(() => result.current.dl('TSV'));
expect(records).toBeDefined();
expect(mockDownloadFn).toHaveBeenCalledTimes(1);
const [filename, getContentFn] = mockDownloadFn.mock.calls[0] ?? [];
expect(filename).toContain('.tsv');
const tsvContents = getContentFn();
expect(tsvContents).toMatch(
`GroupID\tsubjectId\tDate\tsomeValue\r\ntestGroupId\ttestId\t${toBasicISOString(FIXED_TEST_DATE)}\tabc`
`GroupID\tsubjectId\tDate\tUsername\tsomeValue\r\ntestGroupId\ttestId\t${toBasicISOString(FIXED_TEST_DATE)}\ttestusername\tabc`
);
});
});
describe('CSV Long', () => {
it('Should download', () => {
it('Should download', async () => {
const { result } = renderHook(() => useInstrumentVisualization({ params: { subjectId: 'testId' } }));
const { dl, records } = result.current;
act(() => dl('CSV Long'));
const { records } = result.current;
await waitFor(() => {
expect(result.current.records.length).toBeGreaterThan(0);
});
act(() => result.current.dl('CSV Long'));
expect(records).toBeDefined();
expect(mockDownloadFn).toHaveBeenCalledTimes(1);

const [filename, getContentFn] = mockDownloadFn.mock.calls[0] ?? [];
expect(filename).toContain('.csv');
const csvLongContents = getContentFn();
expect(csvLongContents).toMatch(
`GroupID,Date,SubjectID,Value,Variable\r\ntestGroupId,${toBasicISOString(FIXED_TEST_DATE)},testId,abc,someValue`
`GroupID,Date,SubjectID,Username,Value,Variable\r\ntestGroupId,${toBasicISOString(FIXED_TEST_DATE)},testId,testusername,abc,someValue`
);
});
});
describe('TSV Long', () => {
it('Should download', () => {
it('Should download', async () => {
const { result } = renderHook(() => useInstrumentVisualization({ params: { subjectId: 'testId' } }));
const { dl, records } = result.current;
act(() => dl('TSV Long'));
const { records } = result.current;
await waitFor(() => {
expect(result.current.records.length).toBeGreaterThan(0);
});
act(() => result.current.dl('TSV Long'));
expect(records).toBeDefined();
expect(mockDownloadFn).toHaveBeenCalledTimes(1);

const [filename, getContentFn] = mockDownloadFn.mock.calls[0] ?? [];
expect(filename).toMatch('.tsv');
const tsvLongContents = getContentFn();
expect(tsvLongContents).toMatch(
`GroupID\tDate\tSubjectID\tValue\tVariable\r\ntestGroupId\t${toBasicISOString(FIXED_TEST_DATE)}\ttestId\tabc\tsomeValue`
`GroupID\tDate\tSubjectID\tUsername\tValue\tVariable\r\ntestGroupId\t${toBasicISOString(FIXED_TEST_DATE)}\ttestId\ttestusername\tabc\tsomeValue`
);
});
});
describe('Excel', () => {
it('Should download', () => {
it('Should download', async () => {
const { result } = renderHook(() => useInstrumentVisualization({ params: { subjectId: 'testId' } }));
const { dl, records } = result.current;
act(() => dl('Excel'));
const { records } = result.current;
await waitFor(() => {
expect(result.current.records.length).toBeGreaterThan(0);
});
act(() => result.current.dl('Excel'));
expect(records).toBeDefined();
expect(mockExcelDownloadFn).toHaveBeenCalledTimes(1);
const [filename, getContentFn] = mockExcelDownloadFn.mock.calls[0] ?? [];
Expand All @@ -143,20 +183,24 @@

expect(excelContents).toEqual([
{
Date: '2025-04-30',
GroupID: 'testGroupId',
subjectId: 'testId',
// eslint-disable-next-line perfectionist/sort-objects
Date: '2025-04-30',
someValue: 'abc'
someValue: 'abc',
Username: 'testusername'
}
]);
});
});
describe('Excel Long', () => {
it('Should download', () => {
it('Should download', async () => {
const { result } = renderHook(() => useInstrumentVisualization({ params: { subjectId: 'testId' } }));
const { dl, records } = result.current;
act(() => dl('Excel Long'));
const { records } = result.current;
await waitFor(() => {
expect(result.current.records.length).toBeGreaterThan(0);
});
act(() => result.current.dl('Excel Long'));
expect(records).toBeDefined();
expect(mockExcelDownloadFn).toHaveBeenCalledTimes(1);

Expand All @@ -169,6 +213,7 @@
Date: '2025-04-30',
GroupID: 'testGroupId',
SubjectID: 'testId',
Username: 'testusername',
Value: 'abc',
Variable: 'someValue'
}
Expand All @@ -178,8 +223,11 @@
describe('JSON', () => {
it('Should download', async () => {
const { result } = renderHook(() => useInstrumentVisualization({ params: { subjectId: 'testId' } }));
const { dl, records } = result.current;
act(() => dl('JSON'));
const { records } = result.current;
await waitFor(() => {
expect(result.current.records.length).toBeGreaterThan(0);
});
act(() => result.current.dl('JSON'));
expect(records).toBeDefined();
expect(mockDownloadFn).toHaveBeenCalledTimes(1);

Expand Down
37 changes: 37 additions & 0 deletions apps/web/src/hooks/useFindSessionQuery.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import { $SessionWithUser } from '@opendatacapture/schemas/session';
import type { SessionWithUserQueryParams } from '@opendatacapture/schemas/session';
import { useQuery } from '@tanstack/react-query';
import axios from 'axios';

//Change this query to into a hook method and name it useFindSessionQuery

//Change the api call to have an include tag which includes the username from users

//Change the return type to

type UseSessionOptions = {
enabled?: boolean;
params: SessionWithUserQueryParams;
};

export const useFindSessionQuery = (
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be named useSessionQuery, consistent with the other query hooks. The same goes for the user query.

{ enabled, params }: UseSessionOptions = {
enabled: true,
params: {}
}
) => {
return useQuery({
enabled,
queryFn: async () => {
const response = await axios.get('/v1/sessions', {
params
});
const parsedData = $SessionWithUser.array().safeParseAsync(response.data);
if ((await parsedData).error) {
throw new Error(`cant find data`);
}
return (await parsedData).data;
},
queryKey: ['sessions', ...Object.values(params)]
});
};
14 changes: 14 additions & 0 deletions apps/web/src/hooks/useFindUser.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import { $User } from '@opendatacapture/schemas/user';
import type { User } from '@opendatacapture/schemas/user';
import axios from 'axios';

export const userInfo = async (userId: string): Promise<null | User> => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as above.

try {
const response = await axios.get(`/v1/users/${encodeURIComponent(userId)}`);
const parsedResult = $User.safeParse(response.data);
return parsedResult.success ? parsedResult.data : null;
} catch (error) {
console.error('Error fetching user:', error);
throw error;
}
};
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 24, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Inconsistent error handling: validation failures return null, network errors throw.

Lines 9 and 12 use different error handling strategies. Validation failure returns null (silent), while network errors throw (noisy). Callers must handle both patterns.

Consider unifying the approach:

  • Option 1: Always throw with specific error types
  • Option 2: Always return null and log both cases
 export const userInfo = async (userId: string): Promise<null | User> => {
   try {
     const response = await axios.get(`/v1/users/${encodeURIComponent(userId)}`);
     const parsedResult = $User.safeParse(response.data);
-    return parsedResult.success ? parsedResult.data : null;
+    if (!parsedResult.success) {
+      console.error('User validation failed:', parsedResult.error);
+      return null;
+    }
+    return parsedResult.data;
   } catch (error) {
     console.error('Error fetching user:', error);
-    throw error;
+    return null;
   }
 };
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export const userInfo = async (userId: string): Promise<null | User> => {
try {
const response = await axios.get(`/v1/users/${encodeURIComponent(userId)}`);
const parsedResult = $User.safeParse(response.data);
return parsedResult.success ? parsedResult.data : null;
} catch (error) {
console.error('Error fetching user:', error);
throw error;
}
};
export const userInfo = async (userId: string): Promise<null | User> => {
try {
const response = await axios.get(`/v1/users/${encodeURIComponent(userId)}`);
const parsedResult = $User.safeParse(response.data);
if (!parsedResult.success) {
console.error('User validation failed:', parsedResult.error);
return null;
}
return parsedResult.data;
} catch (error) {
console.error('Error fetching user:', error);
return null;
}
};

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes. why are you ignoring errors?

Copy link
Contributor

Choose a reason for hiding this comment

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

@joshunrau You're absolutely right - my suggestion to return null for all errors would hide important error information from callers. Silently returning null means the caller can't distinguish between legitimate cases (user not found) versus actual errors (network failure, invalid API response).

The original code's approach of throwing on network errors is actually more appropriate. For consistency, validation failures should probably also throw rather than return null:

export const userInfo = async (userId: string): Promise<User | null> => {
  try {
    const response = await axios.get(`/v1/users/${encodeURIComponent(userId)}`);
    const parsedResult = $User.safeParse(response.data);
    if (!parsedResult.success) {
      throw new Error(`User validation failed: ${parsedResult.error.message}`);
    }
    return parsedResult.data;
  } catch (error) {
    console.error('Error fetching user:', error);
    throw error;
  }
};

This way, callers are forced to handle error cases explicitly, and you maintain visibility into what went wrong. If you need to handle "user not found" as a non-error case, the API should return a 404 status which can be caught and converted to null specifically for that case.

Loading
Loading