Bug 2065546 - [firefox-devtools-mcp] navigate should accept an option to wait for load - #167
Open
shoemoney wants to merge 1 commit into
Open
Bug 2065546 - [firefox-devtools-mcp] navigate should accept an option to wait for load#167shoemoney wants to merge 1 commit into
shoemoney wants to merge 1 commit into
Conversation
… to wait for load navigate always used the scheme-derived readiness state: "interactive" (DOMContentLoaded) for http/https/data/blob/file, "none" for everything else. An agent had no way to wait for the load event, so it could not tell a page that had finished loading from one still fetching subresources — which matters when deciding it is safe to stop a performance recording. Add an optional `wait` argument to navigate_page and new_page, exposing the three WebDriver BiDi readiness states. Omitting it keeps the existing scheme-derived default, so nothing changes for callers that do not pass it. An explicit value is honoured for every scheme rather than being downgraded for uncommon ones, and an unknown value is rejected instead of ignored — silently falling back to the default would leave the caller believing it waited for something it did not.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
navigatederives its readiness state from the URL scheme and gives the caller no say:So an agent cannot tell a page that has finished loading from one still fetching subresources — which is exactly the distinction Bug 2065546 needs for "when is it safe to stop the performance recording".
This adds an optional
waittonavigate_pageandnew_page, exposing the three BiDi readiness states. Omitting it keeps today's behaviour exactly, so nothing changes for existing callers.Two decisions worth flagging
An explicit
waitis honoured for every scheme, not just common ones. The scheme heuristic only applies when no override is given. Silently downgradingcompletetononebecause the URL happened to bemoz-extension:would discard what the caller asked for with no way to tell — the same failure mode this bug is about. There is a test for it.An unknown value is rejected, not ignored.
wait: "load"is the obvious guess (it is the DOM event name, and the wire value iscomplete), and onmainan unrecognised argument is silently dropped andnavigate_pagereports success:The tool response also echoes the state back (
[0] → https://example.com (waited for: complete)) but only when one was explicitly requested, so the default output is unchanged.new_pagegets the same argument becausecreateNewPagecallsnavigatedirectly — an agent opening a tab has the same need. Happy to drop that half if you would rather keep the change tonavigate_pagealone.Verified against real Firefox
Local server, document commits immediately,
<img>subresource stalls 3s. Median of 5 runs after 2 warm-up loads:waitdocument.readyStateinteractive'none'interactive'interactive'interactive'complete'completecompleteblocks on the stalled subresource; the default is unchanged. Separatingnonefrominteractiveneeds a streamed document rather than a stalled subresource — with a flushed 64KB first chunk and the body trickling for 3s,nonereturns at 1251ms (commit) whileinteractivewaits 3044ms for DOMContentLoaded. Worth knowing thatnonereturns on navigation commit, not instantly.A limit you should know about before taking this
wait: "complete"is capped at 10 seconds by the global BiDi command timeout atsrc/firefox/bidi.ts:61. On a page whoseloadfires later than that,navigatethrows rather than waiting:(Measured with a 12s-stalled subresource.) This is pre-existing and applies to every BiDi command, but
completewill hit it far more often thaninteractivedoes, and heavy real-world pages — ads, third-party trackers, video — routinely take longer than 10s to fireload.I have not changed that timeout. Making it configurable, or special-casing
navigate, is a decision about the BiDi layer that affects every command, and it is not mine to make inside a feature PR. Happy to follow up in whichever shape you prefer — a per-command override, a longer default fornavigate, or leaving it and documenting the ceiling.Tests
12 new unit tests across
tests/firefox/pages.test.tsandtests/tools/pages.test.ts. All 12 fail on unmodifiedmain:No integration test, deliberately. I wrote one, and it passed on its own and alongside the other integration files — but under the full
vitest runit intermittently failed withBiDi command timeout: browsingContext.navigate, becausewait: "complete"holds a BiDi call open for the length of the stall and the suite was already contending for resources. Any stall long enough to reliably separatecompletefrominteractiveis long enough to risk that timeout under load. Given Bug 2036018 is already open about intermittent Firefox CI failures, adding a new source of flake seemed clearly worse than proving the behaviour with deterministic unit tests plus the measurements above. Say the word if you want it anyway and I will add it back.