Change representation of lesson configs from classNo to lesson indices #4225

zehata · 2025-09-05T13:04:12Z

Implementation

The lesson index of a lesson is its index in the timetable in the API response.
Lesson configs are now saved as mapping of lesson types to arrays of lesson indices. Example:

{
    CS1010S: {
        Lecture: [53],
        Tutorial: [19],
        Recitation: [32],
    },
}

TA configs have been removed. TA modules are now saved as an array of module codes. ["CS1010S"]
TA lessons will use the same lesson config as non-TA modules. TA lessons can now be added and removed individually. Example:

{
    CS1010S: {
        Lecture: [53],
        Tutorial: [19],
        Recitation: [32, 33],
    },
}

When a TA module is changed back to non-TA, it will use the closest matching valid non-TA lesson config.
Current format classNo lesson configs and TA lesson configs will be migrated to the new lessonGroup format
Serialization format has been changed

Change	Before	After
`(..., ...)` denotes arrays of lesson indices	`?CG1111A=LAB:03`	`?CG1111A=LAB:(5,6)`
`;` separates different lesson types	`?CS1010S=LEC:1,TUT:1`	`?CS1010S=LEC:(53);TUT:(41)`
TA modules serialized similar to hidden	`?CG1111A=LAB:03&ta=CG1111A(LAB:03)`	`?CG1111A=LAB:(5,6)&ta=CG1111A`

Current share link serialization format will be deserialized into the new format
Optimiser has been updated to share links in the new lessonGroup format.

Resolves

All modules can now be set as TA modules.
Resolves #3929 and #3930 , from which this implementation came from

Before	After

Issue with deserialization of TA module lesson configs with multiple lessons in a module, from #4214 (comment).

Before	After

All lessons of a single classNo now show up, and are highlighted when one of them is hovered over. Provided that the module is not set as a TA mod. Resolves #4168.

Before	After

TA module lessons can now be added and removed individually.
User request from: https://t.me/NUSMods/12508 (No issue opened, at least I can't find one)

Before	After

Known issues

Array Index Stability

Using the index of an array comes with the inherent issue of stability. Furthermore, it may fail silently, since lessons of the same lesson type are typically placed next to each other in the timetable list in the API response. So the module lesson config may remain valid even after the module timetable had been updated.

An alternative is to serialize additional info as proposed in #3930 . It will make share links very long, because of the serialization of weeks information.

Personally I think the lessons provided by the faculties are stable enough, and they will most likely not add an additional class in the middle of the semester, so I think it will be better idea to create UI to inform users that the module info has changed if it changes, which can be done simply by hashing the module info during scraping.

vercel · 2025-09-05T13:04:17Z

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

Project	Deployment	Preview	Comments	Updated (UTC)
nusmods-export	Ready	Preview	Comment	Oct 18, 2025 6:52pm
nusmods-website	Ready	Preview	Comment	Oct 18, 2025 6:52pm

💡 Enable Vercel Agent with $100 free credit for automated AI reviews

vercel · 2025-09-05T13:04: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 · 2025-09-05T13:07:15Z

Codecov Report

❌ Patch coverage is 86.77130% with 59 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.47%. Comparing base (988c6fd) to head (539f662).
⚠️ Report is 147 commits behind head on master.

Files with missing lines	Patch %	Lines
website/src/views/timetable/TimetableContent.tsx	42.46%	42 Missing ⚠️
...site/src/views/timetable/TimetableModulesTable.tsx	14.28%	6 Missing ⚠️
website/src/views/tetris/board.ts	0.00%	3 Missing ⚠️
website/src/reducers/timetables.ts	94.11%	2 Missing ⚠️
website/src/utils/timetables.ts	99.06%	2 Missing ⚠️
website/src/actions/export.ts	0.00%	1 Missing ⚠️
...c/views/components/module-info/LessonTimetable.tsx	66.66%	1 Missing ⚠️
website/src/views/settings/previewTimetable.ts	0.00%	1 Missing ⚠️
website/src/views/timetable/TimetableContainer.tsx	97.67%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4225      +/-   ##
==========================================
+ Coverage   54.52%   56.47%   +1.94%     
==========================================
  Files         274      297      +23     
  Lines        6076     6901     +825     
  Branches     1455     1661     +206     
