diff --git a/browse/src/snapshot.ts b/browse/src/snapshot.ts index ce3a1a466..3b4c610c7 100644 --- a/browse/src/snapshot.ts +++ b/browse/src/snapshot.ts @@ -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; diff --git a/browse/test/fixtures/snapshot-ambiguous.html b/browse/test/fixtures/snapshot-ambiguous.html new file mode 100644 index 000000000..e50d6d8dc --- /dev/null +++ b/browse/test/fixtures/snapshot-ambiguous.html @@ -0,0 +1,24 @@ + + +Ambiguous refs fixture + + +
+

Ambiguity test page

+
+
+

Paragraph one of plain content.

+

Paragraph two of plain content.

+ + + + Jump to end +
+ + + diff --git a/browse/test/snapshot.test.ts b/browse/test/snapshot.test.ts index 107adf49a..96a8b170e 100644 --- a/browse/test/snapshot.test.ts +++ b/browse/test/snapshot.test.ts @@ -321,6 +321,33 @@ describe('Annotated screenshots', () => { fs.unlinkSync(screenshotPath); }); + // PR #2601 (@namtrok): one ambiguous ref must not kill the whole annotated + // screenshot. "Save" is a substring of "Save As", so the Save ref's locator + // matches two buttons — pre-fix, Playwright strict mode aborted every + // remaining annotation and no file was written. + test('snapshot -a survives ambiguous refs and reports them visibly (#2601)', async () => { + const screenshotPath = '/tmp/browse-test-annotated-ambiguous.png'; + await handleWriteCommand('goto', [baseUrl + '/snapshot-ambiguous.html'], bm); + const result = await handleMetaCommand('snapshot', ['-a', '-o', screenshotPath], bm, shutdown); + // The screenshot landed despite the ambiguity... + expect(result).toContain('[annotated screenshot:'); + expect(fs.existsSync(screenshotPath)).toBe(true); + expect(fs.statSync(screenshotPath).size).toBeGreaterThan(1000); + // ...refs after the ambiguous one are still in the snapshot... + expect(result).toContain('Save As'); + expect(result).toContain('Cancel'); + // ...and the first-match fallback is visible, never silent. + expect(result).toContain('ambiguous (first-match)'); + fs.unlinkSync(screenshotPath); + }); + + test('snapshot -o without -a/-H warns instead of silently ignoring (#2601)', async () => { + await handleWriteCommand('goto', [baseUrl + '/snapshot.html'], bm); + const result = await handleMetaCommand('snapshot', ['-o', '/tmp/browse-test-ignored.png'], bm, shutdown); + expect(result).toContain('[warning] -o/--output was ignored'); + expect(fs.existsSync('/tmp/browse-test-ignored.png')).toBe(false); + }); + test('snapshot -a uses default path', async () => { const defaultPath = '/tmp/browse-annotated.png'; await handleWriteCommand('goto', [baseUrl + '/snapshot.html'], bm);