Introduce `LessonId`s by zehata · Pull Request #4427 · nusmodifications/nusmods

zehata · 2026-06-19T10:37:42Z

Generate ModuleLessonMap after module data is fetched, which stores a map of RawLesson to their LessonId so that serializeLessonDetails only needs to be called once.
Generate ModuleLessonMap for sample modules as well.
Generate ModuleLessonMap in the export service so lessons can be rendered.
Currently these are inactive, and would only be used in a future PR.

Context

This is part of the changes meant to fix #4283
Please refer to #4387 for additional info

Implementation

Other than adding the types, it also generates the ModuleLessonMap when modules are downloaded. This memoizes the LessonId so that they do not have to be generated every time they are used. This also applies to the export service. This also includes a couple of helper functions for serializing and deserializing LessonIds.

LessonIds are serialization of all a lesson details, except its module info and lesson type, please refer to #4292 (comment) for why this was chosen.

        {
          "classNo": "22",
          "lessonType": "Tutorial", // not serialized
          "weeks": [2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13],
          "day": "Wednesday",
          "startTime": "0900",
          "endTime": "1000",
          "venue": "S16-0518"
        },

would serialize to
22|TUE|0900|1000|S16-0518|2_3_4_5_6_7_8_9_10_11_12_13.

Serialization and deserialization can be quite expensive. So we calculate and store a map of the LessonType to LessonIds to lessons in SemesterData.

// Semester-specific information of a module.
export type SemesterData = {
  // ...
  readonly lessonMap: LessonMap<RawLesson>;
};

Other Information

Currently these changes are inactive, they will be used in the a future PR. The delta to switch over to use LessonIds is already large enough as is.

- Generate `ModuleLessonMap` after module data is fetched, which stores a map of `RawLesson` to their `LessonId` so that serializeLessonDetails only needs to be called once. - Generate `ModuleLessonMap` for sample modules as well. - Generate `ModuleLessonMap` in the export service so lessons can be rendered. - Currently these are inactive, and would only be used in a future PR.

vercel · 2026-06-19T10:37:47Z

@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-19T10:41:13Z

Codecov Report

❌ Patch coverage is 87.62887% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.76%. Comparing base (988c6fd) to head (7db8157).
⚠️ Report is 240 commits behind head on master.

Files with missing lines	Patch %	Lines
website/src/utils/timetables/lessonId.ts	85.36%	12 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4427      +/-   ##
==========================================
+ Coverage   54.52%   56.76%   +2.23%     
==========================================
  Files         274      317      +43     
  Lines        6076     7059     +983     
  Branches     1455     1699     +244     
==========================================
+ Hits         3313     4007     +694     
- Misses       2763     3052     +289

☔ 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.

leslieyip02 · 2026-06-19T11:02:30Z

@greptileai

greptile-apps · 2026-06-19T11:07:04Z

Greptile Summary

This PR introduces LessonId as a serialization of lesson details (excluding lessonType), adds ModuleLessonMap to SemesterData, and pre-computes that map in the website reducer, mock modules, and the export service — though the maps are currently unused and gated behind a reference to a future PR.

Adds serializeLessonDetails, deserializeLessonDetails, parseWeeks, and related helpers in website/src/utils/timetables/lessonId.ts, with a parallel duplicate in export/src/makeModuleLessonMap.ts; makeModuleLessonMap is now called at load time in the reducer and export service so LessonIds don't need to be recomputed on every use.
Extends SemesterData with a required readonly lessonMap field and adds recovery helpers (getRecoveryClassNo, getRecoverySerializedLessonDetails, getClosestClassNo) for handling invalid timetable configs.
Updates the mock module factory and adds a new test file covering round-trip serialization and recovery behaviour.

Confidence Score: 5/5

Safe to merge — all new code paths are explicitly marked inactive and gated behind a future PR, so no changed logic runs in production today.

The serialization round-trips are well-covered by the new test suite, regex anchoring issues from earlier rounds have been addressed, and the export serializer now matches the website for empty-weeks encoding. The remaining notes are a JSDoc example with a slightly wrong serialized string and a mock factory whose lessonMap values carry lessonIndex from baked-in JSON fixtures — neither affects runtime while the feature stays inactive.

