Fix: UI fixes#71
Merged
Merged
Conversation
BeWelsh
requested review from
Saisri24 and
jobvengalil
and removed request for
Saisri24
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 42 minutes and 58 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughThis PR introduces a user context system for managing authenticated user state across the frontend, adds database-level uniqueness constraints on Course records, extends course data in API responses with subject and code fields, and refactors multiple frontend components to consume user data from the centralized context instead of making individual API calls. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 3
🧹 Nitpick comments (6)
frontend/src/pages/Faculty.test.tsx (1)
27-29: Minor:getMeApiUsersMeGetmock entry is now dead.With
useUsermocked at the context boundary,Faculty.tsxno longer callsgetMeApiUsersMeGetdirectly, so thegetMeApiUsersMeGet: vi.fn().mockResolvedValue(mockAdmin)entry inmockApi(line 56) can be dropped. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/Faculty.test.tsx` around lines 27 - 29, The mock for getMeApiUsersMeGet in mockApi is now unused because useUser is mocked at the context boundary; remove the getMeApiUsersMeGet: vi.fn().mockResolvedValue(mockAdmin) entry from the mockApi object (referencing mockApi and getMeApiUsersMeGet) so the test fixture only includes active mocks and avoids dead entries when Faculty.tsx relies on useUser.frontend/src/context/UserContext.tsx (1)
38-40: Optional: splituseUserout of the component module for React Fast Refresh.Per the Lint check, co-locating a non-component export (
useUser) with a component (UserProvider) disables Fast Refresh for this file. Low-impact, but trivially fixed by moving the hook (and theUserContext/UserContextValuetype) into a sibling module likeuseUser.tsand keeping onlyUserProviderhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/context/UserContext.tsx` around lines 38 - 40, The file currently co-locates a non-component export with a component which disables React Fast Refresh; extract the hook and its context/type into a sibling module (e.g., create useUser.ts) by moving UserContext, UserContextValue and the useUser function into that new file and export them from there, leaving only the UserProvider component in this module; update any imports across the codebase to import useUser, UserContext and/or UserContextValue from the new useUser.ts so behavior and types remain unchanged.frontend/src/pages/Schedules.test.tsx (1)
85-96: StalegetMeApiUsersMeGetmock — the component no longer calls it.Since
Schedules.tsxnow reads identity exclusively fromuseUser()(mocked at line 36-38), thegetMeApiUsersMeGetentry in thisbeforeEachnever fires and can be dropped to avoid misleading future readers into thinking role flows through the API mock.♻️ Drop the dead mock
getFacultyFacultyGet: vi.fn().mockResolvedValue([ { NUID: 100005 }, ]), - getMeApiUsersMeGet: vi.fn().mockResolvedValue(viewerUser), } as unknown as ReturnType<typeof generated.getAutomatedCourseSchedulerAPI>);🤖 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 85 - 96, The test mocks returned by generated.getAutomatedCourseSchedulerAPI include getMeApiUsersMeGet which is no longer used because Schedules.tsx reads identity from useUser() (mocked earlier); remove the dead getMeApiUsersMeGet property from the mocked object in the beforeEach to avoid confusion and keep the mock surface minimal while leaving getScheduleSchedulesScheduleIdGet, getAllCampusesCampusesGet, and getFacultyFacultyGet intact.frontend/src/pages/Faculty.tsx (1)
244-247: Switching back toloadvia the header falls intoasc, losing the new "load defaults to desc" intent.With the default sort now
load/desc, clickingNameand then clickingLoadagain will land onload/ascbecausehandleSorthardcodesascwhenever the key changes. Consider picking a per-key default direction so load keeps defaulting todesc.♻️ Proposed per-key default direction
function handleSort(key: SortKey) { if (sortKey === key) setSortDir((d) => (d === 'asc' ? 'desc' : 'asc')); - else { setSortKey(key); setSortDir('asc'); } + else { setSortKey(key); setSortDir(key === 'load' ? 'desc' : 'asc'); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/Faculty.tsx` around lines 244 - 247, The current handleSort toggles direction only when the same key is clicked and always sets 'asc' when switching keys, which breaks the desired "load defaults to desc" behavior; update handleSort (and introduce a small per-key default map or helper like defaultSortDirFor) so that when setSortKey(key) is called you also setSortDir(defaultSortDirFor(key)) instead of hardcoding 'asc', and keep the existing toggle behavior when sortKey === key; reference the handleSort function, SortKey type, and setSortDir/setSortKey to locate where to apply the change.frontend/src/components/SectionMutationDrawer.tsx (1)
521-529: ResetloadingDatawhencampusNamechanges.If the parent ever remounts this drawer with a different
campusName, the effect refetches butloadingDatastaysfalse, so the body renders with stale/emptyfaculty(and no spinner) until the new request resolves. Today this is unlikely because callers don't swapcampusNamemid-lifetime, but the invariant is easy to make robust:♻️ Proposed change
useEffect(() => { const api = getAutomatedCourseSchedulerAPI(); + setLoadingData(true); api.getFacultyFacultyGet( campusName ? { campus: campusName, active_only: true } : { active_only: true }, ).then((result) => { setFaculty(result); setLoadingData(false); }).catch(() => setLoadingData(false)); }, [campusName]);Also, the
.catch(() => setLoadingData(false))silently swallows the failure — consider surfacing it via the existingerrorstate so admins know instructors are missing and aren't tempted to save a section without its roster. Pre-existing behavior, so optional.🤖 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 521 - 529, The effect that fetches faculty in SectionMutationDrawer should reset loading state and surface errors when campusName changes: inside the useEffect that calls getAutomatedCourseSchedulerAPI().getFacultyFacultyGet, set loadingData to true at the start (so the spinner shows on refetch) and in the promise catch call setLoadingData(false) and setError with the caught error (or a descriptive message) in addition to not swallowing it; ensure successful responses still call setFaculty(result) and setLoadingData(false). Target the existing useEffect, getAutomatedCourseSchedulerAPI/getFacultyFacultyGet, and the setLoadingData/setFaculty/setError state setters.frontend/src/components/ScheduleSectionRowView.tsx (1)
109-112: Catalog fetch has no error surfacing and no loading gate.Two small concerns worth noting:
.catch(() => {})swallows failures silently. If/coursesever fails, admins who open the edit/create drawer will see an empty "Course" select with no hint why.- There's no gating on
catalogCoursesreadiness before opening the drawer — if an admin clicks a row very quickly after mount, they can reach the drawer beforecatalogCoursesresolves. In practice the race window is tiny, but consider either surfacing a load error or disabling the Add/Edit affordances until the catalog is present.Non-blocking for this PR, but logging the error (or a toast) would help triage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/ScheduleSectionRowView.tsx` around lines 109 - 112, The fetch in the useEffect that calls getAutomatedCourseSchedulerAPI().getCoursesCoursesGet() swallows errors and has no loading gate; update the hook to track loading and error state (e.g., catalogLoading, catalogError) and call setCatalogCourses on success, log or surface the caught error (via processLogger/toast) inside the .catch so failures aren't silent, and ensure any UI that opens the Add/Edit drawer checks catalogLoading or catalogError (or disables the Add/Edit affordance) until catalogCourses is populated so the drawer cannot open before the catalog is ready; reference the existing useEffect, getAutomatedCourseSchedulerAPI, api.getCoursesCoursesGet, setCatalogCourses, and catalogCourses to locate where to add these states and checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/models/course.py`:
- Line 19: The POST /courses endpoint currently only catches ValueError and will
return 500 on duplicate (subject, code); update the create_course function in
backend/app/routers/course.py to also catch sqlalchemy.exc.IntegrityError (or
SQLAlchemyError) from course_service.create_course, call db.rollback() in that
except block, and raise HTTPException(status_code=400, detail="A course with
this subject and code already exists") so duplicate key violations return a
friendly 400; keep the existing ValueError handling intact.
In `@frontend/src/context/UserContext.tsx`:
- Around line 13-36: The effect in UserProvider leaves meLoading true when
isAuthenticated is false and doesn't clear me/meError on logout; modify the
useEffect watching isAuthenticated in UserProvider so that when isAuthenticated
is false it immediately sets setMeLoading(false) and also resets setMe(null) and
setMeError(null), and when true proceeds to call
getAutomatedCourseSchedulerAPI().getMeApiUsersMeGet() as before
(clearing/setLoading appropriately in the promise handlers); optionally consider
using Auth0's isLoading if you need to distinguish transient auth hydration from
a definite anonymous state.
In `@frontend/src/pages/ScheduleList.tsx`:
- Line 421: The footer "Done" button bypasses the modal close guard: align its
behavior with the existing canClose logic (where canClose = step !== 'generate'
|| generateDone) by disabling and preventing its onClick when canClose is false;
update the Footer Done button (the element using the Done label and its
onClick/onClose handler) to check canClose (or the equivalent expression using
step/generateDone) before calling the close handler so the footer cannot close
the modal while step === 'generate' && !generateDone (i.e., mirror the
backdrop/header X guard).
---
Nitpick comments:
In `@frontend/src/components/ScheduleSectionRowView.tsx`:
- Around line 109-112: The fetch in the useEffect that calls
getAutomatedCourseSchedulerAPI().getCoursesCoursesGet() swallows errors and has
no loading gate; update the hook to track loading and error state (e.g.,
catalogLoading, catalogError) and call setCatalogCourses on success, log or
surface the caught error (via processLogger/toast) inside the .catch so failures
aren't silent, and ensure any UI that opens the Add/Edit drawer checks
catalogLoading or catalogError (or disables the Add/Edit affordance) until
catalogCourses is populated so the drawer cannot open before the catalog is
ready; reference the existing useEffect, getAutomatedCourseSchedulerAPI,
api.getCoursesCoursesGet, setCatalogCourses, and catalogCourses to locate where
to add these states and checks.
In `@frontend/src/components/SectionMutationDrawer.tsx`:
- Around line 521-529: The effect that fetches faculty in SectionMutationDrawer
should reset loading state and surface errors when campusName changes: inside
the useEffect that calls getAutomatedCourseSchedulerAPI().getFacultyFacultyGet,
set loadingData to true at the start (so the spinner shows on refetch) and in
the promise catch call setLoadingData(false) and setError with the caught error
(or a descriptive message) in addition to not swallowing it; ensure successful
responses still call setFaculty(result) and setLoadingData(false). Target the
existing useEffect, getAutomatedCourseSchedulerAPI/getFacultyFacultyGet, and the
setLoadingData/setFaculty/setError state setters.
In `@frontend/src/context/UserContext.tsx`:
- Around line 38-40: The file currently co-locates a non-component export with a
component which disables React Fast Refresh; extract the hook and its
context/type into a sibling module (e.g., create useUser.ts) by moving
UserContext, UserContextValue and the useUser function into that new file and
export them from there, leaving only the UserProvider component in this module;
update any imports across the codebase to import useUser, UserContext and/or
UserContextValue from the new useUser.ts so behavior and types remain unchanged.
In `@frontend/src/pages/Faculty.test.tsx`:
- Around line 27-29: The mock for getMeApiUsersMeGet in mockApi is now unused
because useUser is mocked at the context boundary; remove the
getMeApiUsersMeGet: vi.fn().mockResolvedValue(mockAdmin) entry from the mockApi
object (referencing mockApi and getMeApiUsersMeGet) so the test fixture only
includes active mocks and avoids dead entries when Faculty.tsx relies on
useUser.
In `@frontend/src/pages/Faculty.tsx`:
- Around line 244-247: The current handleSort toggles direction only when the
same key is clicked and always sets 'asc' when switching keys, which breaks the
desired "load defaults to desc" behavior; update handleSort (and introduce a
small per-key default map or helper like defaultSortDirFor) so that when
setSortKey(key) is called you also setSortDir(defaultSortDirFor(key)) instead of
hardcoding 'asc', and keep the existing toggle behavior when sortKey === key;
reference the handleSort function, SortKey type, and setSortDir/setSortKey to
locate where to apply the change.
In `@frontend/src/pages/Schedules.test.tsx`:
- Around line 85-96: The test mocks returned by
generated.getAutomatedCourseSchedulerAPI include getMeApiUsersMeGet which is no
longer used because Schedules.tsx reads identity from useUser() (mocked
earlier); remove the dead getMeApiUsersMeGet property from the mocked object in
the beforeEach to avoid confusion and keep the mock surface minimal while
leaving getScheduleSchedulesScheduleIdGet, getAllCampusesCampusesGet, and
getFacultyFacultyGet intact.
🪄 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: 8f9b442a-5c89-4bcb-9ce4-a11e22eb4433
📒 Files selected for processing (22)
backend/app/models/course.pybackend/app/routers/upload.pybackend/app/schemas/section.pybackend/app/services/section.pyfrontend/openapi.jsonfrontend/src/App.tsxfrontend/src/api/generated.tsfrontend/src/components/ScheduleSectionRowView.tsxfrontend/src/components/SectionComments.tsxfrontend/src/components/SectionMutationDrawer.tsxfrontend/src/components/Sidebar.tsxfrontend/src/context/UserContext.tsxfrontend/src/hooks/useScheduleWebSocket.test.tsfrontend/src/index.cssfrontend/src/main.tsxfrontend/src/pages/Courses.tsxfrontend/src/pages/Faculty.test.tsxfrontend/src/pages/Faculty.tsxfrontend/src/pages/ScheduleList.test.tsxfrontend/src/pages/ScheduleList.tsxfrontend/src/pages/Schedules.test.tsxfrontend/src/pages/Schedules.tsx
Comment on lines
+13
to
+36
| export function UserProvider({ children }: { children: React.ReactNode }) { | ||
| const { isAuthenticated } = useAuth0(); | ||
| const [me, setMe] = useState<UserResponse | null>(null); | ||
| const [meError, setMeError] = useState<string | null>(null); | ||
| const [meLoading, setMeLoading] = useState(true); | ||
|
|
||
| useEffect(() => { | ||
| if (!isAuthenticated) return; | ||
| getAutomatedCourseSchedulerAPI() | ||
| .getMeApiUsersMeGet() | ||
| .then(setMe) | ||
| .catch((err: unknown) => { | ||
| const status = (err as { response?: { status?: number } })?.response?.status; | ||
| setMeError( | ||
| status === 403 | ||
| ? 'Your Auth0 account is not linked to a DB user yet. Ask an admin to invite you or run bootstrap_admin.py.' | ||
| : 'Could not load your user profile.', | ||
| ); | ||
| }) | ||
| .finally(() => setMeLoading(false)); | ||
| }, [isAuthenticated]); | ||
|
|
||
| return <UserContext.Provider value={{ me, meError, meLoading }}>{children}</UserContext.Provider>; | ||
| } |
Contributor
There was a problem hiding this comment.
meLoading is never cleared when the user is unauthenticated, and state isn't reset on logout.
Two related issues in the provider:
- When
isAuthenticatedisfalseon mount, the effect early-returns beforesetMeLoading(false)runs, someLoadingstaystrueforever. Any consumer that gates rendering onmeLoading(rather than theme != null || meError != nullpattern used inSchedules.tsx) will hang on a loading state during the pre-auth phase or for anonymous views. - When
isAuthenticatedflipstrue → false(e.g., Auth0 logout inside the same SPA lifetime),meandmeErrorare never reset, so stale user data leaks into subsequent sessions until a full page reload.
🛠️ Proposed fix
useEffect(() => {
- if (!isAuthenticated) return;
+ if (!isAuthenticated) {
+ setMe(null);
+ setMeError(null);
+ setMeLoading(false);
+ return;
+ }
+ setMeLoading(true);
getAutomatedCourseSchedulerAPI()
.getMeApiUsersMeGet()
.then(setMe)
.catch((err: unknown) => {
const status = (err as { response?: { status?: number } })?.response?.status;
setMeError(
status === 403
? 'Your Auth0 account is not linked to a DB user yet. Ask an admin to invite you or run bootstrap_admin.py.'
: 'Could not load your user profile.',
);
})
.finally(() => setMeLoading(false));
}, [isAuthenticated]);Also worth considering that Auth0's isAuthenticated is itself false during the initial silent-auth hydration; treating that transient state as "unauthenticated, stop loading" is usually the right behavior, but if any consumer needs to distinguish "auth still loading" vs "definitely anonymous", you may want to pull in Auth0's own isLoading here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/context/UserContext.tsx` around lines 13 - 36, The effect in
UserProvider leaves meLoading true when isAuthenticated is false and doesn't
clear me/meError on logout; modify the useEffect watching isAuthenticated in
UserProvider so that when isAuthenticated is false it immediately sets
setMeLoading(false) and also resets setMe(null) and setMeError(null), and when
true proceeds to call getAutomatedCourseSchedulerAPI().getMeApiUsersMeGet() as
before (clearing/setLoading appropriately in the promise handlers); optionally
consider using Auth0's isLoading if you need to distinguish transient auth
hydration from a definite anonymous state.
| } | ||
| } | ||
|
|
||
| const canClose = step !== 'generate' || generateDone; |
Contributor
There was a problem hiding this comment.
Footer "Done" button bypasses the canClose guard.
canClose disables the backdrop (Line 427) and header X (Lines 438-440) whenever step === 'generate' && !generateDone, but the footer "Done" button at Lines 674-681 has no such guard. Net effect:
- Before the user clicks "Run Algorithm" (
generating=false, generateDone=false): X and backdrop are disabled, but "Done" is clickable → the user can close from the footer while both other close paths are locked. - Mid-generation (
generating=true, generateDone=false): same mismatch — user can abandon the modal via "Done" but not via X/backdrop.
Pick one semantics. If the intent is "don't let the user close until generation finishes," gate "Done" too; if the intent is "only block accidental close during the actual network call," narrow canClose.
🔧 Option 1 — tighten footer to match guard
{step === 'generate' && (
<button
- onClick={onClose}
- className="px-4 py-2 text-sm font-medium text-white bg-burgundy-600 rounded-lg hover:bg-burgundy-700 transition-colors"
+ onClick={canClose ? onClose : undefined}
+ disabled={!canClose}
+ className="px-4 py-2 text-sm font-medium text-white bg-burgundy-600 rounded-lg hover:bg-burgundy-700 disabled:opacity-50 disabled:cursor-not-allowed transition-colors"
>
Done
</button>
)}🔧 Option 2 — loosen guard to only block during the in-flight call
- const canClose = step !== 'generate' || generateDone;
+ const canClose = step !== 'generate' || generateDone || !generating;Also applies to: 674-681
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/pages/ScheduleList.tsx` at line 421, The footer "Done" button
bypasses the modal close guard: align its behavior with the existing canClose
logic (where canClose = step !== 'generate' || generateDone) by disabling and
preventing its onClick when canClose is false; update the Footer Done button
(the element using the Done label and its onClick/onClose handler) to check
canClose (or the equivalent expression using step/generateDone) before calling
the close handler so the footer cannot close the modal while step === 'generate'
&& !generateDone (i.e., mirror the backdrop/header X guard).
jobvengalil
approved these changes
Apr 23, 2026
Collaborator
jobvengalil
left a comment
jobvengalil
left a comment
There was a problem hiding this comment.
Took a quick look, lgtm, maybe worth seeing if the code rabbit comments are valid
suggested by coderabbit
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
Type of Change
What Changed?
Testing & Validation
How this was tested
Screenshots/Recordings
Unfinished Work & Known Issues
Notes & Nuances
Reviewer Notes
Summary by CodeRabbit
New Features
Improvements
Bug Fixes