Skip to content

feat(reports): email SEO audits as PDF instead of raw Markdown - #3

Merged
siva01c merged 3 commits into
mainfrom
feature/report-pdf-export
Aug 17, 2026
Merged

siva01c merged 3 commits into
mainfrom
feature/report-pdf-export

Conversation

@siva01c

@siva01c siva01c commented Aug 16, 2026

Copy link
Copy Markdown
Owner

Why

Report emails carried the raw .md, which is unreadable on a phone — GFM pipe tables wrap into noise and there is no typography at all. This was already logged as wanted in docs/todo.md (Phase D).

The .md is unchanged and stays the canonical artifact on disk — it is what get_report serves and what the cached-report branch reads. Only the attachment changes.

How

Renders the report to a print-ready A4 PDF through the Chromium already present in the base image, so this costs no new binary and no Dockerfile change. apify/actor-node-playwright-chrome also ships NotoColorEmoji and the Liberation family, which is what makes the ✅/🔴/🟡 severity markers and the Czech diacritics come out as glyphs rather than tofu. The only new dependency is marked (zero-dependency, ~50 kB, in dependencies since the production stage installs --omit=dev).

  • src/services/reportPdfService.ts — Markdown → print HTML → PDF: linked table of contents, one page per section, table headers repeated across page breaks, page numbers in the footer. renderMarkdownToHtml is pure and synchronous so the layout is unit-testable without a browser.
  • src/mcp-server.tssendSeoEmail takes an attachment instead of a Markdown string; both call sites (cached report and fresh crawl) go through one buildReportAttachment helper. With several recipients attached to one job the PDF is rendered once and reused.
  • scripts/report-pdf.ts (npm run report:pdf) — renders an existing report, for previewing typography without a crawl and for backfilling older reports.

Safety and resource guards

  • Raw HTML in the Markdown is escaped, and the render page runs with javaScriptEnabled: false and every request aborted. Report sections quote crawled <title>/meta text verbatim, so that text is attacker-controlled by definition — it comes off whatever site was audited. (Partly addresses the sanitisation item flagged at docs/todo.md:76.)
  • Renders are serialised to one at a time and bounded by SEO_PDF_TIMEOUT_MS (default 120 s).
  • A failed render falls back to attaching the .md — Chromium dying degrades the format, it must not lose the report.
  • PDFs are cached next to the .md (write-then-rename) and re-rendered only when the Markdown is newer, so a same-day repeat request launches no browser at all. They live in storage/reports/<domain>/<date>/ and are covered by existing purge-old-data.ts retention.

Memory

Measured in the production image under --memory=1.5g, rendering the real 39-page ludekkvapil.cz report:

Scenario Container peak Duration
Idle 1–7 MB
One PDF render (39 pages, 356 kB out) 310 MB ~0.6 s
After the render returns 5 MB

A render costs about what a crawl does, but for well under a second, and the memory is returned when the browser closes. Worst case of 2 concurrent crawls (~610 MB) plus one render (~310 MB) ≈ 950 MB, inside the 1.5 G limit. Recorded in docs/memory-optimization.md.

Verification

  • Full suite: 247 passed, 23 suites (eslint / prettier / tsc style tests included). New: 13 unit tests plus 2 opt-in integration tests (SEO_PDF_INTEGRATION=1 npm test -- reportPdf) that launch a real Chromium.
  • Rendered the real 187-page ludekkvapil.cz report inside the production base image: 39 pages, 356 kB, 0.6 s. Colour emoji, Czech diacritics and the wide section-3 inventory table all verified on rasterised pages.
  • End-to-end against a stub Mail API: attachment arrived as seo-audit-ludekkvapil.cz.pdf, application/pdf, %PDF- magic. A second same-day request reused the cached PDF (one render, two emails).
  • Degraded path with SEO_PDF_TIMEOUT_MS=1: email still delivered with the .md attached, reason logged to the job log, no orphaned Chromium process and no truncated .pdf left behind.

Deploy note

This changes package.json and src/, so the VPS needs a real image rebuild (npm ci + tsc), not the git pull + up -d shortcut. docs/memory-optimization.md records that the deployed dist/ is already stale relative to main and that a build on that 4 GB no-swap box is itself a memory risk — stop the container first:

docker compose stop mcp && docker compose build mcp && docker compose up -d mcp

That rebuild also finally activates the commits sitting unbuilt in production (58cd3b4 Basic Auth, f578509 merged dated crawl folders, e04fc80 0-page-crawl guard).

Note on the diff

The repo's own lint-staged pre-commit hook ran prettier --write over the staged Markdown, and README.md / docs/todo.md did not conform beforehand. So those two files carry ~100 lines of incidental reformatting on top of the real edits. Happy to split that into a separate commit if preferred.

🤖 Generated with Claude Code

Report emails carried the raw .md, which is unreadable on a phone — GFM
pipe tables wrap into noise and there is no typography at all.

Renders the report to a print-ready A4 PDF through the Chromium already
present in the base image, so this costs no new binary and no Dockerfile
change: apify/actor-node-playwright-chrome also ships NotoColorEmoji and
the Liberation family, which is what makes the ✅/🔴/🟡 severity markers
and Czech diacritics come out as glyphs rather than tofu.

The .md is unchanged and stays the canonical artifact on disk — it is
what get_report serves and what the cached-report branch reads. Only the
attachment changes.

