feat: Arc Browser MCP Server (hybrid AppleScript + CDP)#26
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Disabled knowledge base sources:
📝 WalkthroughWalkthroughAdds a new package Changes
Sequence DiagramsequenceDiagram
participant Client as MCP Client
participant Server as ArcBrowserMCP<br/>(Server)
participant Arc as Arc Browser<br/>(AppleScript)
participant CDP as Chrome DevTools<br/>(Local CDP)
participant Browser as Browser<br/>(Runtime)
Client->>Server: Invoke tool (e.g., listTabs / screenshot)
alt AppleScript-based tool
Server->>Arc: run osascript(...)
Arc->>Browser: query UI / run script
Browser-->>Arc: result
Arc-->>Server: parsed result
else CDP-based tool
Server->>Server: ensureCDP()
alt CDP unavailable
Server-->>Client: error (CDP unavailable)
else CDP available
Server->>CDP: connect / send(method, params)
CDP->>Browser: execute CDP command
Browser-->>CDP: response / events
CDP-->>Server: result / events
end
end
Server-->>Client: return tool result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 8
🧹 Nitpick comments (1)
packages/arc-browser/tsconfig.json (1)
4-5: PreferNodeNextresolution for this ESM package.With
packages/arc-browser/package.jsonset to"type": "module"and the sources importing.jsspecifiers,ESNext+ legacyNoderesolution can type-check against rules that don’t match Node’s real ESM resolution path. UsingNodeNexthere keeps compile-time resolution aligned with runtime behavior.♻️ Suggested config
- "module": "ESNext", - "moduleResolution": "Node", + "module": "NodeNext", + "moduleResolution": "NodeNext",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/arc-browser/tsconfig.json` around lines 4 - 5, Change the TypeScript module resolution to NodeNext to match this package's ESM runtime behavior: update the tsconfig key "moduleResolution" from "Node" to "NodeNext" so that imports (including .js specifiers) are resolved at compile time the same way Node's ESM loader will resolve them; ensure this aligns with "module": "ESNext" in the same tsconfig and the package.json "type": "module".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 73-75: Replace the non-deterministic npm upgrade in the CI step
named "Install latest npm" by pinning to a tested version (e.g., change install
command from npm@latest to npm@11.12.1) and optionally echo the installed npm
version after installation for transparency; update the run command in that step
to use the pinned version and include a quick npm -v log to record the version
used.
In `@packages/arc-browser/package.json`:
- Around line 1-30: Add an "engines" entry to package.json to declare the
minimum Node runtime (e.g., "node": ">=21.0.0") so the package consumers and npm
enforce a compatible Node version; update the package.json JSON object
(alongside "name", "version", etc.) to include this "engines" field. Reference
the runtime-sensitive code paths—src/index.ts (top-level await), src/cdp.ts
(global WebSocket), and any use of fetch() or AbortSignal.timeout()—to ensure
the declared version covers those features. If you prefer not to bump the
engine, instead add explicit polyfills or shims for fetch, AbortSignal.timeout,
and WebSocket used by those modules and document the polyfills in package.json
instead of the engines field. Ensure package.json remains valid JSON and run npm
pack / npm publish checks after the change.
In `@packages/arc-browser/src/arc.ts`:
- Around line 88-97: The current executeJavaScript function embeds raw JS (only
escaping backslashes/quotes) which breaks on newlines; instead base64-encode the
js payload in Node (replace the escaped logic with something like const encoded
= Buffer.from(js, "utf8").toString("base64")), interpolate that single-line
encoded string into the osascript call, and inside the AppleScript-run JS decode
and execute it (e.g., use window.eval(atob("...")) or new
Function(atob("..."))()) so the full multi-line source is transported safely;
update references to the old escaped variable and ensure executeJavaScript and
the osascript invocation use the new encoded value.
In `@packages/arc-browser/src/cdp.ts`:
- Around line 65-95: connectWs currently calls disconnect() which only clears
the pending map leaving send() promises unresolved; add logic to reject all
in-flight pending commands with a clear Error when the socket is closed or
replaced. Modify disconnect() (or create a helper used by disconnect and
connectWs) to iterate this.pending, call handler.reject(new Error("WebSocket
closed or replaced")) for each entry, then clear the map; also add a WebSocket
"close" (and optionally "error") event listener in connectWs to perform the same
rejection/cleanup for this.ws so any in-flight send() promises are always
rejected when the socket closes or is swapped.
- Around line 117-124: The current on/off API stores handlers in the
eventHandlers Map but off(event) deletes all handlers for that event, causing
races when multiple listeners exist; update the API so on(event, handler)
registers the handler and returns an unsubscribe callback (or implement
off(event, handler) that removes only that handler) and change off to accept the
handler and only remove that specific function from the array stored in
eventHandlers (use the eventHandlers.get(event) array, filter out the provided
handler, and set or delete the entry accordingly); refer to the on and off
methods and the eventHandlers Map to locate and change the logic.
In `@packages/arc-browser/src/server.ts`:
- Around line 250-277: The CDP listeners registered via
cdp.on("Network.requestWillBeSent", ...) and cdp.on("Network.responseReceived",
...) (and the analogous Runtime listeners) are only removed on the happy path;
wrap the enable/send, sleep, and disable sequence in a try/finally so that
cdp.off(...) and the domain disable calls always run; specifically, move the
cdp.off("Network.requestWillBeSent"), cdp.off("Network.responseReceived") and
the cdp.send("Network.disable") into a finally block after the await
cdp.send("Network.enable") and sleep, and apply the same try/finally pattern to
the Runtime.enable/disable capture flow so handlers are always unregistered even
on error.
- Around line 20-30: ensureCDP currently returns the existing CDP socket even
when callers pass a different targetId; update ensureCDP to check
this.cdp.currentTargetId against the requested targetId and, if they differ,
treat it like not connected (call CDPClient.isAvailable() and
this.cdp.connectToPage(targetId) to reconnect to the requested page). Keep the
existing availability check (CDPClient.isAvailable()) and error throw path, and
only skip reconnect when this.cdp.connected is true AND this.cdp.currentTargetId
equals the requested targetId.
---
Nitpick comments:
In `@packages/arc-browser/tsconfig.json`:
- Around line 4-5: Change the TypeScript module resolution to NodeNext to match
this package's ESM runtime behavior: update the tsconfig key "moduleResolution"
from "Node" to "NodeNext" so that imports (including .js specifiers) are
resolved at compile time the same way Node's ESM loader will resolve them;
ensure this aligns with "module": "ESNext" in the same tsconfig and the
package.json "type": "module".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0dd84d8b-91fa-4c2e-972e-f27df17d3e33
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
.github/workflows/ci.ymleslint.config.jspackages/arc-browser/package.jsonpackages/arc-browser/src/arc.tspackages/arc-browser/src/cdp.tspackages/arc-browser/src/index.tspackages/arc-browser/src/server.tspackages/arc-browser/tsconfig.json
Comment on lines
+73
to
+75
| - name: Install latest npm | ||
| run: npm install -g npm@latest | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify npm release drift and Node compatibility before choosing a pin
node -v
npm view npm dist-tags --json
npm view npm@latest engines --json
npm view npm@11.5.1 engines --jsonRepository: arvoreeducacao/arvore-mcp-servers
Length of output: 605
🏁 Script executed:
cat -n .github/workflows/ci.yml | sed -n '60,85p'Repository: arvoreeducacao/arvore-mcp-servers
Length of output: 920
🏁 Script executed:
rg "trusted.publishing|npm.*>=|npm@" .github/workflows/ci.yml -A 2 -B 2Repository: arvoreeducacao/arvore-mcp-servers
Length of output: 181
🏁 Script executed:
git log --oneline -20Repository: arvoreeducacao/arvore-mcp-servers
Length of output: 184
🏁 Script executed:
rg "11\.5|trusted.*publish|npm.*11" -iRepository: arvoreeducacao/arvore-mcp-servers
Length of output: 59
🏁 Script executed:
find . -name "*.md" -o -name "*.json" | xargs grep -l "npm.*version\|trusted" 2>/dev/null | head -5Repository: arvoreeducacao/arvore-mcp-servers
Length of output: 389
🏁 Script executed:
cat README.md | grep -i "npm\|version\|publish" -A 2 -B 2Repository: arvoreeducacao/arvore-mcp-servers
Length of output: 3953
🏁 Script executed:
cat package.json | grep -A 5 -B 5 "npm\|engines\|publish"Repository: arvoreeducacao/arvore-mcp-servers
Length of output: 1416
Pin npm to a tested version instead of latest.
Using npm@latest introduces non-determinism in the publish pipeline—npm can update unexpectedly and potentially break the provenance feature. Pin to a known compatible version (e.g., 11.12.1) and optionally log it for transparency.
Proposed change
- - name: Install latest npm
- run: npm install -g npm@latest
+ - name: Install npm for trusted publishing
+ run: |
+ npm install -g npm@11.12.1
+ npm --version📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - name: Install latest npm | |
| run: npm install -g npm@latest | |
| - name: Install npm for trusted publishing | |
| run: | | |
| npm install -g npm@11.12.1 | |
| npm --version | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 73 - 75, Replace the non-deterministic
npm upgrade in the CI step named "Install latest npm" by pinning to a tested
version (e.g., change install command from npm@latest to npm@11.12.1) and
optionally echo the installed npm version after installation for transparency;
update the run command in that step to use the pinned version and include a
quick npm -v log to record the version used.
Comment on lines
+65
to
+95
| private connectWs(url: string): Promise<void> { | ||
| return new Promise((resolve, reject) => { | ||
| this.disconnect(); | ||
| this.ws = new WebSocket(url); | ||
|
|
||
| this.ws.addEventListener("open", () => resolve()); | ||
| this.ws.addEventListener("error", (e) => | ||
| reject(new Error(`WebSocket error: ${(e as ErrorEvent).message ?? "unknown"}`)) | ||
| ); | ||
|
|
||
| this.ws.addEventListener("message", (event) => { | ||
| const msg: CDPMessage = JSON.parse( | ||
| typeof event.data === "string" ? event.data : event.data.toString() | ||
| ); | ||
|
|
||
| if (msg.id !== undefined && this.pending.has(msg.id)) { | ||
| const handler = this.pending.get(msg.id)!; | ||
| this.pending.delete(msg.id); | ||
| if (msg.error) { | ||
| handler.reject(new Error(`CDP Error: ${msg.error.message}`)); | ||
| } else { | ||
| handler.resolve(msg.result ?? {}); | ||
| } | ||
| } | ||
|
|
||
| if (msg.method && msg.params) { | ||
| const handlers = this.eventHandlers.get(msg.method) ?? []; | ||
| for (const h of handlers) h(msg.params); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Reject in-flight CDP commands when the socket closes or is replaced.
connectWs() calls disconnect() before reconnecting, and the shutdown path also uses disconnect(). Right now disconnect() just clears pending, so any in-flight send() promise becomes unresolvable because its timeout sees the entry already gone. The same leak happens on an unexpected socket close because there is no close handler draining the map.
🔧 Suggested cleanup path
+ private failPending(error: Error): void {
+ for (const { reject } of this.pending.values()) {
+ reject(error);
+ }
+ this.pending.clear();
+ }
+
private connectWs(url: string): Promise<void> {
return new Promise((resolve, reject) => {
- this.disconnect();
+ this.disconnect(new Error("CDP connection replaced"));
this.ws = new WebSocket(url);
this.ws.addEventListener("open", () => resolve());
this.ws.addEventListener("error", (e) =>
reject(new Error(`WebSocket error: ${(e as ErrorEvent).message ?? "unknown"}`))
);
+ this.ws.addEventListener("close", () => {
+ this.failPending(new Error("CDP connection closed"));
+ this.ws = null;
+ this.targetId = null;
+ });
this.ws.addEventListener("message", (event) => {
const msg: CDPMessage = JSON.parse(
typeof event.data === "string" ? event.data : event.data.toString()
);
@@
- disconnect(): void {
+ disconnect(error = new Error("CDP connection closed")): void {
if (this.ws) {
this.ws.close();
this.ws = null;
}
- this.pending.clear();
+ this.failPending(error);
this.eventHandlers.clear();
this.targetId = null;
}Also applies to: 127-134
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/arc-browser/src/cdp.ts` around lines 65 - 95, connectWs currently
calls disconnect() which only clears the pending map leaving send() promises
unresolved; add logic to reject all in-flight pending commands with a clear
Error when the socket is closed or replaced. Modify disconnect() (or create a
helper used by disconnect and connectWs) to iterate this.pending, call
handler.reject(new Error("WebSocket closed or replaced")) for each entry, then
clear the map; also add a WebSocket "close" (and optionally "error") event
listener in connectWs to perform the same rejection/cleanup for this.ws so any
in-flight send() promises are always rejected when the socket closes or is
swapped.
- tsconfig: use NodeNext module resolution - package.json: add engines >= 22, files field - arc.ts: base64-encode JS payload for safe AppleScript transport - cdp.ts: reject pending promises on disconnect, granular off() - server.ts: try/finally for network/console listeners, ensureCDP respects targetId - Add package README and update root README with arc-browser entry
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
packages/arc-browser/src/cdp.ts (1)
65-75:⚠️ Potential issue | 🟠 MajorReject in-flight commands on unsolicited socket close.
disconnect()now rejectspending, but an unexpectedclosefrom Arc never reaches that path. Those requests sit until the 30s timeout, andws/targetIdstay stale in the meantime.♻️ Suggested cleanup path
+ private failPending(error: Error): void { + for (const { reject } of this.pending.values()) { + reject(error); + } + this.pending.clear(); + } + private connectWs(url: string): Promise<void> { return new Promise((resolve, reject) => { this.disconnect(); - this.ws = new WebSocket(url); + const ws = new WebSocket(url); + this.ws = ws; - this.ws.addEventListener("open", () => resolve()); - this.ws.addEventListener("error", (e) => + ws.addEventListener("open", () => resolve()); + ws.addEventListener("error", (e) => reject(new Error(`WebSocket error: ${(e as ErrorEvent).message ?? "unknown"}`)) ); + ws.addEventListener("close", () => { + if (this.ws !== ws) return; + this.failPending(new Error("WebSocket closed")); + this.ws = null; + this.targetId = null; + }); - this.ws.addEventListener("message", (event) => { + ws.addEventListener("message", (event) => { const msg: CDPMessage = JSON.parse( typeof event.data === "string" ? event.data : event.data.toString() ); @@ disconnect(): void { if (this.ws) { this.ws.close(); this.ws = null; } - for (const [id, handler] of this.pending) { - handler.reject(new Error("WebSocket closed")); - this.pending.delete(id); - } - this.pending.clear(); + this.failPending(new Error("WebSocket closed")); this.eventHandlers.clear(); this.targetId = null; }Also applies to: 142-154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/arc-browser/src/cdp.ts` around lines 65 - 75, The connectWs method currently only listens for "open", "error", and "message" events, so unexpected socket "close" events never trigger the existing disconnect logic and in-flight requests (pending) hang until timeout; update connectWs to add a "close" event listener that calls the same cleanup path as disconnect() (or invokes disconnect()) and ensures pending requests are rejected and ws/targetId cleared; reference the connectWs method, the ws property, the disconnect() method, and the pending collection to locate where to add the "close" handler and perform the rejection/cleanup (also apply the same fix to the similar block at the other location around lines 142-154).packages/arc-browser/src/server.ts (1)
254-283:⚠️ Potential issue | 🟠 MajorStart cleanup before
*.enable()and unsubscribe by handler.Both flows register listeners before
Network.enable/Runtime.enable, but thetrystarts after the enable call. If enable fails, those handlers stay attached. Alsooff("event")still removes every handler for that event, so overlapping captures on the same client tear each other down.🔧 Safer pattern
- cdp.on("Network.requestWillBeSent", (p) => { + const offRequest = cdp.on("Network.requestWillBeSent", (p) => { const req = p.request as { method: string; url: string }; const id = p.requestId as string; if (urlFilter && !req.url.includes(urlFilter)) return; requests.set(id, { method: req.method, url: req.url, timestamp: p.timestamp as number, }); }); - cdp.on("Network.responseReceived", (p) => { + const offResponse = cdp.on("Network.responseReceived", (p) => { const id = p.requestId as string; const resp = p.response as { status: number; mimeType: string }; const existing = requests.get(id); if (existing) { existing.status = resp.status; existing.mimeType = resp.mimeType; } }); - await cdp.send("Network.enable"); - try { + await cdp.send("Network.enable"); await new Promise((r) => setTimeout(r, duration)); } finally { - await cdp.send("Network.disable"); - cdp.off("Network.requestWillBeSent"); - cdp.off("Network.responseReceived"); + offRequest(); + offResponse(); + await cdp.send("Network.disable"); }Apply the same shape to
Runtime.consoleAPICalled.Also applies to: 358-374
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/arc-browser/src/server.ts` around lines 254 - 283, Handlers for Network.requestWillBeSent and Network.responseReceived are being registered before calling cdp.send("Network.enable") and are removed with cdp.off("Network.requestWillBeSent")/cddp.off("Network.responseReceived") which either leaves handlers attached if enable fails or removes all handlers for that event; fix by registering handlers into local named functions (e.g., requestWillBeSentHandler, responseReceivedHandler), attach them before enable but ensure a try/finally that removes those exact handlers (cdp.off("Network.requestWillBeSent", requestWillBeSentHandler) / cdp.off("Network.responseReceived", responseReceivedHandler)) and move the enable call after you have the handlers and the removal logic prepared so enable failures won't leak listeners; apply the same pattern to Runtime.consoleAPICalled and the other similar block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/arc-browser/src/cdp.ts`:
- Around line 98-113: The send() method hard-codes a 30s reply timeout causing
longer page-side waits (e.g. waitForSelector) to prematurely fail; update
send(method: string, params: Record<string, unknown> = {}, timeoutMs = 30_000)
to accept an optional timeoutMs and use that instead of the fixed 30_000 when
scheduling the rejection timer, and update callers (notably the waitForSelector
path and the other send usage around the 242-260 region) to pass through their
configured timeoutMs so CDP client-side timeouts match page-side waits; keep
msgId, pending map usage and resolve/reject behavior unchanged.
- Around line 53-63: In connectToPage, the code sets this.targetId before
calling connectWs/disconnect which causes disconnect() to clear it and leave
currentTargetId null; move the assignment of this.targetId = target.id so it
happens after a successful reconnect/attach (i.e., after await
this.connectWs(...) completes and any attach logic returns successfully) so the
field isn't cleared by disconnect(), or alternatively update disconnect() to
avoid clearing this.targetId during a reconnect flow; key symbols:
connectToPage, this.targetId, connectWs, disconnect, currentTargetId.
In `@packages/arc-browser/src/server.ts`:
- Around line 8-10: The server currently stores a single mutable CDPClient
instance (this.cdp) that is reconnected to different targets, which causes
cross-talk between concurrent requests; change to create or obtain an isolated
CDPClient per request/target instead of using the shared this.cdp: remove or
stop mutating the shared this.cdp field and update the code paths that call
connect/reconnect (e.g., the handler that performs network_capture and any
connectToTarget/reconnect logic) to instantiate a new CDPClient for each request
or pull clients from a pool keyed by target/session id, ensuring each request's
finally/cleanup runs against the same client instance it started with (or use a
Map<targetOrSession, CDPClient> to isolate clients by target/session).
- Around line 315-331: The fallback to AppleScript uses
arc.getActiveTab()/arc.executeJavaScript even when a specific CDP targetId was
requested, which can return the wrong page; update the logic in the block around
CDPClient.isAvailable() and the use of targetId so that if targetId is provided
but CDPClient.isAvailable() is false you do not fall back to the active tab:
instead return an explicit error or a clear no-data response. Concretely, inside
the method that calls CDPClient.isAvailable(), check the targetId parameter
before using arc.getActiveTab()/arc.executeJavaScript and if targetId is
non-null and CDP is unavailable, short-circuit (throw or return a structured
error/result) rather than using arc.getActiveTab(); reference ensureCDP,
CDPClient.isAvailable, targetId, arc.getActiveTab and arc.executeJavaScript when
applying the change.
---
Duplicate comments:
In `@packages/arc-browser/src/cdp.ts`:
- Around line 65-75: The connectWs method currently only listens for "open",
"error", and "message" events, so unexpected socket "close" events never trigger
the existing disconnect logic and in-flight requests (pending) hang until
timeout; update connectWs to add a "close" event listener that calls the same
cleanup path as disconnect() (or invokes disconnect()) and ensures pending
requests are rejected and ws/targetId cleared; reference the connectWs method,
the ws property, the disconnect() method, and the pending collection to locate
where to add the "close" handler and perform the rejection/cleanup (also apply
the same fix to the similar block at the other location around lines 142-154).
In `@packages/arc-browser/src/server.ts`:
- Around line 254-283: Handlers for Network.requestWillBeSent and
Network.responseReceived are being registered before calling
cdp.send("Network.enable") and are removed with
cdp.off("Network.requestWillBeSent")/cddp.off("Network.responseReceived") which
either leaves handlers attached if enable fails or removes all handlers for that
event; fix by registering handlers into local named functions (e.g.,
requestWillBeSentHandler, responseReceivedHandler), attach them before enable
but ensure a try/finally that removes those exact handlers
(cdp.off("Network.requestWillBeSent", requestWillBeSentHandler) /
cdp.off("Network.responseReceived", responseReceivedHandler)) and move the
enable call after you have the handlers and the removal logic prepared so enable
failures won't leak listeners; apply the same pattern to
Runtime.consoleAPICalled and the other similar block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 027c91d3-42fd-4402-86cb-a1d55ecad427
📒 Files selected for processing (7)
README.mdpackages/arc-browser/README.mdpackages/arc-browser/package.jsonpackages/arc-browser/src/arc.tspackages/arc-browser/src/cdp.tspackages/arc-browser/src/server.tspackages/arc-browser/tsconfig.json
✅ Files skipped from review due to trivial changes (4)
- packages/arc-browser/tsconfig.json
- packages/arc-browser/package.json
- packages/arc-browser/README.md
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/arc-browser/src/arc.ts
Comment on lines
+8
to
+10
| private server: McpServer; | ||
| private cdp: CDPClient; | ||
|
|
There was a problem hiding this comment.
A shared CDPClient will cross-talk between concurrent requests.
Line 16 creates one mutable client for the whole server, and Lines 20-33 reconnect that same instance to different targets. A long-running call like network_capture can therefore be retargeted underneath itself by a later request, then run its finally block against the wrong page.
Use a per-request client, or pool isolated clients by target/session instead of sharing one global this.cdp.
Also applies to: 16-18, 20-35
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/arc-browser/src/server.ts` around lines 8 - 10, The server currently
stores a single mutable CDPClient instance (this.cdp) that is reconnected to
different targets, which causes cross-talk between concurrent requests; change
to create or obtain an isolated CDPClient per request/target instead of using
the shared this.cdp: remove or stop mutating the shared this.cdp field and
update the code paths that call connect/reconnect (e.g., the handler that
performs network_capture and any connectToTarget/reconnect logic) to instantiate
a new CDPClient for each request or pull clients from a pool keyed by
target/session id, ensuring each request's finally/cleanup runs against the same
client instance it started with (or use a Map<targetOrSession, CDPClient> to
isolate clients by target/session).
- arc.ts: guard switchSpace for >10 spaces instead of silent fallback - cdp.ts: set targetId after connectWs to avoid disconnect clearing it - cdp.ts: configurable send timeout, waitForSelector uses timeoutMs+5s - server.ts: error when targetId specified but CDP unavailable in get_page_content - server.ts: add too_many_spaces message to switch_space
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.
O que muda
Novo MCP server para controlar o Arc Browser diretamente do Kiro/Cursor, usando uma abordagem híbrida:
AppleScript (sempre funciona, sem setup)
list_tabs/get_active_tab/open_url/search_tabs/close_tab/focus_tabexecute_js— executa JavaScript na tab ativalist_spaces/get_active_space/switch_spaceCDP — Chrome DevTools Protocol (precisa abrir o Arc com
--remote-debugging-port=9222)screenshot— captura screenshot da página (viewport ou full page)cdp_evaluate— executa JS com suporte a async/awaitnetwork_capture— intercepta requests/responses por X segundosget_console_logs— captura console.log/warn/errorget_cookies— lista cookies da páginaclick/hover/type_text/scroll/wait_for_selector— interação com a páginaget_page_content— texto da página (fallback pra AppleScript se CDP não disponível)cdp_status— verifica se CDP tá ativoSetup
Por que não usar só o Playwright MCP?
O Arc MCP acessa o browser real do usuário — com sessões logadas, cookies, tabs abertas. O Playwright abre um browser limpo. São complementares.
Summary by CodeRabbit
New Features
Documentation
Chores