Repository navigation
Bug 2080970 - Migrate dialog tools to BiDi - #234
Conversation
juliandescottes
left a comment
juliandescottes
left a comment
There was a problem hiding this comment.
Thanks Holger, mostly one question regarding the switch to "ignore" (needed, but maybe too soon?) and a regression with prompt default values.
| context: contextId, | ||
| accept: true, | ||
| userText: promptText, | ||
| }); |
There was a problem hiding this comment.
This will cause a regression in one edge case, when prompt is called with a default value, eg prompt("favorite color", "pink"). Previously the default value would be used if promptText was undefined, but here we will default to empty string.
We can workaround by listening to userPromptOpened and storing the defaultValue, but that sounds quite hacky.
To be honest I think this is rather a BiDi spec issue, because there is no easy way to use the default value of a prompt, but maybe we should still work around this here.
Or we can also wait and handle that when we fix Bug 2081600 - [firefox-devtools-mcp] Report open alert/confirm/prompt dialogs and the tools that handle them, which will require to listen to this event anyway.
There was a problem hiding this comment.
Let's handle this together with 2081600.
| if (this.options.acceptInsecureCerts) { | ||
| caps.set('acceptInsecureCerts', true); | ||
| } | ||
| caps.setAlertBehavior(UserPromptHandler.IGNORE); |
There was a problem hiding this comment.
In practice, can agents react to dialogs at the moment? We are not notifying them about it (this is Bug 2081600 - [firefox-devtools-mcp] Report open alert/confirm/prompt dialogs and the tools that handle them) so I feel like they might be stuck whenever a dialog shows up?
Dismissing all dialogs is definitely bad, but it is the current behavior. I feel like we should switch to "ignore" only when we implement 2081600? What do you think? Unless you plan to tackle 2081600 right after, in which case I'm fine landing this as is.
There was a problem hiding this comment.
OK, let's defer the switch until 2081600 is fixed. I've taken that part out of the PR.
juliandescottes
left a comment
juliandescottes
left a comment
There was a problem hiding this comment.
Thanks for the update Holger, works for me!
One nit about the docs, but this looks ready to land.
| }; | ||
| } | ||
|
|
||
| export const mcpTest = test |
There was a problem hiding this comment.
Fixture is really nice, we don't have that many mcp/ tests, but that looks a lot better.
Should we update docs/testing.md to mention this instead of startMcpServer?
This PR also adds
mcpTestwhich reduces the boilerplate in MCP tests and includes the fix for Bug 2081620 from #211 (with the integration tests migrated to the MCP test harness).