Merged
Conversation
- Remove find_element, scroll_to_element, clear_element, drag_and_drop - Consolidate click/doubleclick/rightclick/hover → interact - Consolidate window management → window (list/switch/switch_latest/close) - Consolidate frame management → frame (switch/default) - Consolidate alert handling → alert (accept/dismiss/get_text/send_text) - Consolidate BiDi diagnostics → diagnostics (console/errors/network) - Add execute_script drag-and-drop test to prove the escape hatch works - Fix flaky window tests (single McpClient per test file) - Add default cases to all action switch statements - Rewrite README with collapsible sections - Trim AGENTS.md to essentials - Add GitHub Actions publish workflow - Bump version to 0.2.0 Closes #60
|
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. 📝 WalkthroughWalkthroughConsolidates many specialized MCP tools into a smaller set of generalized, action-based tools (interact, frame, window, alert, diagnostics), updates docs, bumps package version to 0.2.0, adds an npm publish workflow, and adapts tests and fixtures to the new tool surface. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (MCP client)
participant Server as MCP Selenium Server
participant Browser as Browser (BiDi)
Client->>Server: tool call "interact" {action: "click", selector}
Server->>Browser: BiDi command (resolve element, perform click)
Browser-->>Server: BiDi event/result (success or error)
Server-->>Client: tool response (OK / standardized error)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
test/fixtures/drag-drop.html (1)
31-33: Consider also cancellingdragenterfor broader browser compatibility.Some browsers (notably older Firefox versions) require both
dragenteranddragoverto calle.preventDefault()for a drop target to accept drops. Since CI runs tests across multiple browsers, adding thedragenterhandler is low-risk and defensive.♻️ Proposed addition of
dragenterhandler+ droppable.addEventListener('dragenter', (e) => { + e.preventDefault(); + }); + droppable.addEventListener('dragover', (e) => { e.preventDefault(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/fixtures/drag-drop.html` around lines 31 - 33, The drop target currently calls e.preventDefault() only in the 'dragover' listener; add an analogous 'dragenter' listener on the same element (droppable.addEventListener('dragenter', ...)) that calls e.preventDefault() to ensure older browsers accept drops. Locate the existing droppable.addEventListener('dragover', ...) and add a matching droppable.addEventListener('dragenter', (e) => { e.preventDefault(); }) handler to the same scope so tests run consistently across browsers..github/workflows/publish.yml (1)
7-27: Consider running tests before publishing.The workflow installs dependencies but does not run
npm testbeforenpm publish. A failing test suite would still result in a broken release being published to npm.Suggested addition
- name: Install dependencies run: npm ci + - name: Run tests + run: npm test + - name: Publish to npm run: npm publish --access public🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/publish.yml around lines 7 - 27, The publish job currently installs dependencies then runs npm publish without verifying tests; add a step in the publish job (between the "Install dependencies" step and the "Publish to npm" step) that runs the test suite (e.g., run: npm test) so the workflow fails on test failures before publishing; ensure the new step is named clearly (e.g., "Run tests") and uses the same job context so NODE_AUTH_TOKEN and installed deps are available.README.md (1)
16-19: Add a language identifier to the fenced code block.Static analysis (markdownlint MD040) flags this code block as missing a language specifier. Since this is a plain URL,
textis appropriate.Suggested fix
Paste into your browser address bar: -``` +```text goose://extension?cmd=npx&arg=-y&arg=%40angiejones%2Fmcp-selenium&id=selenium-mcp&name=Selenium%20MCP&description=automates%20browser%20interactions</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@README.mdaround lines 16 - 19, Update the fenced code block that contains
the goose://extension URL so it includes a language identifier; change the
opening fence fromtotext to satisfy markdownlint MD040 and make it
explicit that the block is plain text (the block containing
"goose://extension?cmd=npx&arg=-y&arg=%40angiejones%2Fmcp-selenium…").</details> </blockquote></details> <details> <summary>AGENTS.md (1)</summary><blockquote> `7-13`: **Add a language identifier to the file-map code block.** Static analysis (markdownlint MD040) flags this block. `text` is appropriate here. <details> <summary>Suggested fix</summary> ````diff -``` +```text src/lib/server.js ← ALL server logic: tool definitions, state, helpers, cleanup src/index.js ← Thin CLI wrapper, spawns server.js as child process test/mcp-client.mjs ← Reusable MCP test client (JSON-RPC over stdio) test/*.test.mjs ← Tests grouped by feature test/fixtures/*.html ← HTML files loaded via file:// URLs in tests ```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` around lines 7 - 13, Update the code block in AGENTS.md that lists the file map (the block containing "src/lib/server.js ← ALL server logic: tool definitions, state, helpers, cleanup" through "test/fixtures/*.html ← HTML files loaded via file:// URLs in tests") by adding the language identifier "text" after the opening backticks (i.e., change ``` to ```text) so markdownlint MD040 is satisfied; keep the block content unchanged aside from the language tag.test/new-tools.test.mjs (2)
55-58:assert.ok(length > 0)only confirms a non-empty response; pin it to the fixture's known title.The guideline requires asserting the expected DOM/system effect. Since
interactions.htmlhas a predictable<title>, asserting the exact value makes the test self-documenting and closes the gap between "something returned" and "the right thing returned."♻️ Proposed fix
- const result = await client.callTool('execute_script', { script: 'return document.title;' }); - assert.ok(getResponseText(result).length > 0); + const result = await client.callTool('execute_script', { script: 'return document.title;' }); + assert.equal(getResponseText(result), 'Interactions'); // match the actual <title> in interactions.html🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/new-tools.test.mjs` around lines 55 - 58, The test currently only checks that getResponseText(result).length > 0; change it to assert the exact expected title from the interactions.html fixture so the test verifies the correct DOM effect. Locate the test using client.callTool('execute_script', { script: 'return document.title;' }) in new-tools.test.mjs and replace the loose length assertion on getResponseText(result) with a strict equality assertion against the known title string (the expected <title> value in interactions.html).
166-179: "window close" hard-asserts 2 windows exist, creating implicit dependency on the prior sibling test.Line 169 (
assert.equal(data.all.length, 2)) passes only when "window switch_latest after opening new tab" has already run and left the new tab open. If that test fails mid-execution (e.g.,switch_latestthrows), the close test will fail at the assertion with a confusing2 !== 1message rather than revealing the real cause.Consider either adding the tab-open setup to this test's body or the
beforehook:♻️ Proposed fix
it('window close closes tab and switches back', async () => { + // Ensure a second window/tab is open before testing close + await client.callTool('execute_script', { script: "window.open('about:blank', '_blank');" }); let result = await client.callTool('window', { action: 'list' }); const data = JSON.parse(getResponseText(result)); assert.equal(data.all.length, 2);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/new-tools.test.mjs` around lines 166 - 179, The test "window close closes tab and switches back" currently assumes two windows exist by asserting data.all.length === 2 (variable data from client.callTool('window', { action: 'list' })), creating an implicit dependency on another test; fix it by making the test self-contained: before asserting, open a new tab (e.g., via client.callTool('window', { action: 'open', ... }) or add a before hook that ensures there are two windows), then re-run the list to get a deterministic data.all array, proceed to switch to data.all[1], close it, and finally assert the count decreased to 1; ensure any created tab is cleaned up so this test doesn't depend on sibling tests or leave state behind.
🤖 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 14-17: The hardcoded version passed to new McpServer({ name: "MCP
Selenium", version: "1.0.0" }) is out of sync with package.json; update the code
to read the real package version (e.g., require or import the package.json and
use its version field) and pass that value to the McpServer constructor (keep
the McpServer call and server variable intact, just replace the string "1.0.0"
with the package.json-derived version).
- Around line 551-564: The call to driver.getAllWindowHandles() inside the
'close' case can throw after the last window is closed, preventing session
cleanup; modify the 'close' handling so you treat a failed getAllWindowHandles()
as an empty handles list (e.g., wrap getAllWindowHandles() in its own try/catch
and set handles = [] on error) and ensure the cleanup code that calls
driver.quit(), state.drivers.delete(sessionId), state.bidi.delete(sessionId),
and state.currentSession = null always runs when no handles remain; keep the
existing branch that switches to handles[0] when handles.length > 0 and only
perform the quit/delete cleanup when handles is empty or getting handles failed.
In `@test/new-tools.test.mjs`:
- Line 26: The test calls client.callTool('close_session') without the arguments
object; make it consistent with other tests by calling
client.callTool('close_session', {}) instead. Locate the invocation of
client.callTool('close_session') in the test/new-tools.test.mjs file and change
it to pass an empty object as the second parameter so the call matches other
tests (e.g., bidi.test.mjs) and avoids relying on JSON.stringify dropping
undefined.
- Around line 242-246: The "alert accept" test only asserts the server response
text — update the test that calls client.callTool('alert', { action: 'accept' })
to also verify the DOM effect by selecting the page element that indicates alert
acceptance (e.g., the element with id 'alert-result') and asserting its text
equals the expected value ("Alert accepted"); if the alerts fixture doesn't
currently update the DOM, add a minimal DOM indicator (an element like <div
id="alert-result">) and update the page script to set its text on alert
acceptance so the test can assert the UI change instead of relying solely on
getResponseText(result).
---
Nitpick comments:
In @.github/workflows/publish.yml:
- Around line 7-27: The publish job currently installs dependencies then runs
npm publish without verifying tests; add a step in the publish job (between the
"Install dependencies" step and the "Publish to npm" step) that runs the test
suite (e.g., run: npm test) so the workflow fails on test failures before
publishing; ensure the new step is named clearly (e.g., "Run tests") and uses
the same job context so NODE_AUTH_TOKEN and installed deps are available.
In `@AGENTS.md`:
- Around line 7-13: Update the code block in AGENTS.md that lists the file map
(the block containing "src/lib/server.js ← ALL server logic: tool definitions,
state, helpers, cleanup" through "test/fixtures/*.html ← HTML files loaded via
file:// URLs in tests") by adding the language identifier "text" after the
opening backticks (i.e., change ``` to ```text) so markdownlint MD040 is
satisfied; keep the block content unchanged aside from the language tag.
In `@README.md`:
- Around line 16-19: Update the fenced code block that contains the
goose://extension URL so it includes a language identifier; change the opening
fence from ``` to ```text to satisfy markdownlint MD040 and make it explicit
that the block is plain text (the block containing
"goose://extension?cmd=npx&arg=-y&arg=%40angiejones%2Fmcp-selenium…").
In `@test/fixtures/drag-drop.html`:
- Around line 31-33: The drop target currently calls e.preventDefault() only in
the 'dragover' listener; add an analogous 'dragenter' listener on the same
element (droppable.addEventListener('dragenter', ...)) that calls
e.preventDefault() to ensure older browsers accept drops. Locate the existing
droppable.addEventListener('dragover', ...) and add a matching
droppable.addEventListener('dragenter', (e) => { e.preventDefault(); }) handler
to the same scope so tests run consistently across browsers.
In `@test/new-tools.test.mjs`:
- Around line 55-58: The test currently only checks that
getResponseText(result).length > 0; change it to assert the exact expected title
from the interactions.html fixture so the test verifies the correct DOM effect.
Locate the test using client.callTool('execute_script', { script: 'return
document.title;' }) in new-tools.test.mjs and replace the loose length assertion
on getResponseText(result) with a strict equality assertion against the known
title string (the expected <title> value in interactions.html).
- Around line 166-179: The test "window close closes tab and switches back"
currently assumes two windows exist by asserting data.all.length === 2 (variable
data from client.callTool('window', { action: 'list' })), creating an implicit
dependency on another test; fix it by making the test self-contained: before
asserting, open a new tab (e.g., via client.callTool('window', { action: 'open',
... }) or add a before hook that ensures there are two windows), then re-run the
list to get a deterministic data.all array, proceed to switch to data.all[1],
close it, and finally assert the count decreased to 1; ensure any created tab is
cleaned up so this test doesn't depend on sibling tests or leave state behind.
- Read version from package.json instead of hardcoding 1.0.0
- Guard getAllWindowHandles() after last window close (session may be gone)
- Pass {} to close_session in test teardown for consistency
- Add alert-result DOM indicator to alerts fixture
- Verify DOM effect in alert accept test
- Remove stale comment about find_element history
- Rename new-tools.test.mjs → tools.test.mjs
angiejones
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Around line 7-13: The fenced code block containing the file map (lines
starting with "src/lib/server.js", "src/index.js", "test/mcp-client.mjs", etc.)
is missing a language identifier; update that opening ``` to ```text (or
```plaintext) so the block is recognized as plain text and the MD040 lint
warning is resolved—edit the block that lists src/lib/server.js, src/index.js,
test/mcp-client.mjs, test/*.test.mjs, and test/fixtures/*.html to include the
language specifier.
In `@test/tools.test.mjs`:
- Line 13: Top-level describe label is stale: rename the describe call that
currently reads describe('new-tools', () => { to describe('tools', () => { so
the test suite name matches the file tools.test.mjs and test-runner filtering;
update the string literal in the describe invocation (the describe('new-tools',
...) occurrence) to 'tools'.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
AGENTS.md (2)
41-58: MentionregisterBidiTool()for diagnostic tools.The "Adding a Tool" section only shows the generic
server.tool()pattern. Agents adding a BiDi/diagnostic tool would copy that pattern instead of using theregisterBidiTool()factory, duplicating handler logic.📝 Suggested addition after line 57
});
+For diagnostic (BiDi) tools — those that read
consoleLogs,pageErrors, ornetworkLogs— use theregisterBidiTool()factory instead ofserver.tool()directly. Do not copy-paste the BiDi handler boilerplate.</details> Based on learnings: "Use the `registerBidiTool()` factory function to register diagnostic tools (`get_console_logs`, `get_page_errors`, `get_network_logs`); do not copy-paste diagnostic tool handlers" <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@AGENTS.mdaround lines 41 - 58, The docs currently only show server.tool()
for adding tools which leads authors to copy BiDi/diagnostic handlers; update
the "Adding a Tool" section to mention and instruct using registerBidiTool() for
diagnostic tools (e.g., get_console_logs, get_page_errors, get_network_logs)
instead of server.tool(), and add a short note after the existing pattern
directing readers to use registerBidiTool() to avoid duplicating BiDi handler
boilerplate and ensure proper registration of consoleLogs/pageErrors/networkLogs
collectors.</details> --- `62-82`: **Document the `fixture()` helper in the Testing section.** The Testing section lists fixture files and test modules but never tells an agent how to reference fixture HTML files. The `fixture()` helper from `mcp-client.mjs` is the designated way to resolve `test/fixtures/*.html` paths; omitting it risks agents hand-rolling path resolution and breaking portability. <details> <summary>📝 Suggested addition after line 72</summary> ```diff **Verify outcomes, not absence of errors.** If you click a button, check that the thing it did actually happened. If a test is failing, fix the code — never weaken the assertion. + +Use the `fixture(filename)` helper from `mcp-client.mjs` to resolve `file://` URLs for HTML fixtures: +```js +import { McpClient, fixture } from './mcp-client.mjs'; +const url = fixture('form.html'); // → file:///abs/path/to/test/fixtures/form.html +```Based on learnings: "Use the
fixture()helper frommcp-client.mjsto resolve paths to HTML test fixture files intest/fixtures/"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` around lines 62 - 82, Add documentation describing the fixture() helper exported from mcp-client.mjs so agents know to use it to resolve test fixture HTML paths; specifically state that tests should import fixture (and optionally McpClient) from './mcp-client.mjs' and call fixture('form.html') to obtain a file:// absolute URL for files under test/fixtures/, and include the short usage example showing the import and fixture(...) call to resolve a fixture path.test/tools.test.mjs (1)
138-179: Window tests carry implicit sequential state dependencies.Tests 2 ("switch_latest"), 3 ("switch switches back"), and 4 ("close closes tab") form a chain: test 2 opens a second window and stays on it; test 3 relies on two windows still being open; test 4 asserts
data.all.length === 2and targetsdata.all[1]. A mid-test failure in test 2 or 3 cascades into a misleading failure in subsequent tests.Consider opening a fresh
about:blanktab in thebeforeof test 3/4, or closing the extra window in test 2's cleanup, so each test is self-contained.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tools.test.mjs` around lines 138 - 179, The window tests depend on shared browser state; make each test self-contained by ensuring any opened tab is cleaned up or a fresh tab is created for the test: for the tests using client.callTool('window', {...}) (notably the "window switch_latest", "window switch switches back", and "window close closes tab" it blocks) either (A) open a new about:blank tab via client.callTool('execute_script', { script: "window.open('about:blank','_blank')" }) in a before hook and use that handle for the test, or (B) ensure the test that opens a second window explicitly closes it at the end (callTool('window', { action: 'close' })) so subsequent tests start with a single window; update the tests that reference data.all[1], switch, and close to use the created handle or rely on the cleaned state.
🤖 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/tools.test.mjs`:
- Around line 55-58: The test "executes script and returns a string result"
currently only checks non-empty output; update it to assert the exact page title
by replacing the assert.ok(...) with assert.equal(getResponseText(result),
'<expected title>'); locate the test that calls
client.callTool('execute_script', { script: 'return document.title;' }) and use
getResponseText(result) with assert.equal to compare against the known title
string from interactions.html so the test verifies the exact expected value.
---
Nitpick comments:
In `@AGENTS.md`:
- Around line 41-58: The docs currently only show server.tool() for adding tools
which leads authors to copy BiDi/diagnostic handlers; update the "Adding a Tool"
section to mention and instruct using registerBidiTool() for diagnostic tools
(e.g., get_console_logs, get_page_errors, get_network_logs) instead of
server.tool(), and add a short note after the existing pattern directing readers
to use registerBidiTool() to avoid duplicating BiDi handler boilerplate and
ensure proper registration of consoleLogs/pageErrors/networkLogs collectors.
- Around line 62-82: Add documentation describing the fixture() helper exported
from mcp-client.mjs so agents know to use it to resolve test fixture HTML paths;
specifically state that tests should import fixture (and optionally McpClient)
from './mcp-client.mjs' and call fixture('form.html') to obtain a file://
absolute URL for files under test/fixtures/, and include the short usage example
showing the import and fixture(...) call to resolve a fixture path.
In `@test/tools.test.mjs`:
- Around line 138-179: The window tests depend on shared browser state; make
each test self-contained by ensuring any opened tab is cleaned up or a fresh tab
is created for the test: for the tests using client.callTool('window', {...})
(notably the "window switch_latest", "window switch switches back", and "window
close closes tab" it blocks) either (A) open a new about:blank tab via
client.callTool('execute_script', { script:
"window.open('about:blank','_blank')" }) in a before hook and use that handle
for the test, or (B) ensure the test that opens a second window explicitly
closes it at the end (callTool('window', { action: 'close' })) so subsequent
tests start with a single window; update the tests that reference data.all[1],
switch, and close to use the created handle or rely on the cleaned state.
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.
What
Consolidate the MCP tool surface from 32 tools down to 18, reducing context window cost by ~40%.
Removed (4 tools)
find_element— returned only "Element found", no useful datascroll_to_element— thin wrapper aroundexecute_scriptclear_element—send_keysalready clears firstdrag_and_drop—execute_scripthandles itConsolidated (14 tools → 5)
interactclick_element,double_click,right_click,hoverwindowswitch_to_window,get_window_handles,switch_to_latest_window,close_current_windowframeswitch_to_frame,switch_to_default_contentalertaccept_alert,dismiss_alert,get_alert_text,send_alert_textdiagnosticsget_console_logs,get_page_errors,get_network_logsAlso
execute_scriptdrag-and-drop test to prove the escape hatchToken cost
Measured with
cl100k_basetokenizer: ~2,100 tokens for all 18 tool definitions.Closes #60
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests