fix(desktop-browser): tighten ref reuse, name growth, and covered typing per review

Author: waleedlatif1

apps/desktop/e2e/browser-tools.spec.ts

@@ -1365,7 +1365,7 @@ test.describe('browser tools', () => {
     expect(await formState()).toMatchObject({ name: 'Observed value' })
     const result = fill.result as { observation: { result: { matches: { elementId: number }[] } } }
     const freshId = result.observation.result.matches[0].elementId
-    expect(freshId).not.toBe(ref('Name'))
+    expect(freshId).toBe(ref('Name'))
     const clear = await execute('browser_type', { elementId: freshId, text: '', observe: {} })
     expect(clear.ok, clear.error).toBe(true)
     expect(clear.result).toMatchObject({ observation: { ok: true } })

apps/desktop/src/main/browser-agent/driver.ts

@@ -37,7 +37,6 @@ import { interruptibleSleep, sleep } from '@sim/utils/helpers'
 import { isRecordLike, omit, toArray, toRecord } from '@sim/utils/object'
 import type { BrowserWindow, MenuItemConstructorOptions, WebContents, WebFrameMain } from 'electron'
 import { Menu } from 'electron'
-import { LRUCache } from 'lru-cache'
 import * as cdp from '@/main/browser-agent/cdp'
 import { steppedZoomFactor, zoomPercentOf } from '@/main/browser-agent/context-menu'
 import { ToolError } from '@/main/browser-agent/errors'
@@ -146,9 +145,6 @@ function isBrowserWaitElementState(value: string): value is BrowserWaitElementSt
 
 type PageExecutionTarget = WebContents | WebFrameMain
 
-/** Matches the page's own memory of issued refs; an evicted ref simply gets a new id. */
-const ISSUED_REF_MEMORY = 5000
-
 /** Whether two targets are the same page or frame; Electron may hand out separate wrappers for one frame. */
 function samePageTarget(
   left: PageExecutionTarget | undefined,
@@ -343,7 +339,7 @@ interface DriverScopeState {
   /** Monotonic ref floor shared by every frame in this browser scope. */
   nextElementRefId: number
   /** Every ref this scope issued and its frame: a later snapshot may reuse it there, and an action may still name it. */
-  issuedRefTargets: LRUCache<number, PageExecutionTarget>
+  issuedRefTargets: Map<number, PageExecutionTarget>
   /** Invalidates captures that finish after a tab/navigation/timeout race. */
   snapshotCaptureEpoch: number
   /** Cancels a timed-out multi-step action before any later native dispatch. */
@@ -385,7 +381,7 @@ function createDriverScopeState(): DriverScopeState {
     snapshotTabId: null,
     snapshotTargets: new Map(),
     nextElementRefId: 0,
-    issuedRefTargets: new LRUCache({ max: ISSUED_REF_MEMORY }),
+    issuedRefTargets: new Map(),
     snapshotCaptureEpoch: 0,
     toolExecutionEpoch: 0,
   }
@@ -2608,6 +2604,11 @@ async function captureSnapshot(
         (targetLineIndexes.get(elementId) ?? Number.POSITIVE_INFINITY) < visibleOutlineLines.length
     )
   )
+  // Never forget a ref a live page may still hand back (the page itself
+  // decides when to reissue); only refs of closed tabs and frames go.
+  for (const [ref, target] of state.issuedRefTargets) {
+    if ('isDestroyed' in target && target.isDestroyed()) state.issuedRefTargets.delete(ref)
+  }
   for (const [ref, target] of targets) state.issuedRefTargets.set(ref, target)
   state.snapshotTabId = capturedTabId
   return {

apps/desktop/src/main/browser-agent/page-functions.test.ts

@@ -368,6 +368,17 @@ describe('combobox typing surfaces', () => {
     expect(input.value).toBe('cooking pot')
   })
 
+  it('still refuses a top-page field a modal dialog covers', () => {
+    document.body.innerHTML =
+      '<input type="text" aria-label="Search" /><div role="dialog" aria-modal="true"><p>Sign in</p></div>'
+    const input = visible(document.querySelector('input') as HTMLInputElement)
+    const cover = visible(document.querySelector('p') as HTMLParagraphElement)
+    register(input)
+    Object.defineProperty(document, 'elementFromPoint', { configurable: true, value: () => cover })
+
+    expect(focusElementForTyping(0)).toMatchObject({ error: 'obstructed' })
+  })
+
   it('still refuses a covered field inside a frame', () => {
     const frame = document.createElement('iframe')
     document.body.append(frame)
@@ -705,7 +716,8 @@ describe('collectSnapshot', () => {
   })
 
   it('keeps a connected ref whose name grew when its own submenu opened', () => {
-    document.body.innerHTML = '<div role="button" tabindex="0">Products ▾<ul></ul></div>'
+    document.body.innerHTML =
+      '<div role="button" tabindex="0" aria-haspopup="menu">Products ▾<ul></ul></div>'
     const menu = visible(document.querySelector('[role="button"]') as HTMLDivElement)
     const ref = refFor(outlineOf(collectSnapshot()), 'Products ▾')
 
@@ -732,6 +744,25 @@ describe('collectSnapshot', () => {
     expect(clickElement(ref)).toMatchObject({ dispatched: true })
   })
 
+  it('invalidates a recycled row whose new name merely extends the old one', () => {
+    document.body.innerHTML = '<div role="listitem">Alice</div>'
+    const row = visible(document.querySelector('[role="listitem"]') as HTMLDivElement)
+    const ref = refFor(outlineOf(collectSnapshot()), 'Alice')
+
+    row.textContent = 'Alice Smith'
+
+    expect(clickElement(ref)).toMatchObject({ error: 'stale' })
+  })
+
+  it('gives a row control a new ref when its row now shows a different record', () => {
+    document.body.innerHTML = '<div role="row"><span>Order 1</span><button>Delete</button></div>'
+    for (const el of Array.from(document.querySelectorAll('span, button'))) visible(el)
+    const first = refFor(outlineOf(collectSnapshot()), 'Delete')
+    ;(document.querySelector('span') as HTMLSpanElement).textContent = 'Order 2'
+
+    expect(refFor(outlineOf(collectSnapshot()), 'Delete')).not.toBe(first)
+  })
+
   it('invalidates a connected ref whose name changed to a longer, different word', () => {
     document.body.innerHTML = '<div role="listitem">Item 1</div>'
     const row = visible(document.querySelector('[role="listitem"]') as HTMLDivElement)

apps/desktop/src/main/browser-agent/page-functions.ts

@@ -37,6 +37,8 @@ interface SnapshotRefLocator {
   attributes: Record<string, string>
   ancestor: string
   context: string
+  /** Hash of the element's DOM text, to tell revealed content from replaced content. */
+  textHash: number
 }
 
 declare global {
@@ -606,6 +608,14 @@ export function collectSnapshot(
   const refOfElement = window.__simAgentRefOfElement ?? new WeakMap()
   window.__simAgentRefOfElement = refOfElement
   const issuedRefCap = 5000
+  const textHashOf = (el: Element): number => {
+    const text = el.textContent || ''
+    let hash = 0
+    for (let index = 0; index < text.length; index++) {
+      hash = (hash * 31 + text.charCodeAt(index)) | 0
+    }
+    return hash
+  }
   const registerElement = (el: Element, role: string, name: string): number => {
     const locator = {
       url: pageUrlFor(el),
@@ -615,6 +625,7 @@ export function collectSnapshot(
       attributes: locatorAttributes(el),
       ancestor: ancestorSignature(el),
       context: contextSignature(el),
+      textHash: textHashOf(el),
     }
     const priorId = refOfElement.get(el)
     const prior = priorId === undefined ? undefined : issuedRefs.get(priorId)
@@ -624,6 +635,8 @@ export function collectSnapshot(
       prior.locator.tag === locator.tag &&
       prior.locator.role === locator.role &&
       prior.locator.name === locator.name &&
+      prior.locator.ancestor === locator.ancestor &&
+      prior.locator.context === locator.context &&
       Object.keys({ ...prior.locator.attributes, ...locator.attributes }).every(
         (key) => prior.locator.attributes[key] === locator.attributes[key]
       )
@@ -889,7 +902,10 @@ export function collectSnapshot(
    * real stale ref, never permission to click something nearby.
    */
   window.__simAgentResolveElement = (id: number, allowRecovery = true) => {
-    if (scopedRoot && !scopedRoot.isConnected) {
+    // A ref from an earlier snapshot resolves only to its own live node: no
+    // lookalike recovery, and the latest snapshot's scope does not apply to it.
+    const inLatestSnapshot = locators[id] !== undefined
+    if (inLatestSnapshot && scopedRoot && !scopedRoot.isConnected) {
       window.__simAgentStaleReason = 'the scoped snapshot root left the DOM'
       return null
     }
@@ -937,12 +953,22 @@ export function collectSnapshot(
       ) {
         return false
       }
-      // A still-mounted control whose own menu or disclosure opened (often by
-      // the hover or click being validated) appends its new content to its
-      // name; a recycled row replaces the name instead.
+      // A mounted control whose own menu opened (often by the hover or click
+      // being validated) appends that menu to its name: either hidden content
+      // was revealed (its DOM text is unchanged) or it declares a popup. A
+      // recycled row changes its DOM text with neither.
       const name = nameFor(candidate)
+      const opensPopup =
+        textHashOf(candidate) === locator.textHash ||
+        ['aria-haspopup', 'aria-expanded', 'aria-controls'].some(
+          (attribute) => (candidate.getAttribute(attribute) || 'false') !== 'false'
+        ) ||
+        candidate.querySelector(
+          '[role="menu"], [role="menubar"], [role="listbox"], [role="tooltip"], [role="dialog"], [role="tree"]'
+        ) !== null
       const nameGrewInPlace =
         connected &&
+        opensPopup &&
         locator.name.length > 0 &&
         name.startsWith(locator.name) &&
         (locator.name.length >= 120 || /\s/.test(name.charAt(locator.name.length)))
@@ -1037,7 +1063,7 @@ export function collectSnapshot(
       }
     }
 
-    if (!allowRecovery) {
+    if (!allowRecovery || !inLatestSnapshot) {
       window.__simAgentStaleReason = 'the original snapshot node is detached or hidden'
       return null
     }
@@ -1915,11 +1941,30 @@ export function focusElementForTyping(id: number, moveFocus = true): unknown {
       coveredByRelatedPopup = true
     }
   }
-  // Top-page typing goes through keyboard focus, so a non-modal overlay does not block it (modals
-  // fail the aria-hidden/inert checks above); framed fields keep the check for their mapped point.
+  // Top-page typing goes through keyboard focus, so a non-modal overlay does not block it; a modal
+  // or dialog cover still refuses, and framed fields keep the check for their mapped point.
   const topPageField = view === window && window.top === window
+  const insideModal = (node: Element | null): boolean => {
+    for (let current = node; current; current = composedParent(current)) {
+      const role = current.getAttribute('role')
+      if (
+        current.getAttribute('aria-modal') === 'true' ||
+        role === 'dialog' ||
+        role === 'alertdialog' ||
+        tagFor(current) === 'DIALOG'
+      ) {
+        return true
+      }
+    }
+    return false
+  }
   let coveredBy: string | undefined
-  if (!chosenPoint && topPageField && blockedPoints.length > 0) {
+  if (
+    !chosenPoint &&
+    topPageField &&
+    blockedPoints.length > 0 &&
+    !blockedPoints.some((point) => insideModal(point.hit))
+  ) {
     chosenPoint = { x: blockedPoints[0].x, y: blockedPoints[0].y }
     coveredBy = blockerLabel(firstBlocker)
   }

apps/desktop/src/main/browser-agent/post-action-observation.test.ts

@@ -22,6 +22,14 @@ describe('post-action observation', () => {
     })
   })
 
+  it('rejects an explicit null observe on a wait instead of treating it as omitted', async () => {
+    const action = vi.fn(async () => ({ found: true }))
+    await expect(
+      withPostActionObservation('browser_wait_for', { observe: null }, action, vi.fn(), vi.fn())
+    ).rejects.toThrow('observe must be')
+    expect(action).not.toHaveBeenCalled()
+  })
+
   it('observes after the action, preserving partial form results and replacing refs', async () => {
     const order: string[] = []
     const result = await withPostActionObservation(

apps/desktop/src/main/browser-agent/post-action-observation.ts

@@ -22,7 +22,8 @@ const OBSERVABLE_ACTIONS: ReadonlySet<BrowserToolName> = new Set([
 
 /** The call's post-action observation request; a wait observes by default, to show what the page became. */
 export function observeRequest(tool: BrowserToolName, params: Record<string, unknown>): unknown {
-  return params.observe ?? (tool === 'browser_wait_for' ? {} : undefined)
+  if (params.observe !== undefined) return params.observe
+  return tool === 'browser_wait_for' ? {} : undefined
 }
 
 /** Preserves a dispatched action when its acknowledgement or observation is interrupted. */