@darentanrw 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 Report

❌ Patch coverage is 50.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.39%. Comparing base (988c6fd) to head (1c7b5d2).
⚠️ Report is 236 commits behind head on master.

@@            Coverage Diff             @@
##           master    #4343      +/-   ##
==========================================
+ Coverage   54.52%   56.39%   +1.87%     
==========================================
  Files         274      317      +43     
  Lines        6076     6970     +894     
  Branches     1455     1682     +227     
==========================================
+ Hits         3313     3931     +618     
- Misses       2763     3039     +276     

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

Note: This was also done from my assumption of that you are deploying your monorepo separately onto different vercel instances - there are ways we can reduce this data duplication with packages and etc, but it will require a more significant rewrite?

Sounds like a great idea. I'll be starting on Phase 2 of #4314 soon and we can tackle this extraction.

Some thoughts:

1. The current export service acts like a general screenshotting service, which is in some sense better because it doesn't need to contain any rendering or layout logic. The export service will work regardless of how the website looks, which may or may not be desired.
2. If we're going to share the data model between website and export, we will need to ensure that either data model changes are backwards-compatible, or both apps are deployed at the same time. The current export already is coupled to the data model, but not as much as the new way.

Lastly, can we also explore if https://www.renoun.dev/screenshot works? I think it runs on the client and maybe we can use it for images and avoid a server round-trip.

I'm torn between the export approaches. Decoupling the rendering has good benefits too, so I guess the question is how much the visual consistency between website and export matters.

Thanks for the review! I've fixed the low hanging fruits that you commented on.

I will take a further look into https://www.renoun.dev/screenshot, however a potential problem that I foresee is that it might not be easy to select only the timetable and module info as a PNG export.

The current workflow with Puppeteer screenshots https://nusmods.vercel.app/timetable-only after injecting it with the timetable data. However, what is currently rendered to the user also includes the TimetableActions and ModuleSelectContainer components. Will need to figure out a way to exclude them from the local screenshot, or to redirect to timetable-only, then screenshot, which is not an ideal user experience.

