Conversation
Admit `type: 'pdf'` entries in a card's `static screenshots` roster. A PDF entry is a paged document, not a raster tile: it renders one of the card's viewport-filling formats (isolated/embedded) under print media, paginates onto the card's own @page paper via page.pdf(), and persists under the 'declared' lane with null pixel dimensions — carrying page count and byte size on the manifest and meta.screenshots instead of width/height/deviceScaleFactor. - card-api: ScreenshotSpec becomes a raster/pdf union; assertValidScreenshotSpec refuses raster geometry, box formats, the author render slot, and useAsThumbnail on a pdf entry. - capture-spec/index: payload, manifest, meta, and capture-result types make raster geometry optional and add pageCount/byteSize; a pdf's declared identity folds media=print and drops geometry; add a per-card pdf sub-cap (SCREENSHOT_MAX_PDF_CAPTURES). - prerender: capture pdf slots one at a time (pdf is singular-only) under print media, apart from the raster batch; sub-cap overflow and per-slot failure follow the broken-links model. - persist: putMedia with null dims for pdf; manifest carries pageCount/byteSize. ?name= serving needs no change. Tests: unit persist + canonical-identity + card-api validation refusals, and an end-to-end indexing test (eager capture under print media, %PDF bytes, multi-page count, meta.screenshots join, ?name= serving inline). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014GsYGsuCqHJz9GAti4jheG
Preview deploymentsHost Test Results 1 files ±0 1 suites ±0 2h 19m 51s ⏱️ + 11m 4s Results for commit 7dabd2d. ± Comparison against earlier commit e05419c. Realm Server Test Results 1 files ±0 241 suites ±0 1h 17m 59s ⏱️ - 2m 10s Results for commit 7dabd2d. ± Comparison against earlier commit e05419c. |
Making `width`/`height` optional on the declared payload left this consumer
behind: the route feeds them to a Model that types both as `number`, and the
template interpolates them into an inline `width: ${width}px`.
Declaration validation refuses `render` for `type: 'pdf'`, and this route
serves only render slots, so the box is present today — but that refusal
names itself temporary, and an absent box would reach the template as
`undefinedpx` and capture a collapsed box rather than failing. Check for it
and throw, which also narrows the type.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
lukemelia
left a comment
lukemelia
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] Reviewed the widening — width/height/deviceScaleFactor going optional on the declared payload, the manifest entry, the capture result and the public meta.screenshots entry — out to its consumers across the tree, plus the declared-vs-DSL identity twins, the pdf leg's ordering and media handling against the raster legs, and the carry-forward path. I did not re-review the shipped POST capture or _screenshot/ serving legs this builds on.
No blocking issues. The widening left exactly one consumer behind, CI caught it, and I pushed the guard as 7dabd2d — the host's screenshot route feeds width/height into a Model typed number and a template that interpolates them into width: ${width}px, so an absent box would have reached the DOM as undefinedpx rather than failing. Three non-blocking asks below.
A note on the description's verification section: it says runtime-common, realm-server and host all pass lint:types. Host did not — that is the check that went red, on the route above. Worth re-reading the rest of that list with the same eye before merge.
canonicalDeclaredCaptureStringhas no exhaustiveness guard, while its DSL twin does — see the thread on the pdf identity branch incapture-spec.ts.- The pdf sub-cap and a mixed pdf+raster roster are both unexercised — see the thread on the cap branch in
prerender/utils.ts. DeclaredScreenshotCaptureResult'sdeviceScaleFactorcomment contradicts what a fresh pdf capture actually carries — see the thread on it inindex.ts.
Adjacent, out of scope: in both the raster group loop and the new pdf loop, a slot whose render reports success but returns no capture item for its name is dropped with no entry and no error row — it simply vanishes from the manifest with nothing recorded to retry against. The pdf leg inherits the raster leg's shape here rather than introducing it, so it is not this PR's, but the pdf case is the sharper one: a singular capture with an empty captures array is a whole render that produced nothing while reporting success.
| let canonical: Record<string, unknown> = | ||
| payload.type === 'pdf' | ||
| ? { | ||
| declared: name, | ||
| media: 'print', | ||
| source: payload.render ? 'render' : payload.format, | ||
| type: 'pdf', | ||
| } |
There was a problem hiding this comment.
[Claude Code 🤖] Add the exhaustiveness guard its twin already has. This function hand-builds the identity from named payload reads, so a field added to DeclaredScreenshotSpecPayload later and not added here is silently excluded from the hash — two declarations differing only in that field collapse onto one ledger entry and the second serves the first's bytes. The DSL twin, canonicalOverrides, is protected against exactly that: it destructures CaptureSpec and asserts rest satisfies Record<string, never>, with a comment saying the compile error is the point.
This PR is the change that shows the risk is live: it widens type to carry pdf and folds a new media axis into the identity, and getting both into the string was a manual step. The same treatment here — destructure the payload, satisfies Record<string, never> on the rest, and let the two deliberate exclusions (useAsThumbnail, keyBy — neither changes the pixels) be explicit discards — turns the next field into a compile error instead of a silent hash collision.
Follow-up, non-blocking; the current shape is correct, it just isn't enforced.
| if (pdfEntries.length >= SCREENSHOT_MAX_PDF_CAPTURES) { | ||
| errors.push({ | ||
| name: resolved.name, | ||
| message: `declared pdf screenshots exceed the pdf capture cap of ${SCREENSHOT_MAX_PDF_CAPTURES}; "${resolved.name}" was not captured`, | ||
| }); | ||
| continue; |
There was a problem hiding this comment.
[Claude Code 🤖] Two pins the e2e fixture is one slot away from, both on new logic nothing exercises.
This cap. A new constant, a new user-visible error string, and a counting rule that is easy to get wrong — it counts fresh only, so a carried-forward pdf does not consume a slot. That is the right rule (the cap bounds capture cost, and a carry-forward costs nothing), but nothing states it executably, and a later refactor that moves the check above the carry-forward split would change behaviour with every test still green.
A mixed roster. No fixture card declares both a pdf and a raster slot, so nothing covers the leg order this PR introduces: raster format groups, then the pdf slots one at a time under print media, then the render-based slots. That ordering is where a media or viewport leak out of the pdf leg would land, on the render-based slots that follow it. I checked the restore and it holds — page.emulateMediaType() in a finally, with the outer captureScreenshot restore idempotent behind it — so this is a pin, not a bug report. A second slot on the existing Statement card asserting both manifest entries would cover the split and the ordering together.
Test coverage, non-blocking.
| width: number; | ||
| height: number; | ||
| deviceScaleFactor: number; | ||
| // Absent for a pdf capture, which has no raster geometry — `pageCount` |
There was a problem hiding this comment.
[Claude Code 🤖] The comment and the code disagree about pdf and deviceScaleFactor. This says it is absent for a pdf capture; a fresh pdf capture always carries it. The engine sets it deliberately — captureScreenshot's pdf branch returns deviceScaleFactor with "the scale is the render's, reported for parity with raster captures" — and finish() forwards it onto the result. Only persistDeclaredScreenshots drops it, so it never reaches the manifest.
The consequence is a reader's, not the runtime's: this is the type the capture leg's consumers read, and the comment invites deviceScaleFactor === undefined as a "this is a pdf" discriminator, which is false for every fresh pdf capture. The code itself discriminates on outputType / contentType, which is right.
Either correct the comment or drop the field in finish()'s pdf branch so the result matches both the comment and the carry-forward branch, which already omits it. Non-blocking.
1 participant
Closes CS-12979. Builds on the shipped PDF plumbing from CS-12977 (persist/serve PDFs from the
_screenshot/DSL) and CS-12978 (media=printemulation).What this does
A card declares a PDF in its
static screenshotsroster:and gets it captured eagerly at index time, served at
_screenshot/{path}?name=statement, and joined intometa.screenshots— the same declared-screenshot pipeline as raster tiles, extended for paged output.A PDF entry is a paged document, not a raster tile: it has no capture box (
width/height/deviceScaleFactor/backgroundare refused at declaration), renders one of the card's viewport-filling formats (isolated/embedded) under print media, and paginates onto the card's own@pagepaper viapage.pdf(). It persists under thedeclaredlane with null pixel dimensions, carrying page count / byte size on the manifest andmeta.screenshotsinstead.Changes
base/card-api.gts—ScreenshotSpecis now a raster/PDF union.assertValidScreenshotSpecrefuses raster geometry, the box formats (fitted/atom), the authorrenderslot (that's the follow-up, CS-12980), anduseAsThumbnailon a PDF entry.runtime-common/capture-spec.ts+index.ts— payload, manifest, meta, and capture-result types make raster geometry optional and addpageCount/byteSize. A PDF's declared identity foldsmedia=printand drops geometry (canonicalDeclaredCaptureString); carry-forward is unchanged. New per-card sub-capSCREENSHOT_MAX_PDF_CAPTURES(counted within the batch cap, since a PDF capture is much heavier than a tile).realm-server/prerender/utils.ts— PDF slots are captured one at a time (PDF output is singular-only) under print media, apart from the raster batch. Sub-cap overflow and per-slot failure follow the broken-links model.runtime-common/index-runner/prerender-html-visit.ts—putMediawith null dims for PDF; manifest carriespageCount/byteSize.?name=serving needed no change (it already streamsapplication/pdfinline).Acceptance (CS-12979)
statement: { type: 'pdf' }gets its PDF captured at index time_screenshot/{path}?name=statementmeta.screenshotsTests
card-apivalidation refusals.Statementcard + instance: eager capture under print media,%PDFbytes, multi-page count,meta.screenshotsjoin, and?name=serving inline.Verification
runtime-common,realm-server, andhostall passlint:typesandlint:js.🤖 Generated with Claude Code