feat: support previous special term when academic year changes#4402
feat: support previous special term when academic year changes#4402jloh02 wants to merge 6 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4402 +/- ##
==========================================
+ Coverage 54.52% 56.62% +2.09%
==========================================
Files 274 317 +43
Lines 6076 7031 +955
Branches 1455 1704 +249
==========================================
+ Hits 3313 3981 +668
- Misses 2763 3050 +287 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Confidence Score: 5/5Safe to merge — the overlap-window detection, scraper branching, archive enrichment, and UI gating all follow the intended logic with no data-loss or incorrect-display paths found. The core date-window utilities are well-tested with explicit calendar-boundary cases. The scraper correctly fetches organisations and modules from the previous AY only for sems 3/4, writes venue files under both AYs, and uses preserveModuleInfoSemesters to keep current-AY metadata. The website merge path has a reference-equality short-circuit to avoid redundant dispatches, and the external exam link is now correctly suppressed once archive data provides an exam date. The one observation (warn noise during overlap) is a log-quality matter with no functional impact. scrapers/nus-v2/src/tasks/CollateModules.ts — the moduleDataCheck warn fires ahead of the preserveModuleInfoSemesters guard, so operators monitoring logs during the overlap window will see a large volume of expected differences.
|
| Filename | Overview |
|---|---|
| packages/nusmods-academic-calendar/index.js | New shared library. Date-window utilities are straightforward; isPreviousAySpecialTermActive safely returns false when calendar data is missing. |
| packages/nusmods-academic-calendar/index.d.ts | TypeScript declarations match the JS exports; SPECIAL_TERM_SEMESTERS declared as readonly tuple of literals. |
| scrapers/nus-v2/src/tasks/CollateModules.ts | Adds preserveModuleInfoSemesters option and upsertSemesterData helper; the existing moduleDataCheck warn fires before the preserve-guard, producing one warning per module per special-term semester during the overlap window. |
| scrapers/nus-v2/src/tasks/DataPipeline.ts | Correctly branches on overlap window, fetches previous-AY orgs/modules, runs CollateVenues under both AYs, and passes preserveModuleInfoSemesters={3,4} to CollateModules. |
| website/src/utils/specialTerm.ts | New utility; merge logic and shouldPreferArchiveSemesterData are well-designed; archive preference during overlap is intentional and documented. |
| website/src/actions/moduleBank.ts | enrichModuleWithPreviousAySpecialTerm correctly caches archive lookups, guards with reference-equality to avoid redundant dispatches, and the intentional re-dispatch is documented. |
| website/src/views/modules/ModulePageContent.tsx | Replaces static ST2 exam-link guard with shouldShowSt2ExamExternalLink; now correctly hides the link when archive exam date is present or outside overlap window. |
| website/src/data/academic-calendar.ts | Thin re-export shim replacing the inline JSON cast; all calendar utilities now come from the shared package. |
Sequence Diagram
sequenceDiagram
participant Pipeline as DataPipeline (scraper)
participant PrevAY as Previous AY API
participant CurrAY as Current AY API
participant Files as Data Files
Pipeline->>CurrAY: GetFacultyDepartment + GetAllModules (current AY)
alt usePreviousAySpecialTerms
Pipeline->>PrevAY: GetFacultyDepartment + GetAllModules (prev AY)
end
par Sems 1 and 2 from current AY
Pipeline->>CurrAY: GetSemesterData(1), GetSemesterData(2)
Pipeline->>Files: CollateVenues(sem, currentAY)
and Sems 3 and 4 from previous AY
Pipeline->>PrevAY: GetSemesterData(3), GetSemesterData(4)
Pipeline->>Files: CollateVenues(sem, prevAY)
Pipeline->>Files: CollateVenues(sem, currentAY)
end
Pipeline->>Files: "CollateModules with preserveModuleInfoSemesters={3,4}"
participant Client as Browser
participant Store as Redux Store
participant API as NUSMods API
participant Archive as Archive API
Client->>Store: fetchModule(moduleCode)
Store->>API: "GET /modules/{moduleCode}"
API-->>Store: Module dispatches SUCCESS_KEY(FETCH_MODULE)
Store-->>Client: current-AY module
alt archive not cached
Client->>Archive: fetchModuleArchive(moduleCode, prevAY)
Archive-->>Store: archive module
end
Client->>Client: mergePreviousAySpecialTermData
Client->>Store: re-dispatch SUCCESS_KEY(FETCH_MODULE) with merged module
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
scrapers/nus-v2/src/tasks/CollateModules.ts:125-132
The `moduleDataCheck` warn fires **before** the `preserveModuleInfoSemesters` guard. During the overlap window every module that appears in both the current AY (sems 1/2) and the previous AY (sems 3/4) will log a warning — at minimum because `acadYear` always differs — producing hundreds of expected-but-noisy warnings per scraper run. Consider skipping the check when the incoming semester is intentionally preserved from a different AY.
```suggestion
if (existingData) {
// Skip the info-diff warning for semesters whose module info is intentionally
// preserved from a different AY (e.g. previous-AY special terms during overlap).
if (!semesterData || !preserveModuleInfoSemesters?.has(semesterData.semester)) {
const difference = moduleDataCheck(existingData, semesterModule.module);
if (difference) {
logger.warn(difference, 'Module with different module info between semesters');
}
}
if (semesterData && preserveModuleInfoSemesters?.has(semesterData.semester)) {
```
Reviews (2): Last reviewed commit: "fix: satisfy oxlint property sort order ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| /** | ||
| * Merge Special Term II timetable and exam data from the previous AY into the | ||
| * current module. Prefers archive ST II data when the current module lacks a | ||
| * usable ST II timetable. | ||
| */ | ||
| export function mergePreviousAySt2Data( | ||
| module: Module, | ||
| archiveModule: Module | null | undefined, | ||
| ): Module { | ||
| if (!archiveModule) { | ||
| return module; | ||
| } | ||
|
|
||
| const archiveSt2 = getModuleSemesterData(archiveModule, 4); | ||
| if (!archiveSt2) { | ||
| return module; | ||
| } | ||
|
|
||
| const currentSt2 = getModuleSemesterData(module, 4); | ||
| const currentHasTimetable = Boolean(currentSt2?.timetable?.length); | ||
| const archiveHasTimetable = Boolean(archiveSt2.timetable?.length); | ||
|
|
||
| if (currentHasTimetable && !archiveHasTimetable) { | ||
| return module; | ||
| } | ||
|
|
||
| if ( | ||
| currentHasTimetable && | ||
| archiveHasTimetable && | ||
| JSON.stringify(currentSt2) === JSON.stringify(archiveSt2) | ||
| ) { | ||
| return module; | ||
| } |
There was a problem hiding this comment.
mergePreviousAySt2Data
The JSDoc says "Prefers archive ST II data when the current module lacks a usable ST II timetable," but the code also replaces the current module's ST2 data when both currentSt2 and archiveSt2 have timetables that differ (currentHasTimetable && archiveHasTimetable && !equal). In that case archive wins too — the comment does not document this. A future maintainer reading only the comment might add a guard like if (currentHasTimetable) return module; for the differing case, silently reverting the intended behaviour of preferring authoritative previous-AY data over ModReg-R0 data during the overlap period.
Prompt To Fix With AI
This is a comment left during a code review.
Path: website/src/utils/specialTerm.ts
Line: 66-98
Comment:
**Misleading JSDoc for `mergePreviousAySt2Data`**
The JSDoc says "Prefers archive ST II data when the current module *lacks* a usable ST II timetable," but the code also replaces the current module's ST2 data when both `currentSt2` and `archiveSt2` have timetables that differ (`currentHasTimetable && archiveHasTimetable && !equal`). In that case archive wins too — the comment does not document this. A future maintainer reading only the comment might add a guard like `if (currentHasTimetable) return module;` for the differing case, silently reverting the intended behaviour of preferring authoritative previous-AY data over ModReg-R0 data during the overlap period.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Good catch — agreed. Expanded the JSDoc on mergePreviousAySpecialTermData (and extracted shouldPreferArchiveSemesterData) to document that archive data also wins when both timetables exist but differ, since ModReg R0 on the new AY is not authoritative during the overlap window. Fixed in cf81bde.
| if (semester === 4 && usePreviousAySt2) { | ||
| await new CollateVenues(semester, this.academicYear).run(modules); | ||
| } |
There was a problem hiding this comment.
CollateVenues call purpose is not documented
The block runs CollateVenues a second time with this.academicYear (new AY) using the same modules fetched from semesterAcadYear (previous AY). The returned aliases are silently discarded, and the aliases from the first call (previous AY) flow into allAliases for CollateModules. Without a comment, it is unclear that the goal is to populate the current AY's semester-4 venue files with the previous AY's ST2 venue data so that website venue lookups keyed by the new AY succeed. A brief inline comment here (and possibly a clarifying note on why the second call's aliases can be safely discarded) would prevent future maintainers from removing the call thinking it is redundant.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scrapers/nus-v2/src/tasks/DataPipeline.ts
Line: 87-89
Comment:
**Second `CollateVenues` call purpose is not documented**
The block runs `CollateVenues` a second time with `this.academicYear` (new AY) using the same `modules` fetched from `semesterAcadYear` (previous AY). The returned `aliases` are silently discarded, and the `aliases` from the first call (previous AY) flow into `allAliases` for `CollateModules`. Without a comment, it is unclear that the goal is to populate the current AY's semester-4 venue files with the previous AY's ST2 venue data so that website venue lookups keyed by the new AY succeed. A brief inline comment here (and possibly a clarifying note on why the second call's aliases can be safely discarded) would prevent future maintainers from removing the call thinking it is redundant.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Agreed. Added an inline comment explaining that the second call writes venue files under the current AY for website lookups, and that its aliases are intentionally discarded because CollateModules uses the fetch-AY aliases. Fixed in cf81bde.
| dispatch({ | ||
| type: SUCCESS_KEY(FETCH_MODULE), | ||
| payload: mergedModule, | ||
| }); |
There was a problem hiding this comment.
SUCCESS_KEY(FETCH_MODULE) dispatch causes a brief double-render
After requestAction resolves and the middleware already dispatches SUCCESS_KEY(FETCH_MODULE) with the original (non-enriched) module, the store updates and any subscribed component re-renders. Then enrichModuleWithPreviousAySt2 async-fetches the archive and dispatches the same action type again with the merged module, causing a second store update and re-render. During the gap between the two dispatches the ST2 semester data (timetable / exam) is absent, so a user who navigates to the module page in that window sees incomplete ST2 information before it is corrected. A comment explaining the double-dispatch pattern and its trade-offs would help reviewers understand why this isn't an accidental duplicate.
Prompt To Fix With AI
This is a comment left during a code review.
Path: website/src/actions/moduleBank.ts
Line: 75-78
Comment:
**Manual `SUCCESS_KEY(FETCH_MODULE)` dispatch causes a brief double-render**
After `requestAction` resolves and the middleware already dispatches `SUCCESS_KEY(FETCH_MODULE)` with the original (non-enriched) module, the store updates and any subscribed component re-renders. Then `enrichModuleWithPreviousAySt2` async-fetches the archive and dispatches the same action type again with the merged module, causing a second store update and re-render. During the gap between the two dispatches the ST2 semester data (timetable / exam) is absent, so a user who navigates to the module page in that window sees incomplete ST2 information before it is corrected. A comment explaining the double-dispatch pattern and its trade-offs would help reviewers understand why this isn't an accidental duplicate.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Partially agree on the trade-off, but this is intentional rather than accidental. requestAction must complete first so the module is in the store; archive enrichment is async and may require a separate fetch. Refactoring the request middleware to block the initial SUCCESS until enrichment finishes would be a much larger change for a narrow overlap window.
Also, ST II data is not necessarily absent in the gap — components may briefly show current-AY ModReg R0 placeholders before archive data replaces them, which is the case we are correcting.
Added a comment documenting the intentional re-dispatch in cf81bde.
Clarify merge preference rules, the second CollateVenues call, and the intentional post-fetch enrichment dispatch. Co-authored-by: Cursor <cursoragent@cursor.com>
Reorder config fields and CollateModules inputs so the scraper lint step passes in CI. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@greptileai review |
2 participants
Closes #4109