Skip to content
12 changes: 10 additions & 2 deletions src/Workspace.tsx
Comment thread
devin-ai-integration[bot] marked this conversation as resolved.

@devin-ai-integration devin-ai-integration Bot Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

📝 Info: WorkspaceSearch Fzf instance recreated every render

At src/Workspace.tsx:437, new Fzf(courses, ...) is created on every render of WorkspaceSearch, not memoized. This is a pre-existing issue, not introduced by this PR. With lazy loading, courses is now initially [] and then becomes the full list after loading, so the Fzf instance will be recreated with an empty array first and then with the full array. The firstLoad workaround at line 418 handles the options state, but the Fzf instance itself is always up-to-date since it's recreated from the current courses value.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { use, useState } from "react";
import { use, useEffect, useState } from "react";
import Modal, { useModal } from "./Modal";
import Select from "react-select";
import { SingleValue } from "react-select";
Expand Down Expand Up @@ -419,6 +419,12 @@ function WorkspaceSearch() {
options = courses;
}

// catalogs load asynchronously; re-sync the options whenever the catalog
// changes so the dropdown never shows stale or empty results
useEffect(() => {
setOptions(Object.values(indexedCourses));
}, [indexedCourses]);
Comment on lines +424 to +426

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

📝 Info: useEffect resetting options may briefly override active search filter

The new useEffect at src/Workspace.tsx:424-426 resets options to the full course list whenever indexedCourses changes. If a user happened to have an active search query in the dropdown at the exact moment the catalog finishes loading, the filtered results would be replaced by the full list until the user types again. In practice this is nearly impossible since the catalog loads once on page load (before the user can reasonably start searching), so this is not a real concern.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Acknowledged — as noted, the catalog loads once before a user can realistically have an active query, and typing immediately re-filters; leaving as is.


const [selectedCourse, setCourse] = useState<Maybe<CourseData>>(null);

