Skip to content

fix(ego-browser): reject an unrecordable viewport instead of a 0x0 screencast - #252

Open
Hotragn wants to merge 1 commit into
citrolabs:devfrom
Hotragn:fix/screencast-unusable-viewport
Open

fix(ego-browser): reject an unrecordable viewport instead of a 0x0 screencast#252
Hotragn wants to merge 1 commit into
citrolabs:devfrom
Hotragn:fix/screencast-unusable-viewport

Conversation

@Hotragn

@Hotragn Hotragn commented Aug 11, 2026

Copy link
Copy Markdown

Summary

page.screencast.start() validates an explicitly passed size (rejecting anything under 2 pixels with a clear message) but never validates the size it derives itself. Two reachable states make that derived size 0x0, which reaches ffmpeg as scale=0:0 and fails with an encoder error that says nothing about the real cause:

  • A JavaScript dialog is open. page.info() then returns { dialog } with no w/h, so Math.min(1, 800 / Math.max(undefined, undefined)) is NaN, and NaN & ~1 is 0.
  • The tab reports a zero viewport — the w: 0 / h: 0 state SKILL.md already warns agents about ("stop screenshot/coordinate work until the real tab or viewport is restored").

A third case needs no zero at all: an extreme aspect ratio such as 1000x2 scales to a height of 1, again below the floor the explicit path enforces.

Related issue

No existing issue — found while auditing the screencast path alongside #251 (the other half of "recording fails for a reason the agent cannot act on").

Changes

  • src/driver/screencast.tsdefaultSize() now fails fast with an actionable message instead of handing a degenerate size to the encoder:
    • a pending dialog is named explicitly, pointing at the same remedy SKILL.md gives (handle the dialog first) or passing an explicit size;
    • any derived size below the 2-pixel floor reports the viewport it actually read (w: 0, h: 0), so the agent can tell "no usable tab" from "encoder problem".
      The check runs on the computed size, so the extreme-aspect-ratio case is covered too, and it happens before the recorder is created — no encoder process is started and nothing needs cleaning up.
  • src/driver/screencast.test.mjs — 3 tests: the dialog case (also asserting no encoder is created), the zero-viewport case, and the 1000x2 scale-below-minimum case.

No behavior change for any viewport that already produced a valid recording: the arithmetic is untouched, only a guard is added after it.

Verification

Run from package/ego-browser (Windows 11, Node 24; tests inject pageInfo via the existing __testing.setOverrides seam, so they are platform-neutral):

npm test                      -> 318 pass, 0 fail (315 existing + 3 new)
npm run typecheck             -> ok
npm run validate:site-skills  -> site skills ok
npm audit --audit-level=moderate -> 0 vulnerabilities

Degenerate-size arithmetic confirmed directly before writing the fix:

{ dialog }      -> scale NaN -> floor(NaN) -> evenSize 0 x 0
{ w: 0, h: 0 }  -> 0 x 0
{ w: 1000, h: 2 } -> 800 x 1 -> evenSize 800 x 0

Revert-check: with src/driver/screencast.ts reverted and the new tests kept, all three fail (17 pass / 3 fail); with the fix, 20 pass.

Impact

  • Public helper API or behavior
  • Agent skill or instructions
  • Site learning
  • Installation or update flow
  • Build, CI, or release process
  • Documentation only
  • No externally visible impact

A start that previously produced a broken recording (or a confusing ffmpeg error) now fails immediately with the reason. Recordings that worked before are unaffected — the guard only rejects sizes the encoder could not have used. Independent of #251; both touch the screencast failure path but different files, and either can merge first.

Checklist

  • The PR targets the correct base branch (dev for normal changes; only dev may target main).
  • The change is focused and does not include unrelated cleanup.
  • Tests were added or updated for behavior changes, or the reason they are unnecessary is explained above.
  • Relevant tests and validation commands pass locally.
  • Public helper JSDoc and agent-facing documentation are updated when the helper surface changes.
  • No credentials, tokens, cookies, personal data, or other secrets are included.
  • A release-note label is selected (fix).

@Hotragn

Hotragn commented Aug 11, 2026

Copy link
Copy Markdown
Author

Flagging this myself rather than leaving it to a reviewer to discover: I believe this PR is superseded on the sprint-1.3.0 line.

That branch adds playwright-core@1.52.0 and removes src/driver/ entirely, so the src/driver/screencast.ts whose defaultSize() this PR guards is not present there. Playwright's own video/recording path does its own sizing, so the 0x0 derivation this fixes should not be reachable.

The defect is real on dev, where a pending JavaScript dialog makes page.info() return { dialog } with no w/h (so NaN & ~1 yields 0) and the encoder receives scale=0:0. I have left the PR open against the documented base rather than closing it unilaterally — happy to close on request, or I will close it once the sprint work lands on dev/main.

#224 and #251 are in the same position.

@codeCraft-Ritik codeCraft-Ritik 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.

Great job on the CI validation as well. Adding a targeted validate-reference-links.js check is a strong follow-up because it prevents the same class of broken links from silently returning later without turning the validator into an overly broad Markdown path linter.

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.

2 participants