diff --git a/CHANGELOG.md b/CHANGELOG.md index 4b8aa24..5ac10f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,37 @@ Format inspired by [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] +### Fixed +- `click` no longer reports `{ ok: true }` for clicks that landed but + produced no observable effect. The handler now takes a cheap + before/after observation and returns `dialogDismissed`, `urlChanged`, + `preDialogCount`, `postDialogCount`, and `observedMs` alongside `ok`. + Default budget is 300ms, configurable with `--observe-ms ` (e.g. + `1500` for HubSpot-style confirm modals that wait on a network save + before unmounting). `--no-observe` skips the observation entirely + for callers that want the old shape (benchmarks, tight loops). + Real-world repro: a HubSpot "Save changes?" alertdialog where the + trusted click was dispatched correctly but `loc.click()` resolved + ~1.3s before React actually unmounted the dialog — agents would see + `ok: true`, retry, and double-save. With the new fields, callers + can branch on `dialogDismissed === false`. +- `snapshot` now scopes per-node Locators to the auto-detected modal + when dialog-scope kicks in, not just the rendered ARIA tree. Before + this fix, the displayed tree was modal-only but `getByRole(...)` + ran against the whole page — any same-named button outside the + modal (e.g. a hidden zombie left by the previous render) could + trigger strict-mode errors or, on layouts where Playwright's pick + resolved uniquely, silently click the wrong element. Stacked + dialogs (HubSpot puts an `alertdialog` over an already-open + `dialog`) hit this routinely. +- `console --dedup` smoke check is now resilient to a warm daemon. + The console `CircularBuffer` persists across smoke runs against + the same daemon, and the test previously seeded a literal marker + string and asserted `count === 5` — re-running smoke without + detaching merged 5 leftover entries with 5 fresh ones and reported + count=10. The seeded marker is now per-run (Date-based), so prior + buffer state cannot collide with a fresh run's emits. + ### Added - **Bucket A payload-reduction sprint** — six flags and one new verb to cut context cost for LLM operators driving ghax (sourced from a diff --git a/src/daemon.ts b/src/daemon.ts index fb8740d..9439646 100644 --- a/src/daemon.ts +++ b/src/daemon.ts @@ -29,7 +29,7 @@ import { resolveConfig, type DaemonState, writeState, readState } from './config import { CircularBuffer, parseStack, type ConsoleEntry, type NetworkEntry } from './buffers'; import { SourceMapCache, resolveStack } from './source-maps'; import type { RefEntry } from './snapshot'; -import { snapshot as takeSnapshot } from './snapshot'; +import { snapshot as takeSnapshot, MODAL_SEL } from './snapshot'; import * as fs from 'fs'; import * as http from 'http'; import type { AddressInfo } from 'net'; @@ -881,13 +881,80 @@ async function annotateScreenshot( } } -register('click', async (ctx, args) => { +// Click — Playwright's `loc.click()` resolves the moment the trusted mouse +// event has been dispatched. That tells you "the click was sent" but says +// nothing about whether the page reacted. Real-world failure mode: a +// confirm-modal "Save" button accepts the click but doesn't dismiss for +// >1s because the React handler kicks off an async network round-trip +// before unmounting the dialog. Callers see `{ ok: true }` and assume the +// click no-op'd. +// +// Fix: take a cheap before/after observation around the click and report +// whether observable downstream effects happened within a short budget. +// Two signals worth tracking: +// - dialogDismissed → a visible modal disappeared (count went down) +// - urlChanged → location.href changed (navigation/SPA route) +// Both are O(1) DOM queries; the poll loop short-circuits on first signal, +// so the cost is ~one round-trip when the click is effective. +// +// Defaults: observe on, 300ms budget. Opt-outs: +// --no-observe → skip entirely (back to old behavior, no extras) +// --observe-ms → custom budget (e.g. --observe-ms 1500 for HubSpot +// confirm modals that wait on a network save). +register('click', async (ctx, args, opts) => { const target = String(args[0] ?? ''); if (!target) throw new Error('Usage: click <@ref|selector>'); const page = await activePage(ctx); const loc = resolveRef(ctx, target, page); + + const observe = opts.observe !== false && opts['no-observe'] !== true; + const observeMs = (() => { + const raw = opts['observe-ms'] ?? opts.observeMs; + if (raw === undefined) return 300; + const n = Number(raw); + return Number.isFinite(n) && n >= 0 ? n : 300; + })(); + + const preDialogCount = observe ? await page.locator(MODAL_SEL).count() : 0; + const preUrl = observe ? page.url() : ''; + await loc.click(); - return { ok: true }; + + if (!observe) return { ok: true }; + + // Poll for downstream effects. 30ms steps trades a tiny bit of CPU for + // promptness — most React modal dismissals land in the next animation + // frame (~16ms), and 30ms means the first poll catches them without a + // wasted full sleep when the click was instant. + // + // Always do a final fresh dialog-count sample after the loop so callers + // never see stale `postDialogCount`. Without this, a click that + // navigated AND dismissed a modal would report `urlChanged: true` plus + // an outdated `postDialogCount` (the pre-navigation count) — confusing. + const deadline = Date.now() + observeMs; + let urlChanged = false; + let postDialogCount = preDialogCount; + while (Date.now() < deadline) { + postDialogCount = await page.locator(MODAL_SEL).count(); + if (page.url() !== preUrl) { urlChanged = true; break; } + if (postDialogCount < preDialogCount) break; + await new Promise((r) => setTimeout(r, 30)); + } + // Final sample — covers (a) the early-break-on-urlChanged path where the + // pre-break sample may now be stale post-navigation and (b) the early- + // break-on-dismissal path which is already fresh (no extra cost beyond + // one count() round-trip). + postDialogCount = await page.locator(MODAL_SEL).count().catch(() => postDialogCount); + const dialogDismissed = postDialogCount < preDialogCount; + + return { + ok: true, + dialogDismissed, + urlChanged, + preDialogCount, + postDialogCount, + observedMs: observeMs, + }; }); register('fill', async (ctx, args) => { diff --git a/src/snapshot.ts b/src/snapshot.ts index acf5cb4..957eae6 100644 --- a/src/snapshot.ts +++ b/src/snapshot.ts @@ -52,6 +52,20 @@ const INTERACTIVE_ROLES = new Set([ 'treeitem', ]); +/** + * Single source of truth for "what counts as an open modal." + * Used by the snapshot dialog-scope walker AND by the click handler's + * post-click observation (did the modal actually dismiss?). + * + * Covers `[role=dialog]`, `[role=alertdialog]`, the native ``, + * and ad-hoc `[aria-modal="true"]` scrims (Radix, Headless UI, Material, + * HubSpot's Dialog__StyledDialog). The `:visible` pseudo filters detached + * / display:none dialogs that some frameworks leave in the DOM between + * openings. + */ +export const MODAL_SEL = + '[role=dialog]:visible, [role=alertdialog]:visible, dialog[open]:visible, [aria-modal="true"]:visible'; + interface ParsedNode { indent: number; role: string; @@ -77,21 +91,23 @@ export async function snapshot( opts: SnapshotOptions = {}, ): Promise { let rootLocator = opts.selector ? target.locator(opts.selector) : target.locator('body'); + // Tracks whether the auto-detected modal scope is active. When true, the + // per-node locators below also need to be modal-rooted — otherwise + // `target.getByRole(...)` queries the entire page and a same-named button + // outside the modal can win the strict-mode race or, worse, silently match + // a hidden zombie element. (HubSpot stacks an alertdialog on top of an + // already-open dialog with overlapping button names — exactly this case.) + let modalScopeActive = false; if (opts.selector) { const count = await rootLocator.count(); if (count === 0) throw new Error(`Selector not found: ${opts.selector}`); } else if (opts.dialogScope !== false) { // Dialog-aware walker — if a modal is open, walk from it instead of - // from `body`. Covers `[role=dialog]`, `[role=alertdialog]`, the - // native ``, and ad-hoc `[aria-modal=true]` scrims - // (Radix, Headless UI, Material). `.last()` picks the top-most - // modal if a stack is open. The `:visible` pseudo filters out - // detached / display:none dialogs that some frameworks leave in - // the DOM between openings. - const modalSel = '[role=dialog]:visible, [role=alertdialog]:visible, dialog[open]:visible, [aria-modal="true"]:visible'; - const modal = target.locator(modalSel).last(); + // from `body`. `.last()` picks the top-most modal if a stack is open. + const modal = target.locator(MODAL_SEL).last(); if ((await modal.count()) > 0) { rootLocator = modal; + modalScopeActive = true; } } @@ -140,14 +156,20 @@ export async function snapshot( roleNameSeen.set(key, seenIndex + 1); const totalCount = roleNameCounts.get(key) || 1; - let locator: Locator = target.getByRole(node.role as any, { + // Scope locators to the same root we used for the ARIA tree: + // - explicit user selector wins, + // - else auto-detected modal (when active), + // - else the page/frame root. + // Without this, the rendered tree shows modal-only nodes but locators + // resolve page-wide — strict-mode error or wrong-element pick. + const locatorScope: Page | Frame | Locator = opts.selector + ? target.locator(opts.selector) + : modalScopeActive + ? rootLocator + : target; + let locator: Locator = locatorScope.getByRole(node.role as any, { name: node.name || undefined, }); - if (opts.selector) { - locator = target.locator(opts.selector).getByRole(node.role as any, { - name: node.name || undefined, - }); - } if (totalCount > 1) locator = locator.nth(seenIndex); refs.set(ref, { locator, role: node.role, name: node.name || '' }); diff --git a/test/smoke.ts b/test/smoke.ts index ae8d9d0..245ad25 100644 --- a/test/smoke.ts +++ b/test/smoke.ts @@ -337,6 +337,84 @@ c('click @e resolves against the last snapshot', async () => { assert(r.stdout.trim() !== 'https://example.com/', `click @e1 should navigate away: ${r.stdout}`); }); +c('click reports dialogDismissed when a modal closes', async () => { + // Fixture: a [role=dialog] open at load with a button that hides itself. + // Mirrors the HubSpot confirm-modal shape — synchronous dismissal so the + // 300ms default budget catches it on the first poll. + const html = ` + + `; + await run(['goto', `data:text/html,${encodeURIComponent(html)}`]); + await run(['wait', '200']); + await run(['snapshot', '-i']); + const r = await run(['click', '@e1', '--json']); + const data = parseJson<{ ok: boolean; dialogDismissed: boolean; preDialogCount: number; postDialogCount: number }>(r.stdout); + assert(data.ok === true, 'click should return ok'); + assert(data.dialogDismissed === true, `expected dialogDismissed=true, got ${JSON.stringify(data)}`); + assert(data.preDialogCount === 1 && data.postDialogCount === 0, `expected pre=1 post=0, got pre=${data.preDialogCount} post=${data.postDialogCount}`); +}); + +c('click reports dialogDismissed=false when nothing changes', async () => { + // Inert button, no dialog on the page → both flags should be false and + // pre/postDialogCount both 0. Lets agents distinguish "click landed, + // nothing happened" from "click closed something." + const html = ``; + await run(['goto', `data:text/html,${encodeURIComponent(html)}`]); + await run(['wait', '200']); + await run(['snapshot', '-i']); + const r = await run(['click', '@e1', '--observe-ms', '100', '--json']); + const data = parseJson<{ ok: boolean; dialogDismissed: boolean; urlChanged: boolean }>(r.stdout); + assert(data.ok === true, 'click should return ok'); + assert(data.dialogDismissed === false, `expected dialogDismissed=false, got ${JSON.stringify(data)}`); + assert(data.urlChanged === false, `expected urlChanged=false, got ${JSON.stringify(data)}`); +}); + +c('click --no-observe skips post-click observation', async () => { + // Opt-out path: caller wants raw `{ok:true}` (e.g. in benchmarks where + // the extra count() round-trip is measurable). Verify no extra fields. + const html = ``; + await run(['goto', `data:text/html,${encodeURIComponent(html)}`]); + await run(['wait', '200']); + await run(['snapshot', '-i']); + const r = await run(['click', '@e1', '--no-observe', '--json']); + const data = parseJson>(r.stdout); + assert(data.ok === true, 'click should return ok'); + assert(!('dialogDismissed' in data) && !('urlChanged' in data), `expected no observation fields, got keys=${Object.keys(data)}`); +}); + +c('snapshot scopes locators to the auto-detected modal', async () => { + // Two buttons named "Confirm": one in the modal, one outside (hidden by + // an aria-hidden parent the way most React libs hide the page behind a + // scrim). The snapshot's modal-scope walker shows only the modal's + // button — but the OLD code built `getByRole` page-wide, so on a page + // with a unique-named button it'd accidentally match the outside one. + // Here both have the same accessible name, so a page-wide locator hits + // strict-mode (2 matches) and click would error. With modal-scoped + // locators, click resolves uniquely to the modal button and dismisses + // the dialog. + const html = ` + + + `; + await run(['goto', `data:text/html,${encodeURIComponent(html)}`]); + await run(['wait', '200']); + await run(['snapshot', '-i']); + const r = await run(['click', '@e1', '--json']); + const data = parseJson<{ ok: boolean; dialogDismissed: boolean }>(r.stdout); + assert(data.ok === true, `click should not error under strict-mode: ${JSON.stringify(data)}`); + assert(data.dialogDismissed === true, `expected modal button to dismiss the dialog, got ${JSON.stringify(data)}`); + // And confirm we hit the inside one, not the outside one. + const which = await run(['eval', 'JSON.stringify({inside: !!window.__inside, outside: !!window.__outside})']); + const flags = parseJson<{ inside: boolean; outside: boolean }>(which.stdout); + assert(flags.inside === true && flags.outside === false, `expected inside-button click, got ${JSON.stringify(flags)}`); +}); + c('chain executes multiple steps', async () => { const steps = JSON.stringify([ { cmd: 'goto', args: ['https://example.com'] }, @@ -533,13 +611,20 @@ c('perf returns CWV + navigation timing shape', async () => { }); c('console --dedup collapses repeats with count', async () => { - // Seed 5 identical errors + 1 unique one, then dedup. - await run(['eval', 'for (let i = 0; i < 5; i++) { try { throw new Error("ghax-smoke-repeat"); } catch (e) { console.error(e.message); } } console.error("ghax-smoke-unique")']); + // Seed 5 identical errors + 1 unique one, then dedup. The console + // buffer persists across smoke runs against the same daemon, so use + // a per-run marker — re-running smoke against a warm daemon would + // otherwise see 5 entries from the previous run plus 5 new ones and + // fail with `expected count=5, got 10`. + const tag = `ghax-smoke-${Date.now().toString(36)}`; + const repeatText = `${tag}-repeat`; + const uniqueText = `${tag}-unique`; + await run(['eval', `for (let i = 0; i < 5; i++) { try { throw new Error(${JSON.stringify(repeatText)}); } catch (e) { console.error(e.message); } } console.error(${JSON.stringify(uniqueText)})`]); await run(['wait', '100']); const r = await run(['console', '--errors', '--dedup', '--last', '50', '--json']); const groups = parseJson>(r.stdout); - const repeat = groups.find((g) => g.text === 'ghax-smoke-repeat'); - const unique = groups.find((g) => g.text === 'ghax-smoke-unique'); + const repeat = groups.find((g) => g.text === repeatText); + const unique = groups.find((g) => g.text === uniqueText); assert(repeat && repeat.count === 5, `expected repeat count=5, got ${repeat?.count}`); assert(unique && unique.count === 1, `expected unique count=1, got ${unique?.count}`); });