Add username to export #1244
Add username to export #1244
Conversation
WalkthroughAdds nullable subject to Session and a new SessionWithUser schema. Exposes GET /v1/sessions returning sessions with optional usernames (filterable by group). Backend: SessionsService method to fetch sessions joined with user info and safer group null-check. Frontend: useFindSessionQuery hook and instrument export now includes Username; tests updated. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Frontend as Web App
participant API as Sessions Controller
participant Service as SessionsService
participant DB as Database
User->>Frontend: Open view or trigger export
Frontend->>API: GET /v1/sessions?groupId=...
API->>Service: findAllIncludeUsernames(groupId, ability)
Service->>DB: Query sessions JOIN users (apply ability & optional group filter)
DB-->>Service: SessionWithUser[] rows
Service-->>API: SessionWithUser[]
API-->>Frontend: Sessions with usernames
Frontend->>Frontend: Merge username into instrument records and build export
Frontend-->>User: Present/download export including Username
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)apps/web/src/hooks/useInstrumentVisualization.ts (1)
⏰ 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). (2)
🔇 Additional comments (4)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/routes/_app/instruments/render/$id.tsx (1)
38-66: UsecurrentSession!.subjectIdinstead ofcurrentSession!.subject!.idto avoid runtime crashesThe
Session.subjectfield is nullable (subject: $Subject.nullable()), but the code applies double non-null assertions (currentSession!.subject!.id). If a session hassubject === null, this will throw at runtime.The
Sessionschema provides a direct non-nullablesubjectId: z.string()field that satisfies the sameCreateInstrumentRecordDatarequirement. Using it eliminates the crash risk:const handleSubmit: InstrumentSubmitHandler = async ({ data, instrumentId }) => { await axios.post('/v1/instrument-records', { data, date: new Date(), groupId: currentGroup?.id, instrumentId, sessionId: currentSession!.id, - subjectId: currentSession!.subject!.id + subjectId: currentSession!.subjectId } satisfies CreateInstrumentRecordData);
🧹 Nitpick comments (4)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts (1)
29-230: Tests cover username in tabular exports; consider also asserting JSON usernameThe new mocks and expectations for CSV/TSV/Excel (wide and long) look consistent with the username-enriched records and the
useFindSessionQuerywiring.To fully lock in the behavior that JSON exports also include username (since
exportRecordsretains theusernamefield), you might extend the JSON test to assert it:const jsonContents = await getContentFn(); expect(jsonContents).toContain('"someValue": "abc"'); expect(jsonContents).toContain('"subjectID": "testId"'); + expect(jsonContents).toContain('"username": "testusername"');That way, all export formats are explicitly verified to include Username.
apps/web/src/hooks/useFindSessionQuery.ts (1)
6-32: Clean up outdated TODO comments and consider makingparamsoptionalThe hook implementation itself is aligned with the new
/v1/sessionsAPI andSessionWithUserschema, but the inline comments still read like pending TODOs and are now misleading.You can safely drop them, and optionally make
paramsoptional in the options type to better reflect the default:-//Change this query to into a hook method and name it useFindSessionQuery - -//Change the api call to have an include tag which includes the username from users - -//Change the return type to - -type UseSessionOptions = { - enabled?: boolean; - params: SessionWithUserQueryParams; -}; +type UseSessionOptions = { + enabled?: boolean; + params?: SessionWithUserQueryParams; +}; export const useFindSessionQuery = ( - { enabled, params }: UseSessionOptions = { + { enabled, params }: UseSessionOptions = { enabled: true, params: {} } ) => { return useQuery({ enabled, queryFn: async () => { const response = await axios.get('/v1/sessions', { - params + params }); return $SessionWithUser.array().parseAsync(response.data); },This keeps behavior the same while making the API a bit clearer.
apps/web/src/hooks/useInstrumentVisualization.ts (1)
59-65: GuardgroupIdparam to avoid sendinggroupId: undefinedWhen
currentGroupis null,paramsbecomes{ groupId: undefined }. Depending on axios/query serialization, this might sendgroupId=undefinedinstead of omitting the filter entirely.You can avoid that by only including
groupIdwhen present:- const sessionsUsernameQuery = useFindSessionQuery({ - enabled: instrumentId !== null, - params: { - groupId: currentGroup?.id - } - }); + const sessionsUsernameQuery = useFindSessionQuery({ + enabled: instrumentId !== null, + params: currentGroup?.id ? { groupId: currentGroup.id } : {} + });apps/api/src/sessions/sessions.service.ts (1)
99-117: TightenfindAllIncludeUsernamesfilters and unreachable NotFound guardThe core of
findAllIncludeUsernameslooks good—joiningsubjectanduser.usernameunder the existing access control—but there are a couple of details you may want to adjust:
- Avoid filtering on undefined
groupIdWhen
groupIdis undefined,{ groupId }can introduce agroupId: undefinedcondition. It’s clearer to only add that filter when a real value is provided:- const sessionsWithUsers = await this.sessionModel.findMany({ - include: { - subject: true, - user: { - select: { - username: true - } - } - }, - where: { - AND: [accessibleQuery(ability, 'read', 'Session'), { groupId }] - } - }); - if (!sessionsWithUsers) { - throw new NotFoundException(`Failed to find users`); - } - return sessionsWithUsers; + const where: Prisma.SessionWhereInput = { + AND: [ + accessibleQuery(ability, 'read', 'Session'), + ...(groupId ? [{ groupId }] : []) + ] + }; + + const sessionsWithUsers = await this.sessionModel.findMany({ + include: { + subject: true, + user: { + select: { + username: true + } + } + }, + where + }); + + // Optionally, either keep returning [] or check length if you truly want a 404. + return sessionsWithUsers;
- Unreachable NotFoundException
findManyalways returns an array, so!sessionsWithUsersis never true. If you do want a 404 when nothing is found, checksessionsWithUsers.length === 0and adjust the error message to mention sessions rather than “users”.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/api/src/sessions/sessions.controller.ts(2 hunks)apps/api/src/sessions/sessions.service.ts(2 hunks)apps/web/src/components/Sidebar/Sidebar.tsx(2 hunks)apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts(6 hunks)apps/web/src/hooks/useFindSessionQuery.ts(1 hunks)apps/web/src/hooks/useInstrumentVisualization.ts(7 hunks)apps/web/src/routes/_app/instruments/render/$id.tsx(2 hunks)apps/web/src/utils/excel.ts(0 hunks)packages/schemas/src/session/session.ts(2 hunks)
💤 Files with no reviewable changes (1)
- apps/web/src/utils/excel.ts
🧰 Additional context used
🧬 Code graph analysis (8)
apps/web/src/hooks/useFindSessionQuery.ts (1)
packages/schemas/src/session/session.ts (2)
SessionWithUserQueryParams(37-39)$SessionWithUser(29-35)
apps/web/src/routes/_app/instruments/render/$id.tsx (1)
apps/web/src/testing/stubs.ts (1)
currentSession(15-33)
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)
apps/web/src/hooks/useInstrumentVisualization.ts (1)
apps/web/src/hooks/useFindSessionQuery.ts (1)
useFindSessionQuery(17-33)
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)
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)
packages/schemas/src/session/session.ts (1)
packages/schemas/src/subject/subject.ts (1)
$Subject(20-27)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts (1)
apps/web/src/hooks/useInstrumentVisualization.ts (1)
useInstrumentVisualization(31-268)
⏰ 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). (2)
- GitHub Check: lint-and-test
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
packages/schemas/src/session/session.ts (1)
13-39: Session schema changes look coherent with new username flowMaking
subjectnullable and addingSessionWithUser/SessionWithUserQueryParamsline up with the new/sessionsendpoint and username export behavior. The shapes match what the service returns (session + optional user withusername).Just ensure remaining callers of
Session(beyond those updated in this PR) are audited forsession.subjectnow being nullable (avoiding unsafe non‑null assertions).apps/api/src/sessions/sessions.controller.ts (1)
24-32: New/sessionsendpoint is consistent with schema and hook usageThe
findAllIncludeUsernamesendpoint cleanly wiresabilityand optionalgroupIdthrough to the service and returnsPromise<SessionWithUser[]>, which matches the shared schema and theuseFindSessionQueryhook.No issues from a controller/API perspective.
apps/api/src/sessions/sessions.service.ts (1)
45-52: Group null‑check is safe; behavior unchangedThe additional
if (group)guard aroundaddGroupForSubjectmakes thecreatepath more defensive without altering behavior whengroupsService.findByIdthrows on a missing group.No further issues in this block.
apps/web/src/components/Sidebar/Sidebar.tsx (1)
81-105: I'll verify the concerns raised in this review comment by examining the codebase to confirm the null-safety issue withcurrentSession.subject.
<function_calls>
find . -type f -name ".ts" -o -name ".tsx" | grep -i session | head -20
</function_calls>Let me search for the Session type definition and related types:
<function_calls>
rg -n "type Session|interface Session" --type ts --type tsx -A 10 | head -60
</function_calls><function_calls>
rg -n "export.*Session" --type ts --type tsx | grep -E "(type|interface)" | head -20
</function_calls>Let me directly check the Sidebar.tsx file to see the current implementation:
<function_calls>
cat -n apps/web/src/components/Sidebar/Sidebar.tsx | sed -n '75,115p'
</function_calls><function_calls>
wc -l apps/web/src/components/Sidebar/Sidebar.tsx
</function_calls><function_calls>
cat -n apps/web/src/components/Sidebar/Sidebar.tsx | head -120
</function_calls><function_calls>
rg -n "isSubjectWithPersonalInfo|removeSubjectIdScope" --type ts --type tsx -B 2 -A 2 | head -50
</function_calls><function_calls>
fd -e ts -e tsx | xargs grep -l "isSubjectWithPersonalInfo" | head -10
</function_calls><function_calls>
rg -n "function isSubjectWithPersonalInfo|const isSubjectWithPersonalInfo|export.*isSubjectWithPersonalInfo" --type ts --type tsx -A 5
</function_calls><function_calls>
rg -n "currentSession" apps/web/src/components/Sidebar/Sidebar.tsx -B 3 -A 3 | head -80
</function_calls><function_calls>
cat -n apps/web/src/components/Sidebar/Sidebar.tsx | tail -100
</function_calls><function_calls>
rg -n "Session" apps/web/src/components/Sidebar/Sidebar.tsx | head -20
</function_calls>Let me check the imports and get more context:
<function_calls>
head -50 apps/web/src/components/Sidebar/Sidebar.tsx
</function_calls><function_calls>
rg -n "Subject|Session" --type ts --type tsx "apps/web/src" -l | grep -E "(types|schema|model)" | head -10
</function_calls><function_calls>
fd -name ".ts" -o -name ".tsx" | xargs grep -l "type.*Session|interface.*Session" | head -10
</function_calls><function_calls>
rg "type Session\s*=|interface Session" --type ts --type tsx | head -20
</function_calls><function_calls>
find . -path ./node_modules -prune -o -type f ( -name ".ts" -o -name ".tsx" ) -print | xargs grep -l "Session" | grep -v test | grep -v node_modules | head -20
</function_calls>Let me try a broader search to understand the structure:
<function_calls>
ls -la apps/web/src/
</function_calls><function_calls>
find apps/web/src -maxdepth 3 -type d | head -20
</function_calls><function_calls>
rg "Session" apps/web/src --type ts --type tsx -l | head -15
</function_calls><function_calls>
cat apps/web/src/components/Sidebar/Sidebar.tsx
</function_calls><function_calls>
rg -n "subject" apps/web/src/components/Sidebar/Sidebar.tsx -i
</function_calls><function_calls>
rg -n "currentSession" apps/web/src/components/Sidebar/Sidebar.tsx
</function_calls>Let me check what imports and APIs are used in Sidebar:
<function_calls>
grep -n "import|from" apps/web/src/components/Sidebar/Sidebar.tsx | head -30
</function_calls><function_calls>
rg "currentSession|useSession" apps/web/src -l | head -10
</function_calls><function_calls>
rg "export.*useSession|const useSession" --type ts --type tsx -A 5 | head -40
</function_calls><function_calls>
find apps -name ".ts" -o -name ".tsx" | xargs grep -l "isSubjectWithPersonalInfo|removeSubjectIdScope" | head -10
</function_calls><function_calls>
rg "isSubjectWithPersonalInfo" --type ts --type tsx -B 5 -A 10 | head -60
</function_calls><function_calls>
rg "removeSubjectIdScope" --type ts --type tsx -B 5 -A 10 | head -60
</function_calls>Let me try to find the project structure and type definitions:
<function_calls>
find . -name "tsconfig.json" -o -name "package.json" | grep -E "^./[^/]+/(tsconfig|package)" | head -5
</function_calls><function_calls>
ls -la apps/
</function_calls><function_calls>
find apps -name ".ts" -o -name ".tsx" | wc -l
</function_calls>Let me use Python to search more comprehensively:
<function_calls>
python3 << 'EOF'
import os
import reSearch for Session type definition
for root, dirs, files in os.walk('.'):
# Skip node_modules
dirs[:] = [d for d in dirs if d != 'node_modules' and not d.startswith('.')]for file in files: if file.endswith(('.ts', '.tsx')): filepath = os.path.join(root, file) try: with open(filepath, 'r', encoding='utf-8', errors='ignore') as f: content = f.read() # Look for Session type/interface definitions if re.search(r'(type|interface)\s+Session\s*[={]', content): print(f"Found Session definition in: {filepath}") # Show the relevant lines lines = content.split('\n') for i, line in enumerate(lines): if re.search(r'(type|interface)\s+Session\s*[={]', line): print(f" Line {i+1}: {line}") # Print next 15 lines for j in range(1, min(15, len(lines)-i)): print(f" Line {i+1+j}: {lines[i+j]}") break except: passEOF
</function_calls><function_calls>
python3 << 'EOF'
import os
import reSearch for isSubjectWithPersonalInfo function
found_files = set()
for root, dirs, files in os.walk('.'):
dirs[:] = [d for d in dirs if d != 'node_modules' and not d.startswith('.')]for file in files: if file.endswith(('.ts', '.tsx')): filepath = os.path.join(root, file) try: with open(filepath, 'r', encoding='utf-8', errors='ignore') as f: content = f.read() if 'isSubjectWithPersonalInfo' in content: found_files.add(filepath) except: passprint("Files containing 'isSubjectWithPersonalInfo':")
for f in sorted(found_files):
print(f)
EOF
</function_calls><function_calls>
find . -type f ( -name ".ts" -o -name ".tsx" ) -exec grep -l "isSubjectWithPersonalInfo" {} ;
</function_calls><function_calls>
git ls-files | grep -E ".(ts|tsx)$" | xargs grep -l "isSubjectWithPersonalInfo" 2>/dev/null
</function_calls><function_calls>
git ls-files | head -50
</function_calls><function_calls>
git ls-files | grep "Sidebar"
</function_calls><function_calls>
git ls-files | grep -E ".(ts|tsx)$" | wc -l
</function_calls><function_calls>
git status
</function_calls><function_calls>
pwd
</function_calls><function_calls>
ls -la
</function_calls><function_calls>
git log --oneline -5
</function_calls>Now let me search across all TypeScript files systematically:
<function_calls>
git ls-files ".ts" ".tsx" | while read f; do grep -l "isSubjectWithPersonalInfo|Subject" "$f" 2>/dev/null; done | head -20
</function_calls><function_calls>
git ls-files | grep -E ".(ts|tsx)$" | xargs grep -l "Subject" | head -20
</function_calls><function_calls>
git show HEAD:apps/web/src/components/Sidebar/Sidebar.tsx 2>/dev/null | head -150
</function_calls><function_calls>
git log --oneline -- "apps/web/src/components/Sidebar/Sidebar.tsx" 2>/dev/null | head -10
</function_calls><function_calls>
git branch -a
</function_calls><function_calls>
git ls-files | wc -l
</function_calls>Let me try a different approach with file listing:
<function_calls>
find . -type f ( -name ".ts" -o -name ".tsx" ) 2>/dev/null | grep -v node_modules | head -40
</function_calls><function_calls>
find . -type f -path "/Sidebar/" 2>/dev/null
</function_calls><function_calls>
find . -type f -name "Sidebar.tsx" 2>/dev/null
</function_calls><function_calls>
ls -la apps/web/src/components/
</function_calls><function_calls>
test -f apps/web/src/components/Sidebar/Sidebar.tsx && echo "File exists" || echo "File not found"
</function_calls><function_calls>
ls -la apps/web/src/components/Sidebar/ 2>&1
</function_calls><function_calls>
ls -la apps/web/
</function_calls><function_calls>
ls -la apps/
</function_calls><function_calls>
ls -la
</function_calls><function_calls>
find . -type f -name "*.tsx" 2>/dev/null | head -10
</function_calls><function_calls>
git remote -v
</function_calls>Let me try to understand this from a different angle. The repository may not be fully available, but I can analyze the review comment based on the code snippet provided and logical reasoning:
<function_calls>
cd /tmp && git clone --depth 1 https://github.com/Code-Clinic/clinic.io.git 2>&1 | head -20
</function_calls>Let me analyze the review comment based on the provided code snippet and TypeScript safety principles:
<function_calls>
cat > /tmp/analysis.md << 'EOF'Review Comment Analysis
Issue Assessment:
The code snippet shows:
isSubjectWithPersonalInfo(currentSession.subject!)- uses non-null assertionremoveSubjectIdScope(currentSession.subject!.id)- uses non-null assertion- Falls back to showing only ID when isSubjectWithPersonalInfo returns false
Review claims:
- Session.subject is now nullable
- Non-null assertions without guards can throw
- Should use guard: currentSession.subject &&
- Should use optional chaining in fallback: currentSession.subject?.id ?? currentSession.subjectId
Proposed diff logic:
- Adds guard before calling isSubjectWithPersonalInfo
- Removes non-null assertion from isSubjectWithPersonalInfo call
- Uses optional chaining with nullish coalescing in fallback
TypeScript Safety Perspective:
- If a value is nullable, using ! without checking is a code smell
- The proposed guard
currentSession.subject && isSubjectWithPersonalInfo(...)is proper TypeScript- Optional chaining
?.idis the right pattern for potentially null/undefined values- Nullish coalescing
??is appropriate for fallback to subjectIdLogical Consistency:
- Guard is only needed if Session.subject can be null
- The fallback to subjectId assumes currentSession object always has subjectId property
- This is a reasonable pattern for sessions where full subject data may not be loaded
EOF
cat /tmp/analysis.mdBased on my analysis, I need to verify the key assumptions. Let me search for actual evidence in a way that doesn't depend on repository cloning:
<function_calls>
python3 << 'EOF'Analyze the proposed fix logically
code_snippet = """
{currentSession && (
<motion.div ...>
{t('common.sessionInProgress')}
{isSubjectWithPersonalInfo(currentSession.subject!) ? (
{
${t('core.fullName')}: ${currentSession.subject.firstName} ...}
...
) : (
ID: {removeSubjectIdScope(currentSession.subject!.id)}
)}
</motion.div>
)}
"""proposed_fix = """
{currentSession && (
<motion.div ...>
{t('common.sessionInProgress')}
{currentSession.subject && isSubjectWithPersonalInfo(currentSession.subject) ? (
{
${t('core.fullName')}: ${currentSession.subject.firstName} ...}
...
) : (
ID: {removeSubjectIdScope(currentSession.subject?.id ?? currentSession.subjectId)}
)}
</motion.div>
)}
"""print("ANALYSIS OF PROPOSED FIX:")
print("=" * 60)
print("\n1. GUARD ADDED:")
print(" Old: isSubjectWithPersonalInfo(currentSession.subject!)")
print(" New: currentSession.subject && isSubjectWithPersonalInfo(currentSession.subject)")
print(" Assessment: ✓ Proper null-checking guard added")
print("\n2. FALLBACK CHANGED:")
print(" Old: removeSubjectIdScope(currentSession.subject!.id)")
print(" New: removeSubjectIdScope(currentSession.subject?.id ?? currentSession.subjectId)")
print(" Assessment: ✓ Optional chaining + nullish coalescing is safer")
print("\n3. LOGICAL FLOW:")
print(" - If subject exists and has personal info: show full details")
print(" - If subject doesn't exist OR lacks personal info: show just ID")
print(" Assessment: ✓ Logic remains consistent")
print("\n4. ASSUMPTIONS MADE:")
print(" - currentSession.subject is nullable")
print(" - currentSession always has subjectId as fallback")
print(" - isSubjectWithPersonalInfo is safe to call with non-null subject")
EOF
</function_calls>Let me check the GitHub repository directly for this code:
<function_calls>
clinic.io github repo Sidebar.tsx subject nullable
</function_calls><function_calls>
"isSubjectWithPersonalInfo" clinic typescript
</function_calls><function_calls>
CodeRabbit Sidebar.tsx subject null safety review
</function_calls>Based on my analysis, I cannot access the actual repository or verify the specific TypeScript type definitions to confirm the assumptions stated in the review comment. However, I can assess the review's logic:
What I can verify:
- The code snippet clearly uses non-null assertions (
!) oncurrentSession.subject- The proposed fix properly guards with
currentSession.subject &&before callingisSubjectWithPersonalInfo- The fallback uses modern TypeScript patterns: optional chaining (
?.id) and nullish coalescing (??)- This matches standard TypeScript null-safety best practices
What I cannot verify without repository access:
- Whether
Session.subjectis actually nullable in the current type definition- Whether
currentSession.subjectIdproperty always exists- Whether the code is currently at lines 81-105 as stated
- The exact behavior of
isSubjectWithPersonalInfofunctionThe review comment's logic is sound from a TypeScript safety perspective, but I cannot confirm the factual assumptions (that Session.subject is nullable, that currentSession has subjectId, etc.) without access to the actual type definitions and current code.
Guard against
currentSession.subjectbeing null in session cardWith
Session.subjectnow nullable, using non-null assertions oncurrentSession.subject!without checking can throw at runtime. Guard the check before callingisSubjectWithPersonalInfo, and use optional chaining with fallback tosubjectIdin the else branch:- {isSubjectWithPersonalInfo(currentSession.subject!) ? ( + {currentSession.subject && isSubjectWithPersonalInfo(currentSession.subject) ? ( <div data-testid="current-session-info"> <p>{`${t('core.fullName')}: ${currentSession.subject.firstName} ${currentSession.subject.lastName}`}</p> <p> {`${t('core.identificationData.dateOfBirth.label')}: ${toBasicISOString(currentSession.subject.dateOfBirth)}`}{' '} </p> <p> {`${t('core.identificationData.sex.label')}: ${t( `core.identificationData.sex.${toLowerCase(currentSession.subject.sex)}` )}`} </p> </div> ) : ( <div data-testid="current-session-info"> - <p>ID: {removeSubjectIdScope(currentSession.subject!.id)}</p> + <p>ID: {removeSubjectIdScope(currentSession.subject?.id ?? currentSession.subjectId)}</p> </div> )}This ensures the code safely handles sessions where
subjectis null or undefined by falling back to displaying the subject ID.
…r happy with itself
david-roper
force-pushed
the
add-username-to-export
branch
from
2aa4ee1 to
db53c16
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/web/src/hooks/useInstrumentVisualization.ts (1)
217-254: Dependency array still incomplete despite past review.The past review correctly identified that this effect reads
sessionsUsernameQuery.data(line 220),notifications(line 244), andt(line 245), but the dependency array on line 254 still only includesrecordsQuery.data. The issue flagged in the previous review has not been resolved.Apply the fix suggested in the past review:
- useEffect(() => { - 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 usersSession = sessions.find((s) => s.id === record.sessionId); - - const username = usersSession?.user?.username ?? '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' - }); - } - }; - void fetchRecords(); - }, [recordsQuery.data]); + useEffect(() => { + const sessions = sessionsUsernameQuery.data; + const data = recordsQuery.data; + if (!data || !sessions) { + return; + } + + try { + const nextRecords: InstrumentVisualizationRecord[] = data.map((record) => { + const props = record.data && typeof record.data === 'object' ? record.data : {}; + const usersSession = sessions.find((s) => s.id === record.sessionId); + const username = usersSession?.user?.username ?? 'N/A'; + + return { + __date__: record.date, + __time__: record.date.getTime(), + username, + ...record.computedMeasures, + ...props + }; + }); + + setRecords(nextRecords); + } 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' + }); + } + }, [recordsQuery.data, sessionsUsernameQuery.data, notifications, t]);
🧹 Nitpick comments (3)
apps/web/src/hooks/useFindSessionQuery.ts (2)
17-22: Avoid object literal in default parameter.The default object
{ enabled: true, params: {} }creates a new reference on each render, potentially causing unnecessary re-renders or query refetches.-export const useFindSessionQuery = ( - { enabled, params }: UseSessionOptions = { - enabled: true, - params: {} - } -) => { +export const useFindSessionQuery = (options?: UseSessionOptions) => { + const { enabled = true, params = {} } = options ?? {}; + return useQuery({
23-32: Consider more explicit query key construction.
Object.values(params)works but is fragile if params structure changes. Consider explicit keys for better stability.- queryKey: ['sessions', ...Object.values(params)] + queryKey: ['sessions', params.groupId ?? 'all']This makes cache invalidation patterns clearer.
packages/schemas/src/session/session.ts (1)
28-35: Clarify nullable vs nullish for user.username.Both
useris nullable anduser.usernameis nullish. Ifuseris null, the username check is already protected. Is the nullish username for cases where a user exists but has no username, or for future flexibility?Consider adding a comment explaining when
useris null vs whenuser.usernameis nullish to help future maintainers understand the schema.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/api/src/sessions/sessions.controller.ts(2 hunks)apps/api/src/sessions/sessions.service.ts(2 hunks)apps/web/src/components/Sidebar/Sidebar.tsx(2 hunks)apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts(6 hunks)apps/web/src/hooks/useFindSessionQuery.ts(1 hunks)apps/web/src/hooks/useInstrumentVisualization.ts(7 hunks)apps/web/src/routes/_app/instruments/render/$id.tsx(2 hunks)apps/web/src/utils/excel.ts(0 hunks)packages/schemas/src/session/session.ts(2 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/api/src/sessions/sessions.service.ts
- apps/web/src/components/Sidebar/Sidebar.tsx
🧰 Additional context used
🧬 Code graph analysis (5)
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)
apps/web/src/hooks/useFindSessionQuery.ts (1)
packages/schemas/src/session/session.ts (2)
SessionWithUserQueryParams(37-39)$SessionWithUser(29-35)
packages/schemas/src/session/session.ts (1)
packages/schemas/src/subject/subject.ts (1)
$Subject(20-27)
apps/web/src/routes/_app/instruments/render/$id.tsx (1)
apps/web/src/testing/stubs.ts (1)
currentSession(15-33)
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts (1)
apps/web/src/hooks/useInstrumentVisualization.ts (1)
useInstrumentVisualization(31-265)
⏰ 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/routes/_app/instruments/render/$id.tsx (1)
64-64: Good explicit null handling.Clearly converts null to undefined for the prop. This is safer and more readable than relying on implicit coercion.
apps/web/src/hooks/__tests__/useInstrumentVisualization.test.ts (2)
30-50: Well-structured test mocks.The mock data correctly models the session-to-username relationship, with matching IDs for correlation. This properly exercises the lookup logic.
88-231: Proper async handling in tests.Using
waitForto ensure records are populated before download actions correctly handles the asynchronous query resolution. The comprehensive verification across all export formats provides good coverage.apps/web/src/hooks/useInstrumentVisualization.ts (2)
59-64: Proper query setup.The session query is correctly gated and parameterized for fetching username data.
94-97: Username export logic is consistent.The transformation from internal
usernamefield to exportedUsernamecolumn is handled correctly across all formats, with appropriate 'N/A' fallback.Also applies to: 109-109, 116-119, 129-129, 142-142
packages/schemas/src/session/session.ts (1)
37-39: Query params type is straightforward.Simple TypeScript type for filtering parameters. Appropriate for query param usage.
apps/api/src/sessions/sessions.controller.ts (1)
24-32: No authorization concern: endpoint correctly uses existing permission model.The endpoint properly gates access through
Session:readpermission viaaccessibleQuery, which is the standard pattern for session data access throughout the codebase. Usernames are intentionally included in theSessionWithUserresponse (as reflected in the API description), and this follows the same authorization model as other endpoints that return usernames (e.g.,User:readfor user endpoints,Session:readfor other session endpoints likefindById). Only the username field is selected, so no additional sensitive user data is exposed.
| instrumentId, | ||
| sessionId: currentSession!.id, | ||
| subjectId: currentSession!.subject.id | ||
| subjectId: currentSession!.subject!.id |
There was a problem hiding this comment.
Potential runtime error: unsafe non-null assertion on nullable subject.
The schema now allows session.subject to be null, but this line assumes it exists. If a session without a subject reaches this point, the app will crash.
Add a subject check to the guard at line 33:
useEffect(() => {
- if (!currentSession?.id) {
+ if (!currentSession?.id || !currentSession.subject) {
void navigate({ to: '/instruments/accessible-instruments' });
}
}, [currentSession?.id]);Or handle the null case explicitly in the submission handler.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/web/src/routes/_app/instruments/render/$id.tsx around line 45, the code
uses a non-null assertion currentSession!.subject!.id which will throw if
session.subject is null; update the guard at ~line 33 to also verify
currentSession?.subject exists (e.g., return early or render an error/disabled
form when subject is null) or, if you prefer to handle it in the submit handler,
add an explicit null check before reading subject.id and return/throw a
controlled error (and surface that to the UI) so the runtime crash cannot occur.
| date: z.coerce.date(), | ||
| groupId: z.string().nullable(), | ||
| subject: $Subject, | ||
| subject: $Subject.nullable(), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all places accessing session.subject
rg -n "session\.subject" --type=ts -B 2 -A 1 | head -100Repository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 65
🏁 Script executed:
#!/bin/bash
# Try with unescaped pattern and broader search
echo "=== Searching for session.subject with broader pattern ==="
rg "session\.subject" --type=ts
echo "=== Searching for optional chaining access ==="
rg "session\?.subject" --type=ts
echo "=== Read the session.ts file to see the change context ==="
cat -n packages/schemas/src/session/session.ts | head -30Repository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 1395
🏁 Script executed:
#!/bin/bash
# Search for currentSession usage (mentioned in review context)
echo "=== Searching for currentSession usage ==="
rg "currentSession" --type=ts -B 2 -A 2 | head -80
echo ""
echo "=== Searching for Session type references and usage ==="
rg "\bSession\b" --type=ts -g "!session.ts" | grep -i "subject\|current" | head -40
echo ""
echo "=== Looking for non-null assertions on subject ==="
rg "subject\s*!" --type=ts | head -20Repository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 6991
🏁 Script executed:
#!/bin/bash
# Check the exact context of subject access in render/$id.tsx
echo "=== Checking $id.tsx context ==="
rg -n "subject!\.id" apps/web/src/routes/_app/instruments/render/$id.tsx -B 5 -A 2
echo ""
echo "=== Checking Sidebar.tsx context ==="
rg -n "currentSession\.subject!" apps/web/src/components/Sidebar/Sidebar.tsx -B 3 -A 3
echo ""
echo "=== Check the testing stub to see subject structure ==="
cat -n apps/web/src/testing/stubs.ts | grep -A 20 "export const currentSession"Repository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 1570
🏁 Script executed:
#!/bin/bash
# Find files in the render directory
echo "=== Files in render directory ==="
find apps/web/src/routes/_app/instruments/render -type f | head -20
echo ""
echo "=== Search for subject!.id in that directory ==="
rg -n "subject!\.id" apps/web/src/routes/_app/instruments/render/ -B 5 -A 2Repository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 930
Perfect! I've verified the breaking change concern. Let me create the final rewritten review comment.
Breaking change confirmed: code assumes subject is always present but schema now allows null.
Two locations use non-null assertions on currentSession.subject without defensive checks:
apps/web/src/routes/_app/instruments/render/$id.tsx:45:currentSession!.subject!.idapps/web/src/components/Sidebar/Sidebar.tsx:91,103:currentSession.subject!
If a session's subject is null, these will crash at runtime. Update callers to handle nullable subject or ensure subject is always hydrated when accessed.
🤖 Prompt for AI Agents
packages/schemas/src/session/session.ts around line 13: the schema changed to
make `subject` nullable (`subject: $Subject.nullable()`), but callers use
non-null assertions causing runtime crashes
(apps/web/src/routes/_app/instruments/render/$id.tsx:45 and
apps/web/src/components/Sidebar/Sidebar.tsx:91,103). Fix by either (A) if
`subject` must always exist, revert the schema to non-nullable so types match
runtime expectations, or (B) keep `subject` nullable and update all callers to
handle null safely—remove `!` assertions, add explicit null checks/early returns
or use optional chaining and handle the null case in UI/logic, and update
related type annotations to reflect the nullable type. Ensure tests and any
hydration logic guarantee `subject` presence if you choose (A).
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/web/src/hooks/useFindSessionQuery.ts (1)
27-28: Avoid usingObject.values(params)in the query key.Relying on
Object.valuesordering is brittle ifSessionWithUserQueryParamsgains more fields. Using the params object directly is more robust and matches common React Query patterns:- queryKey: ['sessions', ...Object.values(params)] + queryKey: ['sessions', params]
📜 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 (2)
packages/schemas/src/session/session.ts (2)
SessionWithUserQueryParams(37-39)$SessionWithUser(29-35)vendor/[email protected]/src/index.js (1)
params(93909-93909)
🪛 Biome (2.1.2)
apps/web/src/hooks/useFindSessionQuery.ts
[error] 8-8: ';' expected'
An explicit or implicit semicolon is expected here...
(parse)
[error] 8-8: ';' expected'
An explicit or implicit semicolon is expected here...
(parse)
⏰ 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). (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: lint-and-test
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/web/src/hooks/useFindSessionQuery.ts (2)
6-9: Consider makingparamsoptional for better ergonomics.Requiring
paramsforces callers to write{ enabled: false, params: {} }even when they don't need to filter. Making it optional improves DX without breaking existing usage.type UseSessionOptions = { enabled?: boolean; - params: SessionWithUserQueryParams; + params?: SessionWithUserQueryParams; };
11-27: Nice work! Previous issues resolved.The hook is well-structured and the earlier parse errors and TODO comments have been addressed.
Prefer object in queryKey over
Object.values.Using
Object.values(params)is order-dependent. IfSessionWithUserQueryParamsgains fields later, cache keys become fragile. React Query v5 serializes objects reliably.- queryKey: ['sessions', ...Object.values(params)] + queryKey: ['sessions', params]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/hooks/useFindSessionQuery.ts(1 hunks)
⏰ 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). (2)
- GitHub Check: lint-and-test
- GitHub Check: Analyze (javascript-typescript)
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 (2)
apps/api/src/instrument-records/instrument-records.service.ts(2 hunks)packages/schemas/src/instrument-records/instrument-records.ts(1 hunks)
⏰ 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). (2)
- GitHub Check: lint-and-test
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
apps/api/src/instrument-records/instrument-records.service.ts (1)
189-189: User model confirmation: username field exists and is properly typed.The User model in
apps/api/prisma/schema.prismadefines username as a required String field (username String), and the Session model has an optional user relation. The code correctly uses optional chaining (record.session.user?.username) to handle the case where the user relation is null, with a fallback to 'N/A'. No null safety issues exist.
| subjectSex: null | string; | ||
| timestamp: string; | ||
| userId: string; | ||
| username: string; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read the file to see the full context of the type definition
cat -n packages/schemas/src/instrument-records/instrument-records.ts | head -80Repository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 3005
🏁 Script executed:
#!/bin/bash
# Search for InstrumentRecordsExport usage across the codebase
rg -nP --type=ts --type=tsx 'InstrumentRecordsExport' -B2 -A2Repository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 110
🏁 Script executed:
#!/bin/bash
# Search for references to userId in export/records contexts
rg -nP --type=ts --type=tsx '\.userId|userId\s*[:=]' -C2 | head -100Repository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 110
🏁 Script executed:
#!/bin/bash
# Search for userId references in TypeScript/JavaScript files (without tsx type)
rg -nP 'userId' --type=ts -C2 | head -150Repository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 4436
🏁 Script executed:
#!/bin/bash
# Search for InstrumentRecordsExport imports and usage
rg -nP 'InstrumentRecordsExport' -C3Repository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 4230
🏁 Script executed:
#!/bin/bash
# Search for export-related functions or methods that might use this type
rg -nP 'exportRecords|export.*Instrument.*Record' --type=ts -C2 | head -100Repository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 8385
🏁 Script executed:
#!/bin/bash
# Read the exportRecords method implementation in the service
sed -n '135,200p' apps/api/src/instrument-records/instrument-records.service.tsRepository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 2603
🏁 Script executed:
#!/bin/bash
# Continue reading the exportRecords method
sed -n '135,250p' apps/api/src/instrument-records/instrument-records.service.tsRepository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 4383
🏁 Script executed:
#!/bin/bash
# Check git log for recent changes to instrument-records.ts
git log --oneline -20 -- packages/schemas/src/instrument-records/instrument-records.tsRepository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 149
🏁 Script executed:
#!/bin/bash
# Check git show for the most recent commit to this file
git log -p -1 -- packages/schemas/src/instrument-records/instrument-records.ts | head -100Repository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 65
🏁 Script executed:
#!/bin/bash
# Show the actual diff of that commit
git show 2e4a766cRepository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 50400
🏁 Script executed:
#!/bin/bash
# Search for userId specifically in export contexts (not in vendor or unrelated schemas)
rg -nP '\.userId\s*\??\.' --type=ts -g '!vendor' -g '!node_modules' | head -50Repository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 65
🏁 Script executed:
#!/bin/bash
# Check the excel utility to see if it accesses the username field
cat -n apps/web/src/utils/excel.tsRepository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 1130
🏁 Script executed:
#!/bin/bash
# Search for any explicit access to userId field in export contexts
rg -nP '\brecord.*userId|\.userId\b' --type=ts -g '!vendor' -g '!node_modules'Repository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 65
🏁 Script executed:
#!/bin/bash
# Check if there are any tests for the export functionality
find . -type f \( -name '*.test.ts' -o -name '*.spec.ts' \) -exec grep -l 'exportRecords\|InstrumentRecordsExport' {} \; | head -20Repository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 65
🏁 Script executed:
#!/bin/bash
# Check for any changelog, migration guide, or breaking changes documentation
find . -type f \( -name 'CHANGELOG*' -o -name 'MIGRATION*' -o -name 'BREAKING*' \) | head -10Repository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 909
🏁 Script executed:
#!/bin/bash
# Search for any API documentation that might reference userId or username in exports
rg -i 'userId.*export|export.*userId|username.*export|export.*username' --type=ts --type=md -g '!vendor' -g '!node_modules' | head -20Repository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 65
Breaking change confirmed: userId → username in export format.
The field rename in InstrumentRecordsExport (line 67) is confirmed by commit 2e4a766c ("chore: rename userId in datahub export column to username"). This changes the exported data structure and will affect external systems, scripts, or processes that expect the userId column.
Verification shows:
- Backend
exportRecordsmethod already usesusername: record.session.user?.username ?? 'N/A' - Web consumers (
excel.ts,datahub/index.tsx) pass export data to downstream services without field-level access, so no code updates needed within this repository - The breaking change impacts external data consumers (reports, imports, downstream systems), not internal code
No internal consumers need updates, but external stakeholders relying on exported file format should be notified of the column name change.
🤖 Prompt for AI Agents
In packages/schemas/src/instrument-records/instrument-records.ts around line 67,
the export field was renamed from userId to username which breaks external
consumers; update the repository to reflect this change by (1) updating any
schema/version metadata for InstrumentRecordsExport to indicate the breaking
change, (2) adding a clear inline comment at the field declaration noting the
rename and the commit/reference causing it, (3) adding an entry to CHANGELOG or
release notes describing the column rename and its impact, and (4) notifying
external stakeholders (email/PR/issue) that exported files now contain username
instead of userId so downstream imports/reports must be updated.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/web/src/hooks/useInstrumentVisualization.ts (1)
217-250:useEffectdependency array is incomplete, causing records to stay empty/stale when sessions load after records.This effect reads
sessionsUsernameQuery.data,notifications, andtbut only depends onrecordsQuery.data. IfrecordsQuery.dataresolves beforesessionsUsernameQuery.data, theif (recordsQuery.data && sessions)check fails once,setRecordsis never called, and the effect will not re-run whensessionsUsernameQuery.datalater becomes available, leavingrecordsempty or outdated.You should (a) add an early return when either dataset is missing and (b) include all referenced reactive values in the dependency array:
- useEffect(() => { - 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 usersSession = sessions.find((s) => s.id === record.sessionId); - - const username = usersSession?.user?.username ?? 'N/A'; - - return { - __date__: record.date, - __time__: record.date.getTime(), - username: username, - ...record.computedMeasures, - ...props - }; - }); - - setRecords(records); - } - } catch (error) { - notifications.addNotification({ - message: t({ - en: 'Error occurred finding records', - fr: "Une erreur s'est produite lors de la recherche des enregistrements." - }), - type: 'error' - }); - } - }, [recordsQuery.data]); + useEffect(() => { + const sessions = sessionsUsernameQuery.data; + const data = recordsQuery.data; + if (!data || !sessions) { + return; + } + + try { + const nextRecords: InstrumentVisualizationRecord[] = data.map((record) => { + const props = record.data && typeof record.data === 'object' ? record.data : {}; + const usersSession = sessions.find((s) => s.id === record.sessionId); + + const username = usersSession?.user?.username ?? 'N/A'; + + return { + __date__: record.date, + __time__: record.date.getTime(), + username, + ...record.computedMeasures, + ...props + }; + }); + + setRecords(nextRecords); + } catch (error) { + notifications.addNotification({ + message: t({ + en: 'Error occurred finding records', + fr: "Une erreur s'est produite lors de la recherche des enregistrements." + }), + type: 'error' + }); + } + }, [recordsQuery.data, sessionsUsernameQuery.data, notifications, t]);This ensures
recordsare recomputed whenever either query’s data or the notification/translation callbacks change, and only when all required data is present.
🧹 Nitpick comments (1)
apps/web/src/hooks/useInstrumentVisualization.ts (1)
59-64: Consider simplifying theenabledcondition forsessionsUsernameQuery.Since the query is only parameterized by
groupId, you could enable it purely based oncurrentGroup?.idinstead ofinstrumentId !== nullto prefetch sessions as soon as a group is selected; current behavior is correct but slightly stricter than necessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/hooks/useInstrumentVisualization.ts(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/src/hooks/useInstrumentVisualization.ts (2)
apps/web/src/hooks/useFindSessionQuery.ts (1)
useFindSessionQuery(11-27)vendor/[email protected]/src/v3.d.ts (2)
date(2475-2475)record(2499-2499)
🔇 Additional comments (2)
apps/web/src/hooks/useInstrumentVisualization.ts (2)
16-16: ImportinguseFindSessionQueryis appropriate and consistent.The local import for
useFindSessionQueryfits the existing hook import pattern and keeps concerns well-separated.
94-97: Username is correctly threaded through wide and long exports.Mapping the
usernamefield to aUsernamecolumn in both wide and long formats, with an'N/A'fallback, is consistent and should make the new data available across all export types without breaking existing keys.Also applies to: 109-119, 129-129, 142-142
closes issue #1220
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.