Add username to export #1227
Add username to export #1227
Conversation
|
Warning Rate limit exceeded@david-roper has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 42 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughAdds a POST /sessions/list endpoint and SessionsService.findSessionList to return multiple sessions by IDs with per-session ability checks. Adds frontend helpers Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Controller as SessionsController
participant Service as SessionsService
participant DB as Database
participant Ability as AccessControl
Client->>Controller: POST /sessions/list?ids=id1,id2
Controller->>Controller: normalize ids -> [id1,id2]
Controller->>Service: findSessionList([id1,id2], {ability})
loop per id
Service->>Ability: authorize access for id
Ability-->>Service: allowed/denied
Service->>DB: fetch session by id
DB-->>Service: session | not found
end
Service->>Controller: return Session[]
Controller-->>Client: 200 OK with Session[]
sequenceDiagram
autonumber
participant Hook as useInstrumentVisualization
participant Helpers as sessionInfo/userInfo
participant Backend as API Server
Hook->>Hook: iterate records
loop per record
Hook->>Helpers: sessionInfo(sessionId)
Helpers->>Backend: GET /v1/sessions/{sessionId}
Backend-->>Helpers: Session | error
Helpers-->>Hook: Session | null
alt session has userId
Hook->>Helpers: userInfo(userId)
Helpers->>Backend: GET /v1/users/{userId}
Backend-->>Helpers: User | error
Helpers-->>Hook: User | null
Hook->>Hook: set Username (user.username or 'N/A')
else
Hook->>Hook: set Username = 'N/A'
end
end
Hook->>Hook: setState(enrichedRecords)
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)
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
from
0635ee4 to
fd0a90c
Compare
david-roper
force-pushed
the
add-username-to-export
branch
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.
| 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 | ||
| }); | ||
| const fetchRecords = async () => { | ||
| 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 | ||
| }); | ||
| } | ||
|
|
||
| setRecords(records); | ||
| } | ||
| setRecords(records); | ||
| } | ||
| }; | ||
| void fetchRecords(); | ||
| }, [recordsQuery.data]); |
There was a problem hiding this comment.
Parallelize API calls to avoid performance bottleneck.
The current implementation fetches session and user data sequentially for each record. With N records, this performs 2N sequential API calls, which could take significant time for large datasets.
Refactor to fetch all sessions and users in parallel:
useEffect(() => {
const fetchRecords = async () => {
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);
}
};
void fetchRecords();
}, [recordsQuery.data]);Note: This assumes User has an id field. Adjust the mapping logic if the field name differs.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/web/src/hooks/useInstrumentVisualization.ts around lines 207 to 243, the
code currently awaits sessionInfo and userInfo sequentially for each record
causing 2N serial API calls; change to parallelize: first collect all
record.sessionId values and call sessionInfo for all of them with Promise.all
(or a batched API if available), map results by sessionId, then collect unique
userIds from those session results and call userInfo in parallel for those
unique IDs with Promise.all (or batched API), map users by id, and finally
iterate records to build the records array using the session and user maps
(preserving null checks and using 'N/A' fallbacks for missing user or username)
before calling setRecords.
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
| @ApiOperation({ description: 'Find Sessions by ID' }) | ||
| @Post('list') | ||
| @RouteAccess({ action: 'read', subject: 'Session' }) | ||
| findSessionList(@Query('ids') ids: string[], @CurrentUser('ability') ability: AppAbility): Promise<Session[]> { | ||
| const idArray = Array.isArray(ids) ? ids : (ids as string).split(','); | ||
| return this.sessionsService.findSessionList(idArray, { ability }); | ||
| } |
There was a problem hiding this comment.
Use request body instead of query parameters for POST endpoint.
POST endpoints should use @Body rather than @Query. Query parameters are subject to URL length limits whether using GET or POST, so using POST with query parameters doesn't solve the problem of handling large ID lists. Either switch to GET with query parameters, or use POST with the IDs in the request body.
If POST with body is preferred (recommended for potentially large ID lists):
+import type { string[] } from '@nestjs/common';
- findSessionList(@Query('ids') ids: string[], @CurrentUser('ability') ability: AppAbility): Promise<Session[]> {
+ findSessionList(@Body() body: { ids: string[] }, @CurrentUser('ability') ability: AppAbility): Promise<Session[]> {
- const idArray = Array.isArray(ids) ? ids : (ids as string).split(',');
+ const idArray = body.ids;
return this.sessionsService.findSessionList(idArray, { ability });
}Alternatively, if the ID list is expected to be small, use GET:
+ @Get('list')
- @Post('list')Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/api/src/sessions/sessions.controller.ts around lines 28-34, the POST
'list' endpoint currently reads ids from @Query which is inappropriate for
potentially large ID lists; either change the route to GET if ids will remain
small or (recommended) switch to POST with the request body: replace the
@Query('ids') parameter with a @Body('ids') or, better, a DTO that defines ids:
string[] (and add class-validator decorators), remove the Array.isArray/ split
parsing (expect an array), update the method signature and any
callers/tests/Swagger docs accordingly, and keep calling
this.sessionsService.findSessionList(idArray, { ability }) with the parsed
array.
| findSessionList(@Query('ids') ids: string[], @CurrentUser('ability') ability: AppAbility): Promise<Session[]> { | ||
| const idArray = Array.isArray(ids) ? ids : (ids as string).split(','); | ||
| return this.sessionsService.findSessionList(idArray, { ability }); |
There was a problem hiding this comment.
Add validation for the number of IDs.
No limit is enforced on the number of IDs, which could cause performance degradation or database overload if a large list is provided.
Consider adding validation:
@ApiOperation({ description: 'Find Sessions by ID' })
@Post('list')
@RouteAccess({ action: 'read', subject: 'Session' })
findSessionList(@Query('ids') ids: string[], @CurrentUser('ability') ability: AppAbility): Promise<Session[]> {
const idArray = Array.isArray(ids) ? ids : (ids as string).split(',');
if (idArray.length === 0) {
throw new BadRequestException('At least one ID must be provided');
}
if (idArray.length > 100) { // adjust limit based on requirements
throw new BadRequestException('Too many IDs provided. Maximum 100 allowed.');
}
return this.sessionsService.findSessionList(idArray, { ability });
}Note: You'll need to import BadRequestException from @nestjs/common.
🤖 Prompt for AI Agents
In apps/api/src/sessions/sessions.controller.ts around lines 31 to 33, the
controller accepts an unbounded list of IDs which can cause performance or DB
overload; add validation after parsing ids to ensure at least one ID is provided
and enforce an upper limit (e.g., 100) throwing BadRequestException for empty or
too-large lists, and import BadRequestException from @nestjs/common; preserve
existing ability handling and pass the validated idArray to
sessionsService.findSessionList.
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
|
|
||
| export const sessionInfo = async (sessionId: string): Promise<null | Session> => { | ||
| try { | ||
| const response = await axios.get(`/v1/sessions/${sessionId}`); |
There was a problem hiding this comment.
Apply URL encoding to sessionId.
The sessionId parameter is interpolated directly into the URL without encoding. If it contains special characters, the request could fail or behave unexpectedly.
Apply this diff:
- const response = await axios.get(`/v1/sessions/${sessionId}`);
+ const response = await axios.get(`/v1/sessions/${encodeURIComponent(sessionId)}`);📝 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 response = await axios.get(`/v1/sessions/${sessionId}`); | |
| const response = await axios.get(`/v1/sessions/${encodeURIComponent(sessionId)}`); |
🤖 Prompt for AI Agents
In apps/web/src/hooks/useFindSession.ts around line 6, the sessionId is
interpolated directly into the URL which can break for special characters;
replace the raw interpolation with an encoded value using
encodeURIComponent(sessionId) when building the request URL (e.g.,
axios.get(`/v1/sessions/${encodeURIComponent(sessionId)}`)) so the sessionId is
properly URL-encoded before sending the request.
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
apps/web/src/hooks/useFindUser.ts
Outdated
| @@ -0,0 +1,13 @@ | |||
| import { type User, $User } from '@opendatacapture/schemas/user'; | |||
There was a problem hiding this comment.
Fix import style.
Use top-level type-only imports and sort named imports alphabetically.
-import { type User, $User } from '@opendatacapture/schemas/user';
+import type { User } from '@opendatacapture/schemas/user';
+import { $User } from '@opendatacapture/schemas/user';As per coding guidelines
📝 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.
| import { type User, $User } from '@opendatacapture/schemas/user'; | |
| import type { User } from '@opendatacapture/schemas/user'; | |
| import { $User } from '@opendatacapture/schemas/user'; |
🧰 Tools
🪛 ESLint
[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)
🤖 Prompt for AI Agents
In apps/web/src/hooks/useFindUser.ts around line 1, the import should use a
top-level type-only import and have named/value imports sorted alphabetically;
split the current combined import into an "import type { User } from
'@opendatacapture/schemas/user';" followed by a separate value import "import {
$User } from '@opendatacapture/schemas/user';" ensuring the type-only import
appears first and any multiple value imports are alphabetized.
|
|
||
| export const userInfo = async (userId: string): Promise<null | User> => { | ||
| try { | ||
| const response = await axios.get(`/v1/users/${userId}`); |
There was a problem hiding this comment.
Apply URL encoding to userId.
The userId is interpolated directly without encoding. Special characters will break the request.
- const response = await axios.get(`/v1/users/${userId}`);
+ const response = await axios.get(`/v1/users/${encodeURIComponent(userId)}`);📝 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 response = await axios.get(`/v1/users/${userId}`); | |
| const response = await axios.get(`/v1/users/${encodeURIComponent(userId)}`); |
🤖 Prompt for AI Agents
In apps/web/src/hooks/useFindUser.ts around line 6, the code interpolates userId
directly into the request URL which can break for special characters; URL-encode
userId before building the path (e.g., use encodeURIComponent(userId)) so the
GET request is safe, and update the axios.get call to pass the encoded id.
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
1 participant
will close issue #1220
problems with collecting sessions for then use the userids in the subject table export
Summary by CodeRabbit
New Features
Tests