website/src/mocks/modules/index.ts — the mock does not replicate the reducer pattern of building lessonMap before adding lessonIndex, so test fixtures document a shape that differs from production.

Prompt To Fix All With AI

Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 3
website/src/__mocks__/modules/index.ts:17-24
**Mock `lessonMap` values include `lessonIndex`, production values will not**

The real reducer builds `lessonMap` from `semesterData.timetable` *before* adding `lessonIndex`, so production `lessonMap` values are plain `RawLesson` objects without `lessonIndex`. The mock skips the `lessonIndex`-augmentation step, but the JSON fixtures already have `lessonIndex` baked in (e.g. `CS3216.json` has `"lessonIndex": 0`), so `makeModuleLessonMap(semesterData.timetable)` here embeds `lessonIndex` in every map value. The test in `modules.test.ts` then asserts `lessonIndex: 0` inside the lessonMap, documenting a shape that diverges from what will be stored in production. When the map is actually consumed in a future PR, consumers relying on these test fixtures will find the values include a field the runtime won't have.

### Issue 2 of 3
website/src/utils/timetables/lessonId.ts:111-112
**`serializeWeekRange` JSDoc shows an incorrect example for empty `weeks`**

The comment claims an empty `weeks` array serializes to `'2025-01-13_2025-02-14_'`, but the actual output always includes the `weekInterval` segment (defaulting to `0`), producing `'2025-01-13_2025-02-14_0__'` (when `serializeWeekNumbers([])` returns `'_'`). The test at line 157 confirms this. The misleading example could cause confusion when hand-crafting or validating LessonIds.

```suggestion
 * If weeks is an empty array, the serialized string would be `2025-01-13_2025-02-14_0__` (weekInterval defaults to `0`, and serializeWeekNumbers([]) returns `_`)\
 * If weeks is undefined, the serialized string would be `2025-01-13_2025-02-14_0`, without the trailing separator\
```

### Issue 3 of 3
export/src/makeModuleLessonMap.ts:72-73
**Same `serializeWeekRange` JSDoc inaccuracy as in the website's `lessonId.ts`**

The description of the empty-weeks case is copied from the website code and carries the same error. The serialized output includes the `weekInterval` value before the trailing `_` separator.

```suggestion
 * If `weeks` is an empty array, the serialized string would be `2025-01-13_2025-02-14_0__` (weekInterval defaults to `0`, and serializeWeekNumbers([]) returns `_`)\
 * If `weeks` is undefined, the serialized string would be `2025-01-13_2025-02-14_0`, without the trailing separator\
```

_{Reviews (4): Last reviewed commit: "Modified `getRecoverySerializedLessonDet..." | Re-trigger Greptile}

…r serialization and deserialization of lessons.

zehata · 2026-06-19T23:55:02Z

@leslieyip02 I have made some changes, and you might want to look at the corresponding changes I made to address the issues that Greptile generated, or retrigger the review.

leslieyip02 · 2026-06-21T06:10:06Z

@greptileai

leslieyip02

Mostly LGTM, just some minor nits.

leslieyip02 · 2026-06-21T06:30:21Z

