Add username to export #1227
Add username to export #1227
Conversation
WalkthroughFetches sessions with usernames and enriches instrument-visualization records with a Username field used in CSV/TSV/Excel/JSON exports; adds a frontend query hook, backend controller/service method to return SessionWithUser, updates schema and tests, and fixes a duplicate sheet append in the Excel util. Changes
Sequence DiagramsequenceDiagram
participant UI as Component
participant Hook as useInstrumentVisualization
participant SessionsQ as useFindSessionQuery
participant API as /v1/sessions
participant Export as ExportGenerator
UI->>Hook: Trigger export
Hook->>SessionsQ: Ensure sessions fetched (enabled)
SessionsQ->>API: GET /v1/sessions (params)
API-->>SessionsQ: SessionWithUser[] (includes user.username)
SessionsQ-->>Hook: Sessions with usernames
rect rgb(230,245,235)
Note over Hook: Async enrichment of records
Hook->>Hook: fetchRecords() match by sessionId
Hook->>Hook: attach Username from session.user.username
Hook->>Hook: set enriched records state
end
Hook->>Export: Build rows (include Username)
Export-->>UI: Downloadable file (CSV/TSV/Excel/JSON)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
david-roper
force-pushed
the
add-username-to-export
branch
2 times, most recently
from
4e28260 to
5e1c096
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
apps/api/src/sessions/sessions.service.ts (1)
106-119: N+1 query pattern impacts performance for bulk lookups.The current implementation executes N separate database queries via
Promise.all, which increases latency and database load for large ID arrays. Consider using a singlefindManyquery and validating the results afterward.Apply this refactor:
async findSessionList(ids: string[], { ability }: EntityOperationOptions = {}) { - const sessionsArray = await Promise.all( - ids.map(async (id) => { - const session = await this.sessionModel.findFirst({ - where: { AND: [accessibleQuery(ability, 'read', 'Session')], id } - }); - if (!session) { - throw new NotFoundException(`Failed to find session with ID: ${id}`); - } - return session; - }) - ); - return sessionsArray; + const sessions = await this.sessionModel.findMany({ + where: { AND: [accessibleQuery(ability, 'read', 'Session'), { id: { in: ids } }] } + }); + + if (sessions.length !== ids.length) { + const foundIds = new Set(sessions.map(s => s.id)); + const missingId = ids.find(id => !foundIds.has(id)); + throw new NotFoundException(`Failed to find session with ID: ${missingId}`); + } + + return sessions; }apps/web/src/hooks/useFindUser.ts (1)
4-12: Consider validating the response data.The type assertion
response.data as Userassumes the API response matches the User schema without validation. If the API returns an unexpected shape, this could cause runtime errors downstream.Consider adding runtime validation using a Zod schema if available:
export const userInfo = async (userId: string): Promise<null | User> => { try { const response = await axios.get(`/v1/users/${userId}`); - return response.data ? (response.data as User) : null; + if (!response.data) return null; + // Assuming $User schema is available from @opendatacapture/schemas/user + const parsed = $User.safeParse(response.data); + return parsed.success ? parsed.data : null; } catch (error) { console.error('Error fetching user:', error); return null; } };apps/web/src/hooks/useFindSession.ts (1)
4-12: Consider validating the response data.Similar to
userInfo, the type assertionresponse.data as Sessionassumes the API response matches the Session schema without validation.Consider adding runtime validation:
export const sessionInfo = async (sessionId: string): Promise<null | Session> => { try { const response = await axios.get(`/v1/sessions/${sessionId}`); - return response.data ? (response.data as Session) : null; + if (!response.data) return null; + // Assuming $Session schema is available from @opendatacapture/schemas/session + const parsed = $Session.safeParse(response.data); + return parsed.success ? parsed.data : null; } catch (error) { - console.error('Error fetching user:', error); + console.error('Error fetching session:', error); return null; } };apps/web/src/hooks/useInstrumentVisualization.ts (2)
99-109: Initialize the username variable.The
usernamevariable is declared without initialization. While the current flow ensures it gets assigned, this pattern is fragile.- let username: string; + let username: string = 'N/A';
221-221: Consider renaming the field from userId to username.The field is named
userIdbut contains the username string value. This naming mismatch could confuse future maintainers.records.push({ __date__: record.date, __time__: record.date.getTime(), - userId: 'N/A', + username: 'N/A', ...record.computedMeasures, ...props });And similarly on line 233. Also update the field extraction in
makeLongRows(line 106).Also applies to: 233-233
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/api/src/sessions/sessions.controller.ts(2 hunks)apps/api/src/sessions/sessions.service.ts(1 hunks)apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts(6 hunks)apps/web/src/hooks/useFindSession.ts(1 hunks)apps/web/src/hooks/useFindUser.ts(1 hunks)apps/web/src/hooks/useInstrumentVisualization.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
apps/api/src/sessions/sessions.service.ts (1)
apps/api/src/core/types.ts (1)
EntityOperationOptions(3-5)
apps/web/src/hooks/useFindSession.ts (1)
packages/schemas/src/session/session.ts (1)
Session(9-9)
apps/web/src/hooks/useFindUser.ts (1)
packages/schemas/src/user/user.ts (1)
User(10-10)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts (1)
apps/web/src/hooks/useInstrumentVisualization.ts (1)
useInstrumentVisualization(32-254)
apps/api/src/sessions/sessions.controller.ts (1)
packages/schemas/src/session/session.ts (1)
Session(9-9)
apps/web/src/hooks/useInstrumentVisualization.ts (2)
apps/web/src/hooks/useFindSession.ts (1)
sessionInfo(4-12)apps/web/src/hooks/useFindUser.ts (1)
userInfo(4-12)
🔇 Additional comments (3)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts (3)
29-81: LGTM!The mock setup correctly provides session and user data to support the async enrichment flow. Mock objects are properly structured with the expected fields.
88-123: LGTM!Tests correctly use
waitForto handle async record population and verify username is included in CSV/TSV exports.
124-232: LGTM!All export format tests consistently verify username data is included and properly handle the async record enrichment flow.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/src/sessions/sessions.controller.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/api/src/sessions/sessions.controller.ts (2)
apps/web/src/store/types.ts (1)
CurrentUser(8-10)packages/schemas/src/session/session.ts (1)
Session(9-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint-and-test
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/web/src/hooks/useFindSession.ts (1)
1-12: Filename doesn't match the export.The file is named
useFindSession.ts(React hook convention), but exportssessionInfo, which is a plain async function. Consider renaming the file tosessionInfo.tsor similar for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/hooks/useFindSession.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/src/hooks/useFindSession.ts (1)
packages/schemas/src/session/session.ts (1)
Session(9-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint-and-test
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/web/src/hooks/useInstrumentVisualization.ts (1)
207-247: Sequential API calls create performance bottleneck.This implementation makes 2N sequential API calls (one
sessionInfo+ oneuserInfoper record). For large datasets, this will be slow. Parallelize usingPromise.allto fetch all sessions first, then all unique users.Refactor to batch API calls:
useEffect(() => { const fetchRecords = async () => { try { if (recordsQuery.data) { - const records: InstrumentVisualizationRecord[] = []; - - for (const record of recordsQuery.data) { - const props = record.data && typeof record.data === 'object' ? record.data : {}; - - const sessionData = await sessionInfo(record.sessionId); - - if (!sessionData?.userId) { - records.push({ - __date__: record.date, - __time__: record.date.getTime(), - userId: 'N/A', - ...record.computedMeasures, - ...props - }); - continue; - } - - const userData = await userInfo(sessionData.userId); - // safely check since userData can be null - records.push({ - __date__: record.date, - __time__: record.date.getTime(), - userId: userData?.username ?? 'N/A', - ...record.computedMeasures, - ...props - }); - } + // Fetch all sessions in parallel + const sessionPromises = recordsQuery.data.map(record => sessionInfo(record.sessionId)); + const sessions = await Promise.all(sessionPromises); + + // Extract unique userIds and fetch users in parallel + const userIds = [...new Set(sessions.filter(s => s?.userId).map(s => s!.userId))]; + const userPromises = userIds.map(userId => userInfo(userId)); + const users = await Promise.all(userPromises); + const userMap = new Map(users.filter(u => u).map(u => [u!.id, u!.username])); + + // Build records with looked-up data + const records: InstrumentVisualizationRecord[] = recordsQuery.data.map((record, i) => { + const props = record.data && typeof record.data === 'object' ? record.data : {}; + const session = sessions[i]; + const username = session?.userId ? (userMap.get(session.userId) ?? 'N/A') : 'N/A'; + + return { + __date__: record.date, + __time__: record.date.getTime(), + userId: username, + ...record.computedMeasures, + ...props + }; + }); setRecords(records); } } catch (error) { console.error('Error occurred: ', error); } }; void fetchRecords(); }, [recordsQuery.data]);Note: Adjust
userMapkey if User type doesn't have anidfield.
🧹 Nitpick comments (1)
apps/web/src/hooks/useInstrumentVisualization.ts (1)
242-244: Surface errors to the user.The error is logged but not surfaced to the user via notifications. Users won't know if data enrichment failed.
Add notification on error:
} catch (error) { console.error('Error occurred: ', error); + notifications.addNotification({ + message: t('errors.failedToEnrichRecords'), + type: 'error' + }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/src/hooks/useFindSession.ts(1 hunks)apps/web/src/hooks/useFindUser.ts(1 hunks)apps/web/src/hooks/useInstrumentVisualization.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/src/hooks/useFindUser.ts
- apps/web/src/hooks/useFindSession.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/src/hooks/useInstrumentVisualization.ts (2)
apps/web/src/hooks/useFindSession.ts (1)
sessionInfo(4-12)apps/web/src/hooks/useFindUser.ts (1)
userInfo(4-12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint-and-test
🔇 Additional comments (1)
apps/web/src/hooks/useInstrumentVisualization.ts (1)
16-17: LGTM!Imports are correct and necessary for the username enrichment functionality.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/web/src/hooks/useFindSession.ts (1)
6-6: Apply URL encoding to sessionId.The
sessionIdis interpolated directly without encoding. Special characters will break the request.- const response = await axios.get(`/v1/sessions/${sessionId}`); + const response = await axios.get(`/v1/sessions/${encodeURIComponent(sessionId)}`);
🧹 Nitpick comments (2)
apps/web/src/hooks/useFindUser.ts (1)
7-8: Log parse failures.When
safeParsefails, returningnullwithout logging makes debugging difficult. Consider logging the parse error.const parsedResult = $User.safeParse(response.data); + if (!parsedResult.success) { + console.error('Failed to parse user data:', parsedResult.error); + } return parsedResult.success ? parsedResult.data : null;apps/web/src/hooks/useFindSession.ts (1)
7-8: Log parse failures.When
safeParsefails, returningnullwithout logging makes debugging difficult. Consider logging the parse error.const parsedResult = $Session.safeParse(response.data); + if (!parsedResult.success) { + console.error('Failed to parse session data:', parsedResult.error); + } return parsedResult.success ? parsedResult.data : null;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/src/hooks/useFindSession.ts(1 hunks)apps/web/src/hooks/useFindUser.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/web/src/hooks/useFindSession.ts (1)
packages/schemas/src/session/session.ts (2)
Session(9-9)$Session(10-17)
apps/web/src/hooks/useFindUser.ts (1)
packages/schemas/src/user/user.ts (2)
User(10-10)$User(11-20)
🪛 ESLint
apps/web/src/hooks/useFindSession.ts
[error] 1-1: Prefer using a top-level type-only import instead of inline type specifiers.
(import/consistent-type-specifier-style)
[error] 1-1: Expected "$Session" to come before "Session".
(perfectionist/sort-named-imports)
apps/web/src/hooks/useFindUser.ts
[error] 1-1: Prefer using a top-level type-only import instead of inline type specifiers.
(import/consistent-type-specifier-style)
[error] 1-1: Expected "$User" to come before "User".
(perfectionist/sort-named-imports)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint-and-test
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/web/src/hooks/useInstrumentVisualization.ts (1)
207-247: Performance: Sequential API calls already flagged.This implementation fetches session and user data sequentially for each record (2N serial API calls). A past review comment already provided a detailed refactor to parallelize these calls using
Promise.allfor better performance with large datasets.
🧹 Nitpick comments (2)
apps/web/src/hooks/useInstrumentVisualization.ts (2)
99-99: Optional: Remove inferrable type annotation.ESLint flags this line because TypeScript can infer
stringfrom the literal value.Apply this diff:
- let username: string = 'N/A'; + let username = 'N/A';
242-244: Optional: Add user notification on enrichment failure.When session/user data fetching fails, users see no records without explanation. Consider notifying users of the error.
Example approach:
} catch (error) { console.error('Error occurred: ', error); notifications.addNotification({ message: t('errors.failedToEnrichRecords'), type: 'error' }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts(6 hunks)apps/web/src/hooks/useFindSession.ts(1 hunks)apps/web/src/hooks/useFindUser.ts(1 hunks)apps/web/src/hooks/useInstrumentVisualization.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/src/hooks/useFindSession.ts
- apps/web/src/hooks/useFindUser.ts
🧰 Additional context used
🧬 Code graph analysis (2)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts (1)
apps/web/src/hooks/useInstrumentVisualization.ts (1)
useInstrumentVisualization(32-258)
apps/web/src/hooks/useInstrumentVisualization.ts (2)
apps/web/src/hooks/useFindSession.ts (1)
sessionInfo(5-14)apps/web/src/hooks/useFindUser.ts (1)
userInfo(5-14)
🪛 ESLint
apps/web/src/hooks/useInstrumentVisualization.ts
[error] 99-99: Type string trivially inferred from a string literal, remove type annotation.
(@typescript-eslint/no-inferrable-types)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint-and-test
🔇 Additional comments (7)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts (4)
2-2: LGTM: Import for async testing.The
waitForimport is correctly added to handle the asynchronous record enrichment flow.
35-47: LGTM: Mock data properly structured.The addition of
sessionIdto mock records and the newmockSession/mockUserobjects correctly support the enrichment flow tested throughout the suite.
75-81: LGTM: Session and user mocks configured.The mocks for
sessionInfoanduserInfoare correctly set up to return the test fixtures.
89-230: LGTM: All tests updated for async enrichment.All test cases properly:
- Use
waitForto await the asynchronous record enrichment- Include username/Username in expected outputs across all export formats
- Match the implementation's behavior consistently
apps/web/src/hooks/useInstrumentVisualization.ts (3)
16-17: LGTM: Required imports added.The
sessionInfoanduserInfohelpers are correctly imported for record enrichment.
106-109: LGTM: Username extraction in long-form export.The username field is correctly extracted from enriched records and skipped from variable iteration.
118-118: LGTM: Username included in long-form rows.Username is consistently injected into both array-item and single-value long-form rows.
Also applies to: 131-131
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts (1)
216-231: Consider validating username in JSON output.The JSON test validates
someValueandsubjectIDbut doesn't explicitly check for theusernamefield. Consider adding:const jsonContents = await getContentFn(); expect(jsonContents).toContain('"someValue": "abc"'); expect(jsonContents).toContain('"subjectID": "testId"'); + expect(jsonContents).toContain('"username": "testusername"');apps/web/src/hooks/useInstrumentVisualization.ts (1)
207-246: Add cleanup and improve error handling.The useEffect lacks cleanup for in-flight requests and only logs errors to console without user feedback.
Consider these improvements:
- Add AbortController for cleanup:
useEffect(() => { const abortController = new AbortController(); const fetchRecords = async () => { try { if (recordsQuery.data) { // ... fetch logic with signal: abortController.signal } } catch (error) { if (!abortController.signal.aborted) { console.error('Error occurred: ', error); notifications.addNotification({ message: t('errors.failedToLoadRecords'), type: 'error' }); } } }; void fetchRecords(); return () => abortController.abort(); }, [recordsQuery.data]);
- Sequential API calls: Note that a past review comment already flagged the performance issue with sequential
awaitcalls. Consider addressing that feedback by parallelizing the requests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts(6 hunks)apps/web/src/hooks/useInstrumentVisualization.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts (1)
apps/web/src/hooks/useInstrumentVisualization.ts (1)
useInstrumentVisualization(32-258)
apps/web/src/hooks/useInstrumentVisualization.ts (2)
apps/web/src/hooks/useFindSession.ts (1)
sessionInfo(5-14)apps/web/src/hooks/useFindUser.ts (1)
userInfo(5-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint-and-test
🔇 Additional comments (6)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts (2)
89-104: LGTM! Async test pattern correctly implemented.The test properly waits for record enrichment before triggering the download and validates username inclusion in the output.
125-141: LGTM! Long-format test correctly validates Username field.The test properly verifies the Username field is included in long-format exports.
apps/web/src/hooks/useInstrumentVisualization.ts (4)
16-17: LGTM! Imports correctly added.
99-109: LGTM! Username extraction logic is correct.The username is properly initialized with a fallback and extracted from the record before being used in long-form rows.
75-92: Verify username inclusion in wide-format exports is intentional.The
makeWideRowsfunction includes all record keys, sousernamewill appear in wide-format exports (CSV, TSV, Excel). The AI summary states "wide-form export remains unchanged," but the implementation and tests confirm username is added to both wide and long formats.If username should only appear in long-format exports, filter it out in
makeWideRows:for (const key of columnNames) { + if (key === 'username') { + continue; + } const val = item[key]; if (key === '__date__') { obj.Date = toBasicISOString(val as Date); continue; } obj[key] = typeof val === 'object' ? JSON.stringify(val) : val; }
119-132: LGTM! Username correctly added to long-format rows.Both array-item and single-value code paths properly include the Username field.
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts (2)
92-96: Destructuredrecordswon't reflect updated state.The
recordsconstant (line 91) captures the initial empty array. AfterwaitFor(),result.current.recordsupdates, but the destructuredrecordsdoesn't. The assertion on line 96 checks the stale value. Consider removing this assertion or checkingresult.current.recordsinstead.This pattern repeats across all test cases (lines 109-114, 127-132, 146-151, 165-170, 191-196, 218-222).
216-231: Add username verification to JSON test.All other format tests verify username presence, but the JSON test doesn't. Add an assertion like
expect(jsonContents).toContain('"username": "testusername"');to ensure comprehensive coverage of the PR's main feature.Apply this diff:
const jsonContents = await getContentFn(); expect(jsonContents).toContain('"someValue": "abc"'); expect(jsonContents).toContain('"subjectID": "testId"'); + expect(jsonContents).toContain('"username": "testusername"'); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts (1)
apps/web/src/hooks/useInstrumentVisualization.ts (1)
useInstrumentVisualization(32-258)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint-and-test
🔇 Additional comments (1)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts (1)
75-81: Mocks now correctly return promises.This addresses the previous review concern. The mocks properly match the async contract of the real implementations.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/web/src/hooks/useFindSession.ts (1)
7-7: Apply URL encoding to sessionId.The sessionId is interpolated directly into the URL without encoding. Special characters will break the request.
Apply this diff:
- const response = await axios.get(`/v1/sessions/${sessionId}`); + const response = await axios.get(`/v1/sessions/${encodeURIComponent(sessionId)}`);
🧹 Nitpick comments (1)
apps/web/src/hooks/useFindSession.ts (1)
10-13: Consider using a structured logger.Console.error is acceptable for development but might not integrate with production logging infrastructure. Consider using the application's logging service if available.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/src/hooks/useFindSession.ts(1 hunks)packages/schemas/src/session/session.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/schemas/src/session/session.ts (1)
packages/schemas/src/subject/subject.ts (1)
$Subject(20-27)
apps/web/src/hooks/useFindSession.ts (1)
packages/schemas/src/session/session.ts (2)
Session(9-9)$Session(10-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint-and-test
🔇 Additional comments (1)
apps/web/src/hooks/useFindSession.ts (1)
1-3: Imports look correct.Type and value imports are properly separated and sorted.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/web/src/hooks/useInstrumentVisualization.ts (1)
217-242: Sequential API calls create performance bottleneck and error fragility.This loops through records with
awaitcalls in sequence, causing:
Performance: N records trigger 2N serial API calls. With 100 records and 100ms per call, this takes ~20 seconds instead of <1 second with parallelization.
Error fragility: Both
sessionInfoanduserInfothrow errors (per code snippets). If any single call fails, the loop exits immediately via the catch block, leaving remaining records unprocessed.Parallelize the API calls:
useEffect(() => { const fetchRecords = async () => { try { if (recordsQuery.data) { - const records: InstrumentVisualizationRecord[] = []; - - for (const record of recordsQuery.data) { - const props = record.data && typeof record.data === 'object' ? record.data : {}; - - const sessionData = await sessionInfo(record.sessionId); - - if (!sessionData?.userId) { - records.push({ - __date__: record.date, - __time__: record.date.getTime(), - username: 'N/A', - ...record.computedMeasures, - ...props - }); - continue; - } - - const userData = await userInfo(sessionData.userId); - // safely check since userData can be null - records.push({ - __date__: record.date, - __time__: record.date.getTime(), - username: userData?.username ?? 'N/A', - ...record.computedMeasures, - ...props - }); - } + // Fetch all sessions in parallel + const sessionPromises = recordsQuery.data.map(record => + sessionInfo(record.sessionId).catch(err => { + console.error(`Failed to fetch session ${record.sessionId}:`, err); + return null; + }) + ); + const sessions = await Promise.all(sessionPromises); + + // Collect unique userIds and fetch users in parallel + const userIds = [...new Set( + sessions.filter(s => s?.userId).map(s => s!.userId) + )]; + const userPromises = userIds.map(userId => + userInfo(userId).catch(err => { + console.error(`Failed to fetch user ${userId}:`, err); + return null; + }) + ); + const users = await Promise.all(userPromises); + const userMap = new Map( + users.filter(u => u).map(u => [u!.id, u!.username]) + ); + + // Build records with fetched data + const records: InstrumentVisualizationRecord[] = recordsQuery.data.map((record, i) => { + const props = record.data && typeof record.data === 'object' ? record.data : {}; + const session = sessions[i]; + const username = session?.userId + ? (userMap.get(session.userId) ?? 'N/A') + : 'N/A'; + + return { + __date__: record.date, + __time__: record.date.getTime(), + username, + ...record.computedMeasures, + ...props + }; + }); setRecords(records); } } catch (error) { console.error('Error occurred: ', error); } }; void fetchRecords(); }, [recordsQuery.data]);Note: Assumes
Userhas anidfield for the map key. Adjust if the field name differs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts(6 hunks)apps/web/src/hooks/useFindSession.ts(1 hunks)apps/web/src/hooks/useInstrumentVisualization.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/hooks/useFindSession.ts
🧰 Additional context used
🧬 Code graph analysis (2)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts (1)
apps/web/src/hooks/useInstrumentVisualization.ts (1)
useInstrumentVisualization(32-262)
apps/web/src/hooks/useInstrumentVisualization.ts (2)
apps/web/src/hooks/useFindSession.ts (1)
sessionInfo(4-15)apps/web/src/hooks/useFindUser.ts (1)
userInfo(5-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint-and-test
🔇 Additional comments (6)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts (3)
75-81: Mocks correctly return promises.The mocks now properly return promises, matching the async contract of
sessionInfoanduserInfo.
92-94: Proper async handling in tests.The
waitForblocks correctly ensure records are populated before assertions, matching the async fetch implementation in the hook.
176-185: Test expectations are correct.The expected Excel output structure correctly includes all fields with Username in the appropriate position.
apps/web/src/hooks/useInstrumentVisualization.ts (3)
16-17: Imports are correct.The
sessionInfoanduserInfoimports are properly placed and will be used for record enrichment.
88-91: Username field mapping is correct.The transformation from
usernametoUsernamein wide-format exports follows the same pattern as the date field handling.
110-113: Username extraction logic is correct.The username is properly extracted from record entries and defaults to 'N/A', ensuring it's always available for long-form row construction.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/hooks/useInstrumentVisualization.ts (1)
75-96: Initializedatewith definite-assignment assertion or defensive null pattern inmakeLongRows.The review comment is valid. Although
InstrumentVisualizationRecordtype guarantees__date__: Dateexists, TypeScript cannot statically confirmdateis assigned before use when iteratingObject.entries(item). The uninitialized variable at line 106 is used at lines 121 and 137 without explicit guards.Apply one of the suggested fixes:
- Use
let date!: Date;with a comment explaining the type contract, or- Use
let date: Date | null = null;and guard withdate!at usage sites.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts(6 hunks)apps/web/src/hooks/useInstrumentVisualization.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/hooks/tests/useInstrumentVisualization.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/src/hooks/useInstrumentVisualization.ts (2)
apps/web/src/hooks/useFindSession.ts (1)
sessionInfo(4-15)apps/web/src/hooks/useFindUser.ts (1)
userInfo(5-14)
🔇 Additional comments (1)
apps/web/src/hooks/useInstrumentVisualization.ts (1)
16-17: Imports for session/user helpers look correct.The new imports align with the helper signatures and are only used inside the effect, so no issues here.
| const fetchRecords = async () => { | ||
| try { | ||
| if (recordsQuery.data) { | ||
| // Fetch all sessions in parallel | ||
| const sessionPromises = recordsQuery.data.map((record) => sessionInfo(record.sessionId)); | ||
| const sessions = await Promise.all(sessionPromises); | ||
|
|
||
| // Extract unique userIds and fetch users in parallel | ||
| const userIds = [...new Set(sessions.filter((s) => s?.userId).map((s) => s.userId))]; | ||
|
|
||
| const userPromises = userIds.map((userId) => userInfo(userId!)); | ||
| const users = await Promise.all(userPromises); | ||
| const userMap = new Map(users.filter((u) => u).map((u) => [u!.id, u!.username])); | ||
|
|
||
| // Build records with looked-up data | ||
| const records: InstrumentVisualizationRecord[] = recordsQuery.data.map((record, i) => { | ||
| const props = record.data && typeof record.data === 'object' ? record.data : {}; | ||
| const session = sessions[i]; | ||
| const username = session?.userId ? (userMap.get(session.userId) ?? 'N/A') : 'N/A'; | ||
|
|
||
| return { | ||
| __date__: record.date, | ||
| __time__: record.date.getTime(), | ||
| username: username, | ||
| ...record.computedMeasures, | ||
| ...props | ||
| }; | ||
| }); | ||
|
|
||
| setRecords(records); | ||
| } | ||
| } catch (error) { | ||
| console.error('Error occurred: ', error); | ||
| notifications.addNotification({ | ||
| message: t({ | ||
| en: 'Error occurred finding records', | ||
| fr: "Une erreur s'est produite lors de la recherche des enregistrements." | ||
| }), | ||
| type: 'error' | ||
| }); | ||
| } | ||
| setRecords(records); | ||
| } | ||
| }; | ||
| void fetchRecords(); | ||
| }, [recordsQuery.data]); |
There was a problem hiding this comment.
🧩 Analysis chain
Parallelized fetching is solid, but guard against stale updates/race conditions.
- The new logic correctly parallelizes
sessionInfoanduserInfocalls, deduplicates userIds, and builds ausername‑enrichedrecordsarray. This addresses the previous 2N sequential‑call concern and improves performance. - Errors are now surfaced via
notifications.addNotification, which resolves the earlier “console‑only” issue.
One concern: fetchRecords can still be in flight when recordsQuery.data changes (e.g., instrument switch or filter change), so an older async run may finish after a newer one and overwrite records with stale data.
Consider tracking a requestId or isCancelled flag inside the effect to avoid updating state from an outdated run:
- useEffect(() => {
- const fetchRecords = async () => {
+ useEffect(() => {
+ let cancelled = false;
+ const fetchRecords = async () => {
try {
if (recordsQuery.data) {
@@
- setRecords(records);
+ if (!cancelled) {
+ setRecords(records);
+ }
}
} catch (error) {
console.error('Error occurred: ', error);
notifications.addNotification({
@@
- };
- void fetchRecords();
- }, [recordsQuery.data]);
+ };
+ void fetchRecords();
+
+ return () => {
+ cancelled = true;
+ };
+ }, [recordsQuery.data]);This keeps the performance benefits while preventing stale results from winning races.
🏁 Script executed:
#!/bin/bash
# Locate and examine the file
fd -type f -name "useInstrumentVisualization.ts" 2>/dev/null | head -5Length of output: 0
🏁 Script executed:
#!/bin/bash
# Get the actual code at lines 212-255
if [ -f "apps/web/src/hooks/useInstrumentVisualization.ts" ]; then
sed -n '200,265p' "apps/web/src/hooks/useInstrumentVisualization.ts" | cat -n
fiLength of output: 2836
🏁 Script executed:
#!/bin/bash
# Check the full useEffect hook to understand dependencies and cleanup
if [ -f "apps/web/src/hooks/useInstrumentVisualization.ts" ]; then
rg -n "useEffect|fetchRecords|setRecords|cancelled|return \(\)" "apps/web/src/hooks/useInstrumentVisualization.ts" -A2 -B2
fiLength of output: 732
Add cleanup logic to prevent stale state updates from race conditions.
The code correctly parallelizes fetches and improves error handling, but lacks protection against stale updates. When recordsQuery.data changes while an async operation is in flight, an older fetch may complete after a newer one and overwrite fresh data with stale results.
Implement the suggested cancelled flag pattern to guard setRecords:
useEffect(() => {
+ let cancelled = false;
const fetchRecords = async () => {
try {
if (recordsQuery.data) {
// ... fetch logic ...
- setRecords(records);
+ if (!cancelled) {
+ setRecords(records);
+ }
}
} catch (error) {
// ... error handling ...
}
};
void fetchRecords();
+
+ return () => {
+ cancelled = true;
+ };
}, [recordsQuery.data]);This retains performance gains while preventing race condition failures.
🤖 Prompt for AI Agents
In apps/web/src/hooks/useInstrumentVisualization.ts around lines 212-255, the
async fetchRecords can produce stale state when recordsQuery.data changes
mid-flight; add a cancelled flag (let cancelled = false) scoped to the effect,
set cancelled = true in the effect cleanup, and inside fetchRecords check the
flag before calling setRecords (and before showing notifications) so any
completed older requests do not overwrite newer state; ensure the fetchRecords
closure reads the flag and returns early when cancelled.
✅ Addressed in commit e42611e
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/hooks/useInstrumentVisualization.ts(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/src/hooks/useInstrumentVisualization.ts (2)
apps/web/src/hooks/useFindSession.ts (1)
sessionInfo(4-15)apps/web/src/hooks/useFindUser.ts (1)
userInfo(5-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint-and-test
🔇 Additional comments (3)
apps/web/src/hooks/useInstrumentVisualization.ts (3)
16-17: Clean import additions.The imports are properly added and will be used in the useEffect below.
88-91: Username field handling looks good.The export logic correctly handles the
usernamefield in both wide and long formats. The initialization to 'N/A' on line 103 provides a safe fallback, and the field is consistently propagated through both export paths.Also applies to: 103-103, 110-113, 123-123, 136-136
212-212: Cancellation logic correctly prevents race conditions.The cancelled flag pattern is properly implemented: declared at effect scope (line 212), checked before state updates (lines 243-244), and cleaned up on unmount (lines 259-261). This prevents stale async results from overwriting fresh data when
recordsQuery.datachanges mid-flight.Also applies to: 243-244, 259-261
| const fetchRecords = async () => { | ||
| try { | ||
| if (recordsQuery.data) { | ||
| // Fetch all sessions in parallel | ||
| const sessionPromises = recordsQuery.data.map((record) => sessionInfo(record.sessionId)); | ||
| const sessions = await Promise.all(sessionPromises); | ||
|
|
||
| // Extract unique userIds and fetch users in parallel | ||
| const userIds = [...new Set(sessions.filter((s) => s?.userId).map((s) => s.userId))]; | ||
|
|
||
| //assume userId exists in userId set as we already filtered out the non-existing userIds | ||
| const userPromises = userIds.map((userId) => userInfo(userId!)); | ||
| const users = await Promise.all(userPromises); | ||
| const userMap = new Map(users.filter((u) => u).map((u) => [u!.id, u!.username])); | ||
|
|
||
| // Build records with looked-up data | ||
| const records: InstrumentVisualizationRecord[] = recordsQuery.data.map((record, i) => { | ||
| const props = record.data && typeof record.data === 'object' ? record.data : {}; | ||
| const session = sessions[i]; | ||
| const username = session?.userId ? (userMap.get(session.userId) ?? 'N/A') : 'N/A'; | ||
|
|
||
| return { | ||
| __date__: record.date, | ||
| __time__: record.date.getTime(), | ||
| username: username, | ||
| ...record.computedMeasures, | ||
| ...props | ||
| }; | ||
| }); | ||
|
|
||
| if (!cancelled) { | ||
| setRecords(records); | ||
| } | ||
| } | ||
| } catch (error) { | ||
| console.error('Error occurred: ', error); | ||
| notifications.addNotification({ | ||
| message: t({ | ||
| en: 'Error occurred finding records', | ||
| fr: "Une erreur s'est produite lors de la recherche des enregistrements." | ||
| }), | ||
| type: 'error' | ||
| }); | ||
| } |
There was a problem hiding this comment.
Consider graceful degradation for individual fetch failures.
The current implementation uses Promise.all for fetching sessions and users (lines 217-218, 224-225). If any single session or user fetch fails, the entire operation rejects and users see an error with no data at all—even for valid records.
Consider using Promise.allSettled to handle partial failures more gracefully:
- const sessionPromises = recordsQuery.data.map((record) => sessionInfo(record.sessionId));
- const sessions = await Promise.all(sessionPromises);
+ const sessionPromises = recordsQuery.data.map((record) =>
+ sessionInfo(record.sessionId).catch(() => null)
+ );
+ const sessions = await Promise.all(sessionPromises);- const userPromises = userIds.map((userId) => userInfo(userId!));
- const users = await Promise.all(userPromises);
+ const userPromises = userIds.map((userId) =>
+ userInfo(userId!).catch(() => null)
+ );
+ const users = await Promise.all(userPromises);This way, if a session or user fails to fetch (e.g., deleted, permission issue), that record gets 'N/A' for username instead of blocking all exports.
Optional improvements:
- Lines 224, 226: The non-null assertions (
userId!,u!) could be avoided with explicit type guards, though the current logic is correct. - Line 249: Error notification is shown even if the request was cancelled. Consider wrapping it in
if (!cancelled)to avoid showing stale errors.
🤖 Prompt for AI Agents
In apps/web/src/hooks/useInstrumentVisualization.ts around lines 213 to 256, the
code uses Promise.all for session and user fetches which causes the whole
operation to fail if any single fetch rejects; replace Promise.all with
Promise.allSettled for both sessionPromises and userPromises, then process
results by using fulfilled values and treating rejected entries as undefined (so
sessions/users that fail map to undefined and usernames become 'N/A'); build the
userMap only from fulfilled user results without non-null assertions and filter
out undefineds, and move the error notification inside a check if (!cancelled)
so cancelled requests do not surface an error to the user.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/web/src/hooks/useInstrumentVisualization.ts (1)
217-218: Add graceful degradation for session fetch failures.Session fetching uses
Promise.allwithout individual error handling. If any single session fetch fails, the entire export fails and users see an error with no data—even for records with valid sessions. This is inconsistent with user fetching (line 224), which uses.catch(() => null)to handle individual failures gracefully.Apply this fix to handle session failures gracefully:
- const sessionPromises = recordsQuery.data.map((record) => sessionInfo(record.sessionId)); + const sessionPromises = recordsQuery.data.map((record) => + sessionInfo(record.sessionId).catch(() => null) + ); const sessions = await Promise.all(sessionPromises);With this change, if a session fails to fetch (e.g., deleted, permission issue), that record will use 'N/A' for username instead of blocking all exports.
🧹 Nitpick comments (1)
apps/web/src/hooks/useInstrumentVisualization.ts (1)
243-256: Consider guarding error notification with cancelled check.The error notification on line 249 is shown even if the request was cancelled. While not critical, wrapping it in
if (!cancelled)prevents stale errors from appearing when the user has already moved on.if (!cancelled) { setRecords(records); } } } catch (error) { console.error('Error occurred: ', error); - notifications.addNotification({ - message: t({ - en: 'Error occurred finding records', - fr: "Une erreur s'est produite lors de la recherche des enregistrements." - }), - type: 'error' - }); + if (!cancelled) { + notifications.addNotification({ + message: t({ + en: 'Error occurred finding records', + fr: "Une erreur s'est produite lors de la recherche des enregistrements." + }), + type: 'error' + }); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/hooks/useInstrumentVisualization.ts(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/src/hooks/useInstrumentVisualization.ts (2)
apps/web/src/hooks/useFindSession.ts (1)
sessionInfo(4-15)apps/web/src/hooks/useFindUser.ts (1)
userInfo(5-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint-and-test
🔇 Additional comments (6)
apps/web/src/hooks/useInstrumentVisualization.ts (6)
16-17: LGTM!The imports are correctly added and match the usage in the async fetch logic below.
88-91: LGTM!The username field mapping is correct and follows the established pattern for special field handling.
103-103: LGTM!The username variable is properly initialized with a fallback value.
110-113: LGTM!The username extraction follows the correct pattern and ensures the field is captured before iterating over other properties.
123-123: LGTM!Username field correctly included in long-format array records.
136-136: LGTM!Username field correctly included in long-format non-array records.
apps/web/src/hooks/useFindSession.ts
Outdated
| @@ -0,0 +1,15 @@ | |||
| import type { Session } from '@opendatacapture/schemas/session'; | |||
| import axios from 'axios'; | |||
There was a problem hiding this comment.
This is not a hook. Create a useFindSessionQuery hook. Look at the other query hooks to see how.
apps/web/src/hooks/useFindUser.ts
Outdated
| import type { User } from '@opendatacapture/schemas/user'; | ||
| import axios from 'axios'; | ||
|
|
||
| export const userInfo = async (userId: string): Promise<null | User> => { |
| try { | ||
| if (recordsQuery.data) { | ||
| // Fetch all sessions in parallel | ||
| const sessionPromises = recordsQuery.data.map((record) => sessionInfo(record.sessionId)); |
There was a problem hiding this comment.
okay, so here is looks like you a fetching the session data for every single record. This is very inefficient. In this case, you need to create a new API route.
| const userIds = [...new Set(sessions.filter((s) => s?.userId).map((s) => s.userId))]; | ||
|
|
||
| //assume userId exists in userId set as we already filtered out the non-existing userIds | ||
| const userPromises = userIds.map((userId) => userInfo(userId!).catch(() => null)); |
There was a problem hiding this comment.
so then wouldn't sessions without users be excluded from the graph?
| }; | ||
| void fetchRecords(); | ||
| return () => { | ||
| cancelled = true; |
There was a problem hiding this comment.
what is the point of this?
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/web/src/hooks/useInstrumentVisualization.ts (1)
16-18: Async enrichment works, but still does N session calls and has some cleanup TODOs.
- The parallel
sessionInfo/userInfofetch, dedupeduserIds, and cancellation guard are all good improvements and should perform much better than the original sequential loop.- You’re still making one
sessionInforequest per record; now that there’s aGET /sessionsendpoint returning sessions with usernames, consider introducing a dedicated hook (alluded to in the comment at lines 60–62) that fetches all relevant sessions in a single call and drives this enrichment from that response. That would remove the remaining N session round‑trips.- The comment at 60–62 reads like a half‑finished TODO and no longer matches the actual implementation; it’d be clearer either to remove it or to turn it into a precise TODO that references the new grouped sessions endpoint.
- Minor polish: you already guard
setRecordsbehind!cancelled, but the error notification in thecatchblock still fires even when the effect has been cancelled. Wrapping the notification in the same!cancelledcheck would avoid showing stale errors from aborted requests. Similarly, if you want partial success when somesessionInfocalls fail, you could mirror the.catch(() => null)pattern you’re using foruserInfo.Also applies to: 60-62, 214-265
🧹 Nitpick comments (3)
apps/api/src/sessions/sessions.service.ts (1)
96-109: Clarify return shape / align with$SessionWithUser.The query and access control look good, and including
user.usernamematches the new export needs. However, this now returns sessions with an attacheduserrelation (which may benullwhenuserIdis unset). If you intend consumers to validate this with$SessionWithUser, consider either:
- Allowing
userto be nullable in the Zod schema, or- Normalizing results here so
useris always an object withusername: null.Also consider adding an explicit return type (e.g., based on
$SessionWithUser) so downstream usage can rely on the enriched shape.packages/schemas/src/session/session.ts (1)
28-33: FixSessionWithUsernaming and makeusernullable.Two small but important cleanups here:
- Type naming consistency – elsewhere you use
Sessionfor the inferred type and$Sessionfor the schema. Here the inferred type is named$SessionWithUser, which breaks that pattern. Prefer:-export type $SessionWithUser = z.infer<typeof $SessionWithUser>; -export const $SessionWithUser = $Session.extend({ - user: z.object({ - username: z.string().nullish() - }) -}); +export const $SessionWithUser = $Session.extend({ + user: z + .object({ + username: z.string().nullish() + }) + .nullish() +}); + +export type SessionWithUser = z.infer<typeof $SessionWithUser>;
- Nullability of
user– since sessions can exist without a linked user (you already allowuserIdto be nullish), Prisma will returnuser: nullin those cases. Makinguseritself.nullish()avoids Zod parse errors when validating data coming fromfindAllIncludeUsernames.apps/web/src/hooks/useInstrumentVisualization.ts (1)
91-96: Username is correctly threaded into wide/long exports.The additional handling for the
usernamekey (mapping toUsernamein both wide and long rows, with a sane default of'N/A') looks solid and keeps the export shape consistent across CSV/TSV/Excel formats.One thing to double‑check: JSON exports will currently expose the raw
usernamefield (lowercase) fromrecords, while tabular exports useUsername. If consumers expect a unified field name across formats, you might want to normalize that, but the current behavior is functionally fine if that distinction is intentional.Also applies to: 101-147
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/api/src/sessions/sessions.controller.ts(2 hunks)apps/api/src/sessions/sessions.service.ts(1 hunks)apps/web/src/hooks/useFindSession.ts(1 hunks)apps/web/src/hooks/useInstrumentVisualization.ts(7 hunks)packages/schemas/src/session/session.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/hooks/useFindSession.ts
🧰 Additional context used
🧬 Code graph analysis (2)
apps/api/src/sessions/sessions.service.ts (1)
apps/api/src/core/types.ts (1)
EntityOperationOptions(3-5)
apps/web/src/hooks/useInstrumentVisualization.ts (2)
apps/web/src/hooks/useFindSession.ts (1)
sessionInfo(10-21)apps/web/src/hooks/useFindUser.ts (1)
userInfo(5-14)
🔇 Additional comments (1)
apps/api/src/sessions/sessions.controller.ts (1)
1-3: NewGET /sessionswiring looks correct.Import usage,
RouteAccess,ValidObjectIdPipe, and ability propagation all follow the existing patterns fromfindByID, so authorization and scoping bygroupIdshould work as expected. No functional issues spotted here.Also applies to: 21-29
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/web/src/hooks/useFindSessionQuery.ts (1)
35-51: Consider improving queryKey for cache stability.Using
Object.values(params)at line 49 may cause cache collisions if different params have the same values (e.g.,{userId: 1, groupId: 2}vs.{groupId: 1, limit: 2}).Apply this diff for more stable caching:
- queryKey: ['sessions', ...Object.values(params)] + queryKey: ['sessions', params]React Query will serialize the params object, which is more reliable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/hooks/useFindSessionQuery.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/src/hooks/useFindSessionQuery.ts (1)
packages/schemas/src/session/session.ts (1)
Session(9-9)
🪛 ESLint
apps/web/src/hooks/useFindSessionQuery.ts
[error] 1-1: 'reviver' is defined but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
[error] 4-4: Prefer using a top-level type-only import instead of inline type specifiers.
(import/consistent-type-specifier-style)
[error] 5-5: Prefer using a top-level type-only import instead of inline type specifiers.
(import/consistent-type-specifier-style)
[error] 5-5: 'SessionWithUser' is defined but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
[error] 6-6: Prefer using a top-level type-only import instead of inline type specifiers.
(import/consistent-type-specifier-style)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/web/src/hooks/useInstrumentVisualization.ts (1)
221-272: Parallel fetch logic works but still issues one session request per recordThe effect correctly parallelizes
sessionInfoanduserInfocalls, deduplicatesuserIds, and guardssetRecordswith a cancellation flag. However, it still performs onesessionInfoHTTP call per record, which can be very expensive for large datasets—previous feedback already called out this pattern.Now that you have a
SessionWithUserendpoint on the API side, consider switching this hook to use a batched/grouped session fetch (e.g., viauseFindSessionQueryor a new helper that hits the enriched/sessionsroute and returns aMap<sessionId, username>), so you make O(1) or O(k) backend calls instead of O(N) per record.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/api/src/sessions/sessions.controller.ts(2 hunks)apps/api/src/sessions/sessions.service.ts(1 hunks)apps/web/src/hooks/useInstrumentVisualization.ts(7 hunks)packages/schemas/src/session/session.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/src/sessions/sessions.service.ts
🧰 Additional context used
🧬 Code graph analysis (2)
apps/web/src/hooks/useInstrumentVisualization.ts (2)
apps/web/src/hooks/useFindSessionQuery.ts (1)
sessionInfo(22-33)apps/web/src/hooks/useFindUser.ts (1)
userInfo(5-14)
apps/api/src/sessions/sessions.controller.ts (1)
packages/schemas/src/session/session.ts (1)
SessionWithUser(28-28)
🪛 ESLint
apps/web/src/hooks/useInstrumentVisualization.ts
[error] 16-16: 'useFindSessionQuery' is defined but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
apps/api/src/sessions/sessions.controller.ts
[error] 1-1: 'ValidObjectIdPipe' is defined but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
[error] 9-9: Expected "@opendatacapture/schemas/session" (external-type) to come before "./sessions.service" (sibling).
(perfectionist/sort-imports)
🔇 Additional comments (3)
packages/schemas/src/session/session.ts (1)
28-37: SessionWithUser schema looks consistent with existing Session modelExtending
$Sessionwith auser: { username: string | null | undefined }object cleanly models the enriched response and matches the usage in the API/service layer.SessionWithUserQueryParamsis also straightforward and future‑proof for additional filters.apps/api/src/sessions/sessions.controller.ts (1)
22-30: New findAllIncludeUsernames endpoint wiring looks correctThe GET
/sessionshandler correctly takesabilityfrom@CurrentUser, optionalgroupIdfrom@Query, and delegates tosessionsService.findAllIncludeUsernameswith the properSessionWithUser[]return type. This aligns with the new enriched session schema.apps/web/src/hooks/useInstrumentVisualization.ts (1)
98-101: Username propagation into wide and long exports looks consistentThe additions that (a) map
usernameto aUsernamecolumn in wide exports, and (b) trackusernameper record and emitUsernamein both array and scalar branches ofmakeLongRows, are coherent and should give all export variants the expected username field with'N/A'fallback.Also applies to: 113-114, 120-123, 133-133, 146-146
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
apps/web/src/hooks/useFindSessionQuery.ts (3)
1-5: Remove unused imports and fix import style.Unused imports (
reviver,SessionWithUsertype) and inline type specifiers should be addressed.Per previous review, apply:
-import { $SessionWithUser } from '@opendatacapture/schemas/session'; -import type { Session, SessionWithUserQueryParams } from '@opendatacapture/schemas/session'; +import type { Session, SessionWithUserQueryParams } from '@opendatacapture/schemas/session'; +import { $SessionWithUser } from '@opendatacapture/schemas/session';
6-11: Remove stale TODO comments.These comments describe completed work.
Delete lines 6-11.
17-28: Add validation and fix return type forsessionInfo.The function should return
SessionWithUserand validate with$SessionWithUser.parseAsync()to match the validation pattern used inuseFindSessionQuery.Apply this diff:
-export const sessionInfo = async (sessionId: string): Promise<Session> => { +export const sessionInfo = async (sessionId: string): Promise<SessionWithUser> => { try { const response = await axios.get(`/v1/sessions/${encodeURIComponent(sessionId)}`); if (!response.data) { throw new Error('Session data does not exist'); } - return response.data as Session; + return $SessionWithUser.parseAsync(response.data); } catch (error) { console.error('Error fetching session:', error); throw error; } };
🧹 Nitpick comments (1)
apps/api/src/sessions/sessions.service.ts (1)
98-105: Consider alphabetizing include properties.Static analysis suggests placing
subjectbeforeuserfor consistency, though this is purely stylistic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/src/sessions/sessions.service.ts(1 hunks)apps/web/src/hooks/useFindSessionQuery.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/web/src/hooks/useFindSessionQuery.ts (1)
packages/schemas/src/session/session.ts (3)
SessionWithUserQueryParams(35-37)Session(9-9)$SessionWithUser(29-33)
apps/api/src/sessions/sessions.service.ts (1)
apps/api/src/core/types.ts (1)
EntityOperationOptions(3-5)
🪛 ESLint
apps/api/src/sessions/sessions.service.ts
[error] 104-104: Expected "subject" to come before "user".
(perfectionist/sort-objects)
🔇 Additional comments (1)
apps/web/src/hooks/useFindSessionQuery.ts (1)
30-46: LGTM!The
useFindSessionQueryhook correctly validates responses with$SessionWithUser.array().parseAsync()and uses appropriate query keys for caching.
| 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 }] | ||
| } | ||
| }); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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`); | ||
| } |
There was a problem hiding this comment.
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.
| 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.
There was a problem hiding this comment.
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!
david-roper
force-pushed
the
add-username-to-export
branch
from
d6a0d45 to
4c97dd2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/sessions/sessions.controller.ts (1)
1-12: Reorder imports to satisfyperfectionist/sort-imports.ESLint expects the external type import from
@opendatacapture/schemas/sessionbefore the local./sessions.serviceimport.-import { CreateSessionDto } from './dto/create-session.dto'; -import { SessionsService } from './sessions.service'; -import type { SessionWithUser } from '@opendatacapture/schemas/session'; +import type { SessionWithUser } from '@opendatacapture/schemas/session'; + +import { CreateSessionDto } from './dto/create-session.dto'; +import { SessionsService } from './sessions.service';This should clear the import-order error.
♻️ Duplicate comments (4)
apps/web/src/hooks/useFindSessionQuery.ts (1)
6-11: Remove stale TODO comments now that the hook exists.The
//Change this query...comment block is obsolete and doesn’t describe the current implementation anymore. Dropping it will reduce noise:-//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 -apps/api/src/sessions/sessions.service.ts (2)
97-110: Fix optionalgroupIdfilter soundefineddoesn’t becomeIS NULL.Right now
{ groupId }is always added to theANDarray; whengroupIdisundefined, Prisma interprets it asgroupId IS NULL, returning only sessions without a group instead of all sessions.Guard the filter before adding it to
AND:- async findAllIncludeUsernames(groupId?: string, { ability }: EntityOperationOptions = {}) { + async findAllIncludeUsernames(groupId?: string, { ability }: EntityOperationOptions = {}) { const sessionsWithUsers = await this.sessionModel.findMany({ include: { - user: { - select: { - username: true - } - }, - subject: true + subject: true, + user: { + select: { + username: true + } + } }, where: { - AND: [accessibleQuery(ability, 'read', 'Session'), { groupId }] + AND: [ + accessibleQuery(ability, 'read', 'Session'), + ...(groupId !== undefined ? [{ groupId }] : []) + ] } });This keeps access control in place and only applies the group filter when actually provided.
111-115: Fix ineffective null check and incorrect error message.
findManyalways returns an array;if (!sessionsWithUsers)will never fire. If you really want a 404 when nothing is found, check.length === 0and mention sessions, not users:- if (!sessionsWithUsers) { - throw new NotFoundException(`Failed to find users`); - } + if (sessionsWithUsers.length === 0) { + throw new NotFoundException(`Failed to find sessions`); + }Alternatively, consider just returning an empty array and dropping the exception entirely.
apps/web/src/hooks/useInstrumentVisualization.ts (1)
16-18: Remove unuseduseFindSessionQueryimport and stale commented code.
useFindSessionQueryis imported but never used, and the relatedsessionsUsernameQuerycomment block is dead code. This also matches an ESLint error for an unused import.You can simplify:
-import { sessionInfo, useFindSessionQuery } from './useFindSessionQuery'; -import { userInfo } from './useFindUser'; +import { sessionInfo } from './useFindSessionQuery'; +import { userInfo } from './useFindUser'; @@ - // const sessionsUsernameQuery = useFindSessionQuery({ - // enabled: instrumentId !== null, - // params: { - // groupId: currentGroup?.id - // } - // }); - - // Create a new sessionsUsernameQuery which uses the useFindSessionQuery hook - // have use a different return type with + // (Optional) If you later switch to a pre-joined sessions API, wire it in here.This should clear the lint error and keep the file focused.
Also applies to: 60-69
🧹 Nitpick comments (2)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts (1)
89-161: Username export expectations and async waiting look consistent.
waitForusage before callingdl(...)ensures records are populated from the async effect.- CSV/TSV (wide) and CSV/TSV Long expectations correctly include the
Usernamecolumn in the expected order.- Excel and Excel Long expectations match the object shapes produced by
makeWideRows/makeLongRows(includingUsername).- JSON test still validates payload content and subjectID, aligned with the JSON branch.
The only minor nit is destructuring
const { records } = result.current;beforewaitFor, which doesn’t observe updated values, but it doesn’t affect assertions here.Also applies to: 163-215, 216-233
apps/web/src/hooks/useInstrumentVisualization.ts (1)
221-272: Async enrichment is parallelized; consider softening failures and guarding notifications.The effect correctly:
- Parallelizes
sessionInfoanduserInfocalls.- Deduplicates userIds and builds a username map.
- Uses a
cancelledflag to avoid stalesetRecordscalls.Two optional robustness tweaks:
- Treat individual session failures like user failures so one bad session doesn’t block all exports:
- const sessionPromises = recordsQuery.data.map((record) => sessionInfo(record.sessionId)); + const sessionPromises = recordsQuery.data.map((record) => + sessionInfo(record.sessionId).catch(() => null) + );
- Avoid showing notifications for requests that were cancelled:
- } catch (error) { + } catch (error) { console.error('Error occurred: ', error); - notifications.addNotification({ + if (!cancelled) { + notifications.addNotification({ message: t({ en: 'Error occurred finding records', fr: "Une erreur s'est produite lors de la recherche des enregistrements." }), - type: 'error' - }); + type: 'error' + }); + } }Not strictly required, but they improve UX for edge cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/api/src/sessions/sessions.controller.ts(3 hunks)apps/api/src/sessions/sessions.service.ts(1 hunks)apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts(6 hunks)apps/web/src/hooks/useFindSessionQuery.ts(1 hunks)apps/web/src/hooks/useFindUser.ts(1 hunks)apps/web/src/hooks/useInstrumentVisualization.ts(7 hunks)apps/web/src/utils/excel.ts(0 hunks)packages/schemas/src/session/session.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/web/src/utils/excel.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/src/hooks/useFindUser.ts
- packages/schemas/src/session/session.ts
🧰 Additional context used
🧬 Code graph analysis (5)
apps/api/src/sessions/sessions.service.ts (2)
apps/api/src/core/types.ts (1)
EntityOperationOptions(3-5)apps/api/src/auth/ability.utils.ts (1)
accessibleQuery(26-35)
apps/api/src/sessions/sessions.controller.ts (3)
apps/api/src/core/decorators/route-access.decorator.ts (1)
RouteAccess(18-20)apps/api/src/auth/auth.types.ts (1)
AppAbility(23-23)packages/schemas/src/session/session.ts (1)
SessionWithUser(28-28)
apps/web/src/hooks/useFindSessionQuery.ts (1)
packages/schemas/src/session/session.ts (3)
SessionWithUserQueryParams(35-37)Session(9-9)$SessionWithUser(29-33)
apps/web/src/hooks/useInstrumentVisualization.ts (2)
apps/web/src/hooks/useFindSessionQuery.ts (1)
sessionInfo(17-28)apps/web/src/hooks/useFindUser.ts (1)
userInfo(5-14)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts (1)
apps/web/src/hooks/useInstrumentVisualization.ts (1)
useInstrumentVisualization(32-283)
🪛 ESLint
apps/api/src/sessions/sessions.service.ts
[error] 105-105: Expected "subject" to come before "user".
(perfectionist/sort-objects)
apps/api/src/sessions/sessions.controller.ts
[error] 11-11: Expected "@opendatacapture/schemas/session" (external-type) to come before "./sessions.service" (sibling).
(perfectionist/sort-imports)
apps/web/src/hooks/useInstrumentVisualization.ts
[error] 16-16: 'useFindSessionQuery' is defined but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint-and-test
🔇 Additional comments (1)
apps/web/src/hooks/useInstrumentVisualization.ts (1)
85-107: Username inclusion in wide/long exports is consistent and robust.
makeWideRowsmaps internalusernameto exportedUsername, keepingGroupIDandsubjectIdprefixing.makeLongRowstracksusernameper record with a default'N/A'and injectsUsernameinto each long row, both for scalar and array values.This matches the updated tests and keeps legacy fields intact.
Also applies to: 108-155
| const mockSession = { | ||
| userId: '111' | ||
| }; | ||
|
|
||
| const mockUser = { | ||
| id: '111', | ||
| username: 'testusername' | ||
| }; |
There was a problem hiding this comment.
🧩 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 -C2Length 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.
| export const sessionInfo = async (sessionId: string): Promise<Session> => { | ||
| try { | ||
| const response = await axios.get(`/v1/sessions/${encodeURIComponent(sessionId)}`); | ||
| if (!response.data) { | ||
| throw new Error('Session data does not exist'); | ||
| } | ||
| return response.data as Session; | ||
| } catch (error) { | ||
| console.error('Error fetching session:', error); | ||
| throw error; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
Optional: add Zod validation to sessionInfo for consistency.
sessionInfo currently returns response.data as Session without validation, while useFindSessionQuery validates with $SessionWithUser. If you want stronger guarantees, consider validating here too (with the appropriate schema for the single-session endpoint), similar to the list query.
If you’re comfortable with a lightweight helper and trust the backend shape, keeping the current cast is fine.
🌐 Web query:
What Zod schema (e.g., $Session vs $SessionWithUser) best matches the payload returned by GET /v1/sessions/{id} in this project?
💡 Result:
I don't have the project or the endpoint response. Please paste the actual JSON response from GET /v1/sessions/{id} (or a link to the repo/openAPI file), and I’ll tell you whether $Session or $SessionWithUser (or another Zod schema) matches.
I need to search the codebase to find the Zod schemas and endpoint structure to verify the review comment.
🏁 Script executed:
#!/bin/bash
# Find Zod schema definitions
fd -type f -name "*.ts" -o -name "*.tsx" | head -50 | xargs grep -l "SessionWithUser\|Session.*z\." 2>/dev/null | head -20Length of output: 170
🏁 Script executed:
#!/bin/bash
# Search for schema definitions more broadly
rg "\$Session|\$SessionWithUser|z\.object.*Session" --type ts --type tsx -A 3 -B 1 | head -100Length of output: 27
🏁 Script executed:
#!/bin/bash
# Find useFindSessionQuery to see validation pattern
fd -type f -name "*useFindSessionQuery*"Length of output: 170
🏁 Script executed:
#!/bin/bash
# Look for API endpoint definitions or response examples
rg "v1/sessions" --type ts --type tsx -B 2 -A 2 | head -80Length of output: 27
🏁 Script executed:
#!/bin/bash
# Search for validation patterns with SessionWithUser
rg "SessionWithUser" --type ts --type tsx -B 3 -A 3 | head -100Length of output: 27
🏁 Script executed:
#!/bin/bash
# Check for type definitions
fd -type f \( -name "*.ts" -o -name "*.tsx" \) | xargs grep -l "type Session\|interface Session" | head -10Length of output: 170
🏁 Script executed:
#!/bin/bash
# Find all Zod schema exports
rg "export.*=.*z\." --type ts --type tsx | grep -i session | head -20Length of output: 27
🏁 Script executed:
#!/bin/bash
# Check the actual useFindSessionQuery file
fd -type f -path "*useFindSessionQuery*"Length of output: 170
🏁 Script executed:
#!/bin/bash
# Search for query files in hooks directory
fd -type f -path "*hooks*" -name "*.ts" | xargs grep -l "findSessionQuery\|useFindSessionQuery" | head -10Length of output: 170
🏁 Script executed:
#!/bin/bash
# Look for validation schema imports/usage patterns
rg "useQuery|axios\.get.*sessions" --type ts --type tsx -B 3 -A 5 | head -150Length of output: 27
🏁 Script executed:
#!/bin/bash
# Check for schema directory
fd -type d -name "schema*" -o -name "schemas*" -o -name "types*" -o -name "validation*"Length of output: 170
🏁 Script executed:
#!/bin/bash
# Find common locations for zod schemas
find . -type f \( -name "*schema*" -o -name "*validation*" \) -name "*.ts" 2>/dev/null | head -20Length of output: 1624
🏁 Script executed:
#!/bin/bash
# Search entire codebase for Session schema patterns
rg "(?:const|export)\s+\$?Session" --type ts --type tsx -A 3 | head -150Length of output: 27
🏁 Script executed:
#!/bin/bash
# Look in apps/web structure for schema files
find apps/web -type f -name "*.ts" -o -name "*.tsx" | xargs grep -l "z\.object" 2>/dev/null | head -15Length of output: 303
🏁 Script executed:
#!/bin/bash
# Get the full useFindSessionQuery file
cat apps/web/src/hooks/useFindSessionQuery.ts 2>/dev/null || echo "File not found in expected location"Length of output: 1314
🏁 Script executed:
#!/bin/bash
# Search for any useFindSessionQuery usages
rg "useFindSessionQuery" --type ts --type tsx -B 2 -A 2 | head -50Length of output: 27
🏁 Script executed:
#!/bin/bash
# Check apps/web structure
find apps/web -type f -name "*.ts" -o -name "*.tsx" | grep -E "(schema|type|validation)" | head -20Length of output: 27
🏁 Script executed:
#!/bin/bash
# Look for Session related types in lib or common directories
find apps/web/src -type d -name "lib" -o -name "types" -o -name "common" -o -name "utils" 2>/dev/nullLength of output: 18
🏁 Script executed:
#!/bin/bash
# Search for pattern: validation/parsing with Session
rg "parse|validate|zod" --type ts --type tsx -i | grep -i session | head -30Length of output: 27
🏁 Script executed:
#!/bin/bash
# Try broader approach - list key files in apps/web/src
find apps/web/src -type f -name "*.ts" -o -name "*.tsx" | sort | head -50Length of output: 2582
🏁 Script executed:
#!/bin/bash
# Search for shared schemas or types
find . -type f \( -name "schema.ts" -o -name "types.ts" -o -name "index.ts" \) 2>/dev/null | xargs grep -l "Session" 2>/dev/null | head -10Length of output: 111
🏁 Script executed:
#!/bin/bash
# Examine the specific file mentioned in review
cat "apps/web/src/hooks/useFindSessionQuery.ts" 2>/dev/null || find . -path "*useFindSessionQuery*" 2>/dev/nullLength of output: 1314
🏁 Script executed:
#!/bin/bash
# Try to locate hook files
find apps/web/src -type f -path "*/hooks/*" -name "*.ts" 2>/dev/null | head -20Length of output: 860
🏁 Script executed:
#!/bin/bash
# Get directory structure of apps/web/src
find apps/web/src -type f -name "*.ts" -o -name "*.tsx" | head -30Length of output: 1598
🏁 Script executed:
#!/bin/bash
# Check if hooks directory exists
ls -la apps/web/src/hooks 2>/dev/null || echo "Hooks directory structure unknown"Length of output: 2577
🏁 Script executed:
#!/bin/bash
# Fallback - search more broadly for hook and schema files
find . -name "*.ts" -type f 2>/dev/null | grep -E "(hook|schema)" | head -40Length of output: 1793
🏁 Script executed:
#!/bin/bash
# Search for validation with Zod in all ts files
rg "z\." --type ts | grep -E "(Session|parse|validate)" -i | head -50Length of output: 5311
🏁 Script executed:
#!/bin/bash
# Look for API or endpoint response structure
rg "/v1/sessions|SessionWithUser|Session" --type ts -C 4 | head -200Length of output: 14360
Consider adding Zod validation to sessionInfo for consistency with useFindSessionQuery.
The single-session endpoint (GET /v1/sessions/{id}) should return $Session (not $SessionWithUser), unlike the list endpoint that includes user details. To match the validation pattern in useFindSessionQuery, you could update sessionInfo to validate:
return await $Session.parseAsync(response.data);This is optional polish—the current type cast is acceptable if you trust the backend contract.
🤖 Prompt for AI Agents
In apps/web/src/hooks/useFindSessionQuery.ts around lines 17 to 28, replace the
unchecked type cast of response.data to Session with Zod validation using the
$Session schema: import $Session if not already imported and return await
$Session.parseAsync(response.data) (ensuring the schema is $Session not
$SessionWithUser), and keep the existing try/catch behavior (log and rethrow) so
validation errors are handled the same as network errors.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (6)
apps/api/src/sessions/sessions.service.ts (2)
110-110: Fix optional groupId filtering logic.When
groupIdisundefined, Prisma interprets{ groupId: undefined }as "match rows wheregroupId IS NULL" rather than ignoring the filter. This returns only sessions without a group instead of all sessions.Apply this diff to conditionally include the groupId filter:
where: { - AND: [accessibleQuery(ability, 'read', 'Session'), { groupId }] + AND: [accessibleQuery(ability, 'read', 'Session'), ...(groupId ? [{ groupId }] : [])] }
113-115: Remove incorrect null check and fix error message.
findManyreturns an array, which is nevernull. 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`); }apps/web/src/hooks/useFindSessionQuery.ts (2)
1-2: Remove unused imports.
$SessionandSessionare imported but never used in this file.Apply this diff:
-import { $Session, $SessionWithUser } from '@opendatacapture/schemas/session'; +import { $SessionWithUser } from '@opendatacapture/schemas/session'; -import type { Session, SessionWithUserQueryParams } from '@opendatacapture/schemas/session'; +import type { SessionWithUserQueryParams } from '@opendatacapture/schemas/session';
6-11: Remove stale TODO comments.These comments describe implementation steps that are already complete.
Apply this diff:
- -//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 -apps/web/src/hooks/useInstrumentVisualization.ts (2)
67-69: Remove stale TODO comments.These comments are no longer relevant now that the implementation is complete.
Apply this diff:
}); - - // Create a new sessionsUsernameQuery which uses the useFindSessionQuery hook - // have use a different return type with -
249-257: Guard error notification against stale errors.If the effect is cancelled (e.g., user switches instruments) while an async operation is in flight, the error notification may still appear for a stale request.
Apply this diff:
} catch (error) { console.error('Error occurred: ', error); - notifications.addNotification({ - message: t({ - en: 'Error occurred finding records', - fr: "Une erreur s'est produite lors de la recherche des enregistrements." - }), - type: 'error' - }); + if (!cancelled) { + notifications.addNotification({ + message: t({ + en: 'Error occurred finding records', + fr: "Une erreur s'est produite lors de la recherche des enregistrements." + }), + type: 'error' + }); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/api/src/sessions/sessions.service.ts(2 hunks)apps/web/src/hooks/useFindSessionQuery.ts(1 hunks)apps/web/src/hooks/useInstrumentVisualization.ts(7 hunks)packages/schemas/src/session/session.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/schemas/src/session/session.ts
🧰 Additional context used
🧬 Code graph analysis (3)
apps/api/src/sessions/sessions.service.ts (2)
apps/api/src/core/types.ts (1)
EntityOperationOptions(3-5)apps/api/src/auth/ability.utils.ts (1)
accessibleQuery(26-35)
apps/web/src/hooks/useFindSessionQuery.ts (1)
packages/schemas/src/session/session.ts (2)
SessionWithUserQueryParams(37-39)$SessionWithUser(29-35)
apps/web/src/hooks/useInstrumentVisualization.ts (1)
apps/web/src/hooks/useFindSessionQuery.ts (1)
useFindSessionQuery(31-52)
🪛 ESLint
apps/api/src/sessions/sessions.service.ts
[error] 107-107: Expected "subject" to come before "user".
(perfectionist/sort-objects)
apps/web/src/hooks/useFindSessionQuery.ts
[error] 1-1: '$Session' is defined but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
[error] 2-2: 'Session' is defined but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
[error] 45-45: Unexpected console statement. Only these console methods are allowed: warn, error.
(no-console)
apps/web/src/hooks/useInstrumentVisualization.ts
[error] 17-17: 'userInfo' is defined but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
[error] 230-230: 'i' is defined but never used. Allowed unused args must match /^_/u.
(@typescript-eslint/no-unused-vars)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint-and-test
🔇 Additional comments (3)
apps/api/src/sessions/sessions.service.ts (1)
49-51: LGTM – sensible null guard.The conditional prevents adding a null group to the subject.
apps/web/src/hooks/useInstrumentVisualization.ts (2)
98-101: LGTM – username handling in exports.The logic correctly extracts and propagates usernames in both wide and long export formats, with appropriate fallback to 'N/A'.
Also applies to: 120-123, 133-133, 146-146
113-113: LGTM – proper initialization.Username is initialized with a sensible default.
| const parsedData = $SessionWithUser.array().safeParseAsync(response.data); | ||
| if ((await parsedData).error) { | ||
| console.log((await parsedData).error); | ||
| throw new Error(`cant find data`); | ||
| } | ||
| return (await parsedData).data; |
There was a problem hiding this comment.
Await the parse result once and improve error handling.
The promise is awaited four separate times (lines 43, 44, 45, 48), which is inefficient. Also, line 45 uses console.log for errors (ESLint allows only console.warn and console.error), and the generic error message "cant find data" is not helpful.
Apply this diff:
- const parsedData = $SessionWithUser.array().safeParseAsync(response.data);
- if ((await parsedData).error) {
- console.log((await parsedData).error);
- throw new Error(`cant find data`);
+ const parsedData = await $SessionWithUser.array().safeParseAsync(response.data);
+ if (!parsedData.success) {
+ console.error('Session validation failed:', parsedData.error);
+ throw new Error('Failed to parse session data');
}
- return (await parsedData).data;
+ return parsedData.data;📝 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.
| const parsedData = $SessionWithUser.array().safeParseAsync(response.data); | |
| if ((await parsedData).error) { | |
| console.log((await parsedData).error); | |
| throw new Error(`cant find data`); | |
| } | |
| return (await parsedData).data; | |
| const parsedData = await $SessionWithUser.array().safeParseAsync(response.data); | |
| if (!parsedData.success) { | |
| console.error('Session validation failed:', parsedData.error); | |
| throw new Error('Failed to parse session data'); | |
| } | |
| return parsedData.data; |
🧰 Tools
🪛 ESLint
[error] 45-45: Unexpected console statement. Only these console methods are allowed: warn, error.
(no-console)
🤖 Prompt for AI Agents
In apps/web/src/hooks/useFindSessionQuery.ts around lines 43 to 48, await the
safeParseAsync result once into a local variable (e.g., const parsed = await
$SessionWithUser.array().safeParseAsync(response.data)), then check
parsed.error; if present call console.error with the error details (not
console.log) and throw a more informative Error that includes the error message
or serialized error (e.g., `throw new Error(\`Failed to parse session data:
${parsed.error.message || JSON.stringify(parsed.error)}\`)`); finally return
parsed.data.
| const records: InstrumentVisualizationRecord[] = recordsQuery.data.map((record, i) => { | ||
| const props = record.data && typeof record.data === 'object' ? record.data : {}; | ||
| const usersSessions = sessions.filter((s) => s.id === record.sessionId); | ||
| const session = usersSessions[0]; | ||
| const username = session?.user?.username ?? 'N/A'; | ||
|
|
||
| return { | ||
| __date__: record.date, | ||
| __time__: record.date.getTime(), | ||
| username: username, | ||
| ...record.computedMeasures, | ||
| ...props | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Optimize session lookup with a Map to avoid O(n²) complexity.
Line 232 filters the entire sessions array for every record, resulting in O(n²) time complexity. For datasets with many records, this becomes a significant bottleneck. Build a Map once before the loop.
Apply this diff:
// Build records with looked-up data
+ const sessionMap = new Map(sessions.map(s => [s.id, s]));
- const records: InstrumentVisualizationRecord[] = recordsQuery.data.map((record, i) => {
+ const records: InstrumentVisualizationRecord[] = recordsQuery.data.map((record) => {
const props = record.data && typeof record.data === 'object' ? record.data : {};
- const usersSessions = sessions.filter((s) => s.id === record.sessionId);
- const session = usersSessions[0];
+ const session = sessionMap.get(record.sessionId);
const username = session?.user?.username ?? 'N/A';🧰 Tools
🪛 ESLint
[error] 230-230: 'i' is defined but never used. Allowed unused args must match /^_/u.
(@typescript-eslint/no-unused-vars)
🤖 Prompt for AI Agents
In apps/web/src/hooks/useInstrumentVisualization.ts around lines 230 to 243, the
current code filters the entire sessions array for each record causing O(n²)
complexity; instead, build a Map (or object) once before the records.map loop
keyed by session.id to allow O(1) lookups, then replace the sessions.filter(...)
and usersSessions[0] logic with a single map.get(record.sessionId) (falling back
to undefined) and derive username from that session; ensure the Map construction
happens outside the loop and keep the rest of the record mapping unchanged.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
apps/api/src/sessions/sessions.service.ts (2)
99-112: Fix optional groupId filtering logic.When
groupIdisundefined, Prisma interprets{ groupId: undefined }as "match rows wheregroupIdIS NULL" rather than ignoring the filter. This returns 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: { - subject: true, user: { select: { username: true } - } + }, + subject: true }, where: { - AND: [accessibleQuery(ability, 'read', 'Session'), { groupId }] + AND: [accessibleQuery(ability, 'read', 'Session'), ...(groupId ? [{ groupId }] : [])] } });
113-115: Remove incorrect null check and fix error message.
findManyreturns an array, which is nevernullorundefined. 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`); }apps/web/src/hooks/useFindSessionQuery.ts (2)
6-10: Remove stale TODO comments.These comments describe implementation steps that are already complete.
Apply this diff:
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 = {
29-33: Await the parse result once and improve error handling.The promise is awaited four separate times (lines 30, 31, 33), which is inefficient. Also, line 31 should use
console.error, and the generic error message "cant find data" is not helpful.Apply this diff:
- const parsedData = $SessionWithUser.array().safeParseAsync(response.data); - if ((await parsedData).error) { - throw new Error(`cant find data`); + const parsedData = await $SessionWithUser.array().safeParseAsync(response.data); + if (!parsedData.success) { + console.error('Session validation failed:', parsedData.error); + throw new Error('Failed to parse session data'); } - return (await parsedData).data; + return parsedData.data;apps/web/src/hooks/useInstrumentVisualization.ts (2)
66-67: Remove stale comment.This TODO comment describes implementation that's already complete.
Apply this diff:
}); - // Create a new sessionsUsernameQuery which uses the useFindSessionQuery hook - // have use a different return type with - const dl = (option: 'CSV' | 'CSV Long' | 'Excel' | 'Excel Long' | 'JSON' | 'TSV' | 'TSV Long') => {
229-233: Optimize session lookup with a Map to avoid O(n²) complexity.Line 231 filters the entire
sessionsarray for every record, resulting in O(n²) time complexity. For datasets with many records, this becomes a significant bottleneck.Apply this diff:
// Fetch all sessions in parallel // Build records with looked-up data + const sessionMap = new Map(sessions.map(s => [s.id, s])); const records: InstrumentVisualizationRecord[] = recordsQuery.data.map((record) => { const props = record.data && typeof record.data === 'object' ? record.data : {}; - const usersSessions = sessions.filter((s) => s.id === record.sessionId); - const session = usersSessions[0]; + const session = sessionMap.get(record.sessionId); const username = session?.user?.username ?? 'N/A';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/api/src/sessions/sessions.service.ts(2 hunks)apps/web/src/hooks/useFindSessionQuery.ts(1 hunks)apps/web/src/hooks/useInstrumentVisualization.ts(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
apps/web/src/hooks/useInstrumentVisualization.ts (1)
apps/web/src/hooks/useFindSessionQuery.ts (1)
useFindSessionQuery(17-37)
apps/api/src/sessions/sessions.service.ts (2)
apps/api/src/core/types.ts (1)
EntityOperationOptions(3-5)apps/api/src/auth/ability.utils.ts (1)
accessibleQuery(26-35)
apps/web/src/hooks/useFindSessionQuery.ts (1)
packages/schemas/src/session/session.ts (2)
SessionWithUserQueryParams(37-39)$SessionWithUser(29-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint-and-test
| useEffect(() => { | ||
| if (recordsQuery.data) { | ||
| const records: InstrumentVisualizationRecord[] = []; | ||
| for (const record of recordsQuery.data) { | ||
| const props = record.data && typeof record.data === 'object' ? record.data : {}; | ||
| records.push({ | ||
| __date__: record.date, | ||
| __time__: record.date.getTime(), | ||
| ...record.computedMeasures, | ||
| ...props | ||
| let cancelled = false; | ||
| const fetchRecords = () => { | ||
| try { | ||
| const sessions = sessionsUsernameQuery.data; | ||
| if (recordsQuery.data && sessions) { | ||
| // Fetch all sessions in parallel | ||
|
|
||
| // Build records with looked-up data | ||
| const records: InstrumentVisualizationRecord[] = recordsQuery.data.map((record) => { | ||
| const props = record.data && typeof record.data === 'object' ? record.data : {}; | ||
| const usersSessions = sessions.filter((s) => s.id === record.sessionId); | ||
| const session = usersSessions[0]; | ||
| const username = session?.user?.username ?? 'N/A'; | ||
|
|
||
| return { | ||
| __date__: record.date, | ||
| __time__: record.date.getTime(), | ||
| username: username, | ||
| ...record.computedMeasures, | ||
| ...props | ||
| }; | ||
| }); | ||
|
|
||
| if (!cancelled) { | ||
| setRecords(records); | ||
| } | ||
| } | ||
| } catch (error) { | ||
| console.error('Error occurred: ', error); | ||
| notifications.addNotification({ | ||
| message: t({ | ||
| en: 'Error occurred finding records', | ||
| fr: "Une erreur s'est produite lors de la recherche des enregistrements." | ||
| }), | ||
| type: 'error' | ||
| }); | ||
| } | ||
| setRecords(records); | ||
| } | ||
| }; | ||
| void fetchRecords(); | ||
| return () => { | ||
| cancelled = true; | ||
| }; | ||
| }, [recordsQuery.data]); |
There was a problem hiding this comment.
Add sessionsUsernameQuery.data to dependency array.
The effect reads sessionsUsernameQuery.data (line 224) but only lists recordsQuery.data in the dependency array (line 263). This violates the exhaustive-deps rule and causes stale closures—when sessionsUsernameQuery.data updates, fetchRecords won't re-run with the new session data.
Apply this diff:
};
void fetchRecords();
return () => {
cancelled = true;
};
- }, [recordsQuery.data]);
+ }, [recordsQuery.data, sessionsUsernameQuery.data]);📝 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.
| useEffect(() => { | |
| if (recordsQuery.data) { | |
| const records: InstrumentVisualizationRecord[] = []; | |
| for (const record of recordsQuery.data) { | |
| const props = record.data && typeof record.data === 'object' ? record.data : {}; | |
| records.push({ | |
| __date__: record.date, | |
| __time__: record.date.getTime(), | |
| ...record.computedMeasures, | |
| ...props | |
| let cancelled = false; | |
| const fetchRecords = () => { | |
| try { | |
| const sessions = sessionsUsernameQuery.data; | |
| if (recordsQuery.data && sessions) { | |
| // Fetch all sessions in parallel | |
| // Build records with looked-up data | |
| const records: InstrumentVisualizationRecord[] = recordsQuery.data.map((record) => { | |
| const props = record.data && typeof record.data === 'object' ? record.data : {}; | |
| const usersSessions = sessions.filter((s) => s.id === record.sessionId); | |
| const session = usersSessions[0]; | |
| const username = session?.user?.username ?? 'N/A'; | |
| return { | |
| __date__: record.date, | |
| __time__: record.date.getTime(), | |
| username: username, | |
| ...record.computedMeasures, | |
| ...props | |
| }; | |
| }); | |
| if (!cancelled) { | |
| setRecords(records); | |
| } | |
| } | |
| } catch (error) { | |
| console.error('Error occurred: ', error); | |
| notifications.addNotification({ | |
| message: t({ | |
| en: 'Error occurred finding records', | |
| fr: "Une erreur s'est produite lors de la recherche des enregistrements." | |
| }), | |
| type: 'error' | |
| }); | |
| } | |
| setRecords(records); | |
| } | |
| }; | |
| void fetchRecords(); | |
| return () => { | |
| cancelled = true; | |
| }; | |
| }, [recordsQuery.data]); | |
| useEffect(() => { | |
| let cancelled = false; | |
| const fetchRecords = () => { | |
| try { | |
| const sessions = sessionsUsernameQuery.data; | |
| if (recordsQuery.data && sessions) { | |
| // Fetch all sessions in parallel | |
| // Build records with looked-up data | |
| const records: InstrumentVisualizationRecord[] = recordsQuery.data.map((record) => { | |
| const props = record.data && typeof record.data === 'object' ? record.data : {}; | |
| const usersSessions = sessions.filter((s) => s.id === record.sessionId); | |
| const session = usersSessions[0]; | |
| const username = session?.user?.username ?? 'N/A'; | |
| return { | |
| __date__: record.date, | |
| __time__: record.date.getTime(), | |
| username: username, | |
| ...record.computedMeasures, | |
| ...props | |
| }; | |
| }); | |
| if (!cancelled) { | |
| setRecords(records); | |
| } | |
| } | |
| } catch (error) { | |
| console.error('Error occurred: ', error); | |
| notifications.addNotification({ | |
| message: t({ | |
| en: 'Error occurred finding records', | |
| fr: "Une erreur s'est produite lors de la recherche des enregistrements." | |
| }), | |
| type: 'error' | |
| }); | |
| } | |
| }; | |
| void fetchRecords(); | |
| return () => { | |
| cancelled = true; | |
| }; | |
| }, [recordsQuery.data, sessionsUsernameQuery.data]); |
🤖 Prompt for AI Agents
In apps/web/src/hooks/useInstrumentVisualization.ts around lines 220 to 263, the
effect reads sessionsUsernameQuery.data but the dependency array only includes
recordsQuery.data; update the effect dependencies to include
sessionsUsernameQuery.data as well (i.e., add sessionsUsernameQuery.data to the
array) so fetchRecords re-runs when session data changes and avoid stale
closures.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/web/src/hooks/useInstrumentVisualization.ts (2)
220-257: AddsessionsUsernameQuery.datato dependency array.The effect reads
sessionsUsernameQuery.data(line 223) but only listsrecordsQuery.datain the dependency array (line 257). When session data updates, the effect won't re-run and records will be stale.Apply this diff:
void fetchRecords(); - }, [recordsQuery.data]); + }, [recordsQuery.data, sessionsUsernameQuery.data]);
230-232: Optimize session lookup to avoid O(n²) complexity.Line 230 filters the entire
sessionsarray for every record. For large datasets, this creates a performance bottleneck. Build a Map once before the loop for O(1) lookups.Apply this diff:
// Build records with looked-up data + const sessionMap = new Map(sessions.map(s => [s.id, s])); - const records: InstrumentVisualizationRecord[] = recordsQuery.data.map((record) => { + const records: InstrumentVisualizationRecord[] = recordsQuery.data.map((record) => { const props = record.data && typeof record.data === 'object' ? record.data : {}; - const usersSessions = sessions.filter((s) => s.id === record.sessionId); - const session = usersSessions[0]; + const session = sessionMap.get(record.sessionId); const username = session?.user?.username ?? 'N/A';
🧹 Nitpick comments (1)
apps/web/src/hooks/useInstrumentVisualization.ts (1)
66-67: Remove stale comment block.These comments serve no purpose and clutter the code.
Apply this diff:
- // Create a new sessionsUsernameQuery which uses the useFindSessionQuery hook - // have use a different return type with -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts(6 hunks)apps/web/src/hooks/useInstrumentVisualization.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/hooks/tests/useInstrumentVisualization.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/src/hooks/useInstrumentVisualization.ts (1)
apps/web/src/hooks/useFindSessionQuery.ts (1)
useFindSessionQuery(17-37)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint-and-test
🔇 Additional comments (3)
apps/web/src/hooks/useInstrumentVisualization.ts (3)
97-100: LGTM!Username field correctly mapped to export format.
112-122: LGTM!Username extraction and initialization logic is correct.
132-145: LGTM!Username correctly included in long record format.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/web/src/hooks/useInstrumentVisualization.ts (2)
257-257: Add missing dependency to useEffect.
fetchRecordsreadssessionsUsernameQuery.data(line 223) but the dependency array only includesrecordsQuery.data. This causes stale closures when session data updates.- }, [recordsQuery.data]); + }, [recordsQuery.data, sessionsUsernameQuery.data, notifications, t]);
220-244: O(n²) complexity: Use Map for session lookup.Line 230's
.find()inside.map()creates quadratic time complexity. For large datasets this is a performance bottleneck.Build a Map once before the loop:
const fetchRecords = () => { try { const sessions = sessionsUsernameQuery.data; if (recordsQuery.data && sessions) { - // Fetch all sessions in parallel - // Build records with looked-up data + const sessionMap = new Map(sessions.map(s => [s.id, s])); + const records: InstrumentVisualizationRecord[] = recordsQuery.data.map((record) => { const props = record.data && typeof record.data === 'object' ? record.data : {}; - const usersSession = sessions.find((s) => s.id === record.sessionId); + const usersSession = sessionMap.get(record.sessionId); const username = usersSession?.user?.username ?? 'N/A';
🧹 Nitpick comments (4)
apps/web/src/components/Sidebar/Sidebar.tsx (1)
91-103: Avoid non‑null assertions oncurrentSession.subjectnow that it’s nullable
currentSession.subject!andcurrentSession.subject!.idwill throw if a session without a subject ever lands in the store, andisSubjectWithPersonalInfoitself assumes a non‑null object. Instead of asserting, either:
- Strengthen the
currentSessiontype in the store so that, in this context,subjectis guaranteed non‑null, or- Guard on
currentSession.subjectbefore rendering, e.g. derive a localconst subject = currentSession.subject;and only callisSubjectWithPersonalInfo(subject)/removeSubjectIdScope(subject.id)whensubjectis truthy, with a safe fallback (or no card) otherwise.This keeps the UI from hard‑crashing if subjectless sessions are ever introduced while still aligning with the new nullable schema.
apps/web/src/routes/_app/instruments/render/$id.tsx (1)
64-64: Redundant nullish coalescing.
currentSession?.subject ?? undefinedis redundant—optional chaining already returnsundefinedwhen the chain breaks.Simplify:
- subject={currentSession?.subject ?? undefined} + subject={currentSession?.subject}apps/web/src/hooks/useInstrumentVisualization.ts (2)
66-67: Remove stale comments.These commented lines serve no purpose and clutter the code.
- // Create a new sessionsUsernameQuery which uses the useFindSessionQuery hook - // have use a different return type with -
220-257: Consider adding cleanup to prevent stale state.Although
fetchRecordsis synchronous, rapid changes to dependencies can still cause stalesetRecordscalls. Adding cleanup is defensive.useEffect(() => { + let cancelled = false; const fetchRecords = () => { try { const sessions = sessionsUsernameQuery.data; if (recordsQuery.data && sessions) { // ... build records ... - setRecords(records); + if (!cancelled) { + setRecords(records); + } } } catch (error) { console.error('Error occurred: ', error); - notifications.addNotification({ + if (!cancelled) { + notifications.addNotification({ + // ... + }); + } } }; void fetchRecords(); + + return () => { + cancelled = true; + }; }, [recordsQuery.data, sessionsUsernameQuery.data, notifications, t]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/api/src/sessions/sessions.controller.ts(2 hunks)apps/web/src/components/Sidebar/Sidebar.tsx(2 hunks)apps/web/src/hooks/useInstrumentVisualization.ts(7 hunks)apps/web/src/routes/_app/instruments/render/$id.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/src/sessions/sessions.controller.ts
🧰 Additional context used
🧬 Code graph analysis (3)
apps/web/src/routes/_app/instruments/render/$id.tsx (1)
apps/web/src/testing/stubs.ts (1)
currentSession(15-33)
apps/web/src/hooks/useInstrumentVisualization.ts (1)
apps/web/src/hooks/useFindSessionQuery.ts (1)
useFindSessionQuery(17-37)
apps/web/src/components/Sidebar/Sidebar.tsx (2)
packages/subject-utils/src/index.ts (2)
isSubjectWithPersonalInfo(18-28)removeSubjectIdScope(34-39)apps/web/src/testing/stubs.ts (1)
currentSession(15-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint-and-test
🔇 Additional comments (2)
apps/web/src/hooks/useInstrumentVisualization.ts (2)
16-16: LGTM!The import is properly used by
sessionsUsernameQueryat line 59.
97-100: LGTM!Username field handling is consistent across all export formats with proper 'N/A' fallback.
Also applies to: 112-112, 119-122, 132-132, 145-145
…r happy with itself
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts (2)
52-59: Remove unused mock data.
mockSessionandmockUserare defined but never referenced in the tests. OnlymockSessionWithUsernameis actually used (line 88).Apply this diff:
-const mockSession = { - userId: '111' -}; - -const mockUser = { - id: '111', - username: 'testusername' -}; -
224-239: Consider verifying Username in JSON export.While the JSON test verifies
someValueandsubjectID, it doesn't explicitly check for theUsernamefield like the other format tests do. Consider adding:expect(jsonContents).toContain('"Username": "testusername"');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/api/src/sessions/sessions.controller.ts(2 hunks)apps/api/src/sessions/sessions.service.ts(2 hunks)apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/src/sessions/sessions.service.ts
🧰 Additional context used
🧬 Code graph analysis (2)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts (1)
apps/web/src/hooks/useInstrumentVisualization.ts (1)
useInstrumentVisualization(31-268)
apps/api/src/sessions/sessions.controller.ts (4)
apps/api/src/core/decorators/route-access.decorator.ts (1)
RouteAccess(18-20)apps/web/src/store/types.ts (1)
CurrentUser(8-10)apps/api/src/auth/auth.types.ts (1)
AppAbility(23-23)packages/schemas/src/session/session.ts (1)
SessionWithUser(28-28)
🪛 Biome (2.1.2)
apps/api/src/sessions/sessions.controller.ts
[error] 12-12: Shouldn't redeclare 'SessionWithUser'. Consider to delete it or rename it.
'SessionWithUser' is defined here:
(lint/suspicious/noRedeclare)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint-and-test
🔇 Additional comments (2)
apps/api/src/sessions/sessions.controller.ts (1)
25-33: LGTM!The new endpoint correctly implements access control, follows the existing controller patterns, and properly delegates to the service layer with the required ability parameter.
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts (1)
97-112: LGTM: Proper async handling across all tests.All test cases now correctly wait for records to populate before triggering downloads. This prevents timing issues and ensures stable test execution.
Also applies to: 115-130, 133-149, 152-168, 171-194, 197-221, 224-240
| 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 }] | ||
| } | ||
| }); |
| if (!sessionsWithUsers) { | ||
| throw new NotFoundException(`Failed to find users`); | ||
| } |
| <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!) ? ( |
There was a problem hiding this comment.
Is the old PR superseded by this? If so, please close it.
| ] | ||
| }; | ||
|
|
||
| const mockSession = { |
There was a problem hiding this comment.
make sure all linting passes
| params: SessionWithUserQueryParams; | ||
| }; | ||
|
|
||
| export const useFindSessionQuery = ( |
There was a problem hiding this comment.
This should be named useSessionQuery, consistent with the other query hooks. The same goes for the user query.
apps/web/src/hooks/useFindUser.ts
Outdated
| 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; | ||
| } | ||
| }; |
There was a problem hiding this comment.
yes. why are you ignoring errors?
| __time__: record.date.getTime(), | ||
| ...record.computedMeasures, | ||
| ...props | ||
| const fetchRecords = () => { |
| setRecords(records); | ||
| } | ||
| }; | ||
| void fetchRecords(); |
There was a problem hiding this comment.
or just eliminate entirely. there is no need to have a function that is invoked only one time, immediately after the definition, with no arguments.
| <div className="grow"> | ||
| <InstrumentRenderer | ||
| className="mx-auto max-w-3xl" | ||
| subject={currentSession?.subject} |
There was a problem hiding this comment.
change InstrumentRenderer to accept null or undefined, I understand why you did this but it looks bizzare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/web/src/hooks/useFindSessionQuery.ts (2)
6-11: Remove stale instructional comments.These comments describe steps that are already implemented and now just add noise. Consider deleting them.
-//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 -
17-22: Align hook name with existing query hook naming (useSessionQuery).Previous feedback suggested naming this
useSessionQueryto be consistent with other query hooks. If that convention still holds, consider renaming before merging to avoid churn later.
🧹 Nitpick comments (2)
apps/web/src/hooks/useFindSessionQuery.ts (2)
12-21: Consider makingparamsoptional for better ergonomics.Right now
paramsis required inUseSessionOptions, but the default uses{}and most callers may only care aboutenabled. Makingparamsoptional and defaulting it to{}would simplify usage.-type UseSessionOptions = { - enabled?: boolean; - params: SessionWithUserQueryParams; -}; +type UseSessionOptions = { + enabled?: boolean; + params?: SessionWithUserQueryParams; +}; -export const useFindSessionQuery = ( - { enabled, params }: UseSessionOptions = { - enabled: true, - params: {} - } -) => { +export const useFindSessionQuery = ( + { enabled = true, params = {} }: UseSessionOptions = {} +) => {Please double-check this against existing call sites to ensure it matches how the hook is actually used.
31-32: Useparamsobject directly in the React Query key.
['sessions', ...Object.values(params)]can become fragile as more params are added (different param shapes can collapse to the same values array). React Query works well with object keys, so you can keep it stable and extensible by includingparamsas an object.- queryKey: ['sessions', ...Object.values(params)] + queryKey: ['sessions', params]This also makes it easier to reason about cache entries as you add more filters later.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/hooks/useFindSessionQuery.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/src/hooks/useFindSessionQuery.ts (1)
packages/schemas/src/session/session.ts (2)
SessionWithUserQueryParams(37-39)$SessionWithUser(29-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint-and-test
🔇 Additional comments (1)
apps/web/src/hooks/useFindSessionQuery.ts (1)
1-4: Imports and schema usage look correct.Using
$SessionWithUserfor validation and a type-only import forSessionWithUserQueryParamsmatches the schema design and keeps runtime bundle size minimal.
|
closed due to updated pr #1238 |
2 participants
will close issue #1220
problems with collecting sessions for then use the userids in the subject table export
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.