feat(desktop): 独立パッケージの browser-pane MCP と PID ベース自動バインド#354
feat(desktop): 独立パッケージの browser-pane MCP と PID ベース自動バインド#354
Conversation
Adds a standalone `@superset/superset-browser-mcp` package plus the
Electron app-side HTTP bridge so a Claude / Codex session can drive any
browser pane the user binds to it — set up once, no per-pane command.
### New package `packages/superset-browser-mcp`
- stdio MCP server (`@modelcontextprotocol/sdk`)
- Reads ~/.superset/browser-mcp.json for the app's loopback port +
per-app secret
- Tools: get_connected_pane / navigate / screenshot / evaluate_js /
get_console_logs
- Every request includes `x-superset-mcp-ppid`, the MCP's own
process.ppid — the Superset app uses that to auto-resolve which LLM
session is calling
### App-side bridge `apps/desktop/src/main/lib/browser-mcp-bridge`
- HTTP listener on 127.0.0.1:<random>, auth via shared secret
- Writes ~/.superset/browser-mcp.json on startup
- resolvePpidToSession():
- todo-agent: registered PID from supervisor-engine (new pid-registry
module), see supervisor-engine onChild wiring
- terminal pane: walks each PTY's process tree for the PPID
- ensureDebuggerAttached(paneId): Electron webContents.debugger via CDP
1.3, Page/Runtime/Log enabled, buffers console output
### UI / router wiring
- browserAutomation.getMcpStatus now returns `serverCommand` resolving
to `bun run <repo>/packages/superset-browser-mcp/src/bin.ts` in dev
(falls back to `bunx @superset/superset-browser-mcp` for published
installs). The Connect modal's setup snippet is generated from that
exact argv so copy-pasting produces a working `claude mcp add`.
- stores/browser-automation: getSnippetForSession takes the server
argv; TOML-quotes / shell-quotes it properly.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 38 minutes and 12 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughウォークスルーElectron アプリケーションに MCP (Model Context Protocol) ブリッジサーバーを導入し、ローカル HTTP サーバーを経由して Claude/Codex と UI ペーンの連携を実現しました。新しい 変更内容
シーケンス図sequenceDiagram
participant Claude as Claude/Codex
participant MCP as superset-browser-mcp<br/>(CLI)
participant Bridge as MCP Bridge Server<br/>(Electron)
participant TerminalHost as Terminal Host
participant CDP as Chromium DevTools<br/>Protocol
Claude->>MCP: spawn (with PPID)
MCP->>Bridge: POST /mcp/register<br/>(x-superset-mcp-ppid)
activate Bridge
Bridge->>Bridge: cache PPID registration
Bridge-->>MCP: ok: true
deactivate Bridge
Claude->>MCP: call MCP tool<br/>(navigate, screenshot, etc.)
MCP->>Bridge: POST /mcp/binding<br/>(ppid header)
activate Bridge
Bridge->>Bridge: resolvePpidToSession(ppid)<br/>→ paneId
Bridge->>TerminalHost: listSessions()
TerminalHost-->>Bridge: session list
Bridge->>Bridge: walkPTY tree to find ppid match
Bridge-->>MCP: bound=true, paneUrl, title
deactivate Bridge
MCP->>Bridge: POST /mcp/navigate<br/>(url, ppid)
activate Bridge
Bridge->>Bridge: resolve pane via ppid
Bridge->>CDP: attach debugger to pane
Bridge->>CDP: Page.navigate(url)
CDP-->>Bridge: navigation complete
Bridge-->>MCP: paneId, url
deactivate Bridge
MCP->>Bridge: POST /mcp/screenshot<br/>(ppid)
activate Bridge
Bridge->>Bridge: resolve pane via ppid
Bridge->>CDP: Page.captureScreenshot()
CDP-->>Bridge: base64 PNG
Bridge-->>MCP: base64, mimeType
deactivate Bridge
推定コードレビュー難度🎯 4 (複雑) | ⏱️ ~60 分 関連する可能性のあるプルリクエスト
詩
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ed106f84e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…b snippet - Bridge runtime file moves from ~/.superset/browser-mcp.json to $SUPERSET_HOME_DIR/browser-mcp.json. Propagate SUPERSET_HOME_DIR into terminal-session env so claude/codex spawned inside a Superset terminal read the correct workspace-scoped file. Multiple Superset instances with different SUPERSET_WORKSPACE_NAME values no longer overwrite each other's port/secret. (codex P2) - SetupPanel: if serverCommand.available is false (packaged build before @superset/superset-browser-mcp ships on npm) show an explanatory placeholder instead of a bunx command that cannot start the server. (codex P1)
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b600fcd39
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
resolvePpidToSession used to cache 'null' for the TTL, so a transient listSessions failure or a brief race between MCP spawn and todo-agent PID registration would lock the MCP out for 5s with 'No browser pane bound'. Only cache positive resolutions. (codex P2)
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f84dfa6124
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
BridgeClient.request called this.load() outside the try block, so when browser-mcp.json was missing (MCP starting before Superset writes it) or malformed (wrong SUPERSET_HOME_DIR) the method threw raw ENOENT / JSON parse errors instead of BridgeUnavailableError. Move the load inside the retry scope so both the initial load and the post-reset retry surface the friendly error. (codex P2)
|
@codex review |
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/SessionConnectModal/SessionConnectModal.tsx (1)
543-547:⚠️ Potential issue | 🟡 Minorセットアップ説明の旧パッケージ名を更新してください。
この PR では
packages/superset-browser-mcp/@superset/superset-browser-mcpに移行しているため、packages/desktop-mcpと「文字列を検査」という説明がユーザーに古い案内を出します。文言修正案
- MCP readiness is detected by inspecting the config file for the string{" "} - <code>superset-browser</code>. If you prefer a managed location, the - desktop app also ships the server at <code>packages/desktop-mcp</code> - . + MCP readiness is detected by inspecting the config file for an enabled{" "} + <code>superset-browser</code> MCP entry. The desktop bridge is exposed + through the standalone{" "} + <code>@superset/superset-browser-mcp</code> server.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/SessionConnectModal/SessionConnectModal.tsx` around lines 543 - 547, Update the user-facing text inside the SessionConnectModal component to reference the new package names and detection method: replace the displayed package path "packages/desktop-mcp" with "packages/superset-browser-mcp" (and mention the npm scope "@superset/superset-browser-mcp" where appropriate) and reword the MCP readiness line so it says readiness is detected by checking the app config/package list for "@superset/superset-browser-mcp" (instead of the old "inspecting the config file for the string" wording); locate this text in SessionConnectModal.tsx (the div with className "mt-2 text-[10px] text-muted-foreground leading-relaxed") and update the code fragments (<code>...</code>) and surrounding sentence accordingly.
🧹 Nitpick comments (2)
packages/superset-browser-mcp/package.json (1)
16-16:typecheckスクリプトのフラグが冗長です。
--noEmitを指定している時点で宣言ファイル出力も行われないため、--emitDeclarationOnly falseは無意味です。単にtsc --noEmitで十分です。- "typecheck": "tsc --noEmit --emitDeclarationOnly false" + "typecheck": "tsc --noEmit"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/superset-browser-mcp/package.json` at line 16, Update the "typecheck" npm script entry that currently runs "tsc --noEmit --emitDeclarationOnly false" to remove the redundant flag; change it to simply run "tsc --noEmit" so declaration emission flags are not contradictory and the script is concise (update the package.json "typecheck" script value accordingly).packages/superset-browser-mcp/src/bin.ts (1)
25-27: 登録失敗を完全に握りつぶすと診断が困難になります。
/mcp/registerの失敗をコメント通りに握りつぶすと、bridge 未起動・認証秘密鍵ミスマッチ・ポート衝突などが最初のツールコール時まで見えません。stderr へデバッグレベルで原因を出しておくと、claude --debugやcodex側ログから追跡できます(MCP プロトコルの stdout を汚さない限り、stderr ログは問題ありません)。-client.request("POST", "/mcp/register", { ppid }).catch(() => { - /* ignore — the first tool call will surface a clearer error */ -}); +client.request("POST", "/mcp/register", { ppid }).catch((err) => { + if (process.env.SUPERSET_MCP_DEBUG) { + console.error("[superset-browser-mcp] register failed:", err); + } +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/superset-browser-mcp/src/bin.ts` around lines 25 - 27, The catch on client.request("POST", "/mcp/register", { ppid }) is swallowing errors; change the catch to log the error to stderr at debug level (include the error object and a clear context message like "MCP register failed" and the ppid) instead of ignoring it so startup failures (bridge not running, auth key mismatch, port conflicts) are visible; update the anonymous catch handler for the client.request call in this file to print the error (e.g., via console.error or process.stderr.write with a descriptive message and the error stack) while still allowing the process to continue.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/lib/browser-mcp-bridge/pane-resolver.ts`:
- Around line 46-60: The loop over sessions can throw if getProcessTree or
getProcessName fails, causing the whole request to 500; wrap the per-session
work in a try/catch so a failure for one pane is swallowed and the loop
continues to the next session (i.e., put getProcessTree(...) and
getProcessName(...) and the subsequent checks inside a try block and on catch
simply continue), optionally log the error for that pane; keep the existing
return behavior when a valid agent-like process is found (the object with
sessionId/kind/paneId).
In `@apps/desktop/src/main/lib/browser-mcp-bridge/server.ts`:
- Line 2: writeFileSync(..., { mode: 0o600 }) only sets mode on newly created
files; fix code that writes the bridge/runtime secret by importing chmodSync
from "node:fs" and calling chmodSync(targetPath, 0o600) immediately after
writeFileSync to enforce 0600 on existing files (apply the same fix for the
other writeFileSync occurrence around the runtime file handling); ensure you
reference the existing mkdirSync/writeFileSync usage and add chmodSync calls to
correct permissions after writes.
- Around line 190-198: The POST /mcp/navigate handler currently forwards
whatever URL the client provides into wc.debugger.sendCommand("Page.navigate",
...) which is unsafe; add server-side allowlist/validation before calling
Page.navigate: after reading body via readJson and before
ensureDebuggerAttached/Page.navigate, validate body.url using a whitelist of
allowed schemes (e.g., http, https) and reject disallowed schemes (file:,
javascript:, custom protocols) returning send(res, 400, { error: "invalid or
disallowed url" }); ensure the check is applied inside this handler (referencing
resolvePaneFromRequest, readJson, ensureDebuggerAttached, and
wc.debugger.sendCommand("Page.navigate")) so no unvalidated URL reaches the
debugger.
- Around line 56-83: The CDP listeners registered via wc.debugger.on("message",
...) and wc.debugger.on("detach", ...) are never removed, causing duplicated
handlers on re-attach; fix by extracting the anonymous callbacks into named
functions (e.g., messageHandler and detachHandler) that capture paneId, register
those handlers when attaching, and explicitly remove them on detach using
wc.debugger.removeListener/ off (call wc.debugger.removeListener("message",
messageHandler) and wc.debugger.removeListener("detach", detachHandler)) before
deleting paneId from attachedPanes; ensure attachedPanes.add(paneId) happens
after listeners are registered so state remains consistent.
In `@apps/desktop/src/main/todo-daemon/supervisor-engine.ts`:
- Around line 431-449: The current dynamic-import-then-register pattern can race
with a very short-lived child: attach the child.once("exit", ...) listener
synchronously before any async import/resolution, track a local "exited" boolean
in the scope of the spawn handler (set it inside the exit listener), then after
the static/dynamic import resolves call registerTodoAgentWorker only if the
child PID is valid and the "exited" flag is false; always call
unregisterTodoAgentWorker from the synchronous exit handler (import the
pid-registry module statically at top-level or at least kick off the import
immediately) so unregister cannot be missed — update references to
registerTodoAgentWorker and unregisterTodoAgentWorker accordingly and remove the
register/unregister logic from inside the same .then() race window.
In `@packages/superset-browser-mcp/package.json`:
- Around line 6-14: The package.json "bin" currently points to a TypeScript
source file ("./src/bin.ts"), which will break when publishing; update the
package.json "bin" entry to point to the compiled JS output (e.g.,
"./dist/bin.js") and add a npm-publish checklist/task (and a short note in
README or CONTRIBUTING) to ensure builds produce the JS before publishing;
verify resolveSupersetBrowserMcpCommand() and SessionConnectModal continue to
use the runtime availability flag as-is (no UI change needed) and ensure your
build step outputs the JS file referenced by the updated "bin" entry.
In `@packages/superset-browser-mcp/src/bin.ts`:
- Around line 1-10: The CLI entry currently points at a .ts file with a Node
shebang so it won't run under plain Node; update the UX by adding a small
runtime guard at the top of the binary (in the current bin module) that detects
the environment (e.g., check for global BUN_VERSION or process.release.name !==
'node' / presence of Bun) and prints a clear, actionable error message advising
the user to run via bun (e.g., "This CLI requires Bun: use `bun run` or install
Bun and ensure it is on PATH"), and also update your package.json#bin handling
plan by documenting that published packages must point to a transpiled JS entry
(or swap the bin to a compiled file during publish) so npm-installed
environments won't break; reference the shebang in the current bin file and
package.json#bin in your changes.
In `@packages/superset-browser-mcp/src/tools/index.ts`:
- Around line 40-52: The user-visible copy currently hardcodes "Claude session";
update the description string and the unbound message text to use a neutral term
like "LLM session" (or "AI/LLM session") so this MCP works for Codex too: edit
the description property and the returned content text that now reads "Claude
session" to instead say "LLM session" (also update the similar strings
referenced at the other occurrence around the same block, e.g., the unbound
message and any other "Claude session" literals in this module).
In `@packages/superset-browser-mcp/src/transport/bridge-client.ts`:
- Around line 65-74: The fetch in the perform function (in bridge-client.ts) can
hang indefinitely; add a timeout using AbortController: create an
AbortController inside perform, pass controller.signal to fetch, and start a
setTimeout that calls controller.abort() after a sensible default (or a
configurable value) to cancel the request; ensure the timeout is cleared when
fetch completes (success or error). Keep existing headers/body logic and
this.ppid usage, and surface/throw a clear timeout/abort error so callers of
perform can handle it.
- Around line 23-29: readRuntimeInfo currently only checks types loosely,
allowing NaN, out-of-range ports, or empty secrets; update readRuntimeInfo to
strictly validate the parsed RuntimeInfo (from runtimeInfoPath) by ensuring
parsed.port is a finite integer and within TCP port range (1–65535) and
parsed.secret is a non-empty string (trimmed length > 0); if any check fails,
throw a descriptive Error including runtimeInfoPath() and which field is invalid
so callers can fail fast and debug broken runtime files.
---
Outside diff comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/SessionConnectModal/SessionConnectModal.tsx`:
- Around line 543-547: Update the user-facing text inside the
SessionConnectModal component to reference the new package names and detection
method: replace the displayed package path "packages/desktop-mcp" with
"packages/superset-browser-mcp" (and mention the npm scope
"@superset/superset-browser-mcp" where appropriate) and reword the MCP readiness
line so it says readiness is detected by checking the app config/package list
for "@superset/superset-browser-mcp" (instead of the old "inspecting the config
file for the string" wording); locate this text in SessionConnectModal.tsx (the
div with className "mt-2 text-[10px] text-muted-foreground leading-relaxed") and
update the code fragments (<code>...</code>) and surrounding sentence
accordingly.
---
Nitpick comments:
In `@packages/superset-browser-mcp/package.json`:
- Line 16: Update the "typecheck" npm script entry that currently runs "tsc
--noEmit --emitDeclarationOnly false" to remove the redundant flag; change it to
simply run "tsc --noEmit" so declaration emission flags are not contradictory
and the script is concise (update the package.json "typecheck" script value
accordingly).
In `@packages/superset-browser-mcp/src/bin.ts`:
- Around line 25-27: The catch on client.request("POST", "/mcp/register", { ppid
}) is swallowing errors; change the catch to log the error to stderr at debug
level (include the error object and a clear context message like "MCP register
failed" and the ppid) instead of ignoring it so startup failures (bridge not
running, auth key mismatch, port conflicts) are visible; update the anonymous
catch handler for the client.request call in this file to print the error (e.g.,
via console.error or process.stderr.write with a descriptive message and the
error stack) while still allowing the process to continue.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 979052d9-8fa1-48d5-94c0-17c58316b86a
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
apps/desktop/src/lib/trpc/routers/browser-automation/index.tsapps/desktop/src/main/index.tsapps/desktop/src/main/lib/browser-mcp-bridge/pane-resolver.tsapps/desktop/src/main/lib/browser-mcp-bridge/pid-registry.tsapps/desktop/src/main/lib/browser-mcp-bridge/server.tsapps/desktop/src/main/lib/terminal/env-terminal.tsapps/desktop/src/main/todo-daemon/supervisor-engine.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/SessionConnectModal/SessionConnectModal.tsxapps/desktop/src/renderer/stores/browser-automation.tspackages/superset-browser-mcp/package.jsonpackages/superset-browser-mcp/src/bin.tspackages/superset-browser-mcp/src/index.tspackages/superset-browser-mcp/src/tools/index.tspackages/superset-browser-mcp/src/transport/bridge-client.tspackages/superset-browser-mcp/tsconfig.json
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43d3b732a9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- supervisor-engine: drop the pid-registry wiring. The todo-daemon runs
in a separate process from main, so an in-process map never reaches
the bridge. TODO-agent PID matching will come through daemon-bridge
IPC in a follow-up. (codex P1, CodeRabbit Major)
- pane-resolver: try/catch per pane so a single PTY race failing its
process-tree lookup doesn't take down the whole MCP resolution.
Positive results still cache; negatives do not. (CodeRabbit Major)
- browser-mcp-bridge/server:
- chmodSync(0o600) after write so an existing runtime file can't
stay world-readable. (CodeRabbit Major)
- validateNavigateUrl: allowlist http(s) only, reject file:, js:,
about:, etc. (CodeRabbit Major)
- CDP debugger message/detach listeners are now off()'d on detach so
re-attaching the same pane doesn't double-fire console events.
(CodeRabbit Minor)
- superset-browser-mcp bridge client:
- 15s fetch timeout via AbortController; a hung port no longer stalls
MCP tool calls forever. (CodeRabbit Major)
- Strict runtime-info validation: port is 1..65535 integer, secret is
non-empty string. (CodeRabbit Minor)
- tools/index: wording uses "LLM session" instead of "Claude session"
so Codex sessions read naturally. (CodeRabbit Minor)
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc1a7291c5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…orts them browser-mcp pane-resolver cannot map MCP PIDs to TODO-Agent workers (they run in the separate todo-daemon process). The UI was still offering TODO-Agent sessions in the Connect list, and setBinding accepted them — both led to silently-broken bindings. - useBrowserAutomationData: drop the todoAgent.listAll query and the todo-agent row mapping. Only terminal-derived sessions are offered. - setBinding: reject sessionKind="todo-agent" with an explanatory error. Default sessionKind flipped to "terminal". (codex P1)
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9efe4cf340
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A Superset restart can keep the same loopback port but rotate the shared secret, in which case BridgeClient kept sending the stale Bearer token and got 401 forever. On auth failure, drop the cached runtime info and retry with the freshly-read file once. (codex P2)
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd71dad6b1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Older builds allowed todo-agent bindings to be saved. Now that the Connect flow hides those sessions, a stored todo-agent binding would show 'Connected' on the toolbar even though the MCP bridge cannot reach it. Drop todo-agent rows from the bindings table at the top of listBindingLiveness, and always report live=false for any non-terminal binding. (codex P2)
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87d0d640bf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Makes Superset-browser MCP installable from the app UI with a single
button press instead of copy-pasting a snippet, and makes the bundled
binary the canonical install so readiness detection only trusts entries
pointing at it.
### Bundle
- packages/superset-browser-mcp/package.json: `bun build --compile`
produces a single executable under `dist/superset-browser-mcp`.
- apps/desktop/package.json: `prebuild` compiles the MCP before
electron-builder runs.
- apps/desktop/electron-builder.ts: extraResources ships the binary
into `<app>/Contents/Resources/superset-browser-mcp/`.
- resolveSupersetBrowserMcpCommand returns that absolute path in
packaged builds, `bun run <repo>/src/bin.ts` in dev.
### Installer (tRPC)
- browserAutomation.getMcpInstallState: probes claude / codex CLIs,
reads their current superset-browser registration, compares command +
args against the bundled bin to decide cliFound / installed /
matchesExpected.
- browserAutomation.installMcp({targets}): idempotent `claude mcp
remove + add` / `codex mcp remove + add` against each requested
runtime. Remove-first makes reinstalling correct stale paths without
blowing up when the entry is already there.
### UI
- New McpInstallPanel replaces the old snippet view. Checkboxes for
Claude Code / Codex disabled when the CLI is missing, per-runtime
status line (✓ installed / ⚠ stale command / not installed / CLI not
found), Install button runs the mutation, toast with restart
instructions on success.
### Readiness now command-aware (codex P1)
- isEnabledMcpEntry accepts an `expected` command and requires the
registered command + args to match. Legacy desktop-mcp entries are
no longer reported as ready, so the UI reliably surfaces the install
flow instead of enabling Connect against a broken registration.
- detectClaudeMcp / detectCodexMcp take the same expected argument and
getMcpStatus passes its own resolved serverCommand through.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf9b9ed790
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/desktop/src/renderer/hooks/useBrowserAutomationData.ts (1)
10-24:⚠️ Potential issue | 🟡 Minor関数上部の docblock が実装とずれています。
コメントには依然として「Sessions come from two sources: 1. TODO-Agent sessions …」と書かれていますが、実装は既に 56-62 行のコメント通り TODO-Agent を除外しターミナルのみを返します。メンテナの混乱を避けるため、docblock を更新してください。
♻️ 提案
-/** - * Aggregates browser-automation session and binding data from the - * main-process tRPC routers. Sessions come from two sources: - * - * 1. TODO-Agent sessions (Claude Code workers the supervisor is running). - * 2. Terminal panes that currently have a `claude` or `codex` child - * process. The binding key for this kind is the terminal's paneId so - * that it self-heals across shell re-spawns inside the same pane. +/** + * Aggregates browser-automation session and binding data from the + * main-process tRPC routers. Sessions are currently sourced only from + * terminal panes that have a `claude` or `codex` child process. The + * binding key for this kind is the terminal's paneId so that it + * self-heals across shell re-spawns inside the same pane. TODO-Agent + * sessions are intentionally hidden until the bridge can resolve their + * PIDs across the daemon boundary. * * MCP readiness is resolved against the user's Claude/Codex config files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/hooks/useBrowserAutomationData.ts` around lines 10 - 24, Docblock for useBrowserAutomationData is out of sync: remove the outdated "TODO-Agent sessions" description and update the top comment to state that sessions are now only sourced from terminal panes with a `claude` or `codex` child process (binding key = terminal paneId), keep the note that MCP readiness is resolved against the user's Claude/Codex config files, and mention that `enabled` still controls the expensive queries while the binding query/subscription always run; update the text referenced around the old 56-62 comment to match the actual behavior implemented in the hook.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/SessionConnectModal/SessionConnectModal.tsx (1)
148-160: 🛠️ Refactor suggestion | 🟠 Major
handleCopySnippetとSetupPanelの未使用 props を整理してください。
SetupPanelは内部でMcpInstallPanelに差し替わり、session/mcpConfigPath/onCopyを受け取っても一切使っていません(455-463 行)。それに伴い、このファイルで:
handleCopySnippet(150-160 行)は誰にも渡されなくなるため丸ごとデッドコードgetSnippetForSessionの import(18 行)も未使用- 呼び出し側(246-254 行)で渡している
session/mcpConfigPath/onCopyも一緒に不要…となります。Biome の
noUnusedVariablesが無効になっていると型は通ってしまうので、手で削除しておく方が安全です。♻️ まとめての削除案
import { type AutomationSession, - getSnippetForSession, type ServerCommand, useBrowserAutomationStore, } from "renderer/stores/browser-automation";- const handleCopySnippet = async () => { - if (!session) return; - try { - await navigator.clipboard.writeText( - getSnippetForSession(session, serverCommand), - ); - toast.success("Configuration snippet copied"); - } catch { - toast.error("Failed to copy snippet"); - } - };- <SetupPanel - session={session} - mcpConfigPath={ - session.provider === "Codex" - ? (mcpStatus?.codexConfigPath ?? null) - : (mcpStatus?.claudeConfigPath ?? null) - } - serverCommand={serverCommand} - onCopy={handleCopySnippet} - /> + <SetupPanel serverCommand={serverCommand} />-function SetupPanel({ - serverCommand, -}: { - session: AutomationSession; - mcpConfigPath: string | null; - serverCommand?: ServerCommand; - onCopy: () => void; -}) { +function SetupPanel({ serverCommand }: { serverCommand?: ServerCommand }) { return <McpInstallPanel serverCommand={serverCommand} />; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/SessionConnectModal/SessionConnectModal.tsx` around lines 148 - 160, Remove the now-dead snippet-copying logic: delete the handleCopySnippet function and its import dependency getSnippetForSession, and remove the unused props session, mcpConfigPath, and onCopy from SetupPanel's props and from the call site where SetupPanel is instantiated; note that SetupPanel was replaced by McpInstallPanel internally so ensure you only keep props actually referenced by McpInstallPanel and remove any wiring that passed session/mcpConfigPath/onCopy to SetupPanel (and any toast calls tied to handleCopySnippet) so there are no unused variables or imports left (refer to handleCopySnippet, getSnippetForSession, SetupPanel, and McpInstallPanel).
🧹 Nitpick comments (2)
apps/desktop/src/main/lib/browser-mcp-bridge/server.ts (1)
329-340:will-quit後もcurrentが残り、2 回目のstartBrowserMcpBridge()がクローズ済みサーバーを返します。
will-quitでserver.close()を呼ぶだけでcurrent = nullにしていないため、startBrowserMcpBridge()をテストやリロードで再度呼ばれた際に先頭のif (current) return current;がリスン中でないハンドルを返し、接続できない状態が発生します。クローズ時とエラー時には singleton をクリアしてください。♻️ 修正案
app.on("will-quit", () => { - server.close(); + server.close(); + current = null; }); current = { port, secret, stop: () => new Promise<void>((resolve) => { - server.close(() => resolve()); + server.close(() => { + current = null; + resolve(); + }); }), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/browser-mcp-bridge/server.ts` around lines 329 - 340, The singleton `current` isn't cleared when the server is closed (e.g. in the app.on("will-quit") handler), causing subsequent calls to startBrowserMcpBridge() to return a stale closed handle; update the shutdown and error paths to set `current = null` whenever you call `server.close()` or hit server error—specifically modify the app.on("will-quit") callback, the server.close() completion callback used inside the `stop` method, and any server "error"/"close" listeners to clear `current` so startBrowserMcpBridge() can create a fresh server on next call.apps/desktop/src/lib/trpc/routers/browser-automation/index.ts (1)
551-574:sessionKindの enum から"todo-agent"を外す方がスキーマと挙動が整合します。
z.enum(["todo-agent", "terminal"]).default("terminal")で受け付けた後、mutation 内で"todo-agent"を投げ返す二重構造になっています。このパスは UI 側で既にsessionKind: "terminal"固定になっているため、enum 側からも外してしまえば無効入力を tRPC レイヤで即座に 400 化でき、後段のthrow new Error(...)に到達する経路を消せます(ロールアウト中のクライアント互換が不要なら)。♻️ 整理案
- sessionKind: z.enum(["todo-agent", "terminal"]).default("terminal"), + sessionKind: z.enum(["terminal"]).default("terminal"),後でバインドが対応したら enum を増やす運用の方がクライアント側の状態も追いやすいです。互換維持のため残したい場合はコメントにその旨を記載してください。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/browser-automation/index.ts` around lines 551 - 574, The schema currently accepts sessionKind values ["todo-agent","terminal"] but the mutation immediately rejects "todo-agent"; update the tRPC input for setBinding by removing "todo-agent" from the z.enum (use only "terminal" with default "terminal") so invalid inputs are 400-ed at the schema layer, and remove the runtime branch that throws for input.sessionKind === "todo-agent" (or if you must keep the enum for compatibility, add a clear comment on why it remains and keep the runtime guard). Ensure references: setBinding, input.sessionKind, z.enum([...]).default(...), and bindingStore.set are adjusted 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 `@apps/desktop/electron-builder.ts`:
- Around line 72-80: Update the extraResources entry that packages the
superset-browser-mcp binary so macOS code-signing and notarization succeed:
change the relative "from" to an absolute path (use join(__dirname,
"../../packages/superset-browser-mcp/dist") to avoid CWD breakage), ensure the
binary placed by that object is included in electron-builder's signing scope
(mac.signingIdentity) or is pre-signed before packaging, and add required
entitlements (e.g., mac.entitlements with com.apple.security.cs.allow-jit and
other JIT/runtime flags) while keeping hardenedRuntime: true and notarize: true
so the Mach-O produced by bun build --compile is accepted by Gatekeeper and the
Notary service.
In `@apps/desktop/src/lib/trpc/routers/browser-automation/index.ts`:
- Around line 400-436: Update resolveSupersetBrowserMcpCommand() to handle dev
vs preview path resolution: when !app.isPackaged, branch on NODE_ENV ===
"development" to keep the existing repoRoot/app.getAppPath() logic, add a
preview-mode branch that resolves the binPath from __dirname (e.g.
join(__dirname, "../../../packages/superset-browser-mcp/src/bin.ts")), and
implement a fallback sequence that checks multiple candidate paths
(app.getAppPath()-based, __dirname-based, and any other plausible repoRoot
variants) using existsSync; return the first existing path with available: true,
otherwise return the last attempted command/args with available: false. Ensure
references to binPath, repoRoot, and the returned command/args objects in
resolveSupersetBrowserMcpCommand() are updated accordingly.
In `@apps/desktop/src/main/lib/browser-mcp-bridge/mcp-installer.ts`:
- Around line 74-96: The current parsing in the try block of mcp-installer.ts
(around the handling of stdout, variables commandLine, argsLine, argsRaw and
resulting args) splits args with argsRaw.split(/\s+/), which breaks on spaces
inside quoted arguments (e.g., "C:\Program Files\...") and causes incorrect
matches; replace that split with a small tokenizer that scans argsRaw and
produces tokens treating quoted substrings (single or double quotes) as single
tokens (handling escaped quotes/backslashes minimally), then strip surrounding
quotes from each token and return that array as args so matchesExpected and
currentCommand are computed correctly; keep the rest of the return shape and
still use commandsEqual({ command, args }, expected).
In `@apps/desktop/src/main/lib/browser-mcp-bridge/server.ts`:
- Around line 149-154: readJson currently streams the entire request body into
memory with for await (const chunk of req) and needs a hard size limit to avoid
OOM; modify readJson(IncomingMessage) to track accumulated byte length as you
push each chunk, enforce a configurable max (e.g. 1–10MB), and when the limit is
exceeded immediately stop reading and return/throw an error that the caller can
turn into a 413 response (or directly set status 413 if readJson has access to
the response), ensuring any streams are ended/cleaned up; keep JSON parsing
behavior for valid bodies and return {} for empty bodies as before.
---
Outside diff comments:
In `@apps/desktop/src/renderer/hooks/useBrowserAutomationData.ts`:
- Around line 10-24: Docblock for useBrowserAutomationData is out of sync:
remove the outdated "TODO-Agent sessions" description and update the top comment
to state that sessions are now only sourced from terminal panes with a `claude`
or `codex` child process (binding key = terminal paneId), keep the note that MCP
readiness is resolved against the user's Claude/Codex config files, and mention
that `enabled` still controls the expensive queries while the binding
query/subscription always run; update the text referenced around the old 56-62
comment to match the actual behavior implemented in the hook.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/SessionConnectModal/SessionConnectModal.tsx`:
- Around line 148-160: Remove the now-dead snippet-copying logic: delete the
handleCopySnippet function and its import dependency getSnippetForSession, and
remove the unused props session, mcpConfigPath, and onCopy from SetupPanel's
props and from the call site where SetupPanel is instantiated; note that
SetupPanel was replaced by McpInstallPanel internally so ensure you only keep
props actually referenced by McpInstallPanel and remove any wiring that passed
session/mcpConfigPath/onCopy to SetupPanel (and any toast calls tied to
handleCopySnippet) so there are no unused variables or imports left (refer to
handleCopySnippet, getSnippetForSession, SetupPanel, and McpInstallPanel).
---
Nitpick comments:
In `@apps/desktop/src/lib/trpc/routers/browser-automation/index.ts`:
- Around line 551-574: The schema currently accepts sessionKind values
["todo-agent","terminal"] but the mutation immediately rejects "todo-agent";
update the tRPC input for setBinding by removing "todo-agent" from the z.enum
(use only "terminal" with default "terminal") so invalid inputs are 400-ed at
the schema layer, and remove the runtime branch that throws for
input.sessionKind === "todo-agent" (or if you must keep the enum for
compatibility, add a clear comment on why it remains and keep the runtime
guard). Ensure references: setBinding, input.sessionKind,
z.enum([...]).default(...), and bindingStore.set are adjusted accordingly.
In `@apps/desktop/src/main/lib/browser-mcp-bridge/server.ts`:
- Around line 329-340: The singleton `current` isn't cleared when the server is
closed (e.g. in the app.on("will-quit") handler), causing subsequent calls to
startBrowserMcpBridge() to return a stale closed handle; update the shutdown and
error paths to set `current = null` whenever you call `server.close()` or hit
server error—specifically modify the app.on("will-quit") callback, the
server.close() completion callback used inside the `stop` method, and any server
"error"/"close" listeners to clear `current` so startBrowserMcpBridge() can
create a fresh server on next call.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1ce6e799-244d-4843-a415-8ca417830459
📒 Files selected for processing (14)
apps/desktop/electron-builder.tsapps/desktop/package.jsonapps/desktop/src/lib/trpc/routers/browser-automation/index.tsapps/desktop/src/main/lib/browser-mcp-bridge/mcp-installer.tsapps/desktop/src/main/lib/browser-mcp-bridge/pane-resolver.tsapps/desktop/src/main/lib/browser-mcp-bridge/server.tsapps/desktop/src/main/todo-daemon/supervisor-engine.tsapps/desktop/src/renderer/hooks/useBrowserAutomationData.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/SessionConnectModal/SessionConnectModal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/SessionConnectModal/components/McpInstallPanel/McpInstallPanel.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/SessionConnectModal/components/McpInstallPanel/index.tspackages/superset-browser-mcp/package.jsonpackages/superset-browser-mcp/src/tools/index.tspackages/superset-browser-mcp/src/transport/bridge-client.ts
✅ Files skipped from review due to trivial changes (3)
- apps/desktop/src/main/todo-daemon/supervisor-engine.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/SessionConnectModal/components/McpInstallPanel/index.ts
- packages/superset-browser-mcp/package.json
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/superset-browser-mcp/src/transport/bridge-client.ts
- apps/desktop/src/main/lib/browser-mcp-bridge/pane-resolver.ts
- packages/superset-browser-mcp/src/tools/index.ts
- resolveSupersetBrowserMcpCommand: electron-builder's extraResources 'to: "resources/superset-browser-mcp"' ends up at <app>/Contents/Resources/resources/... , not at process.resourcesPath/superset-browser-mcp/... . Include the 'resources/' segment so packaged builds actually find the bundled bin. (codex P1) - Codex TOML probes now unescape basic-string values (\\, \n, \uXXXX, etc.) and accept single-quoted literal strings, so Windows paths like C:\\Users\\... match correctly instead of being reported as missing. The same helpers are shared between the readiness detector and the install-state probe. (codex P1) - browser-mcp-bridge server: readJson caps request bodies at 8MB and returns 413 Payload Too Large instead of buffering arbitrary data into main-process memory. (CodeRabbit Minor)
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23e8a8129f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- package.json: build:browser-mcp now runs from prepackage too, not only prebuild. Release flows (`bun run package` / the GitHub Actions desktop build) skip prebuild, so the bundled MCP binary was missing from packaged apps. (codex P1) - mcp-installer: route `claude` / `codex` probes and installs through getProcessEnvWithShellPath so macOS GUI launches that lack PATH entries for Homebrew / nvm / asdf etc. still find the CLIs that a terminal session would. One-click install now matches the CLI resolution the terminal pane already sees. (codex P2)
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9537cf37e3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const argsLine = body.find((line) => /^args\s*=/.test(line)); | ||
| const command = parseFirstTomlString(commandLine); | ||
| const args = parseAllTomlStrings(argsLine); |
There was a problem hiding this comment.
Parse full Codex args array before readiness check
detectCodexMcp now enforces an exact command/args match, but it only parses strings from the single args = ... line. For valid TOML configs that use a multiline array (args = [ followed by values on later lines), this produces args = [], so codexReady is reported as false even when the MCP is correctly installed. Since Connect is gated on session.mcpStatus === "ready", affected Codex sessions cannot be bound despite a valid config.
Useful? React with 👍 / 👎.
Summary
browser pane → LLM session のバインディングを、独立した MCP サーバとPID ベースの自動マッチングで完成させます。ユーザーは MCP を一度登録するだけで、以降は UI から Connect を押すだけで任意の pane を任意の session に紐づけられます。
新規パッケージ
packages/superset-browser-mcp@modelcontextprotocol/sdk)~/.superset/browser-mcp.jsonを読んで Superset app の loopback port + 共有 secret を取得get_connected_pane/navigate/screenshot/evaluate_js/get_console_logsx-superset-mcp-ppid(=自 MCP のprocess.ppid) を載せる。Superset app がこれを使って「どの LLM セッションから呼ばれたか」を自動解決App 側 bridge
apps/desktop/src/main/lib/browser-mcp-bridge/server.ts: 127.0.0.1 + 任意 port で HTTP 起動、共有 secret 認証~/.superset/browser-mcp.jsonを書き出し、MCP 側から discover させるsupervisor-engineのonChildで Claude worker PID を新設pid-registryに登録。直接マッチgetProcessTreeで辿って PPID を含む pane を特定ensureDebuggerAttached(paneId): ElectronwebContents.debuggerで CDP 1.3 attach。Page.navigate/Page.captureScreenshot/Runtime.evaluate/ console buffer を提供UI / router
browserAutomation.getMcpStatusがserverCommand: { command, args }を返すように拡張。dev ではbun run <repo>/packages/superset-browser-mcp/src/bin.ts、packaged ではbunx @superset/superset-browser-mcp(将来の公開向け) にフォールバックgetSnippetForSessionが上記 argv を TOML/shell エスケープ込みで生成、Connect modal の setup snippet が copy-paste で動くclaude mcp add superset-browser -s user -- ...になるユーザーから見た流れ
claude mcp add superset-browser -s user -- bun run <path>を実行navigate等のツールを呼ぶ → MCP → HTTP bridge → binding 解決 → 該当 webview を CDP で操作やっていないこと (別 PR で)
process.ppidが Codex CLI を指す場合の特殊な挙動検証)@superset/superset-browser-mcpを bunx 可能にする publishConfig 整備)Test plan
~/.superset/browser-mcp.jsonが port + secret 込みで書かれるclaude起動 → snippet のclaude mcp add ...を実行 →/mcpでsuperset-browserが runningnavigateツールを呼ぶと該当 pane が URL に遷移Summary by CodeRabbit
リリースノート
新機能
変更