Land dependency modernization stack: security bumps, TS6+Vite8, lazy catalogs, MUI 9, dep-shedding#166
Conversation
Co-Authored-By: Rahul Chalamala <22563365+rchalamala@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
caltech-dev | 015eb5e | Commit Preview URL Branch Preview URL |
Jun 11 2026, 11:21 PM |
Test ResultsRan the production build (vite 7.3.5) served by Deviations from the original request (intentional):
Results:
Shell evidence
Devin session: https://app.devin.ai/sessions/44f717e165344a86812f49b427651bc6 |
…ig-paths 6) Co-Authored-By: Rahul Chalamala <22563365+rchalamala@users.noreply.github.com>
Co-Authored-By: Rahul Chalamala <22563365+rchalamala@users.noreply.github.com>
Co-Authored-By: Rahul Chalamala <22563365+rchalamala@users.noreply.github.com>
Co-Authored-By: Rahul Chalamala <22563365+rchalamala@users.noreply.github.com>
Co-Authored-By: Rahul Chalamala <22563365+rchalamala@users.noreply.github.com>
Co-Authored-By: Rahul Chalamala <22563365+rchalamala@users.noreply.github.com>
Co-Authored-By: Rahul Chalamala <22563365+rchalamala@users.noreply.github.com>
Co-Authored-By: Rahul Chalamala <22563365+rchalamala@users.noreply.github.com>
Co-Authored-By: Rahul Chalamala <22563365+rchalamala@users.noreply.github.com>
Co-Authored-By: Rahul Chalamala <22563365+rchalamala@users.noreply.github.com>
…ig-paths 6) (#165) Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: Rahul Chalamala <22563365+rchalamala@users.noreply.github.com>
…MB initial JS) (#164) * perf: lazy-load term catalogs and split vendor chunks Co-Authored-By: Rahul Chalamala <22563365+rchalamala@users.noreply.github.com> * fix: no-op workspace mutations while term catalog is loading Co-Authored-By: Rahul Chalamala <22563365+rchalamala@users.noreply.github.com> * fix: log error when term catalog fails to load Co-Authored-By: Rahul Chalamala <22563365+rchalamala@users.noreply.github.com> * fix: sync catalog state before paint when the term changes Co-Authored-By: Rahul Chalamala <22563365+rchalamala@users.noreply.github.com> * build: use function-form manualChunks for rolldown compatibility Co-Authored-By: Rahul Chalamala <22563365+rchalamala@users.noreply.github.com> * fix: re-sync search options after async catalog load Co-Authored-By: Rahul Chalamala <22563365+rchalamala@users.noreply.github.com> * fix: key search option re-sync on catalog identity Co-Authored-By: Rahul Chalamala <22563365+rchalamala@users.noreply.github.com> --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: Rahul Chalamala <22563365+rchalamala@users.noreply.github.com>
* chore: upgrade MUI to v9.1.0 (HelpOutline -> HelpOutlined icon rename) Co-Authored-By: Rahul Chalamala <22563365+rchalamala@users.noreply.github.com> * fix: use HelpOutlineOutlined to preserve original HelpOutline glyph Co-Authored-By: Rahul Chalamala <22563365+rchalamala@users.noreply.github.com> --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: Rahul Chalamala <22563365+rchalamala@users.noreply.github.com>
…animate (#167) Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: Rahul Chalamala <22563365+rchalamala@users.noreply.github.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| @@ -0,0 +1 @@ | |||
| declare module "*.css"; | |||
There was a problem hiding this comment.
📝 Info: New css.d.ts declaration covers all CSS imports needed by the project
The new src/css.d.ts file declares *.css as a module. This is likely needed because Vite 8 (or TypeScript 6) changed how CSS imports are typed. The project imports CSS in four places (src/index.tsx, src/Planner.tsx × 3), all as side-effect-only imports (import "./foo.css"). The broad declare module "*.css" declaration covers all of these. This is a standard pattern for Vite projects.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Acknowledged — informational only. The declare module "*.css" shim covers the project's four side-effect CSS imports under the TypeScript 6 / Vite 8 toolchain from commit e6c7bf1, and npm run verify (tsc included) passes. No action needed.
…s' into devin/1781144811-audit-fix-bumps
…ogs' into devin/1781144811-audit-fix-bumps
…udit-fix-bumps Co-Authored-By: Rahul Chalamala <22563365+rchalamala@users.noreply.github.com>
devin-ai-integration
Bot
changed the title
…gression) Co-Authored-By: Rahul Chalamala <22563365+rchalamala@users.noreply.github.com>
Co-Authored-By: Rahul Chalamala <22563365+rchalamala@users.noreply.github.com>
| selector: (item) => `${item.number} ${item.name}`, | ||
| }); |
There was a problem hiding this comment.
📝 Info: Fzf instance recreated on every render in WorkspaceSearch
The Fzf instance at src/Workspace.tsx:428-430 is created on every render of WorkspaceSearch, rebuilding its index over the full course list each time. This includes during every keystroke in the Autocomplete input. While functionally correct (each render gets a fresh fzf matching the current courses), wrapping it in useMemo(() => new Fzf(courses, {...}), [courses]) would avoid redundant index construction, especially since the courses array only changes when the catalog loads or the term switches.
(Refers to lines 428-430)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Acknowledged — correct that useMemo(() => new Fzf(courses, ...), [courses]) would avoid rebuilding the index per keystroke. Functionally fine today; noted as a possible perf follow-up rather than expanding this PR further.
| setWorkspaceIdx(storedIdx ? JSON.parse(storedIdx) : 0); | ||
| }, [realPath, defaultWorkspaces, setWorkspaces, setWorkspaceIdx]); | ||
|
|
||
| const catalogReady = Object.keys(indexedCourses).length > 0; |
There was a problem hiding this comment.
🚩 Async loading guards silently discard user interactions during catalog load
All state-modifying callbacks (addCourse, removeCourse, toggleCourse, toggleSectionLock, setCourses, nextArrangement, prevArrangement, updateAvailableTimes) now early-return when !catalogReady (e.g. src/useAppState.ts:144-146). This means if a user has courses from localStorage and interacts with them (toggling, locking, changing sections, navigating arrangements) before the async loadCourseIndex resolves, all those actions are silently ignored with no feedback. The WorkspaceSearch shows "Loading courses..." as placeholder, but the existing workspace entries appear fully interactive. Consider disabling the controls or showing a loading indicator over the workspace during the async load window.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Valid observation, but the window is narrow: the catalog chunk is a single local dynamic import (~135 kB gzip, cached after first load), so catalogReady flips within tens of ms in practice, and the search input already shows a "Loading courses..." placeholder. The guards exist to prevent corrupting persisted workspaces against an empty index, which is the worse failure mode. Leaving as-is for this PR; a loading overlay on the workspace list could be a follow-up if the gap proves noticeable.
| useLayoutEffect(() => { | ||
| let cancelled = false; | ||
| const cached = getCachedCourseIndex(realPath); | ||
| if (cached) { | ||
| setIndexedCourses(cached); | ||
| return; | ||
| } | ||
| setIndexedCourses({}); | ||
| loadCourseIndex(realPath) | ||
| .then((index) => { | ||
| if (!cancelled) { | ||
| setIndexedCourses(index); | ||
| } | ||
| }) | ||
| .catch((error: unknown) => { | ||
| console.error(`Failed to load course data for ${realPath}:`, error); | ||
| }); | ||
| return () => { | ||
| cancelled = true; | ||
| }; | ||
| }, [realPath]); |
There was a problem hiding this comment.
📝 Info: useLayoutEffect used for async side effect is unusual but intentional
The useLayoutEffect at src/useAppState.ts:69-89 is used to kick off an async loadCourseIndex() call. Typically useLayoutEffect is reserved for synchronous DOM measurement/mutation. However, the intent here is to synchronously check the cache (getCachedCourseIndex) and set indexedCourses before paint when navigating between terms, so the calendar doesn't flash stale data. The async promise resolution happens after paint regardless. This is a valid pattern — the synchronous cache path benefits from useLayoutEffect while the async fallback is harmless. The cleanup function correctly cancels stale loads via the cancelled flag.
Was this helpful? React with 👍 or 👎 to provide feedback.
| "/fa2027": () => import("./data/IndexedTotalFA2026-27.json"), | ||
| }; | ||
|
|
||
| const courseIndexCache: { [key: string]: CourseIndex } = {}; |
There was a problem hiding this comment.
📝 Info: Module-level courseIndexCache is a process-global singleton
The courseIndexCache at src/courseData.ts:21 is a module-level mutable object that persists across the entire application lifetime. This is fine for a client-side SPA, but if this module were ever imported in an SSR context, the cache would be shared across requests. Given this is a Vite + Cloudflare Workers (wrangler) project, verify that this module is only used on the client side. Currently it appears to be client-only based on the usage in useAppState.ts (a React hook), so this is safe.
Was this helpful? React with 👍 or 👎 to provide feedback.
…very (flatpickr removed) Co-Authored-By: Rahul Chalamala <22563365+rchalamala@users.noreply.github.com>
| value={ | ||
| course.sectionId !== null | ||
| ? course.courseData.sections.find( | ||
| ? (course.courseData.sections.find( | ||
| (c) => | ||
| c.number === | ||
| course.courseData.sections[course.sectionId!].number, | ||
| ) | ||
| ) ?? null) | ||
| : null |
There was a problem hiding this comment.
📝 Info: SectionDropdown value lookup is redundant but necessary for MUI Autocomplete referential identity
The value computation at lines 215-222 uses sections.find(...) to look up the section object by number, even though sections[course.sectionId!] would directly return it. This is intentional: MUI Autocomplete requires the value to be referentially identical to one of the options array elements. Since find searches the same sections array used as options, it guarantees the correct reference. The ?? null fallback handles the unlikely case where findIndex returns -1.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (!loader) { | ||
| return {}; |
There was a problem hiding this comment.
📝 Info: loadCourseIndex does not cache empty results for unknown terms
When loadCourseIndex is called with an unknown term (no matching loader), it returns {} without caching the result (src/courseData.ts:33-34). This means each call for an unknown term creates a new Promise and returns a new {} object. In the current code, useLayoutEffect only calls loadCourseIndex once per realPath change, so this isn't a performance issue. However, repeated navigation to an invalid term would repeatedly invoke the function. This is minor since there's no actual I/O for unknown terms.
Was this helpful? React with 👍 or 👎 to provide feedback.
| "vite": "^8.0.16", | ||
| "vite-plugin-svgr": "^5.2.0", | ||
| "vite-tsconfig-paths": "^6.1.1", | ||
| "wrangler": "4.98.0" |
There was a problem hiding this comment.
🚩 Wrangler version pinned without caret, unlike other dependencies
In package.json, wrangler changed from ^4.49.1 (allows minor/patch updates) to 4.98.0 (exact version). All other dependencies use ^ ranges. This inconsistency might be intentional (to pin a known-good version) but could lead to the project falling behind on wrangler patches. Worth confirming this was deliberate.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Deliberate: wrangler 4.99.0 (latest at time of writing) has a regression where wrangler dev returns 404 for all static assets (bisected — 4.98.0 and earlier work). The exact pin prevents ^ from pulling in the broken version; it can be relaxed back to ^ once a fixed wrangler release ships.
E2E test results — combined stack branchBuilt this branch locally and served the production bundle via Tests (4/4 passed)
Also at branch head:
Preexisting bug found (NOT from this stack)After unlocking a course and narrowing then re-widening the time window, "No arrangements found :(" persists and the section can't be re-selected. Root cause:
Notes
Tested by Devin |
…edule while catalog loads Co-Authored-By: Rahul Chalamala <22563365+rchalamala@users.noreply.github.com>
| export async function loadCourseIndex(term: string): Promise<CourseIndex> { | ||
| const cached = courseIndexCache[term]; | ||
| if (cached) { | ||
| return cached; | ||
| } | ||
| const loader = courseDataLoaders[term]; | ||
| if (!loader) { | ||
| return {}; | ||
| } | ||
| const module = await loader(); | ||
| courseIndexCache[term] = module.default; | ||
| return module.default; |
There was a problem hiding this comment.
📝 Info: loadCourseIndex has benign duplicate-call race but no deduplication
In src/courseData.ts:27-38, if loadCourseIndex is called concurrently for the same term (e.g., via React strict mode double-effects), both calls will miss the cache and independently call loader(). Both will resolve to the same module.default and both write it to the cache. This is harmless since the data is identical, but a pending-promise cache (storing the promise itself) would avoid the duplicate network/parse work. The useLayoutEffect in src/useAppState.ts:69-89 has a proper cancelled flag that handles this scenario correctly on the consumer side.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Acknowledged — the duplicate-call race is benign (same module record resolves for both callers; dynamic import() is deduped by the module loader, so there's no double network/parse in practice). A pending-promise cache would be a fine micro-optimization but isn't needed. Leaving as-is.
| const onChange = (newSection: Maybe<SectionData>) => { | ||
| course.sectionId = | ||
| newSection !== null | ||
| newSection != null | ||
| ? course.courseData.sections.findIndex( | ||
| (s) => s.number === newSection.number, | ||
| ) |
There was a problem hiding this comment.
📝 Info: SectionDropdown.onChange directly mutates course.sectionId before calling addCourse
At src/Workspace.tsx:201, course.sectionId is mutated in-place (a direct state mutation) before state.addCourse(course) is called. This is a pre-existing pattern (existed before this PR), not introduced by the migration. It works because addCourse subsequently triggers a state update that persists the change. However, if addCourse returns early (e.g., due to the catalogReady guard), the mutation would persist on the object without a corresponding React state update, which could cause subtle inconsistencies. In practice this shouldn't happen since sections are only shown when courses are loaded.
(Refers to lines 200-208)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Acknowledged — pre-existing mutation pattern, not introduced by this PR. As noted, SectionDropdown only renders for courses already resolved from the catalog, so the catalogReady early-return path is effectively unreachable here. Leaving as-is; an immutable-update refactor could be a separate cleanup.
1 participant
Summary
This PR now carries the entire 5-PR modernization stack (the children #165/#164/#163/#167 were merged downward into this branch):
wrangler devassets-404 regression),npm audit= 0 vulnerabilitiessrc/css.d.tsfor TS6 side-effect CSS importsimport()+ function-formmanualChunks(rolldown-compatible): initial JS 8,221 kB → ~1.0 MB (1,883 → ~319 kB gzip)@mui/material/@mui/icons-material9.1.0;HelpOutlineOutlinedicon preserves the original glyph<input type="time">, react-select → MUIAutocomplete(fzf fuzzy search preserved viafilterOptions), auto-animate 0.9, README note that preact/@preact/signals are Schedule-X peersMerge-resolution note: the old react-select
options/firstLoadstate workaround (and its async-catalog re-sync fixes) was removed entirely — the MUIAutocompletederivesoptions={courses}from the catalog on every render, so it handles async catalog loading without extra state; the search placeholder shows "Loading courses..." while the term catalog chunk loads.npm run verifyandnpm audit(0 vulns) pass; all CI checks green.Link to Devin session: https://app.devin.ai/sessions/3b5cf4353db148c09b27d04099b86357
Requested by: @rchalamala