Switch over to use `LessonId` by zehata · Pull Request #4431 · nusmodifications/nusmods

zehata · 2026-06-23T03:32:14Z

Changes validation and serialization/deserialization logic over use V3 format
Reverted changes to optimiser service for it to return V1 serialization strings
Reverted timetable export service to accept ClassNo or LessonId

Context

This activates the LessonId introduced in #4427 to resolve #4283

Implementation

As discussed in #4387 (reply in thread)
Module lesson configs are now stored as

{
  [lessonType: LessonType]: ClassNo[],
}

for non-TA modules, and as

{
  [lessonType: LessonType]: LessonId[],
}

for TA modules
Because ClassNo does not change when the faculty modifies timetables, changes made to lessons will not cause configs to become invalid unless the ClassNo was deleted.

Migration and validation logic has been updated to migrate v1 and v2 configs.

Tested that:

optimiser
https://nusmods.com/timetable/sem-2/share?CS1010E=TUT:09,SEC:2&CS1010S=REC:18,LEC:1,TUT:33
export service

are working.

Other Information

Just pushing this to get Greptile to review it. Not ready for review yet.

… V3 format - Reverted changes to optimiser service for it to return V1 serialization strings - Reverted timetable export service to accept `ClassNo` or `LessonId`

vercel · 2026-06-23T03:32:20Z

@zehata is attempting to deploy a commit to the modsbot's projects Team on Vercel.

A member of the Team first needs to authorize it.

codecov · 2026-06-23T03:35:54Z

Codecov Report

❌ Patch coverage is 92.64706% with 25 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.85%. Comparing base (988c6fd) to head (219e688).
⚠️ Report is 246 commits behind head on master.

Files with missing lines	Patch %	Lines
website/src/utils/timetables/validation.ts	78.84%	11 Missing ⚠️
website/src/views/timetable/TimetableContent.tsx	31.25%	11 Missing ⚠️
website/src/utils/timetables/shareLinks.ts	98.19%	2 Missing ⚠️
website/src/utils/timetables/migration.ts	98.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4431      +/-   ##
==========================================
+ Coverage   54.52%   57.85%   +3.32%     
==========================================
  Files         274      316      +42     
  Lines        6076     7237    +1161     
  Branches     1455     1765     +310     
==========================================
+ Hits         3313     4187     +874     
- Misses       2763     3050     +287

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

greptile-apps · 2026-06-23T03:44:17Z

Greptile Summary

This PR updates requisite parsing so omitted course counts are treated as one-of requirements. The main changes are:

COURSES or MODULES without an explicit count now parses like (1).
Added tests for omitted counts with one course, multiple courses, and separate clauses.
Updated cohort-gated test expectations for the new omitted-count semantics.

Confidence Score: 5/5

No merge-blocking issues were identified in the reviewed changes.

The changes are focused and covered by updated tests for the new omitted-count parsing behavior.

T-Rex Logs

What T-Rex did

Copied the lessonid-sharelinks harness into website/src and ran it with Vitest to exercise the share-links flow.
During the initial run, the base execution failed on the V3 contract with CS1010S v1 ClassNo input parsed to lesson indices [0,3,30].
A head run was performed and it passed, producing actual parsed configs and a serialized mixed share string.
Validated token formatting against the V1 contract; the head output uses V1 tokens REC:18 and LEC:1 with the corresponding default tokens, and the V1 ClassNo token check passed with ordering differences ignored.
Validated input type handling: after the head run, ClassNo strings and LessonId strings are accepted, while numeric indexes are rejected with the error 'must be a string', and the validation harness runtime script used for both commits is present.

_{Ran code and verified through T-Rex}

_{Reviews (18): Last reviewed commit: "Merge branch 'master' into lesson-id" | Re-trigger Greptile}

… one lesson across all lesson types.

…produce a valid `ClassNo`

… does not appear when modifying a lesson

…odifyTaCell

zehata · 2026-06-23T07:43:07Z

Linter didn't catch the debug console.log that was left behind...

greptile-apps · 2026-06-23T08:00:04Z

Want your agent to iterate on Greptile's feedback? Try greploops.

zehata · 2026-06-23T08:26:15Z