In this case of using Serverless functions for export, I believe that a cleaner approach to couple the data models is by shifting the export serverless handlers into the main website APIs (to website/api/export/*).

Personally, I don't think that the visual consistency between website and export matters that much (See Visual Differences in original PR), and I think that we can make it more similar (except for fonts) - but ultimately this is a decision for the maintainers to make.

Thanks for the review! I've fixed the low hanging fruits that you commented on.

I will take a further look into https://www.renoun.dev/screenshot, however a potential problem that I foresee is that it might not be easy to select only the timetable and module info as a PNG export.

The current workflow with Puppeteer screenshots https://nusmods.vercel.app/timetable-only after injecting it with the timetable data. However, what is currently rendered to the user also includes the TimetableActions and ModuleSelectContainer components. Will need to figure out a way to exclude them from the local screenshot, or to redirect to timetable-only, then screenshot, which is not an ideal user experience.

In this case of using Serverless functions for export, I believe that a cleaner approach to couple the data models is by shifting the export serverless handlers into the main website APIs (to website/api/export/*).

Personally, I don't think that the visual consistency between website and export matters that much (See Visual Differences in original PR), and I think that we can make it more similar (except for fonts) - but ultimately this is a decision for the maintainers to make.

Those are great points! I'm convinced this is a good approach. Like you said, we can also co-locate the exporting functions into the web app, which is a good idea for the future.

Btw I've started extracting the types in #4346, depending on whose PR merges first, someone will have to resolve the conflicts, but it shouldn't be hard.

Personally, I don't think that the visual consistency between website and export matters that much (See Visual Differences in original PR), and I think that we can make it more similar (except for fonts) - but ultimately this is a decision for the maintainers to make.

Agree on this. I think this is fine. Ultimately, my biggest barrier is still the configuration/duplicated logic issues which have been lightly addressed for now.

In this case of using Serverless functions for export, I believe that a cleaner approach to couple the data models is by shifting the export serverless handlers into the main website APIs (to website/api/export/*).

I am agreeable to this since technically having 2 separate Vercel projects doesn't yield much benefit if that can ultimately reduce duplicated code. @ravern @leslieyip02 can weigh in on this too.

Btw I've started extracting the types in #4346, depending on whose PR merges first, someone will have to resolve the conflicts, but it shouldn't be hard.

Since we've stopped the effort to extract/share types, let's not include that into the scope of this PR.

I also personally don't think that the visual differences don't matter too much.

I do like the effort here in bringing in a new approach to exporting timetables. And I do think there are pros and cons, so why not we merge this in to sit alongside the current export behind a beta, and then see what users think?

Export is pure-function-ish and we can just have both current and beta endpoints without much hassle.

I can make the changes to support both.

@greptileai review

sequenceDiagram
    participant Browser
    participant ExportMenu
    participant BetaImageHandler as api/export/beta/image
    participant makeExportHandler
    participant renderImage
    participant renderPdf
    participant getModules
    participant buildRenderableTimetable
    participant satori
    participant resvg
    participant PDFKit

    Browser->>ExportMenu: click beta image/pdf (beta testers only)
    ExportMenu->>ExportMenu: extractStateForExport() → JSON.stringify → URL
    ExportMenu-->>Browser: "href with ?data=..."

    Browser->>BetaImageHandler: "GET /api/export/beta/image?data=..."
    BetaImageHandler->>makeExportHandler: JSON.parse(data) + validateExportData
    makeExportHandler->>renderImage: exportData, options
    renderImage->>getModules: fetch modules from API/filesystem
    getModules-->>renderImage: Module[]
    renderImage->>buildRenderableTimetable: exportData + modules
    buildRenderableTimetable-->>renderImage: RenderableTimetable model
    renderImage->>satori: TimetableImage JSX + fonts
    satori-->>renderImage: SVG string
    renderImage->>resvg: SVG → render().asPng()
    resvg-->>renderImage: PNG Buffer
    renderImage-->>BetaImageHandler: PNG Buffer
    BetaImageHandler-->>Browser: image/png

    Browser->>BetaImageHandler: "GET /api/export/beta/pdf?data=..."
    BetaImageHandler->>makeExportHandler: JSON.parse(data) + validateExportData
    makeExportHandler->>renderPdf: exportData
    renderPdf->>getModules: fetch modules
    getModules-->>renderPdf: Module[]
    renderPdf->>buildRenderableTimetable: exportData + modules
    buildRenderableTimetable-->>renderPdf: RenderableTimetable model
    renderPdf->>satori: TimetableImage JSX + fonts
    satori-->>renderPdf: SVG string
    renderPdf->>resvg: SVG → render().asPng() (2× zoom)
    resvg-->>renderPdf: PNG Buffer
    renderPdf->>PDFKit: image(pngBuffer, x, y, dimensions)
    PDFKit-->>renderPdf: PDF Buffer (via stream)
    renderPdf-->>BetaImageHandler: PDF Buffer
    BetaImageHandler-->>Browser: application/pdf

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

---

### Issue 1 of 3
export/api/export/beta/image.ts:14-15
The `as never` cast bypasses TypeScript's knowledge that `request.query.data` is `string | string[] | undefined`. If the caller sends duplicate `?data=` query params, Vercel parses it as `string[]`, and `JSON.parse` silently calls `.toString()` on the array (producing `"item1,item2"`) which almost certainly fails to parse and throws a 422 — but the failure reason is opaque. A `as string` cast makes intent explicit and lets TypeScript warn if the type ever narrows differently.

```suggestion
    if (typeof request.query.data !== 'string') {
      throw new Error('Expected data to be a single string query parameter');
    }
    const exportData = JSON.parse(request.query.data);
    validateExportData(exportData);
```

### Issue 2 of 3
export/api/export/beta/pdf.ts:8-9
Same `as never` cast as the image handler — same concern applies. If `request.query.data` arrives as a `string[]` (duplicate query params), `JSON.parse` will silently mangle the value. Prefer an explicit type-check before parsing.

```suggestion
    if (typeof request.query.data !== 'string') {
      throw new Error('Expected data to be a single string query parameter');
    }
    const exportData = JSON.parse(request.query.data);
    validateExportData(exportData);
```

### Issue 3 of 3
export/src/app.ts:35-50
**Beta routes silently pass `undefined` data in the dev server**

The `parseExportData` middleware guards against a missing `?data` param only for paths matching `EXPORT_ROUTES` (`/api/export/image` and `/api/export/pdf`). The new `/beta/image` and `/beta/pdf` paths don't match that regex, so when the dev server receives a request without `?data`, `ctx.state.data` stays `undefined`. Both handlers immediately destructure it (`const { data } = ctx.state`) and pass it to `renderImage`/`renderPdf`, which crashes with a `TypeError: Cannot read properties of undefined (reading 'timetable')` rather than returning a clean 422. This only affects the local Koa dev server — the Vercel production functions validate inside `makeExportHandler` — but it makes debugging difficult during development.

_{Reviews (2): Last reviewed commit: "fix(export): address satori beta review ..." | Re-trigger Greptile}

@greptileai review