const handleSelect = (courseData: SingleValue<CourseData>) => {
Expand Down Expand Up @@ -447,7 +453,9 @@ function WorkspaceSearch() {
<Select
isClearable
className="my-3"
placeholder="Add a course..."
placeholder={
courses.length === 0 ? "Loading courses..." : "Add a course..."
}
Comment thread
devin-ai-integration[bot] marked this conversation as resolved.
Comment thread
devin-ai-integration[bot] marked this conversation as resolved.
options={options}
value={selectedCourse}
getOptionLabel={(course) => `${course?.number} - ${course?.name}`}
Expand Down
64 changes: 35 additions & 29 deletions src/courseData.ts
Original file line number Diff line number Diff line change
@@ -1,33 +1,39 @@
import DATA_FA2023 from "./data/IndexedTotalFA2022-23.json";
import DATA_WI2023 from "./data/IndexedTotalWI2022-23.json";
import DATA_SP2023 from "./data/IndexedTotalSP2022-23.json";
import DATA_FA2024 from "./data/IndexedTotalFA2023-24.json";
import DATA_WI2024 from "./data/IndexedTotalWI2023-24.json";
import DATA_SP2024 from "./data/IndexedTotalSP2023-24.json";
import DATA_FA2025 from "./data/IndexedTotalFA2024-25.json";
import DATA_WI2025 from "./data/IndexedTotalWI2024-25.json";
import DATA_SP2025 from "./data/IndexedTotalSP2024-25.json";
import DATA_FA2026 from "./data/IndexedTotalFA2025-26.json";
import DATA_WI2026 from "./data/IndexedTotalWI2025-26.json";
import DATA_SP2026 from "./data/IndexedTotalSP2025-26.json";
import DATA_FA2027 from "./data/IndexedTotalFA2026-27.json";

export const CURRENT_TERM = "/fa2027";

export const courseDataSources: {
[key: string]: { [key: string]: CourseData };
const courseDataLoaders: {
[key: string]: () => Promise<{ default: CourseIndex }>;
} = {
"/fa2023": DATA_FA2023,
"/wi2023": DATA_WI2023,
"/sp2023": DATA_SP2023,
"/fa2024": DATA_FA2024,
"/wi2024": DATA_WI2024,
"/sp2024": DATA_SP2024,
"/fa2025": DATA_FA2025,
"/wi2025": DATA_WI2025,
"/sp2025": DATA_SP2025,
"/fa2026": DATA_FA2026,
"/wi2026": DATA_WI2026,
"/sp2026": DATA_SP2026,
"/fa2027": DATA_FA2027,
"/fa2023": () => import("./data/IndexedTotalFA2022-23.json"),
"/wi2023": () => import("./data/IndexedTotalWI2022-23.json"),
"/sp2023": () => import("./data/IndexedTotalSP2022-23.json"),
"/fa2024": () => import("./data/IndexedTotalFA2023-24.json"),
"/wi2024": () => import("./data/IndexedTotalWI2023-24.json"),
"/sp2024": () => import("./data/IndexedTotalSP2023-24.json"),
"/fa2025": () => import("./data/IndexedTotalFA2024-25.json"),
"/wi2025": () => import("./data/IndexedTotalWI2024-25.json"),
"/sp2025": () => import("./data/IndexedTotalSP2024-25.json"),
"/fa2026": () => import("./data/IndexedTotalFA2025-26.json"),
"/wi2026": () => import("./data/IndexedTotalWI2025-26.json"),
"/sp2026": () => import("./data/IndexedTotalSP2025-26.json"),
"/fa2027": () => import("./data/IndexedTotalFA2026-27.json"),
Comment on lines +15 to +18

@devin-ai-integration devin-ai-integration Bot Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

📝 Info: Invalid terms return {} without caching — acceptable but different from valid empty catalogs

When loadCourseIndex is called with a term that has no entry in courseDataLoaders (e.g., /invalid), it returns {} at src/courseData.ts:17 without caching the result. This means future calls re-check the loaders map. In contrast, a valid term whose JSON happens to be {} gets cached at line 37. Both result in catalogReady = false, blocking operations. The non-caching for invalid terms is fine since the useEffect only calls loadCourseIndex once per realPath change, so there's no repeated lookup concern.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

};

const courseIndexCache: { [key: string]: CourseIndex } = {};

export function getCachedCourseIndex(term: string): CourseIndex | undefined {
return courseIndexCache[term];
}

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;
}
Comment on lines +27 to +39

@devin-ai-integration devin-ai-integration Bot Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

📝 Info: Duplicate concurrent loads possible in loadCourseIndex

The loadCourseIndex function at src/courseData.ts:27-39 checks the cache synchronously then awaits the loader. If two callers invoke it for the same term before the first resolves, both will miss the cache and trigger separate dynamic imports. This is harmless (JS runtimes typically deduplicate import() for the same module, and both writes set identical data) but wasteful. Storing the in-flight promise in a secondary map would deduplicate cleanly. In practice this is unlikely to be hit because the useLayoutEffect in src/useAppState.ts:69-89 only fires on realPath changes and the cleanup cancels stale state updates, but React Strict Mode in development will trigger it on every mount.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Acknowledged — concurrent duplicate fetches are harmless (idempotent cache write) and the cancelled flag prevents stale state, so leaving as-is to keep the loader simple.

78 changes: 71 additions & 7 deletions src/useAppState.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,11 @@ import {
shortenCourses,
} from "./appContext";
import { generateCourseSections } from "./scheduler";
import { CURRENT_TERM, courseDataSources } from "./courseData";
import {
CURRENT_TERM,
getCachedCourseIndex,
loadCourseIndex,
} from "./courseData";

function setArrayIdx<T>(arr: Array<T>, idx: number, element: T) {
return arr.map((value, i) => {
Expand Down Expand Up @@ -57,10 +61,32 @@ export function useAppState(): {
// really basic routing
const pathname = useReactPath();
const realPath = pathname === "/" ? CURRENT_TERM : pathname;
const indexedCourses: CourseIndex = useMemo(
() => courseDataSources[realPath] ?? {},
[realPath],
const [indexedCourses, setIndexedCourses] = useState<CourseIndex>(
() => getCachedCourseIndex(realPath) ?? {},
);
// sync from the cache before paint so indexedCourses never lags behind
// the workspace data when the term changes
useLayoutEffect(() => {
let cancelled = false;
const cached = getCachedCourseIndex(realPath);
if (cached) {
setIndexedCourses(cached);
return;
}
setIndexedCourses({});

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

📝 Info: setIndexedCourses({}) in useLayoutEffect triggers an unnecessary re-render on initial mount

At src/useAppState.ts:76, when the cache is not populated (first visit to a term), setIndexedCourses({}) is called even though the state was just initialized to {} via the lazy initializer at line 65. Since React compares state by reference (Object.is), the new {} is a different object from the initial {}, triggering an extra synchronous re-render before paint. This is harmless but wasteful. Comparing against the current state or using a ref to skip the redundant update would eliminate the extra render cycle.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Acknowledged — the extra pre-paint re-render only happens on the first visit to an uncached term and is not user-visible; keeping the code simple here.

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]);
Comment thread
devin-ai-integration[bot] marked this conversation as resolved.
Comment thread
devin-ai-integration[bot] marked this conversation as resolved.
Comment on lines +69 to +89

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

📝 Info: useLayoutEffect with async operation is intentional and correct

Using useLayoutEffect at src/useAppState.ts:69 with an async loadCourseIndex call might look like an anti-pattern, but it serves two purposes: (1) for cache hits, it synchronously sets indexedCourses before paint, preventing a flash of stale data; (2) for cache misses, the synchronous setIndexedCourses({}) at line 76 clears the previous term's data before paint. The async portion (the .then()) naturally completes after paint, which is acceptable since there's no way to avoid a loading state on a cache miss. This is not a bug.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Acknowledged — yes, the pre-paint sync for cache hits/clears is intentional; the async load completing post-paint is the unavoidable loading state.


// 5 blank workspaces by default bc I'm too lazy to implement dynamic tabs and stuff
const defaultWorkspaces = useMemo(
Expand Down Expand Up @@ -98,6 +124,8 @@ export function useAppState(): {
setWorkspaceIdx(storedIdx ? JSON.parse(storedIdx) : 0);
}, [realPath, defaultWorkspaces, setWorkspaces, setWorkspaceIdx]);

const catalogReady = Object.keys(indexedCourses).length > 0;

@devin-ai-integration devin-ai-integration Bot Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

📝 Info: catalogReady blocks all operations during initial async load

The catalogReady guard at line 121 (Object.keys(indexedCourses).length > 0) disables all course operations (add, remove, toggle, lock, navigate arrangements, etc.) while course data is loading. Previously, with static imports, data was immediately available. Now with dynamic imports, there's a loading window where users can see their existing courses (from localStorage) but cannot interact with them. This is actually necessary to prevent crashes — lengthenCourses (src/appContext.ts:80) would produce undefined courseData if indexedCourses is empty. The trade-off is acceptable but represents a UX change: operations silently no-op during loading with no user feedback.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Acknowledged — the guard is intentional to prevent crashes/data loss during the load window. The search Select shows "Loading courses..." as feedback; the load typically completes in well under a second.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

📝 Info: catalogReady conflates 'not yet loaded' with 'empty catalog'

At src/useAppState.ts:127, catalogReady is derived as Object.keys(indexedCourses).length > 0. This means an invalid/unknown term (where loadCourseIndex returns {} at src/courseData.ts:17) produces the same catalogReady = false as a term whose data is still loading. All workspace operations are permanently blocked for invalid terms. This is arguably better than the previous behavior (where lengthenCourses would produce undefined courseData for invalid terms), but it means there's no user-facing distinction between 'loading' and 'unsupported term'. A separate isLoading flag or loading an explicit sentinel would make these states distinguishable.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Acknowledged — invalid terms previously also produced a broken state (undefined courseData); now they safely no-op. A distinct "unsupported term" UI would be a nice follow-up but is out of scope for this perf PR.


const courses = workspaces[workspaceIdx].courses;
const availableTimes: Date[][] = useMemo(
() =>
Expand All @@ -113,6 +141,9 @@ export function useAppState(): {
/** Helper functions to be sent sent through Context */
const addCourse = useCallback(
(newCourse: CourseStorage) => {
if (!catalogReady) {
return;
}
const result = courses.find(
(course) => course.courseData.id === newCourse.courseData.id,
);
Expand Down Expand Up @@ -157,6 +188,7 @@ export function useAppState(): {
);
},
[
catalogReady,
courses,
availableTimes,
workspaces,
Expand All @@ -168,6 +200,9 @@ export function useAppState(): {

const removeCourse = useCallback(
(course: CourseStorage) => {
if (!catalogReady) {
return;
}
let newCourses = courses.filter((currCourse) => currCourse !== course);
const newArrangements = generateCourseSections(
newCourses,
Expand Down Expand Up @@ -199,6 +234,7 @@ export function useAppState(): {
);
},
[
catalogReady,
courses,
availableTimes,
workspaces,
Expand All @@ -210,6 +246,9 @@ export function useAppState(): {

const toggleCourse = useCallback(
(newCourse: CourseStorage) => {
if (!catalogReady) {
return;
}
let newCourses = courses.map((course) => {
if (course.courseData.id === newCourse.courseData.id) {
newCourse.enabled = !newCourse.enabled;
Expand Down Expand Up @@ -253,6 +292,7 @@ export function useAppState(): {
);
},
[
catalogReady,
courses,
availableTimes,
arrangementIdx,
Expand All @@ -265,6 +305,9 @@ export function useAppState(): {

const toggleSectionLock = useCallback(
(newCourse: CourseStorage) => {
if (!catalogReady) {
return;
}
let newCourses = courses.map((course) => {
if (course.courseData.id === newCourse.courseData.id) {
newCourse.locked = !newCourse.locked;
Expand Down Expand Up @@ -304,6 +347,7 @@ export function useAppState(): {
);
},
[
catalogReady,
courses,
availableTimes,
arrangementIdx,
Expand All @@ -315,6 +359,9 @@ export function useAppState(): {
);

const nextArrangement = useCallback(() => {
if (!catalogReady) {
return;
}
const workspace = workspaces[workspaceIdx];
let newIdx = workspace.arrangementIdx;
if (workspace.arrangements.length === 0) {
Expand All @@ -336,9 +383,12 @@ export function useAppState(): {
arrangementIdx: newIdx,
}),
);
}, [workspaces, workspaceIdx, indexedCourses, setWorkspaces]);
}, [catalogReady, workspaces, workspaceIdx, indexedCourses, setWorkspaces]);

const prevArrangement = useCallback(() => {
if (!catalogReady) {
return;
}
const workspace = workspaces[workspaceIdx];
let newIdx = workspace.arrangementIdx;
if (workspace.arrangements.length === 0) {
Expand All @@ -362,10 +412,13 @@ export function useAppState(): {
arrangementIdx: newIdx,
}),
);
}, [workspaces, workspaceIdx, indexedCourses, setWorkspaces]);
}, [catalogReady, workspaces, workspaceIdx, indexedCourses, setWorkspaces]);

const setCourses = useCallback(
(courses: CourseStorage[]) => {
if (!catalogReady) {
return;
}
let newCourses = courses;
const newArrangements = generateCourseSections(
newCourses,
Expand Down Expand Up @@ -396,7 +449,14 @@ export function useAppState(): {
}),
);
},
[availableTimes, workspaces, workspaceIdx, indexedCourses, setWorkspaces],
[
catalogReady,
availableTimes,
workspaces,
workspaceIdx,
indexedCourses,
setWorkspaces,
],
);

const setWorkspace = useCallback(
Expand All @@ -413,6 +473,9 @@ export function useAppState(): {

const updateAvailableTimes = useCallback(
(dayIdx: number, isStart: boolean, day: Date) => {
if (!catalogReady) {
return;
}
const newAvailableTimes = setArrayIdx(availableTimes, dayIdx, [
isStart ? day : availableTimes[dayIdx][0],
isStart ? availableTimes[dayIdx][1] : day,
Expand Down Expand Up @@ -456,6 +519,7 @@ export function useAppState(): {
);
},
[
catalogReady,
courses,
availableTimes,
arrangements,
Expand Down
18 changes: 18 additions & 0 deletions vite.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,24 @@ export default defineConfig({
build: {
sourcemap: true,
outDir: "dist",
chunkSizeWarningLimit: 700,
rollupOptions: {
output: {
manualChunks(id: string) {
if (!id.includes("node_modules")) return;
if (/node_modules\/(react|react-dom|scheduler)\//.test(id))
return "react";
if (id.includes("node_modules/@mui/")) return "mui";
if (id.includes("node_modules/@schedule-x/")) return "schedulex";
if (
/node_modules\/(motion|framer-motion|motion-dom|motion-utils)\//.test(
id,
)
)
return "motion";
},
},
},
Comment on lines +18 to +35

@devin-ai-integration devin-ai-integration Bot Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

📝 Info: Vite manual chunks configuration looks correct

The manualChunks configuration in vite.config.ts splits React, MUI, schedule-x, and motion into separate vendor chunks. This is a standard code-splitting pattern. The react-dom/client entry in the react chunk is valid—Vite/Rollup resolves it as a subpath of react-dom. The chunkSizeWarningLimit: 700 bump from the default 500kB is reasonable given the large JSON data files that will now be separate chunks due to dynamic imports.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

},
server: {
port: 3000,
Expand Down