Skip to content

fix(browser): handle daemon returning { session, data } wrapper from screenshot#1502

Closed
ddandyy74 wants to merge 1 commit into
jackwener:mainfrom
ddandyy74:fix/screenshot-base64-handling
Closed

fix(browser): handle daemon returning { session, data } wrapper from screenshot#1502
ddandyy74 wants to merge 1 commit into
jackwener:mainfrom
ddandyy74:fix/screenshot-base64-handling

Conversation

@ddandyy74

Copy link
Copy Markdown

Description

The daemon'"'"'s screenshot command returns the base64 data wrapped in an object with session info ({ session, data }), but page.screenshot() passes the return value directly to saveBase64ToFile() which calls Buffer.from(base64, '"'"'base64'"'"').

When the value is an object instead of a string, this causes TypeError: The first argument must be of type string or an instance of Buffer, ArrayBuffer, or Array or an Array-like Object. Received an instance of Object.

Fix

Extract the data field when the return value is an object with a string data property, matching the pattern already used in cdp.ts. Falls back to treating the return value as a string for backward compatibility.

Tested

  • opencli browser --session test screenshot output.png — saves PNG file correctly
  • opencli browser --session test screenshot — returns base64 string in console
  • Both with and without --full-page flag

Related

This follows the same defensive pattern used in src/browser/cdp.ts:246.

…screenshot

The daemon's screenshot command returns the base64 data wrapped in an object with session info ({ session, data }). The page.screenshot() method passed this object directly to saveBase64ToFile() which calls Buffer.from(base64, 'base64'), causing a TypeError on Windows. Fix by extracting the data field when the return value is an object, matching the pattern already used in cdp.ts. Closes #N/A
@jackwener jackwener closed this May 13, 2026
@jackwener

Copy link
Copy Markdown
Owner

Thanks @ddandyy74 — your repro and analysis nailed a real bug. We hit the same TypeError: The first argument must be of type string ... Received an instance of Object on our side too, and your trace pointing at the daemon return shape was exactly right.

We landed a root-cause fix earlier today in #1518 (commit 1eac8e07): the extension's pageScopedResult helper was wrapping every page-scoped response as { session, data }, which is what fed the object into Buffer.from(..., 'base64'). That helper now returns the plain { id, ok, data, page } shape it was originally specced for, so sendCommand('screenshot', ...) once again resolves to a base64 string directly.

We went with an extension-side fix rather than a client-side unwrap for two reasons:

  1. Contract stays clean. The data field is meant to carry the handler's return value verbatim. Once we tolerate { data: <real data> } in the client, every handler grows an ambiguous shape and future readers can't tell which layer owns the unwrap.
  2. One fix, all handlers. The same wrapping was silently affecting evaluate / cookies / navigate / click / etc. Screenshot was just the fastest fail because Buffer.from is strict; the other handlers were corrupting return values quietly. Fixing the source repairs all of them at once.

Closing this PR as superseded — the fix will ship in the next release (1.7.19, in flight now). Once you upgrade with npm i -g @jackwener/opencli@latest it should just work.

Really appreciate the contribution. If you run into more rough edges, especially around browser primitives or adapters, please keep reports / PRs coming — they're the kind of issue that's hard to spot internally because we usually catch the symptom on a different handler.

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