Add WebDriver BiDi support for real-time browser diagnostics#58
Merged
angiejones merged 10 commits intomainfrom Feb 22, 2026
Merged
Add WebDriver BiDi support for real-time browser diagnostics#58angiejones merged 10 commits intomainfrom
angiejones merged 10 commits intomainfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds opt-in per-session WebDriver BiDi: dynamic imports with graceful fallback, per-session BiDi state and setup, automatic background capture of console/page/network events, three new MCP diagnostic tools to retrieve logs (with optional clearing), and tests + fixture validating behavior and isolation. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant MCP as MCP Server
participant Driver as WebDriver
participant BiDi as BiDi Modules
participant State as state.bidi
Client->>MCP: start_browser (options.bidi = true)
activate MCP
MCP->>Driver: build driver (enableBiDi if available)
MCP->>BiDi: import modules dynamically (nullable)
MCP->>State: state.bidi.set(sessionId, newBidiState())
MCP->>Driver: setupBidi(driver, sessionId) (register listeners)
deactivate MCP
Client->>Driver: navigate / interact
activate BiDi
BiDi->>State: append consoleLogs / pageErrors / networkLogs
deactivate BiDi
Client->>MCP: get_console_logs (clear?)
MCP->>State: read state.bidi.get(sessionId).consoleLogs
MCP-->>Client: return logs (and clear if requested)
Client->>MCP: close_session
activate MCP
MCP->>State: state.bidi.delete(sessionId)
deactivate MCP
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Automatically enables BiDi when starting a browser session to passively capture console logs, JavaScript errors, and network activity in the background. Falls back silently if the browser/driver doesn't support it. New tools: - get_console_logs: retrieve captured console messages (log, warn, error) - get_page_errors: retrieve JS errors/exceptions with stack traces - get_network_logs: retrieve network responses and failed requests Design decisions: - No opt-in flag — BiDi is an implementation detail, not a user choice - Graceful fallback — older browsers work exactly as before - All BiDi event callbacks wrapped in try/catch to prevent server crashes - Per-session log buffers, cleaned up automatically on close Closes #55
angiejones
force-pushed
the
bidi-support
branch
from
a1fc6d8 to
24fe14b
Compare
…educe test redundancy - Replace 6 separate state Maps with single per-session bidi Map - Extract setupBidi() helper from start_browser - Extract registerBidiTool() factory to replace 3 copy-pasted tool handlers - Simplify cleanup in close_session and cleanup handler - Trim tests from 17 to 8 (remove redundancy from shared factory) - Remove unused fixture elements from bidi.html Net: -203 lines, same coverage, all tests pass.
angiejones
commented
Feb 22, 2026
angiejones
commented
Feb 22, 2026
- README keeps tool reference only; architectural details belong in AGENTS.md - Update state management docs to reflect consolidated bidi Map - Add new helper functions to helper table - Add bidi.test.mjs to test files table
Add 'unhandledPromptBehavior': 'ignore' capability alongside 'webSocketUrl': true. Without this, ChromeDriver's BiDi auto-dismisses alert/confirm/prompt dialogs before tools like accept_alert and get_alert_text can interact with them.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
AGENTS.md (1)
24-46:⚠️ Potential issue | 🟡 MinorFile map tree is missing the new BiDi entries.
The tree listing doesn't include
test/fixtures/bidi.htmlortest/bidi.test.mjs, even though both are added in this PR and the test table at Line 252 does referencebidi.test.mjs.Proposed fix
├── navigation.test.mjs ← navigate, all 6 locator strategies ├── interactions.test.mjs ← click, send_keys, get_element_text, hover, double_click, right_click, press_key, drag_and_drop, upload_file + ├── bidi.test.mjs ← BiDi enablement, console/error/network capture, session isolation └── fixtures/ ← HTML files loaded via file:// URLs ├── locators.html ├── interactions.html ├── mouse-actions.html ├── drag-drop.html - └── upload.html + ├── upload.html + └── bidi.html🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` around lines 24 - 46, AGENTS.md's project tree is missing the new BiDi test files; update the file map under the test/ subtree to include the new entries "test/bidi.test.mjs" and "test/fixtures/bidi.html" so the tree reflects the actual repository and the test table reference to bidi.test.mjs; locate the tree block at the top of AGENTS.md (the mcp-selenium/ listing) and add the two lines in the appropriate places alongside other test files (e.g., near browser.test.mjs and the fixtures list).src/lib/server.js (1)
782-801:⚠️ Potential issue | 🟠 MajorMissing
state.bidi.delete(sessionId)inclose_current_windowwhen last window is closed.When the last window is closed via
close_current_window, the driver is quit and removed fromstate.drivers, but the BiDi state is not cleaned up. This causes a memory leak for the session's BiDi buffers. Theclose_sessionhandler at Line 584 does this correctly.Proposed fix
try { await driver.quit(); } catch (quitError) { console.error(`Error quitting driver for session ${sessionId}:`, quitError); } state.drivers.delete(sessionId); + state.bidi.delete(sessionId); state.currentSession = null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server.js` around lines 782 - 801, In close_current_window where you quit the driver and remove it from state (see the block that calls driver.quit() and uses state.drivers.delete(sessionId) and sets state.currentSession = null), also remove the session's BiDi entry by calling state.bidi.delete(sessionId) so the BiDi buffers are cleaned up (mirror the cleanup done in close_session); place the state.bidi.delete(sessionId) alongside state.drivers.delete(sessionId)/state.currentSession reset to ensure no BiDi memory leak.
🧹 Nitpick comments (3)
test/bidi.test.mjs (2)
138-162: Session isolation test — consider adding anafterhook for robustness.If the test fails between
start_browsercalls (e.g., at Line 149), the secondclose_sessionat Line 160 is never reached. The top-levelclient.stop()will kill the server process, so this isn't a resource leak per se, but other describe blocks useafterwithtry/catchfor cleanup consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/bidi.test.mjs` around lines 138 - 162, The Session Isolation test can leave a browser session open if it fails before the final client.callTool('close_session') call; add an after hook for this describe block that attempts to close the session safely (call client.callTool('close_session') inside a try/catch) to mirror other tests' cleanup. Ensure the after hook runs unconditionally for the describe('Session Isolation') block and uses the same client instance used in the test so any in-progress session started by client.callTool('start_browser') is always closed.
1-3: Usenode:assert/strictfor consistency with other test files.Other test files in the project (e.g.,
test/server.test.mjs) importnode:assert/strict. This file importsnode:assert(non-strict). While most assertions here usestrictEqualandok(which behave the same either way), switching to the strict module keeps the test suite consistent.Suggested fix
-import assert from 'node:assert'; +import assert from 'node:assert/strict';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/bidi.test.mjs` around lines 1 - 3, Replace the non-strict assert import with the strict variant in the test file: update the import statement in test/bidi.test.mjs to import from "node:assert/strict" (the module that other tests use) so existing calls like assert.strictEqual and assert.ok continue to work and the test suite remains consistent with files such as test/server.test.mjs; locate the top-level import of assert in this file and change the module specifier only.src/lib/server.js (1)
60-109: PartialsetupBidifailure leaks state into the map.If
LogInspector(driver)succeeds (Line 72) butNetwork(driver)throws (Line 90), thebidientry is already in the map (Line 70) with some listeners wired.availablestaysfalseso tools report "unavailable", but the console/error listeners are still active and will silently accumulate data that's never retrievable and never cleaned up until session close.Consider moving
state.bidi.set(sessionId, bidi)to the end (after Line 108), or catching internally and cleaning up on partial failure.Suggested fix — set map entry only on full success
async function setupBidi(driver, sessionId) { const bidi = newBidiState(); - state.bidi.set(sessionId, bidi); const logInspector = await LogInspector(driver); // ... listeners ... const network = await Network(driver); // ... listeners ... bidi.available = true; + state.bidi.set(sessionId, bidi); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server.js` around lines 60 - 109, The setupBidi flow currently inserts a bidi entry into state.bidi early (state.bidi.set(sessionId, bidi)) so if LogInspector succeeds but Network(driver) throws you leak listeners; change setupBidi so the map entry is only set after all async setup completes successfully (move state.bidi.set(sessionId, bidi) to after bidi.available = true) or wrap the intermediate setup in a try/catch that on any failure removes/cleans the partially-constructed bidi (remove any registered LogInspector/Network listeners) before rethrowing; locate functions/identifiers setupBidi, newBidiState, state.bidi.set, LogInspector, Network and ensure bidi.available is only true when fully initialized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/server.js`:
- Around line 13-23: The current dynamic import assigns Network =
networkModule.Network which is incorrect because selenium-webdriver's
bidi/network.js uses a default export; update the Network assignment in the try
block to import the module's default export (same pattern used for LogInspector)
so Network is set to the module's default instead of undefined, ensuring BiDi
initialization works when LogInspector and Network are loaded.
---
Outside diff comments:
In `@AGENTS.md`:
- Around line 24-46: AGENTS.md's project tree is missing the new BiDi test
files; update the file map under the test/ subtree to include the new entries
"test/bidi.test.mjs" and "test/fixtures/bidi.html" so the tree reflects the
actual repository and the test table reference to bidi.test.mjs; locate the tree
block at the top of AGENTS.md (the mcp-selenium/ listing) and add the two lines
in the appropriate places alongside other test files (e.g., near
browser.test.mjs and the fixtures list).
In `@src/lib/server.js`:
- Around line 782-801: In close_current_window where you quit the driver and
remove it from state (see the block that calls driver.quit() and uses
state.drivers.delete(sessionId) and sets state.currentSession = null), also
remove the session's BiDi entry by calling state.bidi.delete(sessionId) so the
BiDi buffers are cleaned up (mirror the cleanup done in close_session); place
the state.bidi.delete(sessionId) alongside
state.drivers.delete(sessionId)/state.currentSession reset to ensure no BiDi
memory leak.
---
Nitpick comments:
In `@src/lib/server.js`:
- Around line 60-109: The setupBidi flow currently inserts a bidi entry into
state.bidi early (state.bidi.set(sessionId, bidi)) so if LogInspector succeeds
but Network(driver) throws you leak listeners; change setupBidi so the map entry
is only set after all async setup completes successfully (move
state.bidi.set(sessionId, bidi) to after bidi.available = true) or wrap the
intermediate setup in a try/catch that on any failure removes/cleans the
partially-constructed bidi (remove any registered LogInspector/Network
listeners) before rethrowing; locate functions/identifiers setupBidi,
newBidiState, state.bidi.set, LogInspector, Network and ensure bidi.available is
only true when fully initialized.
In `@test/bidi.test.mjs`:
- Around line 138-162: The Session Isolation test can leave a browser session
open if it fails before the final client.callTool('close_session') call; add an
after hook for this describe block that attempts to close the session safely
(call client.callTool('close_session') inside a try/catch) to mirror other
tests' cleanup. Ensure the after hook runs unconditionally for the
describe('Session Isolation') block and uses the same client instance used in
the test so any in-progress session started by client.callTool('start_browser')
is always closed.
- Around line 1-3: Replace the non-strict assert import with the strict variant
in the test file: update the import statement in test/bidi.test.mjs to import
from "node:assert/strict" (the module that other tests use) so existing calls
like assert.strictEqual and assert.ok continue to work and the test suite
remains consistent with files such as test/server.test.mjs; locate the top-level
import of assert in this file and change the module specifier only.
- Fix setupBidi leak: defer state.bidi.set() until all listeners are wired up - Fix close_current_window missing state.bidi.delete() cleanup - Add after hook to Session Isolation test for reliable cleanup - Use node:assert/strict for consistency with other test files - Add bidi.test.mjs and bidi.html to AGENTS.md file tree
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
test/bidi.test.mjs (1)
53-54:JSON.parseon tool result without checkingresult.isErrorfirst.In three places (console log test, page error test, network log test), the result text is parsed directly. If BiDi isn't active or a tool returns an error,
JSON.parsewill throw aSyntaxErrorwith no indication of what actually went wrong.♻️ Suggested pattern for clearer failures
Apply this pattern before each
JSON.parsecall:const result = await client.callTool('get_console_logs', {}); +assert.ok(!result.isError, `Tool returned an error: ${getResponseText(result)}`); const logs = JSON.parse(getResponseText(result));const result = await client.callTool('get_page_errors', {}); const text = getResponseText(result); +assert.ok(!result.isError, `Tool returned an error: ${text}`); const errors = JSON.parse(text);const result = await client.callTool('get_network_logs', {}); +assert.ok(!result.isError, `Tool returned an error: ${getResponseText(result)}`); const logs = JSON.parse(getResponseText(result));Also applies to: 96-99, 126-128
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/bidi.test.mjs` around lines 53 - 54, The test calls client.callTool('get_console_logs', {}) and then immediately JSON.parse(getResponseText(result)) without checking result.isError; update the three spots (the console log test calling get_console_logs, the page error test, and the network log test) to first assert or guard on result.isError (or throw a helpful error using result) before calling getResponseText and JSON.parse so failures produce clear messages; use the result.isError flag and include result (or its error text) in the thrown/assertion so the subsequent JSON.parse only runs on successful results.src/lib/server.js (2)
68-109:logInspectorleaks ifNetwork(driver)throws mid-setup.If
LogInspector(driver)succeeds (line 71) butNetwork(driver)throws (line 89),logInspectoris left with active BiDi subscriptions that are never closed. The leak is bounded bydriver.quit()at session end, but a try/finally is cleaner:♻️ Proposed fix — close `logInspector` on partial setup failure
async function setupBidi(driver, sessionId) { const bidi = newBidiState(); const logInspector = await LogInspector(driver); - await logInspector.onConsoleEntry((entry) => { - try { - bidi.consoleLogs.push({ - level: entry.level, text: entry.text, timestamp: entry.timestamp, - type: entry.type, method: entry.method, args: entry.args - }); - } catch (_) { /* ignore malformed entry */ } - }); - await logInspector.onJavascriptLog((entry) => { - try { - bidi.pageErrors.push({ - level: entry.level, text: entry.text, timestamp: entry.timestamp, - type: entry.type, stackTrace: entry.stackTrace - }); - } catch (_) { /* ignore malformed entry */ } - }); - - const network = await Network(driver); + try { + await logInspector.onConsoleEntry((entry) => { + try { + bidi.consoleLogs.push({ + level: entry.level, text: entry.text, timestamp: entry.timestamp, + type: entry.type, method: entry.method, args: entry.args + }); + } catch (_) { /* ignore malformed entry */ } + }); + await logInspector.onJavascriptLog((entry) => { + try { + bidi.pageErrors.push({ + level: entry.level, text: entry.text, timestamp: entry.timestamp, + type: entry.type, stackTrace: entry.stackTrace + }); + } catch (_) { /* ignore malformed entry */ } + }); + } catch (e) { + try { await logInspector.close?.(); } catch (_) {} + throw e; + } + + const network = await Network(driver);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server.js` around lines 68 - 109, In setupBidi, if LogInspector(driver) succeeds but Network(driver) throws the created logInspector remains subscribed and leaks; ensure you close/unsubscribe the logInspector on partial setup failure by surrounding the Network(...) and its handlers with a try/catch/finally (or try/finally) that calls logInspector.close() (or the appropriate cleanup method) before rethrowing the error, keeping successful path behavior (bidi.available and state.bidi.set(sessionId, bidi)) unchanged and referencing LogInspector, network = await Network(driver), and the onConsoleEntry/onJavascriptLog subscriptions so the inspector is always cleaned up on errors.
231-237: Silentcatch (_) {}onsetupBidifailure hides unexpected errors.The catch is intentional for the "not supported" case, but swallowing all errors prevents diagnosis of unexpected failures (e.g., driver bugs, malformed BiDi handshake). A
console.errorhere is consistent with the project's allowed pattern for debug output:} catch (_) { - // BiDi not supported by this browser/driver — continue without it + console.error(`[BiDi] setup failed for session ${sessionId} — continuing without BiDi:`, _); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server.js` around lines 231 - 237, Replace the silent catch around the setupBidi call so that unexpected failures are logged: change the catch block after await setupBidi(driver, sessionId) to catch the error (e.g., catch (err)) and call console.error with a clear message and context (mention sessionId and driver/Network/LogInspector state as appropriate) so failures in setupBidi are visible for diagnosis while still allowing the flow to continue for the "not supported" case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/server.js`:
- Around line 161-163: The code sets builder.withCapabilities({ 'webSocketUrl':
true, 'unhandledPromptBehavior': 'ignore' }) inside the LogInspector/Network
branch which silently changes alert semantics for all BiDi sessions; either
remove the 'unhandledPromptBehavior' key so the WebDriver default ('dismiss and
notify') is preserved, or explicitly set it to 'dismiss and notify' to keep
behavior consistent across drivers, and if you truly must use 'ignore' keep it
only where required and add a concise inline comment explaining why BiDi
sessions need 'ignore' (reference builder.withCapabilities and the
'unhandledPromptBehavior' capability in the LogInspector/Network branch).
---
Duplicate comments:
In `@src/lib/server.js`:
- Around line 17-18: The dynamic import of 'selenium-webdriver/bidi/network.js'
may not expose a named export Network; update the code that sets Network (the
import call and the assignment used by setupBidi) to robustly resolve the actual
export by checking in order for m.Network, m.default?.Network, and m.default
(the exported factory/object), and handle the case where none exist by logging
an error and skipping BiDi activation; do not replace the access with a bare
.default (that yields the whole exports object) and ensure setupBidi uses the
resolved value and fails fast if it's undefined.
---
Nitpick comments:
In `@src/lib/server.js`:
- Around line 68-109: In setupBidi, if LogInspector(driver) succeeds but
Network(driver) throws the created logInspector remains subscribed and leaks;
ensure you close/unsubscribe the logInspector on partial setup failure by
surrounding the Network(...) and its handlers with a try/catch/finally (or
try/finally) that calls logInspector.close() (or the appropriate cleanup method)
before rethrowing the error, keeping successful path behavior (bidi.available
and state.bidi.set(sessionId, bidi)) unchanged and referencing LogInspector,
network = await Network(driver), and the onConsoleEntry/onJavascriptLog
subscriptions so the inspector is always cleaned up on errors.
- Around line 231-237: Replace the silent catch around the setupBidi call so
that unexpected failures are logged: change the catch block after await
setupBidi(driver, sessionId) to catch the error (e.g., catch (err)) and call
console.error with a clear message and context (mention sessionId and
driver/Network/LogInspector state as appropriate) so failures in setupBidi are
visible for diagnosis while still allowing the flow to continue for the "not
supported" case.
In `@test/bidi.test.mjs`:
- Around line 53-54: The test calls client.callTool('get_console_logs', {}) and
then immediately JSON.parse(getResponseText(result)) without checking
result.isError; update the three spots (the console log test calling
get_console_logs, the page error test, and the network log test) to first assert
or guard on result.isError (or throw a helpful error using result) before
calling getResponseText and JSON.parse so failures produce clear messages; use
the result.isError flag and include result (or its error text) in the
thrown/assertion so the subsequent JSON.parse only runs on successful results.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/server.js (1)
580-597:⚠️ Potential issue | 🟡 MinorBidi state (and driver state) not cleaned up when
driver.quit()throws.If
driver.quit()rejects, the catch block returns an error butstate.bidi.delete(sessionId)(line 586) is never reached — leaving an orphaned bidi entry. The same pre-existing issue affectsstate.driversandstate.currentSession. Atry/finallyblock fixes all three consistently:🛡️ Proposed fix
async () => { try { const driver = getDriver(); const sessionId = state.currentSession; - await driver.quit(); - state.drivers.delete(sessionId); - state.bidi.delete(sessionId); - state.currentSession = null; - return { - content: [{ type: 'text', text: `Browser session ${sessionId} closed` }] - }; + try { + await driver.quit(); + } finally { + state.drivers.delete(sessionId); + state.bidi.delete(sessionId); + state.currentSession = null; + } + return { + content: [{ type: 'text', text: `Browser session ${sessionId} closed` }] + }; } catch (e) { return { content: [{ type: 'text', text: `Error closing session: ${e.message}` }], isError: true }; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server.js` around lines 580 - 597, The current async shutdown handler calls driver.quit() and, if that throws, never cleans up state.drivers, state.bidi or state.currentSession; wrap the driver.quit() call in a try/finally so that regardless of errors you always run the cleanup: retrieve sessionId and driver via getDriver(), attempt await driver.quit() in the try, and in the finally block call state.drivers.delete(sessionId), state.bidi.delete(sessionId) and set state.currentSession = null, then propagate or return the error/result appropriately so the catch only formats the response.
🧹 Nitpick comments (1)
src/lib/server.js (1)
68-109:logInspectorandnetworkinstances are not retained — explicit teardown is impossible.Both objects are local to
setupBidiand are discarded once the function returns. In practice,driver.quit()closes the BiDi WebSocket and stops event delivery, so this is benign during normal session termination. However, ifdriver.quit()throws (error path inclose_session) the BiDi listeners remain live, continue pushing into the orphanedbidiobject, and there is no handle to unsubscribe them. Storing the instances in thebidistate object enables explicit teardown:♻️ Proposed refactor
const newBidiState = () => ({ available: false, consoleLogs: [], pageErrors: [], - networkLogs: [] + networkLogs: [], + _logInspector: null, + _network: null }); async function setupBidi(driver, sessionId) { const bidi = newBidiState(); const logInspector = await LogInspector(driver); // ...listeners... const network = await Network(driver); // ...listeners... bidi.available = true; + bidi._logInspector = logInspector; + bidi._network = network; state.bidi.set(sessionId, bidi); }Then in
close_session/close_current_window(andcleanup), beforestate.bidi.delete:+ const bidiEntry = state.bidi.get(sessionId); + if (bidiEntry?._logInspector?.close) { + try { await bidiEntry._logInspector.close(); } catch (_) {} + } state.bidi.delete(sessionId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server.js` around lines 68 - 109, setupBidi creates LogInspector and Network instances locally so their event listeners can’t be unsubscribed later; modify setupBidi (and newBidiState if needed) to attach the created logInspector and network objects to the returned bidi state (e.g., bidi.logInspector and bidi.network) and keep existing bidi arrays, then update session teardown paths (close_session, close_current_window, cleanup) to look up the bidi state from state.bidi and call the appropriate teardown/unsubscribe/close methods on bidi.logInspector and bidi.network (or remove their listeners) before deleting state.bidi to ensure listeners are removed and resources released even if driver.quit() throws.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/lib/server.js`:
- Around line 580-597: The current async shutdown handler calls driver.quit()
and, if that throws, never cleans up state.drivers, state.bidi or
state.currentSession; wrap the driver.quit() call in a try/finally so that
regardless of errors you always run the cleanup: retrieve sessionId and driver
via getDriver(), attempt await driver.quit() in the try, and in the finally
block call state.drivers.delete(sessionId), state.bidi.delete(sessionId) and set
state.currentSession = null, then propagate or return the error/result
appropriately so the catch only formats the response.
---
Duplicate comments:
In `@src/lib/server.js`:
- Around line 161-165: The use of unhandledPromptBehavior:'ignore' should not be
set here; update the builder.withCapabilities call in the LogInspector/Network
branch (the code that calls builder.withCapabilities) to remove the
'unhandledPromptBehavior' entry and leave only 'webSocketUrl': true (or
conditionally set unhandledPromptBehavior in a single, well-documented location
if truly required), and keep the inline comment about preventing BiDi
auto-dismiss behavior only once near the place that actually relies on that
behavior.
---
Nitpick comments:
In `@src/lib/server.js`:
- Around line 68-109: setupBidi creates LogInspector and Network instances
locally so their event listeners can’t be unsubscribed later; modify setupBidi
(and newBidiState if needed) to attach the created logInspector and network
objects to the returned bidi state (e.g., bidi.logInspector and bidi.network)
and keep existing bidi arrays, then update session teardown paths
(close_session, close_current_window, cleanup) to look up the bidi state from
state.bidi and call the appropriate teardown/unsubscribe/close methods on
bidi.logInspector and bidi.network (or remove their listeners) before deleting
state.bidi to ensure listeners are removed and resources released even if
driver.quit() throws.
Prevents confusing SyntaxError if a tool returns an error message instead of JSON. Now fails with a clear assertion showing the actual error text.
If driver.quit() throws, state.drivers, state.bidi, and state.currentSession are now still cleaned up via the finally block. Outer try/catch preserved for MCP error response pattern.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/bidi.test.mjs (1)
155-156: MissingisErrorguard produces misleading failure messagesEvery other test in this file guards with
assert.ok(!result.isError, getResponseText(result))before using the response text. The Session Isolation test skips this check on both reads, so if the tool returns an error (e.g., BiDi unavailable for the second session) the assertion message will be something like "Expected 'session-1-log' – got 'BiDi is not enabled for this session'" without clearly identifying the root cause.🛡️ Proposed fix
const firstLogs = await client.callTool('get_console_logs', {}); +assert.ok(!firstLogs.isError, `get_console_logs error: ${getResponseText(firstLogs)}`); assert.ok(getResponseText(firstLogs).includes('session-1-log')); ... const newLogs = await client.callTool('get_console_logs', {}); +assert.ok(!newLogs.isError, `get_console_logs error: ${getResponseText(newLogs)}`); assert.strictEqual(getResponseText(newLogs), 'No console logs captured');Also applies to: 164-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/bidi.test.mjs` around lines 155 - 156, Add the missing isError guards before asserting response contents in the Session Isolation test: after the first call to client.callTool('get_console_logs', {}) (variable firstLogs) add assert.ok(!firstLogs.isError, getResponseText(firstLogs)) before checking for 'session-1-log', and do the same for the second call (variable secondLogs) before checking for 'session-2-log' so any tool error is reported directly via getResponseText(result).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/bidi.test.mjs`:
- Line 51: Replace the hardcoded await new Promise(r => setTimeout(...)) delays
in test/bidi.test.mjs with a poll-until helper that repeatedly checks for the
expected BiDi event/condition and only proceeds when it succeeds (or throws
after a max timeout); add a small poll interval (e.g., 50–100ms) and a clear
timeout (e.g., 5s) and use that helper in place of the five fixed waits so
assertions retry until events arrive; implement the helper (e.g., pollUntil or
waitForCondition) and call it from the test code surrounding the places where
await new Promise(r => setTimeout(r, 500)) is used so tests become robust in CI.
- Around line 123-126: Replace the flaky localhost:1 fetch with a deterministic
non-routable/test address or a controlled local server shutdown: locate the
client.callTool('execute_script', { script:
'fetch("http://localhost:1/nonexistent")...' }) call in test/bidi.test.mjs and
change the script to use a RFC5737 TEST-NET address such as
"http://192.0.2.1/nonexistent" or, alternatively, start a local HTTP server and
explicitly stop it before calling execute_script so the fetch fails immediately;
ensure the rest of the test (the subsequent setTimeout/assertion) remains
unchanged.
---
Nitpick comments:
In `@test/bidi.test.mjs`:
- Around line 155-156: Add the missing isError guards before asserting response
contents in the Session Isolation test: after the first call to
client.callTool('get_console_logs', {}) (variable firstLogs) add
assert.ok(!firstLogs.isError, getResponseText(firstLogs)) before checking for
'session-1-log', and do the same for the second call (variable secondLogs)
before checking for 'session-2-log' so any tool error is reported directly via
getResponseText(result).
| await client.callTool('click_element', { by: 'id', value: 'log-info' }); | ||
| await client.callTool('click_element', { by: 'id', value: 'log-warn' }); | ||
| await client.callTool('click_element', { by: 'id', value: 'log-error' }); | ||
| await new Promise(r => setTimeout(r, 500)); |
There was a problem hiding this comment.
Hardcoded setTimeout waits make the suite flaky in CI
Five independent fixed-delay waits (500 ms / 1000 ms) are used as the sole mechanism to let BiDi events propagate before asserting. On a loaded CI runner these windows are frequently insufficient, causing spurious failures; on fast hardware they waste wall-clock time. There is no retry path when the events haven't arrived yet.
Consider replacing each wait with a poll-until helper, for example:
♻️ Suggested polling helper
+// At the top of the file or in mcp-client.mjs
+async function pollUntil(fn, { intervalMs = 100, timeoutMs = 5000 } = {}) {
+ const deadline = Date.now() + timeoutMs;
+ while (Date.now() < deadline) {
+ const result = await fn();
+ if (result) return result;
+ await new Promise(r => setTimeout(r, intervalMs));
+ }
+ throw new Error('pollUntil: timed out');
+}
// Example: replace the 500 ms wait + read pattern in Console Log Capture
-await new Promise(r => setTimeout(r, 500));
-const result = await client.callTool('get_console_logs', {});
-const logs = JSON.parse(getResponseText(result));
+const logs = await pollUntil(async () => {
+ const r = await client.callTool('get_console_logs', {});
+ if (r.isError) return null;
+ const parsed = JSON.parse(getResponseText(r));
+ return parsed.length > 0 ? parsed : null;
+});
Also applies to: 68-68, 96-96, 126-126, 153-153
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/bidi.test.mjs` at line 51, Replace the hardcoded await new Promise(r =>
setTimeout(...)) delays in test/bidi.test.mjs with a poll-until helper that
repeatedly checks for the expected BiDi event/condition and only proceeds when
it succeeds (or throws after a max timeout); add a small poll interval (e.g.,
50–100ms) and a clear timeout (e.g., 5s) and use that helper in place of the
five fixed waits so assertions retry until events arrive; implement the helper
(e.g., pollUntil or waitForCondition) and call it from the test code surrounding
the places where await new Promise(r => setTimeout(r, 500)) is used so tests
become robust in CI.
Comment on lines
+123
to
+126
| await client.callTool('execute_script', { | ||
| script: 'fetch("http://localhost:1/nonexistent").catch(() => {});' | ||
| }); | ||
| await new Promise(r => setTimeout(r, 1000)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's view the test file around lines 123-126 and 136-137
cat -n test/bidi.test.mjs | sed -n '110,150p'
Repository: angiejones/mcp-selenium
Length of output: 2117
🏁 Script executed:
# Check the server.js implementation for setupBidi
cat -n src/lib/server.js | head -200
Repository: angiejones/mcp-selenium
Length of output: 8876
🏁 Script executed:
# Search for Network event types in the codebase
rg "Network\." --type js --type mjs -A 2 -B 2 | head -100
Repository: angiejones/mcp-selenium
Length of output: 94
🏁 Script executed:
# Look for where event types are defined or used in the context of Network.fetch or Network.error
rg "type.*error" test/ --type mjs | head -20
Repository: angiejones/mcp-selenium
Length of output: 94
🏁 Script executed:
# Search for other fetch/network related tests
rg "fetch\|Network\|network" test/ --max-count 20
Repository: angiejones/mcp-selenium
Length of output: 49
🏁 Script executed:
# Check if there are other test patterns for handling network errors
rg "\.catch\|error\|fail" test/bidi.test.mjs -A 2 -B 2
Repository: angiejones/mcp-selenium
Length of output: 49
🏁 Script executed:
# Look for fixture files to understand what's available
ls -la test/fixtures/
Repository: angiejones/mcp-selenium
Length of output: 918
🏁 Script executed:
# Check if there's a test server or server setup that could be leveraged
rg "localhost\|127.0.0.1\|server" test/ --type mjs | head -30
Repository: angiejones/mcp-selenium
Length of output: 94
🏁 Script executed:
# Use cat to view the entire bidi.test.mjs test file
cat test/bidi.test.mjs | grep -n "fetch\|Network\|error" -A 3 -B 3
Repository: angiejones/mcp-selenium
Length of output: 3712
🏁 Script executed:
# Check the bidi.html fixture to understand the test setup
cat test/fixtures/bidi.html
Repository: angiejones/mcp-selenium
Length of output: 510
🏁 Script executed:
# Search for any test infrastructure or server setup in the entire test directory
cat test/mcp-client.mjs | head -80
Repository: angiejones/mcp-selenium
Length of output: 2311
🏁 Script executed:
# Look for package.json to understand test runner and dependencies
cat package.json | grep -A 20 "test\|selenium\|mocha"
Repository: angiejones/mcp-selenium
Length of output: 491
Port 1 may cause test flakiness in some CI environments
The fetch("http://localhost:1/nonexistent") approach can be unreliable in hardened CI systems with outbound firewall rules. Connection attempts to port 1 (TCPMUX) may stall rather than fail immediately, causing the network error event to arrive after the 1000 ms timeout, making the assertion at line 137 flaky. Consider using a non-routable address like http://192.0.2.1/ (RFC 5737 TEST-NET-1) or a local HTTP server that is deliberately shut down for more deterministic failure.
Also applies to: 136-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/bidi.test.mjs` around lines 123 - 126, Replace the flaky localhost:1
fetch with a deterministic non-routable/test address or a controlled local
server shutdown: locate the client.callTool('execute_script', { script:
'fetch("http://localhost:1/nonexistent")...' }) call in test/bidi.test.mjs and
change the script to use a RFC5737 TEST-NET address such as
"http://192.0.2.1/nonexistent" or, alternatively, start a local HTTP server and
explicitly stop it before calling execute_script so the fetch fails immediately;
ensure the rest of the test (the subsequent setTimeout/assertion) remains
unchanged.
This was referenced Feb 22, 2026
Merged
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
1 participant
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.
Summary
Automatically enables WebDriver BiDi when starting a browser session to passively capture console logs, JavaScript errors, and network activity in the background. Falls back silently if the browser/driver doesn't support it.
Closes #55
New Tools
get_console_logsget_page_errorsget_network_logsAll three accept an optional
clearparameter to reset the buffer after reading. All three are registered via a sharedregisterBidiTool()factory.Design Decisions
No opt-in flag. BiDi is an implementation detail, not a user-facing option. The user says "test this page" and the agent decides what tools to call. Nobody is in a position to set
bidi: true— the user doesn't know what it is, and the agent would either always want it or never think to ask. So the server just tries to enable it and moves on if it can't.This directly addresses the concern from PR #43 — that PR forced BiDi on and broke sessions with older browsers. This implementation:
webSocketUrl: truecapabilitycatchblock just continues without BiDi)"Console log capture is not available (BiDi not supported by this browser/driver)") when BiDi isn't activeHow It Works
start_browsersetswebSocketUrl: truein capabilities, then callssetupBidi()which creates BiDiLogInspectorandNetworklistenersbidistate object ({ available, consoleLogs, pageErrors, networkLogs })try/catch— a malformed BiDi event never crashes the serverclose_sessionand the cleanup handler clear BiDi state with a singlestate.bidi.delete(sessionId)callImplementation
state.bidi(one Map instead of six)setupBidi(driver, sessionId)— extracted helper that wires up all BiDi listeners, keepsstart_browserfocused on browser setupregisterBidiTool()— factory function that registers all three diagnostic tools with identical structure, eliminating copy-pasteTests
test/bidi.test.mjs:server.test.mjs(34-tool check)Summary by CodeRabbit
New Features
Documentation
Tests