Conversation
📝 Walkthrough
WalkthroughThe substitution-candidate logic in ChangesH1/H2-Based Substitution Candidate Logic
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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. 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: 3
🤖 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/chronos/src/routes/timetable/lesson.ts`:
- Around line 787-793: The H2 tie-breaker logic is currently nested inside the
condition that checks if neither candidate has H1, which causes the H2
comparison to be skipped when both candidates have H1. Move the H2 comparison
checks (the conditions checking a.hasH2 and b.hasH2) outside and after the H1
comparison block so that H2 acts as a tie-breaker that applies whenever H1
values are equal, regardless of whether they are both true or both false. This
ensures the comparator properly sorts by H1 first, then H2 as the next sorting
criterion.
- Around line 749-763: The conflict detection logic in the lesson.ts route
currently only triggers when there are NO H1/H2 lessons at a period, but it
should trigger when there ARE any non-H1/H2 lessons present. Replace the
conflict check condition from checking if neither hasH1AtPeriod nor
hasH2AtPeriod exist, to instead checking if any lesson in lessonsAtPeriod has a
subjectShort that is neither H1 nor H2, so that conflicts are properly detected
when real lessons coexist with H1/H2 placeholders at the same period.
In `@apps/iris/src/components/admin/substitution-dialog.tsx`:
- Around line 229-236: The sorting logic in the substitution-dialog.tsx file
incorrectly restricts the hasH2 tie-breaker comparison to only execute when
neither option has hasH1. This causes candidates with hasH1=true to be sorted
alphabetically instead of using hasH2 as a secondary sort criterion. Move the
hasH2 comparison logic (the if statements checking hasH2 values) outside of the
outer if statement that checks !(a.hasH1 || b.hasH1), so that hasH2 is always
evaluated as a secondary sort criterion regardless of the hasH1 values.
🪄 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: 3a8bab14-b85b-462e-be47-caf881ffcba8
📒 Files selected for processing (4)
apps/chronos/src/routes/timetable/lesson.tsapps/iris/public/locales/en/translation.jsonapps/iris/public/locales/hu/translation.jsonapps/iris/src/components/admin/substitution-dialog.tsx
📜 Review details
🧰 Additional context used
🪛 GitHub Actions: Code Quality / 0_lint.txt
apps/iris/src/components/admin/substitution-dialog.tsx
[error] 222-224: Biome lint (lint/complexity/noExcessiveCognitiveComplexity): Excessive complexity of 22 detected (max: 15). Please refactor this function to reduce its complexity score from 22 to the max allowed complexity 15.
apps/chronos/src/routes/timetable/lesson.ts
[error] 583-585: Biome lint (lint/complexity/noExcessiveCognitiveComplexity): Excessive complexity of 16 detected (max: 15). Please refactor this function to reduce its complexity score from 16 to the max allowed complexity 15.
[error] 731-733: Biome lint (lint/complexity/noExcessiveCognitiveComplexity): Excessive complexity of 18 detected (max: 15). Please refactor this function to reduce its complexity score from 18 to the max allowed complexity 15.
[error] 756-756: Biome lint (lint/style/useBlockStatements) FIXABLE: Block statements are preferred in this position. Unsafe fix: Wrap the statement with a JsBlockStatement.
[error] 757-757: Biome lint (lint/style/useBlockStatements) FIXABLE: Block statements are preferred in this position. Unsafe fix: Wrap the statement with a JsBlockStatement.
[error] 780-782: Biome lint (lint/complexity/noExcessiveCognitiveComplexity): Excessive complexity of 17 detected (max: 15). Please refactor this function to reduce its complexity score from 17 to the max allowed complexity 15.
🪛 GitHub Actions: Code Quality / lint
apps/iris/src/components/admin/substitution-dialog.tsx
[error] 222-222: Biome (lint/complexity/noExcessiveCognitiveComplexity): Excessive complexity of 22 detected (max: 15). Consider refactoring to reduce complexity.
apps/chronos/src/routes/timetable/lesson.ts
[error] 583-583: Biome (lint/complexity/noExcessiveCognitiveComplexity): Excessive complexity of 16 detected (max: 15). Consider refactoring to reduce complexity.
[error] 731-731: Biome (lint/complexity/noExcessiveCognitiveComplexity): Excessive complexity of 18 detected (max: 15). Consider refactoring to reduce complexity.
[error] 756-757: Biome (lint/style/useBlockStatements) FIXABLE: Block statements are preferred in this position. Unsafe fix: wrap the statement with a JsBlockStatement.
[error] 757-758: Biome (lint/style/useBlockStatements) FIXABLE: Block statements are preferred in this position. Unsafe fix: wrap the statement with a JsBlockStatement.
[error] 780-780: Biome (lint/complexity/noExcessiveCognitiveComplexity): Excessive complexity of 17 detected (max: 15). Consider refactoring to reduce complexity.
🔇 Additional comments (2)
apps/iris/public/locales/en/translation.json (1)
246-247: LGTM!apps/iris/public/locales/hu/translation.json (1)
246-247: LGTM!
| const hasH1AtPeriod = lessonsAtPeriod.some( | ||
| (l) => l.subjectShort === 'H1' | ||
| ); | ||
| const hasH2AtPeriod = lessonsAtPeriod.some( | ||
| (l) => l.subjectShort === 'H2' | ||
| ); | ||
|
|
||
| if (hasH1AtPeriod) hasH1 = true; | ||
| if (hasH2AtPeriod) hasH2 = true; | ||
|
|
||
| // If none of the lessons at this period are H1 or H2, it's a real conflict | ||
| if (!(hasH1AtPeriod || hasH2AtPeriod)) { | ||
| hasConflict = true; | ||
| break; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Conflict check lets busy teachers pass when H1/H2 coexists with other lessons.
A candidate is currently accepted if any lesson at the selected period is H1/H2, even when another lesson at the same period is a real conflict. Conflict should trigger when any lesson at that period is not H1/H2.
Suggested fix
const hasH1AtPeriod = lessonsAtPeriod.some(
(l) => l.subjectShort === 'H1'
);
const hasH2AtPeriod = lessonsAtPeriod.some(
(l) => l.subjectShort === 'H2'
);
if (hasH1AtPeriod) hasH1 = true;
if (hasH2AtPeriod) hasH2 = true;
- // If none of the lessons at this period are H1 or H2, it's a real conflict
- if (!(hasH1AtPeriod || hasH2AtPeriod)) {
+ const hasNonH1H2AtPeriod = lessonsAtPeriod.some(
+ (l) => l.subjectShort !== 'H1' && l.subjectShort !== 'H2'
+ );
+ if (hasNonH1H2AtPeriod) {
hasConflict = true;
break;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const hasH1AtPeriod = lessonsAtPeriod.some( | |
| (l) => l.subjectShort === 'H1' | |
| ); | |
| const hasH2AtPeriod = lessonsAtPeriod.some( | |
| (l) => l.subjectShort === 'H2' | |
| ); | |
| if (hasH1AtPeriod) hasH1 = true; | |
| if (hasH2AtPeriod) hasH2 = true; | |
| // If none of the lessons at this period are H1 or H2, it's a real conflict | |
| if (!(hasH1AtPeriod || hasH2AtPeriod)) { | |
| hasConflict = true; | |
| break; | |
| } | |
| const hasH1AtPeriod = lessonsAtPeriod.some( | |
| (l) => l.subjectShort === 'H1' | |
| ); | |
| const hasH2AtPeriod = lessonsAtPeriod.some( | |
| (l) => l.subjectShort === 'H2' | |
| ); | |
| if (hasH1AtPeriod) hasH1 = true; | |
| if (hasH2AtPeriod) hasH2 = true; | |
| const hasNonH1H2AtPeriod = lessonsAtPeriod.some( | |
| (l) => l.subjectShort !== 'H1' && l.subjectShort !== 'H2' | |
| ); | |
| if (hasNonH1H2AtPeriod) { | |
| hasConflict = true; | |
| break; | |
| } |
🧰 Tools
🪛 GitHub Actions: Code Quality / 0_lint.txt
[error] 756-756: Biome lint (lint/style/useBlockStatements) FIXABLE: Block statements are preferred in this position. Unsafe fix: Wrap the statement with a JsBlockStatement.
[error] 757-757: Biome lint (lint/style/useBlockStatements) FIXABLE: Block statements are preferred in this position. Unsafe fix: Wrap the statement with a JsBlockStatement.
🪛 GitHub Actions: Code Quality / lint
[error] 756-757: Biome (lint/style/useBlockStatements) FIXABLE: Block statements are preferred in this position. Unsafe fix: wrap the statement with a JsBlockStatement.
[error] 757-758: Biome (lint/style/useBlockStatements) FIXABLE: Block statements are preferred in this position. Unsafe fix: wrap the statement with a JsBlockStatement.
🤖 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/chronos/src/routes/timetable/lesson.ts` around lines 749 - 763, The
conflict detection logic in the lesson.ts route currently only triggers when
there are NO H1/H2 lessons at a period, but it should trigger when there ARE any
non-H1/H2 lessons present. Replace the conflict check condition from checking if
neither hasH1AtPeriod nor hasH2AtPeriod exist, to instead checking if any lesson
in lessonsAtPeriod has a subjectShort that is neither H1 nor H2, so that
conflicts are properly detected when real lessons coexist with H1/H2
placeholders at the same period.
| if (!(a.hasH1 || b.hasH1)) { | ||
| if (a.hasH2 && !b.hasH2) { | ||
| return -1; | ||
| } | ||
| if (!a.hasH2 && b.hasH2) { | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
H2 tie-break is skipped when both candidates already have H1.
The current comparator only checks hasH2 when neither side has hasH1. That breaks “sort by hasH1, then hasH2” for the (true, true) H1 case.
Suggested fix
- if (!(a.hasH1 || b.hasH1)) {
- if (a.hasH2 && !b.hasH2) {
- return -1;
- }
- if (!a.hasH2 && b.hasH2) {
- return 1;
- }
- }
+ if (a.hasH2 && !b.hasH2) {
+ return -1;
+ }
+ if (!a.hasH2 && b.hasH2) {
+ return 1;
+ }🤖 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/chronos/src/routes/timetable/lesson.ts` around lines 787 - 793, The H2
tie-breaker logic is currently nested inside the condition that checks if
neither candidate has H1, which causes the H2 comparison to be skipped when both
candidates have H1. Move the H2 comparison checks (the conditions checking
a.hasH2 and b.hasH2) outside and after the H1 comparison block so that H2 acts
as a tie-breaker that applies whenever H1 values are equal, regardless of
whether they are both true or both false. This ensures the comparator properly
sorts by H1 first, then H2 as the next sorting criterion.
| if (!(a.hasH1 || b.hasH1)) { | ||
| if (a.hasH2 && !b.hasH2) { | ||
| return -1; | ||
| } | ||
| if (!a.hasH2 && b.hasH2) { | ||
| return 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
UI sort has the same H2 tie-break gap as backend.
hasH2 is only considered when neither option has hasH1, so candidates with hasH1=true are incorrectly sorted alphabetically instead of by hasH2 second.
Suggested fix
- if (!(a.hasH1 || b.hasH1)) {
- if (a.hasH2 && !b.hasH2) {
- return -1;
- }
- if (!a.hasH2 && b.hasH2) {
- return 1;
- }
- }
+ if (a.hasH2 && !b.hasH2) {
+ return -1;
+ }
+ if (!a.hasH2 && b.hasH2) {
+ return 1;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!(a.hasH1 || b.hasH1)) { | |
| if (a.hasH2 && !b.hasH2) { | |
| return -1; | |
| } | |
| if (!a.hasH2 && b.hasH2) { | |
| return 1; | |
| } | |
| } | |
| if (a.hasH2 && !b.hasH2) { | |
| return -1; | |
| } | |
| if (!a.hasH2 && b.hasH2) { | |
| return 1; | |
| } |
🤖 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` around lines 229 -
236, The sorting logic in the substitution-dialog.tsx file incorrectly restricts
the hasH2 tie-breaker comparison to only execute when neither option has hasH1.
This causes candidates with hasH1=true to be sorted alphabetically instead of
using hasH2 as a secondary sort criterion. Move the hasH2 comparison logic (the
if statements checking hasH2 values) outside of the outer if statement that
checks !(a.hasH1 || b.hasH1), so that hasH2 is always evaluated as a secondary
sort criterion regardless of the hasH1 values.
1 participant
This pull request updates the substitute teacher selection logic to prioritize teachers with "H1" and "H2" lessons, improves the candidate sorting and display, and adds new translation keys. The changes enhance how substitution candidates are filtered, ranked, and labeled, making the selection process more transparent and flexible.
Substitute Candidate Logic and Data Structure Updates:
hasH1,hasH2) during the selected periods, replacing the previoushasLessonBeforeOrAfterboolean. This allows more nuanced filtering and prioritization of substitute teachers. [1] [2] [3]Sorting and Display Enhancements:
Translation Updates:
closes #232 and #209