Skip to content

Conversation

@KevinWu098
Copy link
Member

@KevinWu098 KevinWu098 commented Mar 16, 2025

Summary

Refactors away the SkeletonSchedule from our (soon-to-be legacy) Schedules store to a Zustand store. Removes event emitters, updates logic for reading and writing skeleton schedules.

This PR largely removes the old event emitter + event listener + useEffect pattern we had in many of our components in favor of Zustand's useStore hook. In some cases, this required additional logic changes (e.g. ScheduleSelect), but the PR is mostly refactoring.

Test Plan

Skeleton Schedules trigger when a user's schedule cannot be fully loaded because the AnteaterAPI is down. This results in us only having access to select fields (what AA stores on our DB), which are primarily the schedule names, custom events, schedule notes, and the course ID of the added courses.

For testing, you'll need to block requests to the AnteaterAPI via Chrome Dev Tools, then attempt to load a schedule with courses added. Fallback does not trigger if there are no courses, as it would make no requests to the API. Blocking the keyword websoc should work here.

Fallback should trigger immediately when the API request fails, displaying schedules correctly and blocking features which could require API data (e.g. save, load, editing).

Issues

Closes #1208

<MuiPickersUtilsProvider utils={DateFnsUtils}>
<PatchNotes />
<InstallPWABanner />
{/* <InstallPWABanner /> */}
Copy link
Member Author

Choose a reason for hiding this comment

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

InstallPWABanner was already disabled via #1213, this just further ensures none of its logic is applied unnecessarily

Copy link
Member

Choose a reason for hiding this comment

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

Just throw it on another PR. It's one line. I'll review it in a snap.

@KevinWu098 KevinWu098 marked this pull request as ready for review April 10, 2025 21:41
@KevinWu098 KevinWu098 changed the title feat: fallback refactor: fallback Apr 10, 2025
@KevinWu098 KevinWu098 changed the title refactor: fallback refactor: fallback schedules Apr 10, 2025
@github-actions github-actions bot added the Stale label May 11, 2025
@MinhxNguyen7 MinhxNguyen7 self-requested a review May 12, 2025 20:00
@MinhxNguyen7 MinhxNguyen7 added codefix Refactoring or improving the codebase and removed Stale labels May 12, 2025
Copy link
Member

@MinhxNguyen7 MinhxNguyen7 left a comment

Choose a reason for hiding this comment

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

This doesn't seem to work. I tried to load the test schedule (which has classes), and nothing showed up in the added pane.

@MinhxNguyen7
Copy link
Member

@KevinWu098 Reminder

@KevinWu098 KevinWu098 requested a review from MinhxNguyen7 May 28, 2025 21:16
Copy link
Member

@MinhxNguyen7 MinhxNguyen7 left a comment

Choose a reason for hiding this comment

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

This is what I see when I try to load the user test, even without blocking requests. I think you broke schedule loading in general,
image

Have you tested this?

<MuiPickersUtilsProvider utils={DateFnsUtils}>
<PatchNotes />
<InstallPWABanner />
{/* <InstallPWABanner /> */}
Copy link
Member

Choose a reason for hiding this comment

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

Just throw it on another PR. It's one line. I'll review it in a snap.

}

deleteCustomEvent(customEventId: number, scheduleIndices: number[]) {
deleteCustomEvent(customEventId: number | string, scheduleIndices: number[]) {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this suddenly accepting a string? What's the type for a custom event on the backend? It should just be consistent instead of allowing anything

Copy link
Member Author

Choose a reason for hiding this comment

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

This fixes some type errors we've had for a while post-RDS

*/
doesCustomEventExistInSchedule(customEventId: number, scheduleIndex: number) {
for (const customEvent of this.schedules.at(scheduleIndex)?.customEvents ?? []) {
doesCustomEventExistInSchedule(customEventId: string | number, scheduleIndex: number) {
Copy link
Member

Choose a reason for hiding this comment

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

Same thing about the types

@KevinWu098
Copy link
Member Author

This is what I see when I try to load the user test, even without blocking requests. I think you broke schedule loading in general, image

Have you tested this?

Loading works locally — isn't staging broken?

@KevinWu098 KevinWu098 requested a review from MinhxNguyen7 May 30, 2025 01:34
@MinhxNguyen7
Copy link
Member

isn't staging broken

Oops. I forgot about that

@github-actions github-actions bot added the Stale label Jul 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

codefix Refactoring or improving the codebase

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor Skeleton Mode

3 participants