diff --git a/skills/agent-browser/SKILL.md b/skills/agent-browser/SKILL.md index 5cd897a..88e2a95 100644 --- a/skills/agent-browser/SKILL.md +++ b/skills/agent-browser/SKILL.md @@ -191,6 +191,35 @@ agent-browser find placeholder "Search" type "query" agent-browser find testid "submit-btn" click ``` +## JavaScript Evaluation (eval) + +Use `eval` to run JavaScript in the browser context. **Shell quoting can corrupt complex expressions** -- use `--stdin` or `-b` to avoid issues. + +```bash +# Simple expressions work with regular quoting +agent-browser eval 'document.title' +agent-browser eval 'document.querySelectorAll("img").length' + +# Complex JS: use --stdin with heredoc (RECOMMENDED) +agent-browser eval --stdin <<'EVALEOF' +JSON.stringify( + Array.from(document.querySelectorAll("img")) + .filter(i => !i.alt) + .map(i => ({ src: i.src.split("/").pop(), width: i.width })) +) +EVALEOF + +# Alternative: base64 encoding (avoids all shell escaping issues) +agent-browser eval -b "$(echo -n 'Array.from(document.querySelectorAll("a")).map(a => a.href)' | base64)" +``` + +**Why this matters:** When the shell processes your command, inner double quotes, `!` characters (history expansion), backticks, and `$()` can all corrupt the JavaScript before it reaches agent-browser. The `--stdin` and `-b` flags bypass shell interpretation entirely. + +**Rules of thumb:** +- Single-line, no nested quotes -> regular `eval 'expression'` with single quotes is fine +- Nested quotes, arrow functions, template literals, or multiline -> use `eval --stdin <<'EVALEOF'` +- Programmatic/generated scripts -> use `eval -b` with base64 + ## Deep-Dive Documentation | Reference | When to Use | diff --git a/src/actions.ts b/src/actions.ts index c420630..4a2c051 100644 --- a/src/actions.ts +++ b/src/actions.ts @@ -651,7 +651,7 @@ async function handleScroll(command: ScrollCommand, browser: BrowserManager): Pr const page = browser.getPage(); if (command.selector) { - const element = page.locator(command.selector); + const element = browser.getLocator(command.selector); await element.scrollIntoViewIfNeeded(); if (command.x !== undefined || command.y !== undefined) { @@ -1843,8 +1843,7 @@ async function handleScrollIntoView( command: ScrollIntoViewCommand, browser: BrowserManager ): Promise { - const page = browser.getPage(); - await page.locator(command.selector).scrollIntoViewIfNeeded(); + await browser.getLocator(command.selector).scrollIntoViewIfNeeded(); return successResponse(command.id, { scrolled: true }); } diff --git a/src/browser.test.ts b/src/browser.test.ts index 4df6b61..5488930 100644 --- a/src/browser.test.ts +++ b/src/browser.test.ts @@ -1,5 +1,6 @@ -import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; +import { describe, it, expect, beforeAll, afterAll, beforeEach, vi } from 'vitest'; import { BrowserManager } from './browser.js'; +import { executeCommand } from './actions.js'; import { chromium } from 'playwright-core'; describe('BrowserManager', () => { @@ -54,6 +55,192 @@ describe('BrowserManager', () => { }); }); + describe('stale session recovery (all pages closed)', () => { + it('should recover when all pages are closed externally', async () => { + const testBrowser = new BrowserManager(); + await testBrowser.launch({ headless: true }); + + // Verify initial state + expect(testBrowser.isLaunched()).toBe(true); + expect(testBrowser.getPage()).toBeDefined(); + + // Close all pages externally (simulates stale daemon state) + const pages = testBrowser.getPages(); + for (const page of [...pages]) { + await page.close(); + } + + // Wait for close events to propagate + await new Promise((resolve) => setTimeout(resolve, 100)); + + // isLaunched() is true but pages array is empty -- this is the stale state + expect(testBrowser.isLaunched()).toBe(true); + expect(testBrowser.getPages().length).toBe(0); + + // ensurePage() should recover by creating a new page + await testBrowser.ensurePage(); + expect(testBrowser.getPages().length).toBe(1); + expect(testBrowser.getPage()).toBeDefined(); + + await testBrowser.close(); + }); + + it('should be a no-op when pages already exist', async () => { + const testBrowser = new BrowserManager(); + await testBrowser.launch({ headless: true }); + + const pageBefore = testBrowser.getPage(); + await testBrowser.ensurePage(); + const pageAfter = testBrowser.getPage(); + + // Should be the same page -- no-op + expect(pageAfter).toBe(pageBefore); + expect(testBrowser.getPages().length).toBe(1); + + await testBrowser.close(); + }); + }); + + describe('scrollintoview with refs', () => { + it('should resolve refs in scrollintoview command', async () => { + const page = browser.getPage(); + await page.setContent(` + + +
+ + + + `); + + // Get snapshot to populate refs + const { refs } = await browser.getSnapshot({ interactive: true }); + + // Find the ref for our button + const buttonRef = Object.keys(refs).find((k) => refs[k].name === 'Far Away Button'); + expect(buttonRef).toBeDefined(); + + // scrollintoview with a ref should work, not throw a CSS selector error + const result = await executeCommand( + { id: 'test-1', action: 'scrollintoview', selector: `@${buttonRef}` }, + browser + ); + expect(result.success).toBe(true); + }); + + it('should resolve refs in scroll command with selector', async () => { + const page = browser.getPage(); + await page.setContent(` + + +
+
Scrollable content
+
+ + + + `); + + const { refs } = await browser.getSnapshot({ interactive: true }); + const buttonRef = Object.keys(refs).find((k) => refs[k].name === 'Target Button'); + expect(buttonRef).toBeDefined(); + + // scroll with a ref selector should work + const result = await executeCommand( + { id: 'test-2', action: 'scroll', selector: `@${buttonRef}`, y: 100 }, + browser + ); + expect(result.success).toBe(true); + }); + }); + + describe('cursor-ref selector uniqueness', () => { + it('should produce unique selectors for repeated DOM structures', async () => { + const page = browser.getPage(); + // Build deeply nested identical structures where the distinguishing + // ancestor (div.branch) is at level 4 from the target element -- + // beyond the previous 3-level path cutoff. + await page.setContent(` + + +
+
+
+
+
Item Alpha
+
+
+
+
+
+
+
Item Beta
+
+
+
+
+ + + `); + + const { refs } = await browser.getSnapshot({ interactive: true, cursor: true }); + + // Find the cursor-interactive refs + const cursorRefs = Object.entries(refs).filter(([, r]) => r.role === 'clickable'); + expect(cursorRefs.length).toBe(2); + + // Each ref's selector must be unique -- clicking it should not + // trigger a strict mode violation. + for (const [refKey] of cursorRefs) { + const locator = browser.getLocator(`@${refKey}`); + const count = await locator.count(); + expect(count).toBe(1); + } + }); + + it('should click the correct element when refs have repeated structure', async () => { + const page = browser.getPage(); + await page.setContent(` + + +
+
+
+
+
Item Alpha
+
+
+
+
+
+
+
Item Beta
+
+
+
+
+
none
+ + + `); + + const { refs } = await browser.getSnapshot({ interactive: true, cursor: true }); + + // Find the ref for "Item Beta" + const betaRef = Object.keys(refs).find((k) => refs[k].name === 'Item Beta'); + expect(betaRef).toBeDefined(); + + // Click it -- should not throw strict mode violation + const locator = browser.getLocator(`@${betaRef}`); + await locator.click(); + + const result = await page.locator('#result').textContent(); + expect(result).toBe('beta'); + }); + }); + describe('navigation', () => { it('should navigate to URL', async () => { const page = browser.getPage(); diff --git a/src/browser.ts b/src/browser.ts index b6a64e4..02f4532 100644 --- a/src/browser.ts +++ b/src/browser.ts @@ -190,6 +190,43 @@ export class BrowserManager { return page.locator(selectorOrRef); } + /** + * Check if the browser has any usable pages + */ + hasPages(): boolean { + return this.pages.length > 0; + } + + /** + * Ensure at least one page exists. If the browser is launched but all pages + * were closed (stale session), creates a new page on the existing context. + * No-op if pages already exist. + */ + async ensurePage(): Promise { + if (this.pages.length > 0) return; + if (!this.browser && !this.isPersistentContext) return; + + // Use the last existing context, or create a new one + let context: BrowserContext; + if (this.contexts.length > 0) { + context = this.contexts[this.contexts.length - 1]; + } else if (this.browser) { + context = await this.browser.newContext(); + context.setDefaultTimeout(60000); + this.contexts.push(context); + this.setupContextTracking(context); + } else { + return; + } + + const page = await context.newPage(); + if (!this.pages.includes(page)) { + this.pages.push(page); + this.setupPageTracking(page); + } + this.activePageIndex = this.pages.length - 1; + } + /** * Get the current active page, throws if not launched */ diff --git a/src/daemon.ts b/src/daemon.ts index 5bb2ec4..76cb002 100644 --- a/src/daemon.ts +++ b/src/daemon.ts @@ -329,6 +329,17 @@ export async function startDaemon(options?: { } } + // Recover from stale state: browser is launched but all pages were closed + if ( + manager instanceof BrowserManager && + manager.isLaunched() && + !manager.hasPages() && + parseResult.command.action !== 'launch' && + parseResult.command.action !== 'close' + ) { + await manager.ensurePage(); + } + // Handle close command specially - shuts down daemon if (parseResult.command.action === 'close') { const response = diff --git a/src/snapshot.ts b/src/snapshot.ts index 11682aa..7afa705 100644 --- a/src/snapshot.ts +++ b/src/snapshot.ts @@ -204,7 +204,16 @@ async function findCursorInteractiveElements( } path.unshift(sel); current = current.parentElement; - if (path.length >= 3) break; + // Stop once the selector uniquely identifies the element (max 10 levels) + if (path.length >= 1) { + try { + const candidate = path.join(' > '); + if (document.querySelectorAll(candidate).length === 1) break; + } catch (e) { + // If selector is invalid, keep building + } + } + if (path.length >= 10) break; } return path.join(' > '); };