- src/services/reportPdfService.ts renders Markdown → print HTML → PDF:
  linked table of contents, one page per section, table headers repeated
  across page breaks, page numbers in the footer.
- Raw HTML from the Markdown is escaped, and the render page runs with
  javaScriptEnabled:false and every request aborted: report sections
  quote crawled <title>/meta text verbatim, so that text is
  attacker-controlled by definition.
- Renders are serialised to one at a time and bounded by
  SEO_PDF_TIMEOUT_MS (default 120 s). A failed render falls back to
  attaching the .md — Chromium dying degrades the format, it must not
  lose the report.
- PDFs are cached next to the .md and re-rendered only when the Markdown
  is newer, so a same-day repeat request launches no browser.
- scripts/report-pdf.ts (npm run report:pdf) renders an existing report
  for previewing typography or backfilling older reports.

Measured in the production image under --memory=1.5g on the real
39-page ludekkvapil.cz report: 310 MB peak for ~0.6 s, returned as soon
as the browser closes. Worst case of 2 crawls plus a render is ~950 MB,
inside the 1.5 G limit (docs/memory-optimization.md).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 878ca60da1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/services/reportPdfService.ts Outdated
Comment on lines +350 to +354
const buffer = await withTimeout(
renderHtmlToPdf(html, renderOptions),
PDF_TIMEOUT_MS,
'PDF render'
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Cancel timed-out PDF renders before releasing the queue

When a render exceeds SEO_PDF_TIMEOUT_MS because Chromium startup or page.pdf() hangs, this Promise.race rejects to the caller but leaves renderHtmlToPdf() running in the background; renderChain is then reset via result.catch(...), so the next queued report can launch another Chromium while the timed-out one is still alive. That defeats the one-render memory guard and can stack/leak browser processes instead of cleanly falling back to Markdown under the exact slow/hung-render scenario the timeout is meant to handle.

Useful? React with 👍 / 👎.

The timeout raced around the outside of renderHtmlToPdf, so a render that
overran rejected to the caller while its browser was still running. The
queue was then released and the next report could launch a second
Chromium alongside the abandoned one — the one-render memory guard
failing in exactly the hung-render case it exists for. Worse, a page.pdf()
that never settled meant the browser never closed at all.

Moves the deadline inside renderHtmlToPdf so the `finally` that closes the
browser runs before the promise settles, and the queue is only released
once the process is actually gone:

- chromium.launch() gets the remaining budget as its own `timeout`; it
  enforces that itself and kills what it started, so it needs no race.
- setContent() gets the remaining budget; page.pdf() takes no timeout
  option, so it keeps an explicit race — but inside the try block.
- browser.close() is itself bounded (10 s), so a Chromium wedged badly
  enough to ignore close() cannot hold the queue shut forever.

SEO_PDF_TIMEOUT_MS is now read per call so tests can vary it.

Verified: with the timeout set to fire during setContent and again during
page.pdf(), `pgrep -x chrome-headless-shell` reports 0 at the moment the
render rejects, and the next render still succeeds. Covered by a
regression test in the opt-in integration suite.

Reported by Codex review on #3.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@siva01c

siva01c commented Aug 16, 2026

Copy link
Copy Markdown
Owner Author

Good catch on the P2 — that was a real bug, fixed in 5ca08dc.

The timeout raced around the outside of renderHtmlToPdf, so an overrunning render rejected to the caller while its browser was still alive. renderChain was then released and the next report could launch a second Chromium alongside the abandoned one. And a page.pdf() that never settled meant the browser never closed at all.

The deadline now lives inside renderHtmlToPdf, so the finally that closes the browser runs before the promise settles and the queue is only released once the process is gone:

  • chromium.launch() takes the remaining budget as its own timeout — it enforces that itself and kills what it started, so it needs no race.
  • page.setContent() takes the remaining budget too.
  • page.pdf() has no timeout option, so it keeps an explicit race — but inside the try, where finally still wins.
  • browser.close() is itself bounded (10 s), so a Chromium wedged badly enough to ignore close() can't hold the queue shut forever either.

SEO_PDF_TIMEOUT_MS is now read per call so tests can vary it.

Verified empirically with the timeout tuned to fire during setContent and again during page.pdf()pgrep -x chrome-headless-shell reports 0 at the moment the render rejects, and a subsequent render still succeeds. A regression test covering that (rejects, leaves no .pdf/.tmp, queue still usable afterwards) is in the opt-in integration suite.

Fixes five issues found in review of PR #3:
- browser.close() timeout left a wedged Chromium alive while the render
  queue released; launchServer()+kill() now force-terminates it.
- SEO_PDF_TIMEOUT_MS was unvalidated, so an empty/non-numeric value
  silently collapsed the render deadline to ~0ms.
- The cover/TOC split matched `## ` via a raw-string regex that could be
  fooled by a fenced code block; it now splits on the lexed heading token.
- TOC entries used raw, only-HTML-escaped heading text instead of the
  same inline-parsed HTML as the real heading.
- Markdown was lexed/parsed three times per render; now once.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@siva01c
siva01c merged commit 9c5ad9f into main Aug 17, 2026
2 checks passed
@siva01c
siva01c deleted the feature/report-pdf-export branch August 26, 2026 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant