-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[STG-2275] feat(cli): default screenshot to file output (--base64 for legacy stdout) #2246
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
55b3466
692f2d6
1552300
81159c5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| "browse": minor | ||
| --- | ||
|
|
||
| `browse screenshot` now writes a file by default instead of printing base64 to stdout. Bare invocations save to `screenshot-<yyyymmdd-hhmmss>.<type>` in the current directory (with a collision counter instead of overwriting) and print `{ "saved": "<path>" }`. A new `--base64` flag preserves the legacy behavior of printing `{ "base64": "..." }` to stdout; it is mutually exclusive with `--path`. `--path` behavior is unchanged. | ||
|
|
||
| Note for scripts that parsed the bare-invocation base64 output: pass `--base64` to keep the old stdout contract. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,6 @@ | ||
| import { closeSync, openSync, statSync, unlinkSync } from "node:fs"; | ||
| import { resolve } from "node:path"; | ||
|
|
||
| import { Flags } from "@oclif/core"; | ||
|
|
||
| import { BrowseCommand } from "../base.js"; | ||
|
|
@@ -12,10 +15,12 @@ export default class Screenshot extends BrowseCommand { | |
| "Capture a screenshot of the active browser page."; | ||
|
|
||
| static override examples = [ | ||
| "browse screenshot", | ||
| "browse screenshot --path page.png", | ||
| "browse screenshot --full-page --path page.png", | ||
| "browse screenshot --full-page", | ||
| "browse screenshot --type jpeg --quality 80", | ||
| "browse screenshot --clip 0,0,800,600 --path clipped.png", | ||
| "browse screenshot --base64", | ||
| ]; | ||
|
|
||
| static override flags = { | ||
|
|
@@ -24,6 +29,11 @@ export default class Screenshot extends BrowseCommand { | |
| description: "Whether CSS animations run during capture.", | ||
| options: ["allow", "disabled"], | ||
| }), | ||
| base64: Flags.boolean({ | ||
| description: | ||
| "Print base64 to stdout instead of writing a file (legacy default).", | ||
| exclusive: ["path"], | ||
| }), | ||
| caret: Flags.string({ | ||
| description: "Whether text caret is hidden during capture.", | ||
| options: ["hide", "initial"], | ||
|
|
@@ -38,7 +48,7 @@ export default class Screenshot extends BrowseCommand { | |
| path: Flags.string({ | ||
| char: "p", | ||
| description: | ||
| "Write the screenshot to a file. Without this flag, base64 is printed.", | ||
| "Write the screenshot to this file. Defaults to screenshot-<timestamp>.png (or .jpeg with --type jpeg) in the current directory.", | ||
| helpValue: "<path>", | ||
| }), | ||
| quality: Flags.integer({ | ||
|
|
@@ -53,18 +63,85 @@ export default class Screenshot extends BrowseCommand { | |
|
|
||
| async run(): Promise<void> { | ||
| const { flags } = await this.parse(Screenshot); | ||
| await runDriverCommandFromFlags( | ||
| "screenshot", | ||
| { | ||
| animations: flags.animations, | ||
| caret: flags.caret, | ||
| clip: parseClip(flags.clip), | ||
| fullPage: flags["full-page"], | ||
| path: flags.path, | ||
| quality: flags.quality, | ||
| type: flags.type, | ||
| }, | ||
| flags, | ||
| ); | ||
| const defaultPath = getDefaultPathFromFlags(flags); | ||
| try { | ||
| await runDriverCommandFromFlags( | ||
| "screenshot", | ||
| { | ||
| animations: flags.animations, | ||
| caret: flags.caret, | ||
| clip: parseClip(flags.clip), | ||
| fullPage: flags["full-page"], | ||
| path: flags.path ?? defaultPath, | ||
| quality: flags.quality, | ||
| type: flags.type, | ||
| }, | ||
| flags, | ||
| ); | ||
| } catch (error) { | ||
| if (defaultPath) removeIfEmpty(defaultPath); | ||
| throw error; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Generous upper bound on filename-collision retries; far beyond any real | ||
| // same-second burst, it just guarantees the loop below always terminates. | ||
| const MAX_RESERVE_ATTEMPTS = 1000; | ||
|
|
||
| /** | ||
| * Resolves the file the screenshot should be written to, or undefined when the | ||
| * driver should return base64 (explicit --path is handled separately; --base64 | ||
| * opts out of a file entirely). | ||
| */ | ||
| function getDefaultPathFromFlags(flags: { | ||
| path?: string; | ||
| base64?: boolean; | ||
| type?: string; | ||
| }): string | undefined { | ||
| if (flags.path || flags.base64) return undefined; | ||
| return reserveDefaultScreenshotPath(flags.type); | ||
| } | ||
|
|
||
| /** | ||
| * Picks the next free screenshot-<timestamp>[-<counter>].<type> name in the | ||
| * current directory and reserves it. "Reserve" = create the file with an | ||
| * exclusive open (`wx` → O_CREAT|O_EXCL), which atomically fails with EEXIST if | ||
| * the name already exists, so two concurrent runs can never claim the same | ||
| * file. On EEXIST we advance the counter and try the next name. | ||
| */ | ||
| function reserveDefaultScreenshotPath(type: string | undefined): string { | ||
|
shrey150 marked this conversation as resolved.
|
||
| const now = new Date(); | ||
| const pad = (value: number) => String(value).padStart(2, "0"); | ||
| const stamp = | ||
| `${now.getFullYear()}${pad(now.getMonth() + 1)}${pad(now.getDate())}` + | ||
| `-${pad(now.getHours())}${pad(now.getMinutes())}${pad(now.getSeconds())}`; | ||
| const extension = type === "jpeg" ? "jpeg" : "png"; | ||
| for (let counter = 1; counter <= MAX_RESERVE_ATTEMPTS; counter += 1) { | ||
| const suffix = counter === 1 ? "" : `-${counter}`; | ||
| const candidate = resolve(`screenshot-${stamp}${suffix}.${extension}`); | ||
| try { | ||
| // Exclusive create reserves the name; close immediately since the driver | ||
| // (re)writes the file. The empty placeholder is cleaned up on failure. | ||
| closeSync(openSync(candidate, "wx")); | ||
|
shrey150 marked this conversation as resolved.
|
||
| return candidate; | ||
| } catch (error) { | ||
| if ((error as NodeJS.ErrnoException).code !== "EEXIST") throw error; | ||
| } | ||
| } | ||
| throw new Error( | ||
| `Could not reserve a screenshot filename after ${MAX_RESERVE_ATTEMPTS} attempts; pass --path to choose one.`, | ||
| ); | ||
| } | ||
|
|
||
| // Removes the reserved placeholder when the screenshot failed before the driver | ||
| // wrote to it. `path` is always a file we created via openSync above, so the | ||
| // isFile guard is just defensive against an unexpected directory/symlink. | ||
| function removeIfEmpty(path: string): void { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this feels like there's a utility function for it
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There isn't a stdlib or repo utility for "delete a file if it's empty" — it's inherently stat + unlink, and we don't depend on fs-extra (and even that has no unlink-if-empty helper). So no clean util to lean on here. But the real question is whether this function needs to exist at all, and it only does as the cleanup half of the atomic Two ways to simplify if the complexity isn't worth it:
I'd lean toward keeping it as-is (zero litter, clean names, and it's tested), but happy to take either simplification — your call. |
||
| try { | ||
| const stats = statSync(path); | ||
| if (stats.isFile() && stats.size === 0) unlinkSync(path); | ||
| } catch { | ||
| // Best effort: leave the placeholder behind rather than mask the error. | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.