only same lessons can be merged#196
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ 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). (4)
🧰 Additional context used📓 Path-based instructions (4)apps/{chronos,iris}/src/**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/instructions/filc-reuse.instructions.md)
Files:
apps/iris/src/{routes,components}/**/*.tsx📄 CodeRabbit inference engine (.github/instructions/iris-data-flow.instructions.md)
Files:
apps/iris/src/components/**/*dialog.tsx📄 CodeRabbit inference engine (.github/instructions/iris-dialog-form.instructions.md)
Files:
apps/iris/src/**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/instructions/iris.instructions.md)
Files:
🔇 Additional comments (2)
📝 WalkthroughWalkthroughThis PR updates the substitution dialog to support multiple merged teachers: it computes a deduplicated ChangesMultiple Parallel Teachers Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/iris/src/components/admin/substitution-dialog.tsx`:
- Line 101: Remove the unused indirection: delete the parallelTeachersRef
useRef<Map<string,string>> declaration and the effect that syncs it, then change
any code that looks up a teacherId via the map (used in onSubmit / handleSubmit
logic) to extract the id directly from the encoded value by parsing the
"__merged__:${teacherId}" string (e.g., value.startsWith("__merged__") ?
value.split("__:")[1] or value.replace("__merged__:", "") /
value.slice("__merged__:".length)). Update onSubmit/submit handler to use this
direct extraction for both single and parallel teacher selections and remove any
map lookups or related state synchronization.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: adef8b81-9824-4072-af63-36f27dac1313
📒 Files selected for processing (1)
apps/iris/src/components/admin/substitution-dialog.tsx
📜 Review details
⏰ 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). (4)
- GitHub Check: typecheck
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (actions)
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (4)
apps/{chronos,iris}/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/instructions/filc-reuse.instructions.md)
apps/{chronos,iris}/src/**/*.{ts,tsx}: Reuse existing helpers, types, schemas, and hooks before adding new ones. Check nearby feature folders first, then shared files such as apps/chronos/src/database/helpers.ts, apps/iris/src/utils/query-keys.ts, apps/iris/src/hooks/use-has-permission.ts, apps/iris/src/components/admin/admin.types.ts, and apps/iris/src/components/doorlock/doorlock.types.ts
When a second call site needs the same logic, prefer extracting or extending the existing abstraction instead of creating a parallel helper with a slightly different name
Keep abstractions local to the narrowest shared boundary that already exists. Do not create cross-app utilities for one feature-specific use
Extend existing dialog props, response shapes, and query key families instead of re-declaring near-identical types in each file
Prefer the smallest root-cause fix that matches neighboring code over broad rewrites or speculative cleanup
Keep imports on the app alias boundary:#...for Chronos and@/...for Iris
Files:
apps/iris/src/components/admin/substitution-dialog.tsx
apps/iris/src/{routes,components}/**/*.tsx
📄 CodeRabbit inference engine (.github/instructions/iris-data-flow.instructions.md)
apps/iris/src/{routes,components}/**/*.tsx: Always use centralized keys from apps/iris/src/utils/query-keys.ts for React Query. Do not introduce inline array query keys for existing domains
UseparseResponse(...)and the generated Hono client from apps/iris/src/utils/hc.ts for API requests when that is the local pattern
When a mutation changes server state, invalidate every affected query family, not just the page-local list. Follow the multi-invalidation pattern already used in admin news and doorlock screens
Reuse apps/iris/src/hooks/use-has-permission.ts and existing permission guard components instead of duplicating permission logic in the view
New user-facing error and success messages should go throught(...)and the locale files, even when surfaced through toasts
Files:
apps/iris/src/components/admin/substitution-dialog.tsx
apps/iris/src/components/**/*dialog.tsx
📄 CodeRabbit inference engine (.github/instructions/iris-dialog-form.instructions.md)
apps/iris/src/components/**/*dialog.tsx: Follow the dialog structure used in files like card-dialog.tsx and user-dialog.tsx: create the form near the top of the component, derive reactive slices withuseStore(form.store, selector), and render fields with<form.Field>{(field) => ...}</form.Field>
Reuse validation schemas from apps/iris/src/utils/form-schemas.ts when available. If a schema becomes shared by multiple dialogs, move it there instead of copying validation logic
form.resettakes raw values, not{ values: ... }. Becauseform.resetandform.setFieldValueare not stable dependencies, do not add them touseEffectarrays when synchronizing dialog state
Reuse or extend shared dialog prop types such as admin.types.ts and doorlock.types.ts instead of defining near-duplicate props in each dialog
Keep submit side effects together: mutation success should close the dialog, invalidate the relevant query keys, and surface translated success or failure feedback
New labels, button text, placeholders, and empty states belong int(...)and the locale files, even if older dialogs still have hardcoded strings
Files:
apps/iris/src/components/admin/substitution-dialog.tsx
apps/iris/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/instructions/iris.instructions.md)
apps/iris/src/**/*.{ts,tsx}: Keep user-facing text int(...)and update both locale trees under apps/iris/public/locales/en and apps/iris/public/locales/hu
TanStack Form is the default form pattern; follow examples withuseForm,useStore(form.store, selector), and<form.Field>{(field) => ...}</form.Field>
form.reset(values)takes raw values, not{ values }.form.resetandform.setFieldValueare not stableuseEffectdependencies, so omit them from dependency arrays when needed
Base UI dropdown wrappers useonClick, not Radix-styleonSelect, unless the local component explicitly exposes a different API
apps/iris/src/components/ui/chart.tsx already ownsResponsiveContainer; do not wrap chart children in another one
Keep public timetable filter state in TanStack Router search params instead of duplicating it in unrelated local state
Files:
apps/iris/src/components/admin/substitution-dialog.tsx
🔇 Additional comments (2)
apps/iris/src/components/admin/substitution-dialog.tsx (2)
175-195: LGTM!
351-354: LGTM!
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/iris/src/components/admin/substitution-dialog.tsx (1)
186-186:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd null/undefined guard for
lesson.teachers.The code iterates
lesson.teacherswithout checking if it exists or is an array. If the API returns a parallel lesson without ateachersproperty or withnull/undefined, this will throw a TypeError and break the dialog.🛡️ Proposed fix
for (const teacher of lesson.teachers) { + for (const teacher of lesson.teachers ?? []) { if (!seen.has(teacher.id)) { seen.set(teacher.id, { id: teacher.id, name: teacher.name }); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/iris/src/components/admin/substitution-dialog.tsx` at line 186, The loop "for (const teacher of lesson.teachers)" can throw if lesson.teachers is null/undefined or not an array; update the surrounding code in substitution-dialog.tsx to guard or normalize before iterating (e.g., check Array.isArray(lesson.teachers) and skip the loop when false, or assign const teachers = Array.isArray(lesson.teachers) ? lesson.teachers : [] and iterate over teachers) so parallel lessons without a teachers property no longer cause a TypeError.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/iris/src/components/admin/substitution-dialog.tsx`:
- Line 186: The loop "for (const teacher of lesson.teachers)" can throw if
lesson.teachers is null/undefined or not an array; update the surrounding code
in substitution-dialog.tsx to guard or normalize before iterating (e.g., check
Array.isArray(lesson.teachers) and skip the loop when false, or assign const
teachers = Array.isArray(lesson.teachers) ? lesson.teachers : [] and iterate
over teachers) so parallel lessons without a teachers property no longer cause a
TypeError.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c4c1e61e-e774-4422-bdfa-84a9cdd9b0df
📒 Files selected for processing (1)
apps/iris/src/components/admin/substitution-dialog.tsx
📜 Review details
⏰ 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: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (4)
apps/{chronos,iris}/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/instructions/filc-reuse.instructions.md)
apps/{chronos,iris}/src/**/*.{ts,tsx}: Reuse existing helpers, types, schemas, and hooks before adding new ones. Check nearby feature folders first, then shared files such as apps/chronos/src/database/helpers.ts, apps/iris/src/utils/query-keys.ts, apps/iris/src/hooks/use-has-permission.ts, apps/iris/src/components/admin/admin.types.ts, and apps/iris/src/components/doorlock/doorlock.types.ts
When a second call site needs the same logic, prefer extracting or extending the existing abstraction instead of creating a parallel helper with a slightly different name
Keep abstractions local to the narrowest shared boundary that already exists. Do not create cross-app utilities for one feature-specific use
Extend existing dialog props, response shapes, and query key families instead of re-declaring near-identical types in each file
Prefer the smallest root-cause fix that matches neighboring code over broad rewrites or speculative cleanup
Keep imports on the app alias boundary:#...for Chronos and@/...for Iris
Files:
apps/iris/src/components/admin/substitution-dialog.tsx
apps/iris/src/{routes,components}/**/*.tsx
📄 CodeRabbit inference engine (.github/instructions/iris-data-flow.instructions.md)
apps/iris/src/{routes,components}/**/*.tsx: Always use centralized keys from apps/iris/src/utils/query-keys.ts for React Query. Do not introduce inline array query keys for existing domains
UseparseResponse(...)and the generated Hono client from apps/iris/src/utils/hc.ts for API requests when that is the local pattern
When a mutation changes server state, invalidate every affected query family, not just the page-local list. Follow the multi-invalidation pattern already used in admin news and doorlock screens
Reuse apps/iris/src/hooks/use-has-permission.ts and existing permission guard components instead of duplicating permission logic in the view
New user-facing error and success messages should go throught(...)and the locale files, even when surfaced through toasts
Files:
apps/iris/src/components/admin/substitution-dialog.tsx
apps/iris/src/components/**/*dialog.tsx
📄 CodeRabbit inference engine (.github/instructions/iris-dialog-form.instructions.md)
apps/iris/src/components/**/*dialog.tsx: Follow the dialog structure used in files like card-dialog.tsx and user-dialog.tsx: create the form near the top of the component, derive reactive slices withuseStore(form.store, selector), and render fields with<form.Field>{(field) => ...}</form.Field>
Reuse validation schemas from apps/iris/src/utils/form-schemas.ts when available. If a schema becomes shared by multiple dialogs, move it there instead of copying validation logic
form.resettakes raw values, not{ values: ... }. Becauseform.resetandform.setFieldValueare not stable dependencies, do not add them touseEffectarrays when synchronizing dialog state
Reuse or extend shared dialog prop types such as admin.types.ts and doorlock.types.ts instead of defining near-duplicate props in each dialog
Keep submit side effects together: mutation success should close the dialog, invalidate the relevant query keys, and surface translated success or failure feedback
New labels, button text, placeholders, and empty states belong int(...)and the locale files, even if older dialogs still have hardcoded strings
Files:
apps/iris/src/components/admin/substitution-dialog.tsx
apps/iris/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/instructions/iris.instructions.md)
apps/iris/src/**/*.{ts,tsx}: Keep user-facing text int(...)and update both locale trees under apps/iris/public/locales/en and apps/iris/public/locales/hu
TanStack Form is the default form pattern; follow examples withuseForm,useStore(form.store, selector), and<form.Field>{(field) => ...}</form.Field>
form.reset(values)takes raw values, not{ values }.form.resetandform.setFieldValueare not stableuseEffectdependencies, so omit them from dependency arrays when needed
Base UI dropdown wrappers useonClick, not Radix-styleonSelect, unless the local component explicitly exposes a different API
apps/iris/src/components/ui/chart.tsx already ownsResponsiveContainer; do not wrap chart children in another one
Keep public timetable filter state in TanStack Router search params instead of duplicating it in unrelated local state
Files:
apps/iris/src/components/admin/substitution-dialog.tsx
🔇 Additional comments (3)
apps/iris/src/components/admin/substitution-dialog.tsx (3)
9-9: LGTM!
104-105: LGTM!
343-346: LGTM!
2 participants
Summary by CodeRabbit