Fix/final UI fixes#73
Merged
Merged
Conversation
Contributor
WalkthroughMultiple pages (Faculty, ScheduleList, Schedules) are updated to load semester metadata on mount and display human-readable season/year labels instead of numeric semester IDs. SectionMutationDrawer's TimeSelect component now maintains local draft state for hour/minute selections and adds a "Standard only" toggle for filtering time block options. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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}
Contributor
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)
frontend/src/pages/Schedules.tsx (1)
61-80:⚠️ Potential issue | 🟡 MinorStale
semesterLabel/ race onscheduleIdchange.Two related concerns with the new semester fetch:
- No reset on
scheduleIdchange —schedule,campusName, and nowsemesterLabelare never cleared whenscheduleIdchanges. While the new schedule is loading, the header will keep rendering the previous schedule's semester label (and campus name). The{schedule && semesterLabel && …}guard doesn't help because both values remain truthy from the prior schedule.- Unguarded async chain — if the user navigates between schedules quickly, the inner
getSemesterSemestersSemesterIdGetfrom an earlier run can resolve after the newer run and overwritesemesterLabelwith the previous semester. Same class of race applies tosetSchedule/setCampusName, butsemesterLabelis new here.A cancellation flag in the effect addresses both:
🛠 Proposed fix
useEffect(() => { + let cancelled = false; + setSchedule(null); + setSemesterLabel(null); + setCampusName(null); const api = getAutomatedCourseSchedulerAPI(); api.getScheduleSchedulesScheduleIdGet(scheduleId) .then((s) => { + if (cancelled) return; setSchedule(s); api.getSemesterSemestersSemesterIdGet(s.semester_id) - .then((sem) => setSemesterLabel(`${sem.season} ${sem.year}`)) + .then((sem) => { if (!cancelled) setSemesterLabel(`${sem.season} ${sem.year}`); }) .catch(() => {}); // Resolve campus name for drawer filtering return api.getAllCampusesCampusesGet().then((campuses) => { + if (cancelled) return; const match = campuses.find((c) => c.campus_id === s.campus); if (match) setCampusName(match.name); }); }) .catch(() => {}); + return () => { cancelled = true; }; }, [scheduleId]);Also applies to: 135-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/Schedules.tsx` around lines 61 - 80, When scheduleId changes you must clear previous state and guard async callbacks to avoid races: inside the useEffect that calls getAutomatedCourseSchedulerAPI()/getScheduleSchedulesScheduleIdGet(scheduleId) reset setSchedule(null), setCampusName(null), and setSemesterLabel(null) immediately, then use a local cancellation flag (e.g., let cancelled = false; and a cleanup that sets it true) before running each async .then/.catch chain; after each awaited API response check if cancelled is false before calling setSchedule, setCampusName, or setSemesterLabel (references: useEffect, scheduleId, getAutomatedCourseSchedulerAPI, getScheduleSchedulesScheduleIdGet, getSemesterSemestersSemesterIdGet, getAllCampusesCampusesGet, setSchedule, setCampusName, setSemesterLabel).
🧹 Nitpick comments (3)
frontend/src/pages/Schedules.test.tsx (1)
89-91: Consider adding coverage for the new semester label render.The mock is wired correctly, but none of the existing assertions actually exercise the new
semesterLabel<p>added inSchedules.tsx(line 136). Becauseschedule.nameis also'Fall 2025'in the mock,findByRole('heading', { name: 'Fall 2025' })would pass even if the semester label weren't rendered. An explicit assertion (e.g. targeting the sub-paragraph with a different season/year than the schedule name) would guard the new behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/Schedules.test.tsx` around lines 89 - 91, Add an explicit assertion in the Schedules.test.tsx to ensure the new semester label paragraph renders: after the existing mock for getSemesterSemestersSemesterIdGet and rendering of the Schedules component, query for the semester label element (the <p> rendered from semesterLabel in Schedules.tsx) and assert its text content matches the semester mock (e.g., "Fall 2025") separately from the heading assertion; this can be done by using getByText or findByText targeting the paragraph string or by querying a role/aria-label if you add one to the paragraph, referencing the getSemesterSemestersSemesterIdGet mock and the semesterLabel output to locate the code to assert.frontend/src/components/SectionMutationDrawer.tsx (2)
120-135: JSDoc block is misplaced — it describesTimeSelect, notparseTimeValue.The doc comment at lines 120–125 (“Compound AM/PM time selector…”) sits directly above
parseTimeValuedue to the refactor, so tooling/hover docs will attach it to the wrong symbol. Move it back aboveTimeSelectand add a brief one-liner forparseTimeValue.♻️ Proposed fix
-/** - * Compound AM/PM time selector — three linked selects for hour, minute, period. - * Reports the chosen time as a "HH:MM" 24-hour string via `onChange`, but only - * once hour and minute are both set. Local state retains partial selections so - * each dropdown "sticks" as the user works through them. - */ +/** Parse a "HH:MM" 24-hour string into the {hour, minute, period} draft shape. */ function parseTimeValue(value: string): { hour: string; minute: string; period: 'AM' | 'PM' } { ... } +/** + * Compound AM/PM time selector — three linked selects for hour, minute, period. + * Reports the chosen time as a "HH:MM" 24-hour string via `onChange`, but only + * once hour and minute are both set. Local state retains partial selections so + * each dropdown "sticks" as the user works through them. + */ function TimeSelect({ value, onChange }: { value: string; onChange: (v: string) => void }) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/SectionMutationDrawer.tsx` around lines 120 - 135, The JSDoc describing the compound AM/PM selector is currently above parseTimeValue; move that multi-line comment so it sits immediately above the TimeSelect component (symbol TimeSelect) and add a short one-line JSDoc summary above parseTimeValue (symbol parseTimeValue) such as "Parse a 'HH:MM' 24-hour string into hour, minute, and AM/PM period." Ensure the new placement preserves existing wording for TimeSelect and the one-liner succinctly describes parseTimeValue's return shape.
670-712: Filter logic looks correct; minor DRY opportunity.The
showOnlyStandardbranch correctly preserves (a) the currently-selected block and (b) every split-block half (block_group != null), so toggling the checkbox can't hide the user's selection or orphan a split pair. Sorting bystart_timebefore the partner lookup is also the right order so the earlier half of a pair drives the label.Nit: the inline
STANDARD_SEQUENCES.some(...)at lines 678–680 duplicates the exact predicate already encapsulated infindSequenceName. You could reuse it:♻️ Proposed fix
- if (tb.block_group != null) return true; - return STANDARD_SEQUENCES.some( - (s) => s.days === tb.days && s.start === tb.start_time && s.end === tb.end_time, - ); + if (tb.block_group != null) return true; + return findSequenceName(tb.days, tb.start_time, tb.end_time) !== null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/SectionMutationDrawer.tsx` around lines 670 - 712, The filter duplicates the sequence-matching predicate; in the useMemo for timeBlockOptions (the showOnlyStandard branch) replace the inline STANDARD_SEQUENCES.some(...) test with a call to the existing sequence helper (e.g., use findSequenceName or the function that returns the sequence name) and check its truthiness so you reuse that logic and avoid duplication while keeping the existing behavior of preserving the selected block and block_group items.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/SectionMutationDrawer.tsx`:
- Around line 145-148: The update function currently only calls onChange when
both next.hour and next.minute are truthy, so clearing either select leaves the
parent with the previous complete time; modify update (in
SectionMutationDrawer.tsx) to call onChange with an empty string (or null if
your app prefers) whenever the draft transitions from a complete state to an
incomplete state, and only call onChange(to24Hour(...)) when both next.hour and
next.minute are present; use the existing setDraft, onChange, and to24Hour
identifiers to implement this guard/emit-empty behavior so the parent reflects
cleared selections immediately.
---
Outside diff comments:
In `@frontend/src/pages/Schedules.tsx`:
- Around line 61-80: When scheduleId changes you must clear previous state and
guard async callbacks to avoid races: inside the useEffect that calls
getAutomatedCourseSchedulerAPI()/getScheduleSchedulesScheduleIdGet(scheduleId)
reset setSchedule(null), setCampusName(null), and setSemesterLabel(null)
immediately, then use a local cancellation flag (e.g., let cancelled = false;
and a cleanup that sets it true) before running each async .then/.catch chain;
after each awaited API response check if cancelled is false before calling
setSchedule, setCampusName, or setSemesterLabel (references: useEffect,
scheduleId, getAutomatedCourseSchedulerAPI, getScheduleSchedulesScheduleIdGet,
getSemesterSemestersSemesterIdGet, getAllCampusesCampusesGet, setSchedule,
setCampusName, setSemesterLabel).
---
Nitpick comments:
In `@frontend/src/components/SectionMutationDrawer.tsx`:
- Around line 120-135: The JSDoc describing the compound AM/PM selector is
currently above parseTimeValue; move that multi-line comment so it sits
immediately above the TimeSelect component (symbol TimeSelect) and add a short
one-line JSDoc summary above parseTimeValue (symbol parseTimeValue) such as
"Parse a 'HH:MM' 24-hour string into hour, minute, and AM/PM period." Ensure the
new placement preserves existing wording for TimeSelect and the one-liner
succinctly describes parseTimeValue's return shape.
- Around line 670-712: The filter duplicates the sequence-matching predicate; in
the useMemo for timeBlockOptions (the showOnlyStandard branch) replace the
inline STANDARD_SEQUENCES.some(...) test with a call to the existing sequence
helper (e.g., use findSequenceName or the function that returns the sequence
name) and check its truthiness so you reuse that logic and avoid duplication
while keeping the existing behavior of preserving the selected block and
block_group items.
In `@frontend/src/pages/Schedules.test.tsx`:
- Around line 89-91: Add an explicit assertion in the Schedules.test.tsx to
ensure the new semester label paragraph renders: after the existing mock for
getSemesterSemestersSemesterIdGet and rendering of the Schedules component,
query for the semester label element (the <p> rendered from semesterLabel in
Schedules.tsx) and assert its text content matches the semester mock (e.g.,
"Fall 2025") separately from the heading assertion; this can be done by using
getByText or findByText targeting the paragraph string or by querying a
role/aria-label if you add one to the paragraph, referencing the
getSemesterSemestersSemesterIdGet mock and the semesterLabel output to locate
the code to assert.
🪄 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: CHILL
Plan: Pro
Run ID: fe5b574a-c3b3-4efc-9af5-640380510558
📒 Files selected for processing (6)
frontend/src/components/SectionMutationDrawer.tsxfrontend/src/pages/Faculty.test.tsxfrontend/src/pages/Faculty.tsxfrontend/src/pages/ScheduleList.tsxfrontend/src/pages/Schedules.test.tsxfrontend/src/pages/Schedules.tsx
Comment on lines
+145
to
148
| function update(next: { hour: string; minute: string; period: 'AM' | 'PM' }) { | ||
| setDraft(next); | ||
| if (next.hour && next.minute) onChange(to24Hour(next.hour, next.minute, next.period)); | ||
| } |
Contributor
There was a problem hiding this comment.
Clearing a completed selection leaves stale time in parent.
update only calls onChange when both next.hour and next.minute are truthy. If a user has already committed a valid time (e.g. 08:30) and then reopens the Hour or Minute select and picks the placeholder (""), the draft clears visually but the parent state retains the old 08:30. Submitting the form then persists a value the user believes they cleared.
Consider emitting an empty string to the parent when the draft transitions from complete → incomplete, or simply guarding the complete-case:
🛡️ Suggested handling
function update(next: { hour: string; minute: string; period: 'AM' | 'PM' }) {
setDraft(next);
- if (next.hour && next.minute) onChange(to24Hour(next.hour, next.minute, next.period));
+ if (next.hour && next.minute) {
+ onChange(to24Hour(next.hour, next.minute, next.period));
+ } else if (value) {
+ // User cleared a previously-complete selection — reflect that upstream.
+ onChange('');
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/SectionMutationDrawer.tsx` around lines 145 - 148,
The update function currently only calls onChange when both next.hour and
next.minute are truthy, so clearing either select leaves the parent with the
previous complete time; modify update (in SectionMutationDrawer.tsx) to call
onChange with an empty string (or null if your app prefers) whenever the draft
transitions from a complete state to an incomplete state, and only call
onChange(to24Hour(...)) when both next.hour and next.minute are present; use the
existing setDraft, onChange, and to24Hour identifiers to implement this
guard/emit-empty behavior so the parent reflects cleared selections immediately.
Saisri24
approved these changes
Apr 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Time block creation/selection and semester name display
Type of Change
What Changed?
Testing & Validation
How this was tested
Screenshots/Recordings
Unfinished Work & Known Issues
Notes & Nuances
Reviewer Notes
Summary by CodeRabbit
Release Notes
New Features
Tests