==========================================
+ Hits         3313     3897     +584     
- Misses       2763     3004     +241

☔ View full report in Codecov by Sentry.
📢 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.

zehata · 2025-09-09T09:23:49Z

Changes from #4226 is only a bugfix. The delta from this PR will apply to the fixed master too.

zehata · 2025-10-08T03:05:46Z

@leslieyip02 Any updates?

leslieyip02 · 2025-10-10T18:56:44Z

Sorry about the delay, I'm a bit busy at the moment but I will try to review once I have the time. Thanks for your patience!

leslieyip02

No major comments on the implementation, just a few things to add/fix.

Also, just in case we missed something in the migration logic, I think we should wait until the end of the semester before merging this PR (lest we nuke everyone's active timetables).

That said, I really appreciate the thought and effort that went into this! Let me know if you want help for anything (e.g. implementing tests).

leslieyip02 · 2025-09-07T17:53:00Z

website/api/optimiser/_solver/nusmods_link.go

 			abbrev := constants.LessonTypeAbbrev[strings.ToUpper(lessonType)]

-			lessonParams = append(lessonParams, fmt.Sprintf("%s:%s", abbrev, classNo))
+			lessonParams = append(lessonParams, fmt.Sprintf("%s:%s", abbrev, "("+strings.Trim(strings.Join(strings.Fields(fmt.Sprint(lessonIndex)), ","), "[]"))+")")


Could we make a function for this instead? The nesting makes it harder to read.

leslieyip02 · 2025-09-07T17:55:33Z

website/api/optimiser/_solver/nusmods_link.go

 		}
 		if len(lessonParams) > 0 {
-			moduleParams = append(moduleParams, fmt.Sprintf("%s=%s", moduleCode, strings.Join(lessonParams, ",")))
+			moduleParams = append(moduleParams, fmt.Sprintf("%s=%s", moduleCode, strings.Join(lessonParams, ";")))


I think we can define a constant for the separator.

leslieyip02 · 2025-10-18T18:59:53Z

website/src/actions/timetables.test.ts

+
+    action(dispatch, () => state);
+
+    expect(dispatch).toHaveBeenCalled();


Other than just checking that dispatch is called, I think we could also add a toMatchSnapshot() check in this test.

leslieyip02 · 2025-10-18T19:24:43Z

website/src/types/timetables.ts


-//  ModuleLessonConfig is a mapping of lessonType to ClassNo for a module.
 export type ModuleLessonConfig = {
+  [lessonType: LessonType]: LessonIndex[];
+};
+
+//
+/**
+ * ModuleLessonConfig is the v1 representation of module configs\
+ * It is a mapping of lessonType to classNo\
+ * It is only used for type annotations in the migration logic
+ */
+export type ClassNoModuleLessonConfig = {
  [lessonType: LessonType]: ClassNo;
 };

-// SemTimetableConfig is the timetable data for each semester.
 export type SemTimetableConfig = {
  [moduleCode: ModuleCode]: ModuleLessonConfig;
 };

-// TaModulesConfig is a mapping of moduleCode to the TA's lesson types.
-export type TaModulesConfig = {
+/**
+ * ClassNoSemTimetableConfig is the v1 representation of semester timetables\
+ * It is a mapping of module code to the module config\
+ * It is only used for type annotations in the migration logic
+ */
+export type ClassNoSemTimetableConfig = {
+  [moduleCode: ModuleCode]: ClassNoModuleLessonConfig;
+};
+
+export type TaModulesConfig = ModuleCode[];
+
+/**
+ * ClassNoTaModulesConfig is the v1 representation of TA modules\
+ * It is a mapping of moduleCode to the TA's lesson types\
+ * It is only used for type annotations in the migration logic
+ */
+export type ClassNoTaModulesConfig = {
  [moduleCode: ModuleCode]: [lessonType: LessonType, classNo: ClassNo][];
 };


I think it could be clearer to just name these ModuleLessonConfigV1/ModuleLessonConfigV2 and TaModulesConfigV1/TaModulesConfigV2. It would make it more explicit that the V1 versions are no longer used and only kept for compatibility.

Also, I personally think ClassNoTaModulesConfig is a bit too verbose and the NoTa substring in the name could be misleading.

leslieyip02 · 2025-10-18T19:38:32Z

website/src/actions/timetables.ts

+export function changeLesson(
+  semester: Semester,
+  moduleCode: ModuleCode,
+  lessonType: LessonType,
+  lessonIndices: LessonIndex[],
+) {
+  return setLesson(semester, moduleCode, lessonType, lessonIndices);
 }


Maybe we can just rename setLesson to changeLesson instead of adding this function. setLesson doesn't seem to be used anywhere else.

leslieyip02 · 2025-10-18T22:54:31Z

website/src/utils/timetables.ts

+export const groupLessonsByLessonTypeByClassNo = (
+  lessonsWithIndex: readonly RawLessonWithIndex[],
+): LessonsByLessonTypeByClassNo => {
+  const lessonsByLessonType = groupBy(lessonsWithIndex, 'lessonType');
+  return mapValues(lessonsByLessonType, (lessonsWithLessonType) => {
+    const lessonsByClassNo = groupBy(lessonsWithLessonType, 'classNo');
+    return mapValues(lessonsByClassNo, (lessonsWithClassNo) =>
+      map(lessonsWithClassNo, 'lessonIndex'),
+    );
+  });
+};
+


This is grouping lesson indices, and not lessons, so we might want to rename this.

LessonsByLessonTypeByClassNo and groupLessonsByLessonTypeByClassNo are both quite verbose, so one possibility is to rename them to LessonsIndicesMap and makeLessonIndicesMap(). Then we define a getLessonIndices(lessonIndicesMap, lessonType, classNo) helper function to get the lesson indices. That way, the exact order of grouping doesn't matter since it will handled by the helper.

leslieyip02 · 2025-10-18T22:55:15Z

website/src/utils/timetables.ts

-export function deserializeTimetable(serialized: string): SemTimetableConfig {
-  const params = qs.parse(serialized);
-  return mapValues(omit(params, ['hidden', 'ta']), parseModuleConfig);
+// TODO merge logic for TA modules and hidden modules


nusmods/website/src/views/routes/paths.ts

Lines 25 to 34 in 3f774f3

export function timetableShare(

semester: Semester,

timetable: SemTimetableConfig,

hiddenModules: ModuleCode[],

taModules: TaModulesConfig,

): string {

// Convert the list of hidden modules to a comma-separated string, if there are any

const serializedHidden = hiddenModules.length === 0 ? '' : serializeHidden(hiddenModules);

const serializedTa = isEmpty(taModules) ? '' : serializeTa(taModules);

Are you referring to abstracting the serialization + empty list logic into a common helper function?

export function serializeModuleListWithKey(modules: ModuleCode[], key: string): string { return isEmpty(modules) ? '' : `&${key}=${modules.join(LESSON_SEP)}`; }

If so, I think that would be worth doing.

leslieyip02 · 2025-10-18T23:19:44Z

website/src/utils/timetables.ts

+  // CS2100(TUT:2,TUT:3,LAB:1),CS2107(TUT:8)
+  const trimmedSerializedTaModulesConfig = taSerialized.slice(0, -1);
+  // CS2100(TUT:2,TUT:3,LAB:1),CS2107(TUT:8
+  return reduce(
+    trimmedSerializedTaModulesConfig.split(`)${LESSON_SEP}`),
+    (accumulatedTaTimetableConfig, moduleConfig) => {
+      // CS2100(TUT:2,TUT:3,LAB:1
+      // CS2107(TUT:8


Truncating the last character feels a bit hacky to me. Instead, maybe we could use a regex:

taSerialized.split(/(?<=\)),/); // ['CS2100(TUT:2,TUT:3,LAB:1)', 'CS2107(TUT:8)']

leslieyip02 · 2025-10-18T23:30:48Z

website/src/views/components/module-info/AddModuleDropdown.test.tsx

  test('should show remove button when the module is in timetable', () => {
    // eslint-disable-next-line no-useless-computed-key
-    const container = make(CS3216, { [1]: { CS3216: { Lecture: '1' } } });
+    const container = make(CS3216, { [1]: { CS3216: { Lecture: [0] } } });


I think the test at line 62 needs to be edited too.

nusmods/website/src/views/components/module-info/AddModuleDropdown.test.tsx

Lines 62 to 65 in 3f774f3

test('should show "loading" when the module is added timetable', () => {

// eslint-disable-next-line no-useless-computed-key

const timetables = { [1]: { CS3216: { Lecture: '1' } } };

const container = make(CS3216);

leslieyip02 · 2025-10-18T23:46:33Z

website/src/views/timetable/TimetableContent.tsx

-        colorIndex: colors[lesson.moduleCode],
-        isTaInTimetable: this.isTaInTimetable(lesson.moduleCode),
-      }),
+    const coloredTimetableLessons: InteractableLesson[] = this.getInteractableLessons(


Let's rename this to interactableLessons to reflect the change.

Change representation of lesson configs from classNo to lesson indices

32b5f05

vercel bot deployed to Preview – nusmods-export September 5, 2025 13:04 View deployment

Fix: TA Lessons being highlighted when only classNo matches

2bcf233

vercel bot deployed to Preview – nusmods-export September 5, 2025 23:08 View deployment

zehata added 2 commits September 6, 2025 11:45

Fix: Rendering crashes on first load before migration

8ef6c06

Fix: Empty TA and Hidden module configs do not overwrite current config

b1f27a1

vercel bot deployed to Preview – nusmods-export September 6, 2025 04:12 View deployment

Fix: Imported TA and Hidden module configs not removed after importing

bd1a455

vercel bot deployed to Preview – nusmods-export September 6, 2025 05:38 View deployment

zehata marked this pull request as ready for review September 6, 2025 05:43

leslieyip02 self-requested a review September 7, 2025 17:50

vercel bot deployed to Preview – nusmods-website September 7, 2025 18:01 View deployment

Merge branch 'master' into lesson-groups

e0d9ba3

vercel bot deployed to Preview – nusmods-export September 9, 2025 09:19 View deployment

vercel bot deployed to Preview – nusmods-website September 12, 2025 21:24 View deployment

Merge branch 'master' into lesson-groups

539f662

leslieyip02 force-pushed the lesson-groups branch from debd302 to 539f662 Compare October 18, 2025 18:20

vercel bot deployed to Preview – nusmods-website October 18, 2025 18:52 View deployment

vercel bot deployed to Preview – nusmods-export October 18, 2025 18:52 View deployment

leslieyip02 requested changes Oct 18, 2025

View reviewed changes

leslieyip02 mentioned this pull request Oct 20, 2025

Enable Audit mode for courses #4191

Open


		action(dispatch, () => state);

		expect(dispatch).toHaveBeenCalled();

	export function timetableShare(
	semester: Semester,
	timetable: SemTimetableConfig,
	hiddenModules: ModuleCode[],
	taModules: TaModulesConfig,
	): string {
	// Convert the list of hidden modules to a comma-separated string, if there are any
	const serializedHidden = hiddenModules.length === 0 ? '' : serializeHidden(hiddenModules);
	const serializedTa = isEmpty(taModules) ? '' : serializeTa(taModules);

	test('should show "loading" when the module is added timetable', () => {
	// eslint-disable-next-line no-useless-computed-key
	const timetables = { [1]: { CS3216: { Lecture: '1' } } };
	const container = make(CS3216);

Change representation of lesson configs from classNo to lesson indices #4225

Are you sure you want to change the base?

Change representation of lesson configs from classNo to lesson indices #4225

Conversation

zehata commented Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Implementation

Resolves

Known issues

Array Index Stability

Uh oh!

vercel bot commented Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vercel bot commented Sep 5, 2025

Uh oh!

codecov bot commented Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zehata commented Sep 9, 2025

Uh oh!

zehata commented Oct 8, 2025

Uh oh!

leslieyip02 commented Oct 10, 2025

Uh oh!

leslieyip02 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

zehata commented Sep 5, 2025 •

edited

Loading

vercel bot commented Sep 5, 2025 •

edited

Loading

codecov bot commented Sep 5, 2025 •

edited

Loading