fix(click): observe post-click effects + scope snapshot locators to modal

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 <n>` (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

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 <n>    → 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) => {

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 `<dialog open>`,
+ * 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<SnapshotResult> {
   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 `<dialog open>`, 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 || '' });

test/smoke.ts

@@ -337,6 +337,84 @@ c('click @e<n> 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 = `
+    <div role="dialog" aria-modal="true" id="d">
+      <button id="ok" onclick="document.getElementById('d').style.display='none'">OK</button>
+    </div>
+  `;
+  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 = `<button id="b" onclick="void 0">noop</button>`;
+  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 = `<button>x</button>`;
+  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<Record<string, unknown>>(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 = `
+    <div aria-hidden="true">
+      <button id="outside" onclick="window.__outside=true">Confirm</button>
+    </div>
+    <div role="dialog" aria-modal="true" id="d">
+      <button id="inside" onclick="window.__inside=true; document.getElementById('d').style.display='none'">Confirm</button>
+    </div>
+  `;
+  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<Array<{ text: string; count: number }>>(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}`);
 });