Skip to content

feat: Store courses across terms#67

Open
squidink7 wants to merge 2 commits intocompsci-adl:mainfrom
squidink7:multi-semester
Open

feat: Store courses across terms#67
squidink7 wants to merge 2 commits intocompsci-adl:mainfrom
squidink7:multi-semester

Conversation

@squidink7
Copy link
Copy Markdown
Contributor

@squidink7 squidink7 commented Dec 12, 2025

Description

Store classes across multiple terms to allow switching terms without clearing classes.

Changes Made

Add useSelectedTerm state hook to store selected term.
Store year and term properties on each course in localStorage.
Add useTermCourses hook to get current courses for the selected term.

Related Issues

Removes the need for #2, contributes to #1

Additional Notes

Unsure how migration of existing timetables saved before this PR is handled, need to test before merging.

@squidink7 squidink7 changed the title feat: Store classes across terms feat: Store courses across terms Dec 12, 2025
@rayokamoto
Copy link
Copy Markdown

I suggest taking a look at #1 which is related

@squidink7
Copy link
Copy Markdown
Contributor Author

I don't think I'll get to doing a tabbed UI at least in this PR, however I could tweak this to save each course with an associated timetable name/id and hopefully avoid having to do another migration later on.
I'd also welcome input on the design of the course store. I see 2 main options for loading courses, we could either:

Store all courses together.

  • simple to implement
  • easier to migrate existing timetables
  • requires loading all courses then filtering down the ones in the current timetable, potentially slow for users with many courses.

or;

Group courses into timetables.

  • requires more work and would be a larger refactor
  • potentially more flexible for adding/deleting/sharing entire timetables.
  • scales well for many courses

I'm not very familiar with this project and the frameworks it uses so I'd hesitate before committing to doing the work required for option 2, however that would probably end up being best long-term from what I can see.

@squidink7
Copy link
Copy Markdown
Contributor Author

Rebased this PR on main and implemented functionality to migrate existing timetables to support term switching. Should be ready for review.

@squidink7 squidink7 marked this pull request as ready for review February 12, 2026 13:00
@squidink7
Copy link
Copy Markdown
Contributor Author

To note: this doesn't use zustand's migration feature (https://zustand.docs.pmnd.rs/integrations/persisting-store-data#migrate), as I only learned of said feature after I finished the implementation. It should be relatively simple to refactor the migration code to use zustand however, so if that would be preferred then I can do as such.

@squidink7
Copy link
Copy Markdown
Contributor Author

It looks like the unit tests are failing because new versions of node don't have localstorage enabled by default for some reason. You can fix it by specifying a localStorage file for node to use like this: NODE_OPTIONS=--localstorage-file=/tmp/localstorage npm run test (the actual file can be any valid file path, the file doesn't even need to exist afaik).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

3 participants