feat: make pageId routing always-on#1777
Conversation
bcfmtolgahan
force-pushed
the
feat/pageid-routing-always-on
branch
from
cceb6af to
394fbd2
Compare
| describe: | ||
| 'Whether to expose pageId on page-scoped tools and route requests by page ID.', | ||
| '(Deprecated, now always enabled) pageId is always exposed on page-scoped tools.', | ||
| hidden: true, |
There was a problem hiding this comment.
so we have options:
- remove the flag altogether
- remove hidden: true an experimental prefix.
If we keep the flag, I think we can decide to keep it off by default or keep an option to turn off pageId routing to save tokens.
cc @natorion wdyt?
There was a problem hiding this comment.
The overhead is only the resident token cost of having pageId in help, right? The parameter per-se is optional.
In CLI scenarios I expect such a resident token cost to be miniscule compared to whatever is returned via take_snapshot.
There was a problem hiding this comment.
It's optional but it is included on each llm iteration even if no heavy output like take_snapshot is used.
There was a problem hiding this comment.
PR #1780 addresses the wait helpers, waitForEventsAfterAction is now on McpPage and each tool uses the correct page's throttling settings.
On the flag: happy to go either way. If keeping it as an opt-out for token savings makes sense, I can rewire index.ts to respect it as a disable toggle and rename it (e.g. --noPageIdRouting). If the consensus is that the overhead is acceptable, I'll remove it entirely. What's your preference?
There was a problem hiding this comment.
We need some time to evaluate if the overhead is acceptable. For now, let's keep the flag in case some users are not happy with the overhead or in case the pageId routing does not work as expected. There is no need to rename to --noPageIdRouting as yargs automatically handles this for boolean args. Right now you can already use --noExperimentalPageRouting, so I would say let's keep the flag, drop the experimental prefix, remove hidden: true and set default to true. If we find the token overhead to be too much, we can update the arg to default to false. Or users can use --no-page-id-routing or --page-id-routing=false.
There was a problem hiding this comment.
Done. Dropped the experimental prefix, removed hidden: true, set default: true. Users can opt out with --no-page-id-routing.
|
TODO: this cannot land before the fixes to the wait helpers are made to correctly operate on the specified page. |
|
TODO: evaluate that the model uses pageId correctly via eval scenarios in https://github.com/ChromeDevTools/chrome-devtools-mcp/tree/main/scripts/eval_scenarios |
Splits out from #1244 per review feedback. 'waitForEventsAfterAction' previously lived in 'McpContext' and always used the selected page's CPU/network throttling settings. With pageId routing, a tool can target a different page than the selected one, meaning wrong throttling multipliers were applied. Moving the method to 'McpPage' fixes this: each tool now calls 'page.waitForEventsAfterAction(...)' and gets the correct page's emulation settings. 'getNetworkMultiplierFromString' is extracted to 'WaitForHelper.ts' to avoid a circular import (McpContext → McpPage already exists). Unblocks #1777.
Splits out from #1244 per review feedback. 'waitForEventsAfterAction' previously lived in 'McpContext' and always used the selected page's CPU/network throttling settings. With pageId routing, a tool can target a different page than the selected one, meaning wrong throttling multipliers were applied. Moving the method to 'McpPage' fixes this: each tool now calls 'page.waitForEventsAfterAction(...)' and gets the correct page's emulation settings. 'getNetworkMultiplierFromString' is extracted to 'WaitForHelper.ts' to avoid a circular import (McpContext → McpPage already exists). Unblocks #1777.
|
@bcfmtolgahan would you be up to rebasing this PR? |
Rename --experimental-page-id-routing to --page-id-routing and enable it
by default. Users can opt out with --no-page-id-routing if token overhead
is unacceptable or routing does not work as expected.
- Rename serverArgs.experimentalPageIdRouting to pageIdRouting and set
default: true; describe text updated to reflect opt-out semantics.
- Update ToolHandler.ts schema injection and page resolution to use the
new flag name.
- Drop the now-redundant delete of pageIdRouting from chrome-devtools-cli
start options (was previously removing experimentalPageIdRouting).
- Update evaluate_script to use cliArgs.pageIdRouting and guard
getPageById with a request.params.pageId check so passing the flag
without a pageId no longer throws (mirrors the guard used in
ToolHandler.ts).
- Improve pageIdSchema description: explain list_pages and selected-page
fallback so the field is self-documenting.
- Fix generate-docs.ts to inject pageIdSchema for page-scoped tools so
tool-reference.md surfaces pageId on every page-scoped tool; pass a
slim flag so slim docs stay unchanged.
- Update eval scenarios to rely on the new default instead of passing
--experimental-page-id-routing; update the example flag in the
TestScenario JSDoc accordingly.
- Fix pageId type in script.test.ts (string -> number).
- Regenerated docs/tool-reference.md, README options section, and
src/telemetry/{flag_usage_metrics,tool_call_metrics}.json.
- Add pageIdRouting/page-id-routing to the cli.test.ts default args
fixture to reflect the new default.
bcfmtolgahan
force-pushed
the
feat/pageid-routing-always-on
branch
from
5f96b59 to
7bb6554
Compare
|
@OrKoN rebased onto latest main. Adapted to the new 'ToolHandler.ts', location for schema injection and routing. Squashed to a single commit since the changes got intertwined during the adaptation, happy to split if preferred. |
3 participants
Splits out from #1244 per review feedback.
Removes the
experimentalPageIdRoutingflag —pageIdis now alwaysexposed on page-scoped tools. The flag is kept as a no-op for backwards
compatibility.