Add fix for page.url() race when navigation happens as a result of act()#1795
Add fix for page.url() race when navigation happens as a result of act()#1795
act()#1795Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Greptile SummaryThis PR fixes a race condition where calling Key changes:
Notable concern: Every Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant performUnderstudyMethod
participant navWatcher
participant CDP Session
participant waitForDomNetworkQuiet
Caller->>performUnderstudyMethod: act("click", xpath)
performUnderstudyMethod->>navWatcher: watchMainFrameUrlChangeStart(page, page.url())
navWatcher->>CDP Session: session.on("Page.frameNavigated")
navWatcher->>CDP Session: session.on("Page.navigatedWithinDocument")
performUnderstudyMethod->>performUnderstudyMethod: clickElement(ctx)
alt Navigation fires during click (fast)
CDP Session-->>navWatcher: Page.frameNavigated / NavigatedWithinDocument
navWatcher->>navWatcher: matched = true
end
performUnderstudyMethod->>navWatcher: wait(400ms)
alt Navigation already matched
navWatcher-->>performUnderstudyMethod: true (immediate)
else Navigation fires within 400ms window
CDP Session-->>navWatcher: Page.frameNavigated / NavigatedWithinDocument
navWatcher-->>performUnderstudyMethod: true (early resolve)
else No navigation within 400ms
navWatcher-->>performUnderstudyMethod: false (timeout)
end
alt wait() returned true
performUnderstudyMethod->>waitForDomNetworkQuiet: waitForDomNetworkQuiet(mainFrame, timeout)
waitForDomNetworkQuiet-->>performUnderstudyMethod: settled
end
performUnderstudyMethod->>navWatcher: dispose()
navWatcher->>CDP Session: session.off("Page.frameNavigated")
navWatcher->>CDP Session: session.off("Page.navigatedWithinDocument")
performUnderstudyMethod-->>Caller: return (page.url() reflects new page)
Last reviewed commit: 14c9ea4 |
| if (navWatcher && (await navWatcher.wait(400))) { | ||
| await waitForDomNetworkQuiet(page.mainFrame(), domSettleTimeoutMs).catch( | ||
| () => {}, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Unconditional 400ms latency on every click/doubleClick
navWatcher.wait(400) always blocks for up to 400ms after every click or doubleClick action, even when the click does not trigger any navigation. The timer in wait() only resolves early if a Page.frameNavigated or Page.navigatedWithinDocument event fires within the window — if neither fires, the 400ms timeout runs to completion before returning false.
In agentic workflows that perform many clicks (selecting options, toggling checkboxes, closing modals, etc.) this adds up to 400ms of dead wait per non-navigating click. A task with 20 clicks would incur up to 8 seconds of pure idle time.
Consider whether the window should start from when the click handler is dispatched rather than when wait() is called, or whether a shorter window (e.g. 150–200ms) would be enough to catch the typical "JS deferred with setTimeout" pattern while reducing the worst-case overhead on non-navigating clicks.
There was a problem hiding this comment.
ALTERNATIVE IDEA (which I prefer) would be add an arg to act({..., expectNavigation: true | false}) and only wait for nav if true is passed.
There was a problem hiding this comment.
That's a much better approach. The explicit expectNavigation flag avoids the entire detection problem — no 400ms timeout penalty on non-navigating clicks, and the caller already knows whether they expect navigation (e.g., clicking a submit button vs toggling a checkbox).
The automatic detection forces a trade-off between latency (shorter window = faster but might miss slow navigations) and reliability (longer window = catches more cases but adds overhead everywhere). An explicit flag sidesteps this entirely and makes the behavior predictable.
The only downside is slightly more verbose call sites when navigation is expected, but that's a small price for eliminating 8+ seconds of idle time across a multi-click task.
why
act()call results in a navigation and we immediately callpage.url()or.title()after we may still get the old page, causing the agent to think it needs to navigate again or making it think it's on the wrong page.what changed
act()call, and wait until the nav settles before returning so that subsequent steps see the next page instead of the old one.Important
ALTERNATIVE IDEA (which I prefer) would be add an arg to
act({..., expectNavigation: true | false})and only wait for nav iftrueis passed.test plan
Summary by cubic
Fixes a race where
page.url()/page.title()could return the old page right after anact()-triggered navigation. We now detect main-frame navigation that starts within 400ms of aclick/doubleClickand wait for it to settle before returning.performUnderstudyMethod, arm a main-frame nav watcher beforeclick/doubleClick; if it reports a start within 400ms after the click returns, wait forwaitForDomNetworkQuiet.watchMainFrameUrlChangeStart(page, initialUrl)to listen forPage.frameNavigated/Page.navigatedWithinDocumentonly on real URL changes, handle main-frame ID changes, and dispose cleanly.perform-understudy-navigation-race.spec.tsverifies wait for <400ms and no wait for >400ms, using livehttps://example.comURLs.Written for commit a6bb6ef. Summary will update on new commits. Review in cubic