[codex] Modernize frontend toolchain and strict linting#143
Conversation
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
caltech-dev | ff34d6e | Commit Preview URL Branch Preview URL |
May 12 2026, 10:01 AM |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 Boolean false rendered as literal "false" CSS class name
In the WorkspaceEntry component, the template literal ${course.locked && "bg-neutral-100"} evaluates to the boolean false when course.locked is false. In a JavaScript template literal, ${false} becomes the string "false", so the class attribute will contain the literal text false as a class name. The correct pattern is a ternary: course.locked ? "bg-neutral-100" : "".
(Refers to lines 270-272)
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (result) { | ||
| // course was already in workspace | ||
| newCourses = courses; | ||
| newCourses = courses.map((course) => | ||
| course.courseData.id === newCourse.courseData.id ? newCourse : course, | ||
| ); |
There was a problem hiding this comment.
🔴 addCourse overwrites existing course state when re-adding from search
When addCourse is called for a course that already exists in the workspace, the new code replaces the existing course object with newCourse (src/App.tsx:606-608). The old code preserved existing state with newCourses = courses. This is a behavioral regression: when WorkspaceSearch.handleSelect (src/Workspace.tsx:397-403) calls addCourse with sectionId: null, enabled: true, locked: false, re-selecting an already-added course from the search bar silently resets the user's section selection, lock state, and enable state.
Old vs new behavior when course already exists
Old code: newCourses = courses — no change to existing course.
New code:
newCourses = courses.map((course) =>
course.courseData.id === newCourse.courseData.id ? newCourse : course,
);
Replaces existing course with newCourse, which from search has sectionId: null, locked: false.
| if (result) { | |
| // course was already in workspace | |
| newCourses = courses; | |
| newCourses = courses.map((course) => | |
| course.courseData.id === newCourse.courseData.id ? newCourse : course, | |
| ); | |
| if (result) { | |
| // course was already in workspace — keep existing state | |
| newCourses = courses; | |
| } else { |
Was this helpful? React with 👍 or 👎 to provide feedback.
| // 5 blank workspaces by default bc I'm too lazy to implement dynamic tabs and stuff | ||
| const localWorkspaces = localStorage.getItem("workspaces" + realPath); | ||
| const [workspaces, setWorkspaces] = useState<Workspace[]>( | ||
| localWorkspaces | ||
| ? JSON.parse(localWorkspaces) | ||
| : [ | ||
| emptyWorkspace(), | ||
| emptyWorkspace(), | ||
| emptyWorkspace(), | ||
| emptyWorkspace(), | ||
| emptyWorkspace(), | ||
| ], | ||
| const [workspaces, setWorkspaces] = useState<Workspace[]>(() => | ||
| parseStoredWorkspaces(localWorkspaces), | ||
| ); | ||
| const localWorkspaceIdx = localStorage.getItem("workspaceIdx" + realPath); | ||
| const [workspaceIdx, setWorkspaceIdx] = useState<number>( | ||
| localWorkspaceIdx ? JSON.parse(localWorkspaceIdx) : 0, | ||
| const [workspaceIdx, setWorkspaceIdx] = useState<number>(() => | ||
| parseStoredWorkspaceIdx(localWorkspaceIdx, workspaces.length), | ||
| ); |
There was a problem hiding this comment.
🚩 Workspace state not re-initialized when navigating between terms
When realPath changes (e.g., user navigates from /sp2026 to /fa2025), localStorage.getItem("workspaces" + realPath) at src/App.tsx:559 is re-evaluated but the useState initializer at line 560-562 only runs once. The save effect at lines 578-584 then fires because realPath changed, writing the OLD term's workspace data to the NEW term's localStorage key — corrupting saved state. This is a pre-existing bug (the old code had the same pattern) so it's not introduced by this PR, but the async data loading amplifies it: setIndexedCourses(null) at line 531 triggers a loading screen, during which the save effect runs with stale data. A fix would involve keying the component on realPath or using a useEffect to re-load workspace state when the path changes.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let className = | ||
| "mb-8 flex flex-col items-center rounded border border-neutral-300 p-2 transition-colors shadow-lg"; | ||
| className += course.enabled ? " bg-white" : " bg-neutral-300"; | ||
| className += course.locked ? " bg-neutral-100" : ""; | ||
|
|
||
| const [infoModalOpen, setInfoModalOpen] = useState(false); | ||
| return ( |
There was a problem hiding this comment.
🚩 Duplicate and conflicting Tailwind classes in WorkspaceEntry
The className variable at lines 259-262 builds up conditional background colors (bg-white/bg-neutral-300 for enabled/disabled, bg-neutral-100 for locked), but the div template at line 270 unconditionally appends bg-white shadow-lg border-0. This means: (1) bg-white always appears, making the bg-neutral-300 disabled state from line 261 unreliable — in Tailwind v4, conflicting utilities of the same property have undefined winner order based on stylesheet generation; (2) border-0 conflicts with border border-neutral-300 from line 260; (3) shadow-lg is duplicated. The net effect is the disabled course visual state (bg-neutral-300) likely never renders correctly. This appears to be a refactoring oversight — the old CSS classes had explicit specificity order that Tailwind utilities don't replicate.
(Refers to lines 259-272)
Was this helpful? React with 👍 or 👎 to provide feedback.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0149c5983c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| newCourses = courses.map((course) => | ||
| course.courseData.id === newCourse.courseData.id ? newCourse : course, | ||
| ); |
There was a problem hiding this comment.
When addCourse sees an existing course ID, it now replaces that entry with newCourse instead of leaving it unchanged. WorkspaceSearch passes sectionId: null and locked: false on selection, so selecting a course that's already in the workspace will silently reset its chosen section/lock state and can reshuffle arrangements unexpectedly. The previous behavior was effectively idempotent for duplicate adds, so this is a user-visible regression.
Useful? React with 👍 / 👎.
1 participant
Summary
Modernizes the Caltech course scheduler frontend toolchain and tightens the project rules around typing and linting.
What changed
react-beautiful-dndwith@hello-pangea/dnd.public/dataso Vite serves them as static assets..oxlintrc.jsonc.Why
The repo was still carrying years-old CRA assumptions and a loose toolchain. This makes the build chain current, trims dead configuration, and makes type/lint failures more explicit so future changes have sharper guardrails.
Validation
bun install --frozen-lockfilebun run checkhttp://127.0.0.1:4173/fa2023: page loaded, calendar/search/workspace rendered, Default Schedule populated courses, Remove All reset the workspace, and the console had zero errors.Note: Vite now reports the default large chunk warning during production build because the old custom chunk warning suppression was removed intentionally.
Need help on this PR? Tag
@codesmithwith what you need.