Skip to content

Fix/final UI fixes #73

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
BeWelsh merged 2 commits into from
Apr 24, 2026
Merged

Fix/final UI fixes #73

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
444ecb7
fix time block creation
BeWelsh Apr 24, 2026
607c388
use semester name rather than id in frontend
BeWelsh Apr 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
126 changes: 84 additions & 42 deletions frontend/src/components/SectionMutationDrawer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,37 +119,67 @@ function generateBlockGroup(): string {

/**
* 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`.
* 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 parseTimeValue(value: string): { hour: string; minute: string; period: 'AM' | 'PM' } {
if (!value) return { hour: '', minute: '', period: 'AM' };
const [h24, m] = value.split(':');
const hNum = Number(h24);
return {
hour: Number.isFinite(hNum) ? String(hNum % 12 || 12) : '',
minute: m ?? '',
period: Number.isFinite(hNum) && hNum >= 12 ? 'PM' : 'AM',
};
}

function TimeSelect({ value, onChange }: { value: string; onChange: (v: string) => void }) {
const [h24, m] = value ? value.split(':') : ['', ''];
const hNum = h24 ? Number(h24) : null;
const hour = hNum !== null ? String(hNum % 12 || 12) : '';
const minute = m ?? '';
const period: 'AM' | 'PM' = hNum !== null && hNum >= 12 ? 'PM' : 'AM';

function emit(newHour: string, newMinute: string, newPeriod: 'AM' | 'PM') {
if (newHour && newMinute) onChange(to24Hour(newHour, newMinute, newPeriod));
const [draft, setDraft] = useState(() => parseTimeValue(value));

// Re-sync local draft when the parent changes `value` externally.
useEffect(() => {
setDraft(parseTimeValue(value));
}, [value]);

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));
}
Comment on lines +145 to 148
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.


const sel = 'text-xs border border-gray-200 rounded px-1.5 py-1 focus:outline-none focus:ring-1 focus:ring-burgundy-500 bg-white';
const sel = 'text-xs border border-gray-200 rounded px-1.5 py-1 hover:border-gray-300 focus:outline-none focus:ring-1 focus:ring-burgundy-500 bg-white';

return (
<div className="flex items-center gap-1">
<select className={sel} value={hour} onChange={(e) => emit(e.target.value, minute, period)}>
<select
aria-label="Hour"
className={sel}
value={draft.hour}
onChange={(e) => update({ ...draft, hour: e.target.value })}
>
<option value="">hr</option>
{[1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12].map((n) => (
<option key={n} value={String(n)}>{n}</option>
))}
</select>
<span className="text-xs text-gray-400">:</span>
<select className={sel} value={minute} onChange={(e) => emit(hour, e.target.value, period)}>
<span className="text-xs text-gray-400" aria-hidden="true">:</span>
<select
aria-label="Minute"
className={sel}
value={draft.minute}
onChange={(e) => update({ ...draft, minute: e.target.value })}
>
<option value="">min</option>
{['00', '05', '10', '15', '20', '25', '30', '35', '40', '45', '50', '55'].map((mm) => (
<option key={mm} value={mm}>{mm}</option>
))}
</select>
<select className={sel} value={period} onChange={(e) => emit(hour, minute, e.target.value as 'AM' | 'PM')}>
<select
aria-label="AM or PM"
className={sel}
value={draft.period}
onChange={(e) => update({ ...draft, period: e.target.value as 'AM' | 'PM' })}
>
<option value="AM">AM</option>
<option value="PM">PM</option>
</select>
Expand Down Expand Up @@ -511,6 +541,8 @@ export default function SectionMutationDrawer(props: Props) {

// Controls visibility of the inline "create time block" form
const [showAddBlock, setShowAddBlock] = useState(false);
// When true, narrow the time block dropdown to the curated NEU sequences
const [showOnlyStandard, setShowOnlyStandard] = useState(false);

// Submission state
const [saving, setSaving] = useState(false);
Expand Down Expand Up @@ -636,22 +668,21 @@ export default function SectionMutationDrawer(props: Props) {
const courseOptions: SelectOption<number>[] = courses.map(courseOptionFromApi);

const timeBlockOptions: SelectOption<number>[] = useMemo(() => {
// Only show multi-day blocks (the standard named sequences) and split block halves.
// Exclude single-day blocks (days.length === 1 and no block_group) — those are legacy
// seed blocks that aren't useful for manual section assignment.
const eligible = timeBlocks.filter((tb) => {
// Always keep split-block halves
if (tb.block_group != null) return true;
// Only show blocks that exactly match a known standard sequence —
// this filters out legacy seed blocks that don't correspond to any
// official NEU meeting pattern.
return STANDARD_SEQUENCES.some(
(s) => s.days === tb.days && s.start === tb.start_time && s.end === tb.end_time,
);
});

// Sort by start time so the earlier block of a pair is always processed first
const sorted = [...eligible].sort((a, b) => a.start_time.localeCompare(b.start_time));
const filtered = showOnlyStandard
? timeBlocks.filter((tb) => {
// Always keep the currently-selected block so toggling the filter
// never hides the user's existing choice.
if (tb.time_block_id === timeBlockId) return true;
// Split-block halves are part of a named pattern — keep them.
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,
);
})
: timeBlocks;

// Sort by start time so the earlier block of a split pair is processed first
const sorted = [...filtered].sort((a, b) => a.start_time.localeCompare(b.start_time));
const seen = new Set<number>();
const opts: SelectOption<number>[] = [];

Expand All @@ -678,7 +709,7 @@ export default function SectionMutationDrawer(props: Props) {
}

return opts.sort((a, b) => a.label.localeCompare(b.label));
}, [timeBlocks]);
}, [timeBlocks, showOnlyStandard, timeBlockId]);

const facultyOptions: SelectOption<number>[] = useMemo(() => {
const rows = faculty.map((f) => {
Expand Down Expand Up @@ -824,18 +855,29 @@ export default function SectionMutationDrawer(props: Props) {

{/* Time Block */}
<div>
<div className="flex items-center justify-between mb-1.5">
<div className="flex items-center justify-between mb-1.5 gap-3">
<Label>Time block</Label>
{/* Only show "Add new" when a campus is known — we need it to create the block */}
{campusId != null && !showAddBlock && (
<button
type="button"
onClick={() => setShowAddBlock(true)}
className="text-xs text-burgundy-600 hover:text-burgundy-800 transition-colors"
>
+ Add new
</button>
)}
<div className="flex items-center gap-3">
<label className="flex items-center gap-1.5 text-xs text-gray-500 cursor-pointer select-none normal-case tracking-normal">
<input
type="checkbox"
checked={showOnlyStandard}
onChange={(e) => setShowOnlyStandard(e.target.checked)}
className="h-3 w-3 rounded border-gray-300 text-burgundy-600 focus:ring-burgundy-500"
/>
Standard only
</label>
{/* Only show "Add new" when a campus is known — we need it to create the block */}
{campusId != null && !showAddBlock && (
<button
type="button"
onClick={() => setShowAddBlock(true)}
className="text-xs text-burgundy-600 hover:text-burgundy-800 transition-colors"
>
+ Add new
</button>
)}
</div>
</div>
<SearchableSelect
options={timeBlockOptions}
Expand Down
1 change: 1 addition & 0 deletions frontend/src/pages/Faculty.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ describe('Faculty page — Export Invite CSV', () => {
mockApi = {
getMeApiUsersMeGet: vi.fn().mockResolvedValue(mockAdmin),
getAllCampusesCampusesGet: vi.fn().mockResolvedValue([]),
getAllSemestersSemestersGet: vi.fn().mockResolvedValue([]),
getSchedulesSchedulesGet: vi.fn().mockResolvedValue([]),
listUsersApiUsersGet: vi.fn().mockResolvedValue([]),
getFacultyFacultyGet: vi.fn().mockResolvedValue([]),
Expand Down
15 changes: 13 additions & 2 deletions frontend/src/pages/Faculty.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
type CampusResponse,
type InviteResponse,
type ScheduleResponse,
type SemesterResponse,
} from '../api/generated';
import FacultyDrawer, { type FacultyRecord } from '../components/FacultyDrawer';
import SearchableSelect, { type SelectOption } from '../components/SearchableSelect';
Expand Down Expand Up @@ -73,6 +74,9 @@ export default function Faculty() {
// Campuses
const [campuses, setCampuses] = useState<CampusResponse[]>([]);

// Semesters
const [semesters, setSemesters] = useState<SemesterResponse[]>([]);

// Faculty list
const [facultyList, setFacultyList] = useState<FacultyRecord[]>([]);
const [facultyLoading, setFacultyLoading] = useState(true);
Expand Down Expand Up @@ -111,6 +115,7 @@ export default function Faculty() {
useEffect(() => {
const api = getAutomatedCourseSchedulerAPI();
api.getAllCampusesCampusesGet().then(setCampuses).catch(() => {});
api.getAllSemestersSemestersGet().then(setSemesters).catch(() => {});
api
.getSchedulesSchedulesGet()
.then((data) => {
Expand Down Expand Up @@ -172,15 +177,21 @@ export default function Faculty() {
}, [sections]);
const totalTimeAssignments = timeCounts.first + timeCounts.second + timeCounts.third + timeCounts.none;

// Semester label lookup
const semesterLabelMap = useMemo(
() => new Map(semesters.map((s) => [s.semester_id, `${s.season} ${s.year}`])),
[semesters],
);

// Schedule dropdown options
const scheduleOptions: SelectOption<number>[] = useMemo(
() =>
schedules.map((s) => ({
value: s.schedule_id,
label: s.name,
sublabel: `Semester ${s.semester_id}`,
sublabel: semesterLabelMap.get(s.semester_id) ?? '',
})),
[schedules],
[schedules, semesterLabelMap],
);

// Campus name lookup
Expand Down
16 changes: 14 additions & 2 deletions frontend/src/pages/ScheduleList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { useEffect, useRef, useState } from 'react';
import { useEffect, useMemo, useRef, useState } from 'react';
import { useNavigate } from 'react-router-dom';
import {
getAutomatedCourseSchedulerAPI,
Expand All @@ -17,12 +17,14 @@ import { useUser } from '../context/UserContext';

function ScheduleCard({
schedule,
semesterLabel,
onClick,
isAdmin,
onUpdate,
onDelete,
}: {
schedule: ScheduleResponse;
semesterLabel: string | null;
onClick: () => void;
isAdmin: boolean;
onUpdate: (id: number, data: { name?: string; draft?: boolean }) => Promise<void>;
Expand Down Expand Up @@ -161,7 +163,9 @@ function ScheduleCard({
<h3 className="text-base font-semibold text-gray-900 truncate">
{schedule.name}
</h3>
<p className="mt-0.5 text-sm text-gray-500">Semester {schedule.semester_id}</p>
{semesterLabel && (
<p className="mt-0.5 text-sm text-gray-500">{semesterLabel}</p>
)}
<div className="mt-3 flex items-center gap-2">
{schedule.draft ? (
<span className="inline-flex items-center gap-1 px-2 py-0.5 rounded-full text-xs font-medium bg-amber-50 text-amber-700 border border-amber-200">
Expand Down Expand Up @@ -699,6 +703,7 @@ function CreateScheduleModal({ onClose, onCreated }: { onClose: () => void; onCr

export default function ScheduleList() {
const [schedules, setSchedules] = useState<ScheduleResponse[]>([]);
const [semesters, setSemesters] = useState<SemesterResponse[]>([]);
const [loading, setLoading] = useState(true);
const [error, setError] = useState<string | null>(null);
const [showCreate, setShowCreate] = useState(false);
Expand All @@ -717,8 +722,14 @@ export default function ScheduleList() {
setError('Failed to load schedules.');
setLoading(false);
});
api.getAllSemestersSemestersGet().then(setSemesters).catch(() => {});
}, []);

const semesterLabelById = useMemo(
() => new Map(semesters.map((s) => [s.semester_id, `${s.season} ${s.year}`])),
[semesters],
);

function handleCreated(s: ScheduleResponse) {
setSchedules((prev) => {
if (prev.some((p) => p.schedule_id === s.schedule_id)) return prev;
Expand Down Expand Up @@ -790,6 +801,7 @@ export default function ScheduleList() {
<ScheduleCard
key={s.schedule_id}
schedule={s}
semesterLabel={semesterLabelById.get(s.semester_id) ?? null}
isAdmin={isAdmin}
onClick={() => navigate(`/schedules/${s.schedule_id}`)}
onUpdate={handleUpdate}
Expand Down
3 changes: 3 additions & 0 deletions frontend/src/pages/Schedules.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,9 @@ describe('Schedules page', () => {
getScheduleSchedulesScheduleIdGet: vi.fn().mockResolvedValue({
schedule_id: 42, name: 'Fall 2025', semester_id: 1, draft: false, campus: 1, active: true,
}),
getSemesterSemestersSemesterIdGet: vi.fn().mockResolvedValue({
semester_id: 1, season: 'Fall', year: 2025, active: true,
}),
getAllCampusesCampusesGet: vi.fn().mockResolvedValue([
{ campus_id: 1, name: 'Boston', active: true },
]),
Expand Down
8 changes: 6 additions & 2 deletions frontend/src/pages/Schedules.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,12 +60,16 @@ function ScheduleView({ scheduleId, readOnly }: { scheduleId: number; readOnly?:
const { me, meError } = useUser();
const [schedule, setSchedule] = useState<ScheduleResponse | null>(null);
const [campusName, setCampusName] = useState<string | null>(null);
const [semesterLabel, setSemesterLabel] = useState<string | null>(null);

useEffect(() => {
const api = getAutomatedCourseSchedulerAPI();
api.getScheduleSchedulesScheduleIdGet(scheduleId)
.then((s) => {
setSchedule(s);
api.getSemesterSemestersSemesterIdGet(s.semester_id)
.then((sem) => setSemesterLabel(`${sem.season} ${sem.year}`))
.catch(() => {});
// Resolve campus name for drawer filtering
return api.getAllCampusesCampusesGet().then((campuses) => {
const match = campuses.find((c) => c.campus_id === s.campus);
Expand Down Expand Up @@ -128,8 +132,8 @@ function ScheduleView({ scheduleId, readOnly }: { scheduleId: number; readOnly?:
{modeLabel}
</span>
</div>
{schedule && (
<p className="mt-0.5 text-sm text-gray-500">Semester {schedule.semester_id}</p>
{schedule && semesterLabel && (
<p className="mt-0.5 text-sm text-gray-500">{semesterLabel}</p>
)}
{meError && (
<div className="mt-3 text-sm text-amber-800 bg-amber-50 border border-amber-200 rounded-lg px-3 py-2">
Expand Down
Loading