Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
aidenybai
force-pushed
the
feat/mcp-install
branch
from
b0eb11c to
d397707
Compare
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Critical race condition and logic issues found. The most severe problem is a shared global latestContext variable accessed by all MCP sessions, which will cause data corruption when multiple agents connect simultaneously. Additionally, there's an inconsistent exit flow in the third call site where MCP install doesn't exit after completion, and TOML string escaping is incomplete.
packages/mcp/src/server.ts
Outdated
There was a problem hiding this comment.
Critical race condition: This global latestContext is shared across all MCP sessions. When multiple agents (e.g., Cursor and VS Code) connect simultaneously, they'll overwrite each other's context.
Each session creates its own McpServer instance (line 133), but they all share this single global variable. Agent A could submit context, then Agent B overwrites it before Agent A reads it.
Solution: Move latestContext inside createMcpServer() as a closure variable so each session has its own context storage.
| const result = textResult(formatContext(latestContext)); | ||
| latestContext = null; |
There was a problem hiding this comment.
Setting latestContext = null after reading creates a one-shot behavior. If an agent calls get_element_context twice before the user submits new context, the second call returns "No context has been submitted yet" even though context was just retrieved.
Is this intentional? Consider documenting this behavior or making it configurable. This could be confusing for agents that retry failed operations.
| const startHttpServer = async (port: number): Promise<Server> => { | ||
| const isAlreadyRunning = await checkIfServerIsRunning(port); | ||
|
|
||
| if (!isAlreadyRunning) { | ||
| await fkill(`:${port}`, { force: true, silent: true }).catch(() => {}); | ||
| await sleep(POST_KILL_DELAY_MS); | ||
| } |
There was a problem hiding this comment.
Logic issue: isAlreadyRunning is checked, but when it's false, you kill the port anyway. This conditional has no effect—you're killing the port regardless of the health check result.
If the intent is to only kill when a server IS running (but not responding to health checks), the condition should be inverted. If the intent is to always force-kill before starting, remove the health check entirely to save the timeout delay.
| const listenWithRetry = (httpServer: Server, port: number): Promise<void> => | ||
| new Promise((resolve, reject) => { | ||
| httpServer.once("error", async (error: NodeJS.ErrnoException) => { | ||
| if (error.code !== "EADDRINUSE") { | ||
| reject(error); | ||
| return; | ||
| } | ||
|
|
||
| await fkill(`:${port}`, { force: true, silent: true }).catch(() => {}); | ||
| await sleep(POST_KILL_DELAY_MS); | ||
|
|
||
| httpServer.listen(port, () => resolve()); | ||
| }); | ||
|
|
||
| httpServer.listen(port, () => resolve()); | ||
| }); |
There was a problem hiding this comment.
Error handling bug: The .once('error') listener is set up, then listen() is called. If the initial listen() fails with EADDRINUSE, the error handler fires, kills the port, waits, then calls listen() again.
But what if the second listen() call (line 174) also fails? There's no error handler attached for that call—it will throw an unhandled promise rejection.
You should either attach a full error handler to the httpServer, or structure this as a recursive retry with a max attempts limit.
| export const buildTomlSection = ( | ||
| configKey: string, | ||
| serverConfig: Record<string, unknown>, | ||
| ): string => { | ||
| const lines = [`[${configKey}.${SERVER_NAME}]`]; | ||
| for (const [key, value] of Object.entries(serverConfig)) { | ||
| if (typeof value === "string") { | ||
| lines.push(`${key} = "${value}"`); | ||
| } else if (Array.isArray(value)) { | ||
| const items = value.map((item) => `"${item}"`).join(", "); | ||
| lines.push(`${key} = [${items}]`); | ||
| } | ||
| } | ||
| return lines.join("\n"); | ||
| }; |
There was a problem hiding this comment.
TOML string escaping is incomplete. Strings with special characters (quotes, backslashes, newlines) need to be escaped in TOML.
For example, if value contains a double quote, the generated TOML will be syntactically invalid:
command = "npm "run" test" # Invalid!You're escaping array items (line 225) but not the string values (line 223). Either implement proper TOML escaping or use a TOML library like @iarna/toml or smol-toml.
packages/cli/src/commands/init.ts
Outdated
| if (connectionMode === "mcp") { | ||
| await promptMcpInstall(); |
There was a problem hiding this comment.
Inconsistent behavior: In the other two call sites (lines 437-448 and 158-171), selecting MCP mode exits the process with process.exit(0) after installing.
Here, execution continues to previewTransform (line 881) which expects agentIntegration to be set, but it will be undefined after MCP install.
This will cause incorrect behavior downstream. Either:
- Add the same exit logic as the other call sites, OR
- Set a flag to skip the preview/transform step for MCP mode
| serverName: string, | ||
| serverConfig: Record<string, unknown>, | ||
| ): void => { | ||
| if (content.includes(`"${serverName}"`)) return; |
There was a problem hiding this comment.
Early return when server name exists prevents updates to the config. If a user runs the installer again with a different version or config, it will silently skip the update.
The JSON handler (line 199) correctly overwrites existing configs. This JSONC handler should do the same for consistency.
| continue; | ||
| } | ||
|
|
||
| if (isInsideOurSection && line.startsWith("[")) { |
There was a problem hiding this comment.
This condition is too loose. If a TOML section contains a line that starts with [ anywhere (like a nested table or array of tables), it will incorrectly exit the section early.
For example:
[mcp_servers.react-grab-mcp]
command = "npx"
# Comment with [brackets]
args = ["--stdio"]The args line starts with [ and would trigger exit. Use line.trim().startsWith('[') and check that it's a valid section header pattern.
| if (url.pathname === "/context" && request.method === "POST") { | ||
| const chunks: Buffer[] = []; | ||
| for await (const chunk of request) { | ||
| chunks.push(chunk as Buffer); | ||
| } | ||
|
|
||
| try { | ||
| const body = JSON.parse(Buffer.concat(chunks).toString()); | ||
| latestContext = agentContextSchema.parse(body); | ||
| response.writeHead(200, { "Content-Type": "application/json" }).end( | ||
| JSON.stringify({ status: "ok" }), | ||
| ); | ||
| } catch { | ||
| response.writeHead(400, { "Content-Type": "application/json" }).end( | ||
| JSON.stringify({ error: "Invalid context payload" }), | ||
| ); | ||
| } | ||
| return; |
There was a problem hiding this comment.
Missing DoS protection: There's no limit on the request body size. A malicious client could send a massive payload and exhaust memory.
Add a size limit check:
const MAX_BODY_SIZE = 10 * 1024 * 1024; // 10MB
let totalSize = 0;
for await (const chunk of request) {
totalSize += chunk.length;
if (totalSize > MAX_BODY_SIZE) {
response.writeHead(413).end();
return;
}
chunks.push(chunk as Buffer);
}|
|
||
| process.on("SIGTERM", handleShutdown); | ||
| process.on("SIGINT", handleShutdown); |
There was a problem hiding this comment.
Multiple shutdown handlers can be registered. If startMcpServer is called twice (or if HTTP server starts in stdio mode), you'll have duplicate SIGTERM/SIGINT handlers registered, and process.exit(0) will be called multiple times.
While calling exit() multiple times is mostly harmless, the httpServer.close() may throw if called on an already-closing server. Use .once() or track whether handlers are already registered.
@react-grab/cli
grab
@react-grab/ami
@react-grab/amp
@react-grab/claude-code
@react-grab/codex
@react-grab/cursor
@react-grab/droid
@react-grab/gemini
@react-grab/opencode
react-grab
@react-grab/relay
@react-grab/utils
commit: |
Add MCP server config installation to the `init` and `add` commands. When connecting an agent, users now choose between MCP (recommended) and Legacy, with a multiselect to pick which agents to install for. Supports Cursor, VS Code, Claude Code, Amp, Droid, Codex, Zed, and Windsurf. Handles JSON, JSONC (preserves comments), and TOML configs. Also adds the @react-grab/mcp provider package (server + client) and wires it into the gym playground. The MCP server auto-starts via Next.js instrumentation — no separate server process needed. Co-authored-by: Cursor <cursoragent@cursor.com>
aidenybai
force-pushed
the
feat/mcp-install
branch
from
d397707 to
ea7c254
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 12 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/provider-mcp/src/cli.ts">
<violation number="1" location="packages/provider-mcp/src/cli.ts:4">
P2: Handle the async startup promise from `startMcpServer` so errors are reported and the process exits with a non-zero status instead of producing an unhandled rejection.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| #!/usr/bin/env node | ||
| import { startMcpServer } from "./server.js"; | ||
|
|
||
| startMcpServer({ |
There was a problem hiding this comment.
P2: Handle the async startup promise from startMcpServer so errors are reported and the process exits with a non-zero status instead of producing an unhandled rejection.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/provider-mcp/src/cli.ts, line 4:
<comment>Handle the async startup promise from `startMcpServer` so errors are reported and the process exits with a non-zero status instead of producing an unhandled rejection.</comment>
<file context>
@@ -0,0 +1,7 @@
+#!/usr/bin/env node
+import { startMcpServer } from "./server.js";
+
+startMcpServer({
+ port: Number(process.env.PORT) || undefined,
+ stdio: process.argv.includes("--stdio"),
</file context>
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| .option( | ||
| "-a, --agent <agent>", | ||
| "agent integration (claude-code, cursor, opencode, codex, gemini, amp, droid)", | ||
| "agent integration (claude-code, cursor, opencode, codex, gemini, amp)", |
There was a problem hiding this comment.
Help text for --agent option in init.ts is missing "ami" and "droid" agents that are available in AGENTS array
| const homeDir = os.homedir(); | ||
| if (process.platform === "win32") { | ||
| const appData = | ||
| process.env.APPDATA || path.join(homeDir, "AppData", "Roaming"); | ||
| return path.join(appData, "Zed", "settings.json"); | ||
| } | ||
| return path.join(homeDir, ".config", "zed", "settings.json"); |
There was a problem hiding this comment.
The getZedConfigPath function returns the wrong path for macOS. It returns ~/.config/zed/settings.json for macOS, but should return ~/Library/Application Support/Zed/settings.json
| serverConfig: Record<string, unknown>, | ||
| ): void => { | ||
| if (content.includes(`"${serverName}"`)) return; | ||
|
|
There was a problem hiding this comment.
The insertIntoJsonc function checks for duplicate servers using a global string search that matches the serverName anywhere in the file, causing false positives when the server name appears in comments or other non-config contexts.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
- Fix JSONC comma placement when inline comments present by stripping comments before checking last character - Add error handler to retry listen in listenWithRetry to prevent unhandled error exceptions
|
Bugbot Autofix prepared fixes for 2 of the 2 bugs found in the latest run.
|
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
|
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/cli/src/commands/init.ts">
<violation number="1" location="packages/cli/src/commands/init.ts:857">
P1: Choosing MCP in `init` now exits before the project setup runs, so React Grab never gets installed/configured. Remove the early `process.exit(0)` so the init flow continues after MCP config.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
When selecting MCP mode during fresh init, removed early exit that prevented React Grab project setup from completing. The MCP path now continues to previewTransform and package installation like the Legacy path does.
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
|
- Fix promptMcpInstall to check install results and return false if all installations fail - Fix regex pattern in insertIntoJsonc to escape special chars in configKey (e.g., amp.mcpServers)
|
Bugbot Autofix prepared fixes for 2 of the 2 bugs found in the latest run.
|
2 participants
Summary
initandaddCLI commands with a new MCP/Legacy connection mode prompt@react-grab/mcpprovider package (MCP server + browser client plugin)Details
When users run
react-grab initorreact-grab addand choose to connect an agent, they now see:Selecting MCP shows a multiselect of all agents (all checked by default):
Handles JSON, JSONC (preserves comments for Zed settings), and TOML (Codex) config formats. Shared prompt helpers (
promptConnectionMode,promptMcpInstall) eliminate duplication across the 3 call sites.Test plan
node packages/cli/dist/cli.js init→ select MCP → verify multiselect → check config files writtennode packages/cli/dist/cli.js add→ select MCP → verify configsMade with Cursor
Note
Medium Risk
Touches interactive CLI installation flows and writes to user-level editor/agent config files, so failures could affect local setups; mitigated by being opt-in via prompts and covered by unit tests for the config writers.
Overview
Adds an MCP vs Legacy connection mode prompt to
react-grab initandreact-grab add; choosing MCP installs React Grab’s MCP server configuration into supported agent/editor config files via a multiselect, then exits (or continues install in oneinitpath), while Legacy keeps the existing per-project agent integration flow.Introduces
packages/cli/src/utils/install-mcp.tsto write/merge MCP server entries across 8 clients (JSON, JSONC insertion to preserve comments, and TOML), plus unit tests covering merge/overwrite/creation cases.Adds a new
@react-grab/mcpworkspace package providing an MCP server (stdio + HTTP context endpoint/tool) and a browser client plugin that forwards selected element context to the server; the gym playground is updated to recognize/load anmcpprovider and addsdev:mcp/dev:allsupport.Written by Cursor Bugbot for commit 4f25540. This will update automatically on new commits. Configure here.
Summary by cubic
Adds MCP install to the CLI so users can configure the React Grab MCP server across multiple agents in one step, and introduces the @react-grab/mcp provider (server + browser client plugin) wired into the gym with a dev:mcp script and included in dev:all.
New Features
Migration
Written for commit 4f25540. Summary will update on new commits.