Substitution page filter and UI fixes#218
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 (2)
💤 Files with no reviewable changes (1)
📜 Recent review details🔇 Additional comments (1)
📝 WalkthroughWalkthroughAdds a filter bar and filtering helpers to SubstitutionView with per-cohort card splitting; extends SubsV to accept cohort/date props and apply cohort-scoped lesson filtering; sorts substitute-teacher options (nearby first, then locale-sorted) and sorts available lessons by period; updates timetable query key shape and usages; adds clearFilter translations. ChangesSubstitution View Filtering and Per-Cohort Display
Substitution Admin Dialog Sorting
Query key shape and usages
Localization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 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: 4
🤖 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`:
- Around line 209-221: The sorting logic in sortedSubstituteOptions currently
inspects rendered text via label.includes('H1'), which breaks with translations;
change it to sort using a stable flag on the option objects (e.g., option.isH1
or option.priority on each substituteOptions entry) first, then fall back to
localeCompare on label for ties; update the comparator in useMemo to use that
flag (e.g., compare a.isH1 vs b.isH1 or a.priority vs b.priority) instead of
checking label.includes('H1') while keeping the final localeCompare for
alphabetical order.
In `@apps/iris/src/components/subs-view.tsx`:
- Around line 443-445: Replace the inline query key array
['timetables','latestValid'] used in subs-view.tsx with a centralized key from
query-keys.ts: add a new timetables.latestValid function that returns the tuple
['timetables','latestValid'] as const (matching the existing timetables keys
shape) and then update the queryKey in the hook in components/subs-view.tsx to
call queryKeys.timetables.latestValid(); this keeps key generation consistent
with other keys and enables proper cache invalidation.
- Around line 393-401: Replace the hardcoded aria-label on the Button with a
localized string using the i18n `t(...)` function (e.g., use t('clearFilter')
for the Button with onClear and XIcon) so accessibility labels are localized;
then add the key "clearFilter" to both locale trees (English and Hungarian) with
values "Clear filter" and "Szűrő törlése" respectively. Ensure the component
imports/has access to the translation hook/namespace used in this file and
update any tests or snapshots that assert the aria-label if present.
In `@apps/iris/src/components/subs.tsx`:
- Around line 267-274: The badge count is inconsistent: when cohortFilter is set
you count matching lessons, but when unset you count substitutions with any
lesson; update the fallback branch so filteredLessonCount always counts lessons
(non-null) across all subs. Modify the ternary on filteredLessonCount to, when
!cohortFilter, compute data.flatMap(sub => sub.lessons.filter((l): l is Lesson
=> l != null)).length so both branches consistently count individual lessons
(use the same sub.lessons filter predicate as the cohort branch).
🪄 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: b961e057-ae4a-467c-a7f9-cc79230b769e
📒 Files selected for processing (3)
apps/iris/src/components/admin/substitution-dialog.tsxapps/iris/src/components/subs-view.tsxapps/iris/src/components/subs.tsx
📜 Review details
🧰 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.tsxapps/iris/src/components/subs.tsxapps/iris/src/components/subs-view.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.tsxapps/iris/src/components/subs.tsxapps/iris/src/components/subs-view.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.tsxapps/iris/src/components/subs.tsxapps/iris/src/components/subs-view.tsx
🔇 Additional comments (3)
apps/iris/src/components/admin/substitution-dialog.tsx (1)
318-338: LGTM!Also applies to: 366-367
apps/iris/src/components/subs.tsx (1)
16-23: LGTM!Also applies to: 229-234, 254-265, 280-281, 289-289, 296-300, 306-306, 336-339
apps/iris/src/components/subs-view.tsx (1)
55-60: LGTM!Also applies to: 88-172, 174-248, 251-363, 365-406, 410-419, 421-492, 518-559, 561-593, 608-630, 648-651
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 (2)
apps/iris/src/routes/_private/admin/timetable/manage.tsx (1)
163-180: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winCentralize the preview-delete query key under
queryKeys.timetables.Line 179 still uses an inline timetable-domain key; this should live in
apps/iris/src/utils/query-keys.tslike the other timetable keys.Suggested fix
--- a/apps/iris/src/utils/query-keys.ts +++ b/apps/iris/src/utils/query-keys.ts @@ timetables: { all: () => ['timetables'] as const, latestValid: () => ['timetables', 'latestValid'] as const, + previewDelete: (id: string | undefined) => + ['timetables', 'preview-delete', id] as const, },--- a/apps/iris/src/routes/_private/admin/timetable/manage.tsx +++ b/apps/iris/src/routes/_private/admin/timetable/manage.tsx @@ - queryKey: ['timetables', 'preview-delete', itemToDelete?.id] as const, + queryKey: queryKeys.timetables.previewDelete(itemToDelete?.id), });As per coding guidelines, "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".
🤖 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/routes/_private/admin/timetable/manage.tsx` around lines 163 - 180, Replace the inline React Query key used in previewQuery with the centralized key from queryKeys.timetables: import queryKeys and change the queryKey from ['timetables','preview-delete', itemToDelete?.id] to the corresponding helper on queryKeys.timetables (e.g., queryKeys.timetables.previewDelete(itemToDelete?.id) or whatever preview-delete helper exists); keep the rest of the previewQuery (queryFn, enabled) unchanged and ensure the import of queryKeys is added at the top of the file.apps/iris/src/components/timetable/index.tsx (1)
131-150: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse the centralized
latestValidquery key helper in this block.Line 149 still inlines the key, so this migration is only partial and can drift from
queryKeys.Suggested fix
const latestValidTimetableQuery = useQuery({ ...QUERY_OPTIONS, @@ - queryKey: ['timetables', 'latestValid'] as const, + queryKey: queryKeys.timetables.latestValid(), });As per coding guidelines, "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".
🤖 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/timetable/index.tsx` around lines 131 - 150, Replace the inline queryKey array used in latestValidTimetableQuery with the centralized helper from query-keys; specifically, change the queryKey in the useQuery that creates latestValidTimetableQuery to use queryKeys.timetables.latestValid() (or the appropriate latestValid helper) instead of ['timetables', 'latestValid'] so the key is consistent with the rest of the codebase.
🤖 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/public/locales/hu/translation.json`:
- Line 175: The Hungarian translation for the "clearFilter" key contains a typo
("FIlterek törlése"); update the value for clearFilter in translation.json to
use correct casing ("Filterek törlése") by editing the "clearFilter" entry so
the UI/ARIA strings display the corrected word.
---
Outside diff comments:
In `@apps/iris/src/components/timetable/index.tsx`:
- Around line 131-150: Replace the inline queryKey array used in
latestValidTimetableQuery with the centralized helper from query-keys;
specifically, change the queryKey in the useQuery that creates
latestValidTimetableQuery to use queryKeys.timetables.latestValid() (or the
appropriate latestValid helper) instead of ['timetables', 'latestValid'] so the
key is consistent with the rest of the codebase.
In `@apps/iris/src/routes/_private/admin/timetable/manage.tsx`:
- Around line 163-180: Replace the inline React Query key used in previewQuery
with the centralized key from queryKeys.timetables: import queryKeys and change
the queryKey from ['timetables','preview-delete', itemToDelete?.id] to the
corresponding helper on queryKeys.timetables (e.g.,
queryKeys.timetables.previewDelete(itemToDelete?.id) or whatever preview-delete
helper exists); keep the rest of the previewQuery (queryFn, enabled) unchanged
and ensure the import of queryKeys is added at the top of the file.
🪄 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: 773af6a2-680f-4b6d-be36-201ce49643e2
📒 Files selected for processing (9)
apps/iris/public/locales/en/translation.jsonapps/iris/public/locales/hu/translation.jsonapps/iris/src/components/admin/substitution-dialog.tsxapps/iris/src/components/admin/timetable-import-dialog.tsxapps/iris/src/components/subs-view.tsxapps/iris/src/components/subs.tsxapps/iris/src/components/timetable/index.tsxapps/iris/src/routes/_private/admin/timetable/manage.tsxapps/iris/src/utils/query-keys.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
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/timetable-import-dialog.tsxapps/iris/src/utils/query-keys.tsapps/iris/src/components/timetable/index.tsxapps/iris/src/routes/_private/admin/timetable/manage.tsxapps/iris/src/components/admin/substitution-dialog.tsxapps/iris/src/components/subs-view.tsxapps/iris/src/components/subs.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/timetable-import-dialog.tsxapps/iris/src/components/timetable/index.tsxapps/iris/src/routes/_private/admin/timetable/manage.tsxapps/iris/src/components/admin/substitution-dialog.tsxapps/iris/src/components/subs-view.tsxapps/iris/src/components/subs.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/timetable-import-dialog.tsxapps/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/timetable-import-dialog.tsxapps/iris/src/utils/query-keys.tsapps/iris/src/components/timetable/index.tsxapps/iris/src/routes/_private/admin/timetable/manage.tsxapps/iris/src/components/admin/substitution-dialog.tsxapps/iris/src/components/subs-view.tsxapps/iris/src/components/subs.tsx
apps/iris/src/routes/**/*.tsx
📄 CodeRabbit inference engine (.github/instructions/iris-data-flow.instructions.md)
apps/iris/src/routes/**/*.tsx: Follow route composition patterns from apps/iris/src/routes/_private/admin/news/system-messages.tsx: usecreateFileRoute(...)at the top, then permission gating, then queries and mutations grouped near the component that owns them
Keep search-param-driven page state in TanStack Router when the page already uses it for filters or selection. Do not fork that state into unrelated local state
Files:
apps/iris/src/routes/_private/admin/timetable/manage.tsx
apps/iris/src/routes/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/instructions/iris.instructions.md)
Treat apps/iris/src/route-tree.gen.ts as generated; change source files under apps/iris/src/routes instead
Files:
apps/iris/src/routes/_private/admin/timetable/manage.tsx
🔇 Additional comments (7)
apps/iris/src/components/admin/substitution-dialog.tsx (1)
200-207: LGTM!Also applies to: 210-220
apps/iris/public/locales/en/translation.json (1)
185-186: LGTM!apps/iris/src/components/subs-view.tsx (1)
393-395: LGTM!Also applies to: 421-445
apps/iris/src/components/subs.tsx (1)
267-274: LGTM!apps/iris/src/utils/query-keys.ts (1)
96-99: LGTM!apps/iris/src/routes/_private/admin/timetable/manage.tsx (1)
160-161: LGTM!Also applies to: 208-230
apps/iris/src/components/admin/timetable-import-dialog.tsx (1)
87-90: LGTM!
KoZsombat
dismissed
coderabbitai[bot]’s stale review
The merge-base changed after approval.
2 participants
Summary by CodeRabbit
New Features
Improvements
Bug Fixes