Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThis PR refactors React Query usage in the teacher app's local notes feature by centralizing query options and cache key definitions. Rather than scattering inline query configurations throughout hooks, the code now imports reusable query option builders and structured cache key helpers from a dedicated queries module. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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
🧹 Nitpick comments (2)
apps/teacher/src/hooks/useLocalNotes.ts (1)
180-198: Consider extractingpublishAllMutationtolocalNotesMutations.The
publishAllMutationandclearAfterPublishMutationare defined inline while other mutations use centralized definitions fromlocalNotesMutations. For consistency, consider moving these to the centralized module as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/teacher/src/hooks/useLocalNotes.ts` around lines 180 - 198, Extract publishAllMutation and clearAfterPublishMutation into the centralized localNotesMutations module to match other mutations: move the mutation definitions (the async publishAllMutation that loops calling localNotesService.publishNote(note.id) and the clearAfterPublishMutation that calls localNotesService.clearLocalDataAfterPublish) into localNotesMutations, export them, and replace the inline declarations in useLocalNotes.ts with imports from localNotesMutations; ensure the exported mutations keep the same mutationKey values (teacherMutationKeys.localNotes.publishAll and teacherMutationKeys.localNotes.clearAfterPublish) and preserve the onSettled behavior that invalidates queryClient.invalidateQueries({ queryKey: localNotesKeys.all }).apps/teacher/src/lib/queries/local-notes.ts (1)
18-42: UnreachableclassIdon teacher branch (line 33).When execution reaches line 31,
params.classIdis guaranteed to be falsy (theifon line 26 already checked it). Passing it togetNotesByTeacheris harmless but misleading—readers may assume it carries a valid value.♻️ Suggested fix
if (params.teacherId) { return localNotesService.getNotesByTeacher(params.teacherId, { - classId: params.classId, includeUnpublished: params.includeUnpublished ?? true, }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/teacher/src/lib/queries/local-notes.ts` around lines 18 - 42, The teacher branch in localNotesQueryOptions passes params.classId to localNotesService.getNotesByTeacher even though params.classId is falsy there; remove the misleading classId property from the options object in the second branch (inside the if (params.teacherId) block) so getNotesByTeacher is called with only relevant options (e.g., { includeUnpublished: params.includeUnpublished ?? true }), updating the call site in localNotesQueryOptions and leaving the class branch unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/teacher/src/lib/queries/local-notes.ts`:
- Around line 44-58: The code in noteGradesQueryOptions currently defaults
missing detail.value to the string '0', conflating "no grade" with a zero grade;
change the Map value type to allow absence (e.g., Map<string, string | null>)
and set map.set(detail.studentId, detail.value ?? null) instead of '0'; update
any related type annotations/usages expecting Map<string, string> (and
tests/consumers) to handle null values and preserve the rest of the logic around
localNotesService.getGradesByNote and localNotesKeys.grades.
---
Nitpick comments:
In `@apps/teacher/src/hooks/useLocalNotes.ts`:
- Around line 180-198: Extract publishAllMutation and clearAfterPublishMutation
into the centralized localNotesMutations module to match other mutations: move
the mutation definitions (the async publishAllMutation that loops calling
localNotesService.publishNote(note.id) and the clearAfterPublishMutation that
calls localNotesService.clearLocalDataAfterPublish) into localNotesMutations,
export them, and replace the inline declarations in useLocalNotes.ts with
imports from localNotesMutations; ensure the exported mutations keep the same
mutationKey values (teacherMutationKeys.localNotes.publishAll and
teacherMutationKeys.localNotes.clearAfterPublish) and preserve the onSettled
behavior that invalidates queryClient.invalidateQueries({ queryKey:
localNotesKeys.all }).
In `@apps/teacher/src/lib/queries/local-notes.ts`:
- Around line 18-42: The teacher branch in localNotesQueryOptions passes
params.classId to localNotesService.getNotesByTeacher even though params.classId
is falsy there; remove the misleading classId property from the options object
in the second branch (inside the if (params.teacherId) block) so
getNotesByTeacher is called with only relevant options (e.g., {
includeUnpublished: params.includeUnpublished ?? true }), updating the call site
in localNotesQueryOptions and leaving the class branch unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c833930-73cc-48b1-b584-0be0a37eb051
📒 Files selected for processing (2)
apps/teacher/src/hooks/useLocalNotes.tsapps/teacher/src/lib/queries/local-notes.ts
| export function noteGradesQueryOptions(noteId: string) { | ||
| return queryOptions({ | ||
| queryKey: localNotesKeys.grades(noteId), | ||
| queryFn: async () => { | ||
| const noteDetails = await localNotesService.getGradesByNote(noteId) | ||
| const map = new Map<string, string>() | ||
| for (const detail of noteDetails) { | ||
| map.set(detail.studentId, detail.value ?? '0') | ||
| } | ||
| return map | ||
| }, | ||
| networkMode: 'always', | ||
| throwOnError: true, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Default grade value '0' may be semantically incorrect.
Line 51 defaults a missing detail.value to '0'. If '0' represents a valid grade (e.g., zero points), this conflates "no grade entered" with "student scored zero." Consider using null, undefined, or '' to distinguish an absent value.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/teacher/src/lib/queries/local-notes.ts` around lines 44 - 58, The code
in noteGradesQueryOptions currently defaults missing detail.value to the string
'0', conflating "no grade" with a zero grade; change the Map value type to allow
absence (e.g., Map<string, string | null>) and set map.set(detail.studentId,
detail.value ?? null) instead of '0'; update any related type annotations/usages
expecting Map<string, string> (and tests/consumers) to handle null values and
preserve the rest of the logic around localNotesService.getGradesByNote and
localNotesKeys.grades.
1 participant
Detailed logic on cache management and UI feedback:
useQueryconfigurations inuseLocalNotes.tstoqueryOptionsfactories located inlocal-notes.ts.networkMode: 'always'to all local note queries to ensure offline capabilities are seamlessly supported for local PGlite interactions, bypassing traditional network states.throwOnError: trueconfiguration inside query options to directly wire local note queries into React Error Boundaries.['notes']vs.localNotesKeys.all). Cache mutations like.save,.update,.publish, and.deletenow reliably and globally invalidate matching queries mapping vialocalNotesKeys.all, completely resolving prior UI feedback and optimistic caching bugs where the interface failed to reflect mutation success.PR created automatically by Jules for task 2311753973866608502 started by @ldsgroups225
Summary by CodeRabbit