CalvinBackup/gstack
1
0
Fork 0
mirror of https://github.com/garrytan/gstack.git synced 2026-09-15 01:15:29 +02:00
Code Issues Packages Projects Releases Wiki Activity

fix(browse): one ambiguous ref no longer kills the whole annotated screenshot

Browse Source 
`snapshot -a` exits 1 with "Selector matched multiple elements" on most real
pages, so /qa, /canary and /land-and-deploy silently produce reports whose
screenshots do not exist. Plain `screenshot <path>` is unaffected.

Refs are built as getByRole(role, {name}) and disambiguated with .nth() when
role+name repeats. That disambiguation cannot fire for a node with NO accessible
name: the locator degrades to getByRole(role) with no name filter, and the count
driving .nth() is taken from the FILTERED aria snapshot while getByRole matches
the unfiltered DOM. Measured on a live page: the tree surfaced 2 unnamed
paragraphs, the DOM had 9. Landmarks (banner/main/contentinfo) and paragraphs are
correctly unnamed per ARIA, so this is the common case rather than an edge case.

boundingBox() then hits Playwright strict mode, and the catch allowlisted only
timeout/closed/Target/Execution-context messages — so the strict-mode error was
re-thrown and aborted every remaining annotation.

Two changes:

- `.first()` before boundingBox(), so an ambiguous ref draws a box on its first
  match instead of aborting. The heatmap path below has always tolerated this via
  a bare `catch {}`; annotate was the only path that could be killed outright.
- the catch no longer re-throws on unrecognised messages. A box we cannot measure
  is a box we do not draw, never a reason to lose the rest of the page. Set
  BROWSE_DEBUG to see what was skipped.

Also: `-o` passed without `-a`/`-H` was silently ignored (exit 0, no file), which
reads as "screenshots are broken" rather than "you forgot a flag". It now warns
and points at `browse screenshot <path>`.

Verified by rebuilding both ways against the same page with 51 refs present:
  before — "Selector matched multiple elements", no file written
  after  — exit 0, 229KB PNG

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Connex Client Access
2026-08-17 10:08:53 -07:00
committed by Garry Tan
co-authored by Claude Fable 5
parent ca671d6f65
commit 9c4de4fe5b
3 changed files with 104 additions and 3 deletions
Download Patch File Download Diff File Expand all files Collapse all files
browse/src/snapshot.ts
+53 -3
View File
@@ -353,6 +353,14 @@ export async function handleSnapshot(
const snapshotText = output.join('\n');
// `-o` only means something to the two modes that PRODUCE an image. Passed
// alone it used to be silently ignored: exit 0, no file, no explanation —
// which reads as "the screenshot feature is broken" rather than "you forgot a
// flag", and cost a real debugging session before anyone noticed.
if (opts.outputPath && !opts.annotate && !opts.heatmap) {
output.push(`[warning] -o/--output was ignored: it names the file for an annotated screenshot, so it needs -a/--annotate (or -C/--cursor-interactive). For a plain screenshot use: browse screenshot ${opts.outputPath}`);
}
// ─── Annotated screenshot (-a) ────────────────────────────
if (opts.annotate) {
const screenshotPath = opts.outputPath || `${TEMP_DIR}/browse-annotated.png`;
@@ -387,15 +395,48 @@ export async function handleSnapshot(
try {
// Inject overlay divs at each ref's bounding box
const boxes: Array<{ ref: string; box: { x: number; y: number; width: number; height: number } }> = [];
const ambiguousRefs: string[] = [];
const skippedRefs: string[] = [];
for (const [ref, entry] of refMap) {
try {
const box = await entry.locator.boundingBox({ timeout: 1000 });
// A ref's locator can resolve to MORE than one element, and Playwright
// strict mode throws on that. It happens whenever a node has no
// accessible name: the locator degrades to `getByRole(role)` with no
// name filter, and the `.nth()` disambiguation above cannot help
// because its count comes from the FILTERED aria snapshot while
// getByRole matches the unfiltered DOM. Measured on a real page: the
// tree surfaced 2 unnamed paragraphs, the DOM had 9. Landmarks
// (banner/main/contentinfo) and paragraphs are correctly unnamed per
// ARIA, so this is the common case, not an edge.
//
// The exact nth-resolved locator stays the primary path; `.first()`
// is the AMBIGUITY FALLBACK only, and every fallback use is counted
// so first-match annotation is never silent. Before this, ONE such
// ref aborted the entire annotated screenshot (see the catch below) —
// which silently cost /qa, /canary and /land-and-deploy the
// screenshots their reports reference.
let locator = entry.locator;
const matchCount = await locator.count();
if (matchCount > 1) {
ambiguousRefs.push(`@${ref}`);
locator = locator.first();
}
const box = await locator.boundingBox({ timeout: 1000 });
if (box) {
boxes.push({ ref: `@${ref}`, box });
} else {
skippedRefs.push(`@${ref}`);
}
} catch (err: any) {
// Element may be offscreen, hidden, or page navigated — skip
if (!err?.message?.includes('Timeout') && !err?.message?.includes('timeout') && !err?.message?.includes('closed') && !err?.message?.includes('Target') && !err?.message?.includes('Execution context')) throw err;
// Element may be offscreen, hidden, or page navigated — skip.
//
// The allowlist is deliberately not exhaustive-by-message any more: a
// box we cannot measure is a box we do not draw, never a reason to
// lose every other annotation on the page. The heatmap path below has
// always used a bare `catch {}` for exactly this reason; annotate was
// the only path that could be killed by a single unmeasurable ref.
skippedRefs.push(`@${ref}`);
if (process.env.BROWSE_DEBUG) console.error(`[annotate] skipped @${ref}: ${err?.message?.split('\n')[0]}`);
}
}
@@ -428,6 +469,15 @@ export async function handleSnapshot(
output.push('');
output.push(`[annotated screenshot: ${screenshotPath}]`);
// Ambiguity and skips are visible, not buried behind BROWSE_DEBUG: a
// first-match box or a missing box changes what the screenshot claims.
if (ambiguousRefs.length || skippedRefs.length) {
const cap = (arr: string[]) => arr.slice(0, 8).join(', ') + (arr.length > 8 ? `, +${arr.length - 8} more` : '');
const parts: string[] = [];
if (ambiguousRefs.length) parts.push(`${ambiguousRefs.length} ambiguous (first-match): ${cap(ambiguousRefs)}`);
if (skippedRefs.length) parts.push(`${skippedRefs.length} skipped: ${cap(skippedRefs)}`);
output.push(`[annotated: ${parts.join(' | ')}]`);
}
} catch (err: any) {
// Remove overlays even on screenshot failure — but only swallow page/browser errors
if (!err?.message?.includes('closed') && !err?.message?.includes('Target') && !err?.message?.includes('Execution context') && !err?.message?.includes('screenshot')) throw err;