Merged
Conversation
📝 WalkthroughWalkthroughAdds an accessibility snapshot resource and in-page DOM walker, refactors server tool/resource registration to schema-driven APIs, exposes resource listing/reading in test client, updates tests for resources, bumps package/dependency, narrows a .gitignore rule, and duplicates a README Resources block. Changes
Sequence DiagramsequenceDiagram
actor Client
participant Server
participant Browser
participant DOM
Client->>Server: resources/read("accessibility://current")
activate Server
Server->>Browser: executeScript(accessibilitySnapshotScript)
activate Browser
Browser->>DOM: walk(document.body)
activate DOM
DOM->>DOM: traverse nodes and build tree
DOM-->>Browser: accessibility tree (JSON)
deactivate DOM
Browser-->>Server: script result
deactivate Browser
Server-->>Client: JSON payload or McpError
deactivate Server
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 |
![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 (2)
src/lib/server.js (1)
877-882: Consider reusinggetDriver()to reduce duplication.Lines 879-880 duplicate the session-check logic from
getDriver()(lines 45-51). You could callgetDriver()and let itsErrorbe caught and wrapped by the catch block at line 883-885, which already translates non-McpErrorexceptions intoMcpError(ErrorCode.InternalError, ...). The only loss is the custom-32002code, which could be addressed by makinggetDriver()throw anMcpErrordirectly — though that would couple the helper to MCP types.This is a minor duplication with a reasonable justification if you want the custom error code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server.js` around lines 877 - 882, Replace the duplicated session-check/driver retrieval in the accessibility snapshot handler by reusing the existing getDriver() helper: instead of directly accessing state.drivers.get(state.currentSession) and throwing a custom McpError, call await getDriver() to obtain the driver and then use it to run accessibilitySnapshotScript; this removes the duplicated logic and lets the existing catch block convert any thrown errors (including missing-session errors) into McpError as currently handled. Ensure you remove the manual state.drivers/state.currentSession access and the local throw so only getDriver() is responsible for validating the session.test/resources.test.mjs (1)
63-74: SecondMcpClientinstance violates the one-client-per-file guideline.The comment explains the motivation, but the coding guideline explicitly states "use one McpClient per test file rather than spawning multiple clients per file." An alternative that avoids the extra client: tear down the browser session within this test (
close_session→ assert error →start_browser+ navigate to restore), eliminating the need forfreshCliententirely.Based on learnings from
AGENTS.md: "use one McpClient per test file rather than spawning multiple clients per file."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/resources.test.mjs` around lines 63 - 74, Replace the second McpClient usage with operations on the existing client: instead of creating freshClient and calling start/stop, call the session teardown method (e.g., close_session) on the existing McpClient, perform the readResource('accessibility://current') to assert it throws the -32002/No active browser session error, then restore the browser session by invoking the start_browser (or equivalent start/navigation) method before continuing other tests; reference McpClient, readResource, start/stop, close_session and start_browser to locate and change the test accordingly.
🤖 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/accessibility-snapshot.js`:
- Around line 24-62: The walk function currently assumes el is non-null which
causes a crash if document.body is null; add a guard at the start of walk (and
before calling walk(document.body)) to return null when el is falsy, update
ROLE_MAP to include OPTION: 'option' so <option> nodes are represented
(preventing dropdown choices from being collapsed), and add a short comment near
the CSS-hidden checks in walk explaining that display:none / visibility:hidden
are intentionally not filtered for performance (or call out where to add
getComputedStyle if you want that behavior).
In `@src/lib/server.js`:
- Around line 870-888: Replace the magic numeric error code -32002 with the SDK
enum ErrorCode.InternalError in the McpError thrown when no active browser
session (inside the server.registerResource callback that checks
state.drivers.get(state.currentSession)); update the throw to use new
McpError(ErrorCode.InternalError, "...") and ensure ErrorCode is imported from
the MCP SDK where McpError is referenced so the enum is available.
In `@test/resources.test.mjs`:
- Around line 20-23: Wrap the call to client.callTool('close_session') inside a
try/catch within the after hook in test/resources.test.mjs so that any error
from close_session is caught (e.g., try { await client.callTool('close_session')
} catch (err) { /* optionally log */ }) and then always call await client.stop()
after the try/catch; this mirrors the guarded pattern used in
navigation.test.mjs and ensures client.stop() runs even if close_session throws.
- Line 16: The start_browser invocation via client.callTool should include the
Chromium flags used elsewhere so it runs in containerized CI; update the call to
client.callTool('start_browser', { browser: 'chrome', options: { headless: true,
args: ['--no-sandbox', '--disable-dev-shm-usage'] } }) so the options object
adds the --no-sandbox and --disable-dev-shm-usage flags alongside headless.
---
Nitpick comments:
In `@src/lib/server.js`:
- Around line 877-882: Replace the duplicated session-check/driver retrieval in
the accessibility snapshot handler by reusing the existing getDriver() helper:
instead of directly accessing state.drivers.get(state.currentSession) and
throwing a custom McpError, call await getDriver() to obtain the driver and then
use it to run accessibilitySnapshotScript; this removes the duplicated logic and
lets the existing catch block convert any thrown errors (including
missing-session errors) into McpError as currently handled. Ensure you remove
the manual state.drivers/state.currentSession access and the local throw so only
getDriver() is responsible for validating the session.
In `@test/resources.test.mjs`:
- Around line 63-74: Replace the second McpClient usage with operations on the
existing client: instead of creating freshClient and calling start/stop, call
the session teardown method (e.g., close_session) on the existing McpClient,
perform the readResource('accessibility://current') to assert it throws the
-32002/No active browser session error, then restore the browser session by
invoking the start_browser (or equivalent start/navigation) method before
continuing other tests; reference McpClient, readResource, start/stop,
close_session and start_browser to locate and change the test accordingly.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/lib/accessibility-snapshot.js (1)
46-46: Nit: preferRegExp.prototype.test()overString.prototype.match()for boolean checks.
test()is marginally faster and conveys intent more clearly when you only need a boolean.Suggested change
- if (tag.match(/^H[1-6]$/)) node.level = parseInt(tag[1]); + if (/^H[1-6]$/.test(tag)) node.level = parseInt(tag[1]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/accessibility-snapshot.js` at line 46, Replace the boolean check using String.prototype.match with RegExp.prototype.test for clarity and performance: in the code that sets node.level (the if using tag.match(/^H[1-6]$/)), change the condition to use /^H[1-6]$/.test(tag) and keep the existing parseInt(tag[1]) assignment to node.level (optionally add radix 10 if desired) so the intent is clearer and the check returns a boolean directly.src/lib/server.js (1)
870-890: Consider guarding against anullaccessibility tree.
driver.executeScript(accessibilitySnapshotScript)can returnnullwhendocument.bodyis absent or the page has no semantically meaningful content (thewalkfunction returnsnullin those cases).JSON.stringify(null)produces the string"null", which is valid JSON but may surprise consumers expecting an object.A small fallback keeps the contract predictable:
Suggested guard
const tree = await driver.executeScript(accessibilitySnapshotScript); - return { contents: [{ uri: uri.href, mimeType: "application/json", text: JSON.stringify(tree, null, 2) }] }; + const result = tree ?? { role: "none", name: "(empty page)" }; + return { contents: [{ uri: uri.href, mimeType: "application/json", text: JSON.stringify(result, null, 2) }] };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server.js` around lines 870 - 890, The accessibility snapshot handler registered in server.registerResource ("accessibility-snapshot") should guard against driver.executeScript(accessibilitySnapshotScript) returning null; check the result from driver.executeScript(accessibilitySnapshotScript) (in the async callback) and if it is null/undefined replace it with a stable empty structure (e.g., {} or an empty tree object) before calling JSON.stringify so consumers always get a predictable JSON object rather than the literal "null". Update the variable used to build the return (tree) accordingly and keep the existing McpError handling unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/lib/accessibility-snapshot.js`:
- Line 46: Replace the boolean check using String.prototype.match with
RegExp.prototype.test for clarity and performance: in the code that sets
node.level (the if using tag.match(/^H[1-6]$/)), change the condition to use
/^H[1-6]$/.test(tag) and keep the existing parseInt(tag[1]) assignment to
node.level (optionally add radix 10 if desired) so the intent is clearer and the
check returns a boolean directly.
In `@src/lib/server.js`:
- Around line 870-890: The accessibility snapshot handler registered in
server.registerResource ("accessibility-snapshot") should guard against
driver.executeScript(accessibilitySnapshotScript) returning null; check the
result from driver.executeScript(accessibilitySnapshotScript) (in the async
callback) and if it is null/undefined replace it with a stable empty structure
(e.g., {} or an empty tree object) before calling JSON.stringify so consumers
always get a predictable JSON object rather than the literal "null". Update the
variable used to build the return (tree) accordingly and keep the existing
McpError handling unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/accessibility-snapshot.js`:
- Around line 16-21: Add an explicit 'hidden' mapping to INPUT_ROLES (set to
null) and short-circuit element traversal when an INPUT's resolved role is null:
update INPUT_ROLES to include 'hidden': null so ROLE_MAP['INPUT'] won't be used
for hidden inputs, then in the walk function (after resolving role for an
element, particularly for tagName 'INPUT') return early when role === null to
avoid emitting hidden inputs into the accessibility tree.
Comment on lines
+16
to
+21
| var INPUT_ROLES = { | ||
| checkbox: 'checkbox', radio: 'radio', button: 'button', | ||
| submit: 'button', reset: 'button', range: 'slider', | ||
| search: 'searchbox', email: 'textbox', url: 'textbox', | ||
| tel: 'textbox', number: 'spinbutton' | ||
| }; |
There was a problem hiding this comment.
<input type="hidden"> leaks into the tree as role: 'textbox'.
INPUT_ROLES has no 'hidden' entry, so hidden inputs fall through to ROLE_MAP['INPUT'] ('textbox'). They commonly carry a name attribute (e.g., csrf_token, _method), so they'll appear as { role: 'textbox', name: 'csrf_token' } — semantically wrong and noisy for LLM consumers.
🐛 Proposed fix
var INPUT_ROLES = {
checkbox: 'checkbox', radio: 'radio', button: 'button',
submit: 'button', reset: 'button', range: 'slider',
- search: 'searchbox', email: 'textbox', url: 'textbox',
- tel: 'textbox', number: 'spinbutton'
+ search: 'searchbox', email: 'textbox', url: 'textbox',
+ tel: 'textbox', number: 'spinbutton', hidden: null
};Then early-exit in walk when the resolved role is explicitly null for an INPUT:
- var role = el.getAttribute('role') || (tag === 'INPUT' ? INPUT_ROLES[el.type] : null) || ROLE_MAP[tag] || null;
+ var inputRole = tag === 'INPUT' ? INPUT_ROLES[el.type] : undefined;
+ if (inputRole === null) return null; // explicitly excluded type (e.g. hidden)
+ var role = el.getAttribute('role') || inputRole || ROLE_MAP[tag] || null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/accessibility-snapshot.js` around lines 16 - 21, Add an explicit
'hidden' mapping to INPUT_ROLES (set to null) and short-circuit element
traversal when an INPUT's resolved role is null: update INPUT_ROLES to include
'hidden': null so ROLE_MAP['INPUT'] won't be used for hidden inputs, then in the
walk function (after resolving role for an element, particularly for tagName
'INPUT') return early when role === null to avoid emitting hidden inputs into
the accessibility tree.
Closed
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.
Fixes #62 and #63
Introduces a new accessibility snapshot resource to enable clients to retrieve a structured representation of a page's interactive elements, improving automation and accessibility analysis.
Also updates codebase to use MCP SDK 1.26.0
Summary by CodeRabbit
New Features
Tests
Chores