feat(record): raise the default recording viewport to 1280x1600 - #26
Merged
Merged
Conversation
The viewport decides what the video contains. Puppeteer captures exactly it,
with no scrolling and no letterboxing, so anything below the fold is simply
absent — the filter it builds says so outright:
-vf crop='min(1280,iw):min(800,ih):0:0',pad=1280:800:0:0
Measured on twd-vue-example: at 720 the recording cut the todos page just below
the filter buttons, putting the list the tests assert on off-frame. The clip
looked fine and showed none of the behaviour under test. Nothing in the run says
the frame was cropped, so the failure is silent and only a human eye catches it.
1600 is the height that was reached for by hand after watching that video. It is
still a guess, and it is wrong in the other direction for an app that fits — but
that costs encoder time on empty space rather than a clip that documents nothing,
which is the cheaper mistake.
Both axes stay even, which the H.264 conversion needs for yuv420p.
Also corrects two stale claims about the viewport in the docs, both left over
from before 1.6.0 gave every run an explicit one: README and CLAUDE.md each said
a normal run inherits Puppeteer's implicit 800x600. It gets config.viewport,
1280x800 by default.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
TWD Contract Validation
23 passed · 41 failed · 3 warnings · 1 skipped Failed validations./contracts/users-3.0.json
./contracts/posts-3.1.json
./contracts/products-3.0.json
./contracts/events-3.1.json
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase 1 of
docs/spec-record-viewport.md, and only Phase 1 — norecord.fullPageoption, as agreed.The change
DEFAULT_RECORD.viewportgoes from1280x720to1280x1600.That is the whole behaviour change. The rest is the comment explaining why the number is what it is, so the next person does not "tidy" it back down.
Why it matters
The viewport decides what the video contains. Puppeteer captures exactly it — no scrolling, no letterboxing — so anything below the fold is simply absent. Its own filter says so:
Measured on
twd-vue-example: at 720 the recording cut the todos page just below the filter buttons, which put the list the tests assert on off-frame. The clip looked fine and showed none of the behaviour under test.The expensive part is that nothing in the run mentions it. The frame is cropped silently, and only a human watching the video finds out.
What this does not fix
Worth being straight about, since the spec is: 1600 is still a guess. It is the height that was reached for by hand after watching that video, and it will be wrong for some app. For an app that fits, it now wastes encoder time on empty space — the cheaper mistake than a clip that documents nothing, but a mistake.
The spec's own recommendation was to ship Phase 2 (
record.fullPage, measuring the document) instead, which would make the number unnecessary. You've called that out of scope for now, so this is the stopgap it describes.The cheap middle option, if you ever want it: compare
scrollHeightagainst the capture height after navigation and log one warning when content is being cropped. That would have turned an afternoon of watching videos into a line of output, and it does not need thefullPagemachinery. Not built here.Also in this PR
Two stale claims in the docs, both left from before 1.6.0 gave every run an explicit viewport. README and CLAUDE.md each said a normal run inherits Puppeteer's implicit
800x600; it getsconfig.viewport,1280x800by default. Corrected while editing the same sentences.Notes
yuv420p.record.viewportset explicitly still wins, so existing configs are unaffected.viewport(layout snapshots) is untouched at1280x800— committed.snapreferences stay valid.559 tests passing.🤖 Generated with Claude Code