+
+describe('serialize/parse weeks', () => {
+  test('serialized weekRange should be deserialized to the same weekRange', () => {
+    const weekRanges: WeekRange[] = [


I think we should add 1 more test case where with only start and end. (BME5802 is an example with just start and end in its week range).

leslieyip02 · 2026-06-21T06:46:45Z

+  );
+};
+
+const deserializeWeekNumbers = async (serializedWeekNumbers: string): Promise<number[]> => {


Does this need to be async?

No, I don't mind rewriting with try catch

leslieyip02 · 2026-06-21T06:47:10Z

+ * @param serializedWeeks
+ * @returns
+ */
+export const parseWeeks = async (serializedWeeks: string): Promise<Weeks> => {


Does this need to be async?

leslieyip02 · 2026-06-21T06:54:37Z

+ * @param lessonsWithLessonType lessons with the same lesson type to generate a valid lesson config from
+ * @returns a `ClassNo`. The current implementation generates a config containing the first `ClassNo`
+ */
+export function getRecoveryClassNo(lessonsWithLessonType: Record<LessonId, RawLesson>): LessonId[] {


Nit: I think the return type is supposed to be ClassNo.

Also, is there a reason to return LessonId[] instead of a single nullable LessonId? The function name getRecoveryClassNo implies a single return value.

Ah right, fair point there

leslieyip02 · 2026-06-21T06:55:58Z

+ * If weeks is an empty array, the serialized string would be `2025-01-13_2025-02-14_`\
+ * If weeks is undefined, the serialized string would be `2025-01-13_2025-02-14`, without the dangling `_`\


Minor nit: weeks should be enclosed by backticks (i.e. `weeks`).

- Improved regex to better reject malformed strings

…Id`s if any - Added tests for `getRecoverySerializedLessonDetails`

vercel · 2026-06-21T12:51:13Z

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

Project	Deployment	Actions	Updated (UTC)
nusmods-website	Ready	Preview, Comment	Jun 21, 2026 1:14pm

1 Skipped Deployment

Project	Deployment	Actions	Updated (UTC)
nusmods-export	Ignored	Preview	Jun 21, 2026 1:14pm

leslieyip02

LGTM 👍

greptile-apps Bot reviewed Jun 19, 2026

View reviewed changes

zehata added 5 commits June 19, 2026 19:47

Fix lint issues in export service

5ca327a

Removed unnecessary deserialization logic

82ad189

Address malformed async handling

65c4e47

Fix linting issue related to unnecessary Promise.reject

8759168

Addressed edge case regarding empty week numbers list. Added tests fo…

47bff90

…r serialization and deserialization of lessons.

greptile-apps Bot reviewed Jun 21, 2026

View reviewed changes

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

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

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

Comment thread export/src/makeModuleLessonMap.ts

Comment thread website/src/utils/timetables/lessonId.test.ts Outdated

leslieyip02 reviewed Jun 21, 2026

View reviewed changes

zehata added 3 commits June 21, 2026 17:51

Fixed mismatched serialization logic

07cefef

Fixed missing backticks in JSDoc

262a919

- Replaced async deserialization logic with try-catch logic

f67940c

- Improved regex to better reject malformed strings

greptile-apps Bot reviewed Jun 21, 2026

View reviewed changes

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

zehata added 2 commits June 21, 2026 19:35

- Modified getRecoverySerializedLessonDetails to pick valid `lesson…

f1d62f4

…Id`s if any - Added tests for `getRecoverySerializedLessonDetails`

Modified getRecoverySerializedLessonDetails to use intersection

7db8157

vercel Bot deployed to Preview – nusmods-website June 21, 2026 13:14 View deployment

leslieyip02 approved these changes Jun 21, 2026

View reviewed changes

leslieyip02 merged commit d424d32 into nusmodifications:master Jun 21, 2026
7 checks passed

zehata mentioned this pull request Jun 23, 2026

Switch over to use LessonId #4431

Merged

		* If weeks is an empty array, the serialized string would be `2025-01-13_2025-02-14_`\
		* If weeks is undefined, the serialized string would be `2025-01-13_2025-02-14`, without the dangling `_`\

Uh oh!

Conversation

zehata commented Jun 19, 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 19, 2026

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

leslieyip02 commented Jun 19, 2026

Uh oh!

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

Uh oh!

Greptile Summary

Confidence Score: 5/5

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zehata commented Jun 19, 2026

Uh oh!

leslieyip02 commented Jun 21, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

leslieyip02 left a comment

Choose a reason for hiding this comment

Uh oh!

leslieyip02 Jun 21, 2026

Choose a reason for hiding this comment

Uh oh!

leslieyip02 Jun 21, 2026

Choose a reason for hiding this comment

Uh oh!

zehata Jun 21, 2026

Choose a reason for hiding this comment

Uh oh!

leslieyip02 Jun 21, 2026

Choose a reason for hiding this comment

Uh oh!

leslieyip02 Jun 21, 2026

Choose a reason for hiding this comment

Uh oh!

zehata Jun 21, 2026

Choose a reason for hiding this comment

Uh oh!

leslieyip02 Jun 21, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

leslieyip02 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

zehata commented Jun 19, 2026 •

edited

Loading

codecov Bot commented Jun 19, 2026 •

edited

Loading

greptile-apps Bot commented Jun 19, 2026 •

edited

Loading

vercel Bot commented Jun 21, 2026 •

edited

Loading