feat: make screenshot base64 encoding optional#15
Draft
cryptotavares wants to merge 1 commit into
Draft
Conversation
Add an `encode` option to `DeviceBackend.screenshot()` so callers can
capture straight to disk and skip base64 via `{ encode: false }`, which
returns only the file `path`. The default (`encode: true`) preserves the
existing behavior. Adds the `ScreenshotOptions` and `ScreenshotFileResult`
public types and makes `ScreenshotResult.data` optional.
Also fix `device_screenshot` failing on macOS: the captured PNG is now
read in-process with `fs.readFile(path, 'base64')` instead of shelling out
to the `base64` CLI, whose BSD variant on macOS rejects the GNU
positional-file syntax used previously.
The Appium backend now writes the screenshot to `outputPath` when one is
provided (the argument was previously ignored).
Add screenshot tests across the adb, idb, and appium backends and raise
the coverage thresholds accordingly.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| const path = resolvePath( | ||
| outputPath ?? `/tmp/device-mcp-screenshot-${Date.now()}.png`, | ||
| ); | ||
| await writeFile(path, Buffer.from(data, 'base64')); |
| const path = resolvePath( | ||
| outputPath ?? `/tmp/device-mcp-screenshot-${Date.now()}.png`, | ||
| ); | ||
| await writeFile(path, Buffer.from(data, 'base64')); |
| let path: string | undefined; | ||
| if (outputPath) { | ||
| path = resolvePath(outputPath); | ||
| await writeFile(path, Buffer.from(data, 'base64')); |
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
2 participants
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.
What is the current state of things and why does it need to change?
device_screenshotfails on macOS. The backends captured the PNG to disk and then shelled out tobase64 <file>to produce the image data. BSDbase64(the default on macOS) does not accept the GNU positional-file syntax, so the command errors and the tool never returns an image.Separately, base64 encoding was always performed even when a caller only needed the screenshot written to a file, and the Appium backend ignored the
outputPathargument entirely.What is the solution your changes offer and how does it work?
fs.readFile(path, 'base64')instead of the non-portablebase64CLI — fixes the macOS failure on the ADB and IDB backends.encodeoption toDeviceBackend.screenshot(). Passing{ encode: false }skips base64 encoding and returns only the filepath; the default ({ encode: true }) preserves existing behavior. NewScreenshotOptionsandScreenshotFileResulttypes are exported, andScreenshotResult.datais now optional.outputPath: the Appium backend now decodes and writes the screenshot tooutputPathwhen provided.device_screenshottool now handles a missingdatafield, returning text-only content when no image data is present.Notes
Targets
cryptotavares/add-hermes-cdp(stacked on the Hermes CDP work), so the diff is limited to the screenshot changes.