I ran the tests again on my machine, and couldn't reproduce the test failure. Can we re-run the CircleCI test?

leslieyip02

This is gonna take a while. Will continue reviewing tomorrow. Mostly seems good so far though!

- Added recovery path for empty TA module lesson configs - Added tests for enabling TA modules

zehata · 2026-06-24T10:08:12Z

I haven't actually reviewed this back yet. I'm just pushing it because I have something else to do tonight.

leslieyip02

I've finished looking at most files, and the implementations seems fine. I'm just left with sharing/serialization/deserialization. I'll give it another pass tomorrow.

leslieyip02 · 2026-06-24T14:58:37Z

 * Hydrate timetable lessons with interactability info\
 * See type defintion of {@link InteractableLesson} for properties added
 */
 export function getInteractableLessons(


Can we refactor this function for better readability?

Extract some of the more complex boolean expressions into helpers (e.g. canBeSelectedAsActiveLesson, canBeAddedToLessonConfig, etc).

I'm not a fan of the triply-nested mapValues. Maybe we can extract each level to a helper (e.g. getInteractableLesson(lesson, lessonId, ...), groupInteractableLessonsByLessonId(lessonMap, ...), etc)?

leslieyip02 · 2026-06-24T16:19:35Z

+      if (!isV1(lessonsIdentifier)) {
+        const configIsV2 = isV2(lessonsIdentifier);
+        const migratedLessonConfig: [ClassNo] | LessonId[] = configIsV2
+          ? migrateLessonTypeLessonsFromLessonIndicesToLessonIds(


I think we should split the logic in migrateModuleLessonConfig into migrateLessonConfigV1, migrateLessonConfigV2 and migrateLessonConfig for better readability.

- Removed some frivolous docstring from `interactabilityHydration.ts`, adjusted logic

zehata · 2026-06-25T14:29:39Z

~~@leslieyip02 Sorry I am making some refactors to deserialization right now~~ Done as below

…hat classNo

When a lesson is active - For non-TA modules, only lessons in the config will show up for other lesson types, thus `alreadyAddedToLessonConfig` is necessarily true, and the function would have returned before that check - For TA modules, we show all lessons regardless of lesson type Thus, whether the lesson is of the same lesson type as the active lesson does not affect the result

zehata · 2026-06-25T16:10:59Z

Thanks for covering for me for some of the changes.

I am still going to be doing a final manual check later, just running everything manually as a final check.

vercel · 2026-06-27T08:58:33Z

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments

Project	Deployment	Actions	Updated (UTC)
nusmods-export	Ignored	Preview	Jun 28, 2026 2:14am
nusmods-website	Ignored	Preview	Jun 28, 2026 2:14am

leslieyip02 · 2026-06-27T09:41:23Z

I've updated the export deployment to point at the website deployment. This should simplify testing the export service: https://nusmods-export-iu6oqsuwj-modsbots-projects.vercel.app/.

zehata · 2026-06-27T14:42:51Z

@leslieyip02 I have done a manual check (which is by no means thorough) as a sanity check. I tested migration from v2 config with multiple populated semesters, validating a malformed config, running optimiser and the export service. I did not notice anything out of the blue, so I think it should be ok.

leslieyip02

LGTM. Thanks for the massive effort on this issue! Let's keep monitoring this.

zehata · 2026-06-28T02:50:41Z

Thanks a lot for reviewing and helping with the changes!

- Changes validation and serialization/deserialization logic over use…

1b39b81

… V3 format - Reverted changes to optimiser service for it to return V1 serialization strings - Reverted timetable export service to accept `ClassNo` or `LessonId`

greptile-apps Bot reviewed Jun 23, 2026

View reviewed changes

Comment thread website/src/utils/timetables/migration.ts Outdated

Comment thread website/src/reducers/timetables.ts Outdated

Comment thread website/src/types/timetables.ts

zehata added 10 commits June 23, 2026 12:23

Updated documentations

6dbeafd

Added guard against malformed ADD_LESSON actions

0219dcc

Fixed incorrect migration logic caused by order-sensitive check

3922ce0

Fixed missing AND condition for validity check

82a72cf

Fixed outdated documentation. TA module configs only require at least…

314c34e

… one lesson across all lesson types.

Generate recovery ClassNo if deserializing lesson details does not …

94b1e51

…produce a valid `ClassNo`

Fixed issue caused by copying logic without modifying the variable name

5257081

Fixed regression where lessons from other lesson types of a TA module…

3bf8144

… does not appear when modifying a lesson

Removed leftoever debug log

092d409

Fixed issue where only lessons of the same lesson type is passed to m…

d9f34aa

…odifyTaCell

greptile-apps Bot reviewed Jun 23, 2026

View reviewed changes

Comment thread website/src/actions/timetables.ts

Comment thread website/src/utils/timetables/migration.ts Outdated

leslieyip02 requested changes Jun 23, 2026

View reviewed changes

Comment thread website/src/actions/timetables.test.ts Outdated

Comment thread website/src/actions/timetables.ts

Comment thread website/src/types/timetables.ts

Comment thread website/src/utils/timetables/lessonHydration.ts Outdated

Comment thread website/src/utils/timetables/index.ts Outdated

- Added branded types, adjusted module lesson config typedef

d053312

- Added recovery path for empty TA module lesson configs - Added tests for enabling TA modules

leslieyip02 added 6 commits June 24, 2026 21:38

Add unit tests for actions.addModule

582a123

Add defensive randomClassNo check to randomModuleLessonConfig

9b07f65

Add docstring and unit test for isClassNo

fa330fe

Tighten tests for getInteractableLessons

41150a3

Update serializeLessonDetails return type

cdde184

Update docstrings

6440707

leslieyip02 reviewed Jun 24, 2026

View reviewed changes

Refactor interactability hydration logic

7d0a4ed

- Refactor migration.ts

ad66863

- Removed some frivolous docstring from `interactabilityHydration.ts`, adjusted logic

Refactor deserialization logic

ced8ce6

zehata commented Jun 25, 2026

View reviewed changes

Comment thread website/src/utils/timetables/shareLinks.ts

greptile-apps Bot reviewed Jun 25, 2026

View reviewed changes

Comment thread website/src/utils/timetables/shareLinks.ts

zehata added 3 commits June 25, 2026 22:59

Ensure set of lessons deserialized is identical to the lessons with t…

b481ec7

…hat classNo

Fixed outdated serialization documentation

5ee4890

Merge branch 'master' into lesson-id

f7f7ff0

vercel Bot deployed to Preview – nusmods-export June 27, 2026 09:01 View deployment

vercel Bot deployed to Preview – nusmods-website June 27, 2026 09:01 View deployment

leslieyip02 approved these changes Jun 28, 2026

View reviewed changes

Merge branch 'master' into lesson-id

219e688

leslieyip02 merged commit 71beeb4 into nusmodifications:master Jun 28, 2026
7 checks passed

Uh oh!

Conversation

zehata commented Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Implementation

Other Information

Uh oh!

vercel Bot commented Jun 23, 2026

Uh oh!

codecov Bot commented Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

greptile-apps Bot commented Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

T-Rex Logs

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zehata commented Jun 23, 2026

Uh oh!

greptile-apps Bot commented Jun 23, 2026

Uh oh!

Uh oh!

Uh oh!

zehata commented Jun 23, 2026

Uh oh!

leslieyip02 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zehata commented Jun 24, 2026

Uh oh!

leslieyip02 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leslieyip02 Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

leslieyip02 Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

zehata commented Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zehata commented Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vercel Bot commented Jun 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leslieyip02 commented Jun 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zehata commented Jun 27, 2026

Uh oh!

leslieyip02 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zehata commented Jun 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

zehata commented Jun 23, 2026 •

edited

Loading

codecov Bot commented Jun 23, 2026 •

edited

Loading

greptile-apps Bot commented Jun 23, 2026 •

edited

Loading

leslieyip02 left a comment •

edited

Loading

zehata commented Jun 25, 2026 •

edited

Loading

zehata commented Jun 25, 2026 •

edited

Loading

vercel Bot commented Jun 27, 2026 •

edited

Loading

leslieyip02 commented Jun 27, 2026 •

edited

Loading