feat: extract embedded Electron MCP package#11
Merged
Conversation
🦋 Changeset detectedLatest commit: 6d5e105 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds repo governance, CI/release automation, Changesets, ESM packaging, test tooling and fixtures, an embeddable loopback HTTP MCP server with configurable path/name/logger, many tool unit tests and fakes, and several module/import specifier and type-export adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as MCP Client
participant HTTP as HTTP Server (loopback)
participant MCP as MCP Runtime
participant CDP as CDP Session
participant Electron as Electron WebContents
Client->>HTTP: POST /<configured-path> (initialize / call_tool)
HTTP->>MCP: routeRequest -> dispatch tool handler
MCP->>CDP: getOrAttachSession(surface)
CDP->>Electron: attach / Runtime.evaluate / Input.* (CDP calls)
Electron-->>CDP: CDP responses / events
CDP-->>MCP: return results/events
MCP-->>HTTP: tool result (structuredContent / content)
HTTP-->>Client: call_tool response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
🚥 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 docstrings
🧪 Generate unit tests (beta)
Comment |
jnsdls
force-pushed
the
extract-electron-mcp-package
branch
from
1bd6873 to
58fac1c
Compare
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
tsdown emits dist/index.mjs (package is "type": "module"); the fixture imported dist/index.js which doesn't exist, causing the main process import to fail silently and __electronMcpSmokeUrl never to be set — surfacing as "MCP server did not start" in CI. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server.ts (1)
247-264:⚠️ Potential issue | 🟠 MajorDestroy the socket from the
res.end()callback.Calling
req.socket?.destroy()immediately afterres.end()can tear down the connection before the 413 body flushes, which turns the intended JSON-RPC error intoECONNRESETon the client.Suggested fix
res.setHeader("connection", "close"); - res.end( - JSON.stringify({ - jsonrpc: "2.0", - error: { - code: -32600, - message: `Request body exceeds ${MAX_BODY_BYTES} bytes`, - }, - id: null, - }), - ); - req.socket?.destroy(); + res.end( + JSON.stringify({ + jsonrpc: "2.0", + error: { + code: -32600, + message: `Request body exceeds ${MAX_BODY_BYTES} bytes`, + }, + id: null, + }), + () => { + req.socket?.destroy(); + }, + ); return;As per coding guidelines, any new
res.end(...)followed by a synchronoussocket.destroy()should pair the destroy as theres.endcallback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server.ts` around lines 247 - 264, The response currently calls res.end(...) and then immediately req.socket?.destroy() which can abort the flush; change this so the socket is destroyed in the res.end callback: when handling parsed.reason === "too-large" (the branch that sets 413 and the JSON-RPC error mentioning MAX_BODY_BYTES), pass a callback to res.end that calls req.socket?.destroy(), ensuring the body is flushed before the socket is torn down.
🧹 Nitpick comments (2)
src/tools/evaluate.ts (1)
42-57: Consider:unserializableValueis defined but unused.The
RuntimeEvaluateResultinterface includesunserializableValue(line 48), but the result formatting logic no longer uses it—falling back todescriptionortypeinstead. If this simplification is intentional (relying ondescriptionto convey the same info), consider removing the unused field from the interface for clarity, or add a brief comment explaining why it's retained.Also applies to: 107-113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/evaluate.ts` around lines 42 - 57, The interface RuntimeEvaluateResult declares result.unserializableValue but the formatting logic never reads it; remove the unused unserializableValue property from RuntimeEvaluateResult (and any duplicate occurrences around the other block at 107-113) to keep the type accurate, or alternatively add a one-line comment above RuntimeEvaluateResult explaining why unserializableValue must remain if you choose to keep it; update only the interface and its duplicates (RuntimeEvaluateResult/result.unserializableValue) without changing the runtime formatting logic.package.json (1)
56-72: Keepasync-mutexin one dependency section.It is declared in both
devDependenciesanddependencies. Sincesrc/index.tsimports it at runtime, keeping it only independenciesavoids version drift between the two lists.Suggested cleanup
"devDependencies": { "@biomejs/biome": "^2.4.12", "@changesets/cli": "^2.29.7", "@modelcontextprotocol/sdk": "^1.29.0", "@playwright/test": "^1.57.0", "@types/node": "^24.9.0", - "async-mutex": "^0.5.0", "electron": "^41.2.1", "playwright": "^1.57.0", "tsdown": "^0.21.9", "typescript": "^6.0.3", "vitest": "4.1.5", "zod": "^4.3.6" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 56 - 72, The package "async-mutex" is declared in both devDependencies and dependencies; since it is imported at runtime (see src/index.ts), remove the duplicate from "devDependencies" and keep a single entry under "dependencies" to prevent version drift—update the package.json by deleting the "async-mutex" line inside the "devDependencies" object and ensure the existing "dependencies" entry remains the authoritative one.
🤖 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 12-17: The workflow currently references mutable tags for GitHub
Actions (uses: actions/checkout@v6, uses: pnpm/action-setup@v4, uses:
actions/setup-node@v6); update each "uses:" entry for actions/checkout,
pnpm/action-setup, and actions/setup-node to pin the action to its corresponding
full commit SHA instead of a version tag (replace e.g. actions/checkout@v6 ->
actions/checkout@<full-commit-sha>, etc.), ensuring you update every occurrence
in the file so all mutable tags are replaced with their immutable commit SHAs.
In `@CONTRIBUTING.md`:
- Around line 15-19: The CONTRIBUTING.md sentence saying "Electron smoke
coverage lives under `test/electron/`" is ambiguous because there is also a
`tests/smoke.electron.test.ts`; update that line to clearly state the canonical
location (e.g., "Electron smoke tests live under `test/electron/` — legacy
`tests/smoke.electron.test.ts` may still exist and will be migrated") or choose
the canonical path and mark the other as legacy; edit the exact phrase
referencing `test/electron/` and mention `tests/smoke.electron.test.ts` so
contributors know which location to use.
In `@src/cdp.ts`:
- Around line 74-79: The onDetach listener and CdpSession.detach are deleting
entries from the attached cache without the required binding-equality guard and
without self-unregistering; update onDetach to first call
wc.debugger.off("detach", onDetach) and then check that rec =
attached.get(surface) has rec?.win.webContents === wc before calling
attached.delete(surface), and update CdpSession.detach to only call
attached.delete(surface) when rec?.win.webContents === wc (not just existence),
ensuring the listener is removed in the detach flow as well.
- Around line 49-51: The cached entry from attached.get(surface) may belong to a
different WebContents; before reusing it you must verify binding equality by
checking existing.win.webContents === wc in the reuse condition. Update the if
that currently checks existing && !existing.win.isDestroyed() &&
wc.debugger.isAttached() to also ensure existing.win.webContents === wc so
buildSession(existing.surface, wc) only returns a session bound to the same
webContents (symbols: attached, surface, existing, existing.win.webContents, wc,
buildSession).
In `@src/server.ts`:
- Around line 154-156: The shutdown race is caused by accepting new sessions
after stop() snapshots/clears sessions; reintroduce and use the shuttingDown
latch so no new transport/session can be minted after shutdown begins.
Specifically: set shuttingDown = true at the start of stop(), check shuttingDown
in routeRequest (and in createSession/transport-minting code paths) and return a
503 when true, and additionally ensure the createServer handler (where
routeRequest(req, res, sessions, createSession, path) is called) consults that
same shuttingDown latch before allowing routeRequest to mint a session. Update
functions named stop, routeRequest, and any createSession/transport creation
paths to consult the shuttingDown flag so post-snapshot sessions are rejected
and httpServer.close() can complete.
In `@src/surfaces.ts`:
- Around line 25-26: The code reads candidate windows from getSurfaces() using
the provided surface key without guarding for own properties, so keys like
"__proto__" or "constructor" can return inherited values and cause
win.isDestroyed() to throw; update the lookup in the resolution path that uses
getSurfaces()[surface] (and subsequent uses of win and win.isDestroyed()) to
first check Object.prototype.hasOwnProperty.call(getSurfaces(), surface) and
only then read the value, and if the key is not an own property or the value is
missing/invalid throw or return the existing SurfaceNotFoundError instead of
calling isDestroyed() on inherited prototypes.
In `@src/testing.ts`:
- Around line 170-189: The fake debugger currently allows sendCommand() to
succeed when detached and ignores listener management; update the debugger mock
so attach()/detach() toggle the attached boolean and detach() clears any
registered listeners, make sendCommand(method, params) throw an Error (or
reject) when attached is false, and implement on(event, handler) and off(event,
handler) to actually register and deregister handlers in a local listeners map
(e.g., Map<string, Set<Function>>) so tests can verify listener lifecycle;
ensure detach() also removes or invokes appropriate detach-related listeners and
that cdpCalls/cdpResponses usage in sendCommand remains unchanged except for the
detached check.
In `@src/tools/ax-snapshot.test.ts`:
- Around line 52-58: Assertion uses the wrong key (id) while the mocked AX nodes
and formatter expect nodeId; update the test assertions that inspect
result.structuredContent.nodes (specifically the expect(...).toMatchObject call)
to assert nodeId: "1" instead of id: "1" and keep the children: [] check so it
matches the formatter/mocked node shape.
In `@src/tools/list-surfaces.test.ts`:
- Around line 10-16: The test for list_surfaces currently asserts inputSchema is
undefined and calls the handler with an empty object; update it so the test
enforces the required surface contract: when registering with
registerListSurfaces and retrieving the tool via getTool(server,
"list_surfaces"), assert the tool.config.inputSchema requires a surface: string
(e.g., verify it has a `surface` property of type string) instead of being
undefined, and invoke the tool handler with an explicit surface value (e.g.,
handler({ surface: "some-surface" })) so the test reflects the rule that every
tool must accept a surface string.
---
Outside diff comments:
In `@src/server.ts`:
- Around line 247-264: The response currently calls res.end(...) and then
immediately req.socket?.destroy() which can abort the flush; change this so the
socket is destroyed in the res.end callback: when handling parsed.reason ===
"too-large" (the branch that sets 413 and the JSON-RPC error mentioning
MAX_BODY_BYTES), pass a callback to res.end that calls req.socket?.destroy(),
ensuring the body is flushed before the socket is torn down.
---
Nitpick comments:
In `@package.json`:
- Around line 56-72: The package "async-mutex" is declared in both
devDependencies and dependencies; since it is imported at runtime (see
src/index.ts), remove the duplicate from "devDependencies" and keep a single
entry under "dependencies" to prevent version drift—update the package.json by
deleting the "async-mutex" line inside the "devDependencies" object and ensure
the existing "dependencies" entry remains the authoritative one.
In `@src/tools/evaluate.ts`:
- Around line 42-57: The interface RuntimeEvaluateResult declares
result.unserializableValue but the formatting logic never reads it; remove the
unused unserializableValue property from RuntimeEvaluateResult (and any
duplicate occurrences around the other block at 107-113) to keep the type
accurate, or alternatively add a one-line comment above RuntimeEvaluateResult
explaining why unserializableValue must remain if you choose to keep it; update
only the interface and its duplicates
(RuntimeEvaluateResult/result.unserializableValue) without changing the runtime
formatting logic.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9bfb4b77-1784-4a92-b432-e63f9cf5393b
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!pnpm-lock.yaml
📒 Files selected for processing (63)
.changeset/config.json.changeset/initial-release.md.github/ISSUE_TEMPLATE/bug_report.yml.github/ISSUE_TEMPLATE/feature_request.yml.github/pull_request_template.md.github/workflows/ci.yml.github/workflows/release.yml.gitignoreCODE_OF_CONDUCT.mdCONTRIBUTING.mdREADME.mdSECURITY.mdbiome.jsondocs/repo-setup.mdpackage.jsonsrc/cdp-helpers.tssrc/cdp.tssrc/index.test.tssrc/index.tssrc/server.tssrc/surfaces.tssrc/testing.tssrc/tool-def.tssrc/tools/ax-snapshot.test.tssrc/tools/ax-snapshot.tssrc/tools/click.test.tssrc/tools/click.tssrc/tools/evaluate.test.tssrc/tools/evaluate.tssrc/tools/fill-form.test.tssrc/tools/fill-form.tssrc/tools/focus-surface.test.tssrc/tools/focus-surface.tssrc/tools/hide-surface.test.tssrc/tools/hide-surface.tssrc/tools/hover.test.tssrc/tools/hover.tssrc/tools/index.tssrc/tools/list-surfaces.test.tssrc/tools/list-surfaces.tssrc/tools/press-key.test.tssrc/tools/press-key.tssrc/tools/query-dom.test.tssrc/tools/query-dom.tssrc/tools/reload-surface.test.tssrc/tools/reload-surface.tssrc/tools/screenshot.test.tssrc/tools/screenshot.tssrc/tools/show-surface.test.tssrc/tools/show-surface.tssrc/tools/type-text.test.tssrc/tools/type-text.tssrc/tools/wait-for-load.test.tssrc/tools/wait-for-load.tssrc/types.tstest/electron/fixture/main.mjstest/electron/globals.d.tstest/electron/playwright.config.tstest/electron/smoke.spec.tstests/smoke.electron.test.tstsconfig.jsontsdown.config.tsvitest.config.ts
💤 Files with no reviewable changes (1)
- .gitignore
Critical correctness fixes:
- src/cdp.ts: add binding-equality guards (existing.win.webContents === wc)
before reusing or deleting cache entries; self-unregister the detach
listener inside onDetach so a stale listener can't fire after a new
attach replaces the cache entry for the same surface key.
- src/server.ts: reintroduce a `shuttingDown` latch that routeRequest
consults and rejects post-stop initialize requests with 503 — without
it, a racing initialize after the sessions snapshot creates an orphan
transport that hangs httpServer.close().
- src/surfaces.ts: own-property guard so lookups like "__proto__" or
"constructor" surface SurfaceNotFoundError instead of throwing a raw
TypeError on a prototype value.
Test infrastructure:
- src/testing.ts: fake debugger now (a) fails sendCommand when not
attached and (b) actually tracks debugger.on/off listeners, plus
emitDebugger/debuggerListenerCount escape hatches so tests can
exercise CdpSession listener-cleanup paths instead of silently
passing on tools that skip getOrAttachSession.
Hardening / housekeeping:
- .github/workflows/{ci,release}.yml: pin actions/checkout,
pnpm/action-setup, actions/setup-node, and changesets/action to
full commit SHAs (Dependabot's github-actions ecosystem will keep
them current).
- src/server.ts: destroy the 413 socket from the res.end callback so
the JSON-RPC error body flushes before the connection tears down.
- src/tools/evaluate.ts: drop unused `unserializableValue` from
RuntimeEvaluateResult.
- package.json: keep async-mutex only in `dependencies` (it was also
listed under devDependencies, risking version drift).
- CONTRIBUTING.md: clarify that test/electron/smoke.spec.ts is the
canonical Electron smoke test and tests/smoke.electron.test.ts is
an unwired exploratory fixture.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Member
Author
|
@coderabbitai — also addressed the items from the review body in 391daab:
Two findings I pushed back on (replies on the inline threads):
|
|
Tip For best results, initiate chat on the files or code changes.
On the pushbacks — both explanations make sense and I've noted them as learnings:
✏️ Learnings added
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/server.ts (1)
138-140:⚠️ Potential issue | 🔴 CriticalShutdown race still allows post-stop session minting.
Line [253] checks
isShuttingDown()once, but an in-flightinitializecan still reach Line [336] afterstop()flips the latch and snapshots sessions. That can mint a post-snapshot session and blockhttpServer.close().Suggested fix
@@ const transport = new StreamableHTTPServerTransport({ sessionIdGenerator: () => randomUUID(), onsessioninitialized: (sid) => { + if (shuttingDown) { + void transport.close().catch(() => {}); + return; + } sessions.set(sid, { transport, server: sessionServer }); }, }); @@ if (isInitializeRequest(body)) { - const { transport } = await createSession(); + if (isShuttingDown()) { + res.statusCode = 503; + res.setHeader("content-type", "application/json"); + res.setHeader("connection", "close"); + res.end( + JSON.stringify({ + jsonrpc: "2.0", + error: { code: -32000, message: "Server shutting down" }, + id: null, + }), + ); + return; + } + const { transport, server } = await createSession(); + if (isShuttingDown()) { + await server.close().catch(() => {}); + res.statusCode = 503; + res.setHeader("content-type", "application/json"); + res.setHeader("connection", "close"); + res.end( + JSON.stringify({ + jsonrpc: "2.0", + error: { code: -32000, message: "Server shutting down" }, + id: null, + }), + ); + return; + } await transport.handleRequest(req, res, body); return; }As per coding guidelines, do not remove or weaken the
shuttingDownlatch / 503 path inrouteRequest; otherwise aninitializeracingstop()can create a post-snapshot session and stallhttpServer.close().Also applies to: 251-265, 333-338
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server.ts` around lines 138 - 140, The race allows an initialize/onsessioninitialized callback to add a session after stop() flipped the shuttingDown latch, so change onsessioninitialized (and the initialize path that leads to it) to re-check isShuttingDown/shuttingDown before mutating sessions: if shuttingDown is set, refuse to mint the session (e.g., close/cleanup transport and do not call sessions.set) and return an error/early-exit so routeRequest’s 503 semantics and httpServer.close() cannot be blocked; update initialize to perform the same post-await check before creating/attaching the session and ensure any session creation code paths (onsessioninitialized, initialize, and the code that snapshots sessions) all consult the same shuttingDown latch atomically.src/testing.ts (1)
195-197:⚠️ Potential issue | 🟠 Major
debugger.detach()never dispatches"detach"in the fake, so detach-cleanup paths are easy to miss in tests.At Line 195-197, detach only updates
attached. Since listeners are tracked (Line 175-186), not emitting"detach"means lifecycle cleanup wired to that event won’t run unless each test manually callsemitDebugger().Proposed fix
detach: vi.fn(() => { attached = false; + emitDebugger("detach"); }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/testing.ts` around lines 195 - 197, detach currently only flips the attached flag and never notifies registered listeners, so update the detach implementation (the detach fake) to both set attached = false and dispatch a "detach" event to the tracked listeners—use the same listener dispatch mechanism used elsewhere (referencing listeners and the existing emitDebugger or listener-invocation logic) so any lifecycle cleanup bound to "detach" runs automatically in tests.
🧹 Nitpick comments (2)
package.json (2)
66-66: Inconsistent version pinning for vitest.
vitestis pinned to exact version4.1.5without a caret (^), while all other devDependencies use caret notation for minor/patch flexibility. If this is intentional for reproducibility, consider adding a comment or aligning the approach across all dependencies.🔧 Suggested change for consistency
- "vitest": "4.1.5", + "vitest": "^4.1.5",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 66, The devDependency entry for "vitest" is pinned to "4.1.5" without a caret, inconsistent with other devDependencies; update the "vitest" entry to use caret semver (e.g., change "vitest": "4.1.5" to use a caret like "^4.1.5") to match the project's dependency style, or if the exact pin is intentional, add a clear inline comment near the "vitest" entry explaining why it must remain exact for reproducibility so reviewers understand the deviation.
51-55: Narrow Electron peer dependency range.The
electron: ">=41 <42"constraint only supports Electron 41.x. While this may be intentional given Electron's rapid release cycle and potential breaking changes (the PR mentions Electron 41 rejecting--remote-debugging-port=0), this tight constraint will require a new package release for each Electron major version.Consider documenting this constraint rationale in the README or CONTRIBUTING.md so consumers understand the compatibility expectations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 51 - 55, The peerDependency for Electron ("electron": ">=41 <42") is overly narrow and will force a package bump for each Electron major; either broaden the allowed range in package.json (for example change the "electron" entry from ">=41 <42" to a suitable wider range like ">=41 <43" or a policy you choose) OR, if the tight constraint is intentional, add a short explanation in your README or CONTRIBUTING.md that references the package.json peerDependencies electron pin and explains the compatibility rationale (e.g., Electron 41 rejects --remote-debugging-port=0), so consumers understand why only 41.x is allowed.
🤖 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/testing.ts`:
- Around line 165-169: The emit implementation currently invokes onceListeners
then clears them, which breaks Node.js semantics; change emit(event, ...args) to
take snapshots of listeners and onceListeners (e.g., const normal =
[...(listeners.get(event) ?? [])]; const once = [...(onceListeners.get(event) ??
[])]), clear the onceListeners for the event before invoking (e.g.,
onceListeners.get(event)?.clear() or onceListeners.set(event, new Set())), then
iterate and call handlers from the snapshots (for const l of normal) and (for
const l of once) so once-handlers are unregistered before they run and handlers
added during invocation aren’t invoked in the same emit; use the existing emit,
listeners, and onceListeners symbols to locate and apply this change.
---
Duplicate comments:
In `@src/server.ts`:
- Around line 138-140: The race allows an initialize/onsessioninitialized
callback to add a session after stop() flipped the shuttingDown latch, so change
onsessioninitialized (and the initialize path that leads to it) to re-check
isShuttingDown/shuttingDown before mutating sessions: if shuttingDown is set,
refuse to mint the session (e.g., close/cleanup transport and do not call
sessions.set) and return an error/early-exit so routeRequest’s 503 semantics and
httpServer.close() cannot be blocked; update initialize to perform the same
post-await check before creating/attaching the session and ensure any session
creation code paths (onsessioninitialized, initialize, and the code that
snapshots sessions) all consult the same shuttingDown latch atomically.
In `@src/testing.ts`:
- Around line 195-197: detach currently only flips the attached flag and never
notifies registered listeners, so update the detach implementation (the detach
fake) to both set attached = false and dispatch a "detach" event to the tracked
listeners—use the same listener dispatch mechanism used elsewhere (referencing
listeners and the existing emitDebugger or listener-invocation logic) so any
lifecycle cleanup bound to "detach" runs automatically in tests.
---
Nitpick comments:
In `@package.json`:
- Line 66: The devDependency entry for "vitest" is pinned to "4.1.5" without a
caret, inconsistent with other devDependencies; update the "vitest" entry to use
caret semver (e.g., change "vitest": "4.1.5" to use a caret like "^4.1.5") to
match the project's dependency style, or if the exact pin is intentional, add a
clear inline comment near the "vitest" entry explaining why it must remain exact
for reproducibility so reviewers understand the deviation.
- Around line 51-55: The peerDependency for Electron ("electron": ">=41 <42") is
overly narrow and will force a package bump for each Electron major; either
broaden the allowed range in package.json (for example change the "electron"
entry from ">=41 <42" to a suitable wider range like ">=41 <43" or a policy you
choose) OR, if the tight constraint is intentional, add a short explanation in
your README or CONTRIBUTING.md that references the package.json peerDependencies
electron pin and explains the compatibility rationale (e.g., Electron 41 rejects
--remote-debugging-port=0), so consumers understand why only 41.x is allowed.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e520ffec-31e0-4215-ba6b-5b84b0af7669
📒 Files selected for processing (9)
.github/workflows/ci.yml.github/workflows/release.ymlCONTRIBUTING.mdpackage.jsonsrc/cdp.tssrc/server.tssrc/surfaces.tssrc/testing.tssrc/tools/evaluate.ts
✅ Files skipped from review due to trivial changes (3)
- CONTRIBUTING.md
- src/surfaces.ts
- .github/workflows/ci.yml
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/release.yml
- src/tools/evaluate.ts
- src/cdp.ts
The fake emit() invoked once-listeners and then cleared them, which diverges from Node.js EventEmitter semantics that unregister once-listeners *before* invoking the handler. If a handler called emit() recursively or added a new once-listener mid-flight, the fake could fire a stale once-listener or include a freshly-added listener in the same emit batch. Snapshot listeners + once-listeners first, delete the once bucket, then iterate — matches the real EventEmitter contract that src/tools/wait-for-load.ts relies on. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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
@nebula-agents/electron-mcppackage with explicit lifecycle, generic surface keys, bundled renderer automation tools, and public type exports._electronsmoke fixture forlist_surfaces,evaluate, andscreenshot.Verification
pnpm checkpnpm test:electronif Electron/CDP behavior changedNotes:
pnpm test:electroncurrently skips on macOS because Playwright_electronpasses--remote-debugging-port=0, which Electron 41 rejects on Darwin. The smoke test remains enabled for Linux CI.Closes
Summary by CodeRabbit
New Features
Documentation
API
Testing & CI/CD
Chores