feat: add MCP latency benchmark script and Windows investigation (#11)#70
feat: add MCP latency benchmark script and Windows investigation (#11)#70Kunall7890 wants to merge 3 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a React Debugger extension, along with a cross-platform MCP latency benchmark script and Windows investigation notes. The reviewer identified several key issues: overriding addEventListener in mcp/inject.js without overriding removeEventListener breaks React effect cleanups and causes memory leaks; the benchmark script fails to clear its 15-second timeout timer, leaving active timers on the event loop; the where command used in the benchmark is Windows-specific and fails on Unix-like systems; and obj instanceof HTMLElement in the serializer can fail across different realms, which should be replaced with a cross-realm safe check using obj.nodeType === 1.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| EventTarget.prototype.addEventListener = function(type, listener, options) { | ||
| if (!listener || typeof listener !== "function") { | ||
| return originalAddEventListener.call(this, type, listener, options); | ||
| } | ||
| const context = getCurrentComponentContext(); | ||
| if (!context) { | ||
| return originalAddEventListener.call(this, type, listener, options); | ||
| } | ||
| const closureId = trackClosure(listener, "eventListener", context); | ||
| const wrappedListener = function(event) { | ||
| checkStaleClosureOnExecution(closureId); | ||
| return listener.call(this, event); | ||
| }; | ||
| wrappedListener.__reactDebuggerOriginal = listener; | ||
| wrappedListener.__reactDebuggerClosureId = closureId; | ||
| return originalAddEventListener.call(this, type, wrappedListener, options); | ||
| }; |
There was a problem hiding this comment.
Since addEventListener wraps the original listener in a wrappedListener, any subsequent call to removeEventListener with the original listener will fail to remove it. This breaks cleanup functions in React effects, leading to severe memory leaks and duplicate event handlers. We should override removeEventListener as well, and keep a registry of original -> wrapped listeners on the EventTarget instance.
EventTarget.prototype.addEventListener = function(type, listener, options) {
if (!listener || typeof listener !== "function") {
return originalAddEventListener.call(this, type, listener, options);
}
const context = getCurrentComponentContext();
if (!context) {
return originalAddEventListener.call(this, type, listener, options);
}
const closureId = trackClosure(listener, "eventListener", context);
const wrappedListener = function(event) {
checkStaleClosureOnExecution(closureId);
return listener.call(this, event);
};
wrappedListener.__reactDebuggerOriginal = listener;
wrappedListener.__reactDebuggerClosureId = closureId;
if (!this.__reactDebuggerListeners) {
this.__reactDebuggerListeners = {};
}
if (!this.__reactDebuggerListeners[type]) {
this.__reactDebuggerListeners[type] = [];
}
this.__reactDebuggerListeners[type].push({ original: listener, wrapped: wrappedListener });
return originalAddEventListener.call(this, type, wrappedListener, options);
};
const originalRemoveEventListener = EventTarget.prototype.removeEventListener;
EventTarget.prototype.removeEventListener = function(type, listener, options) {
if (this.__reactDebuggerListeners?.[type]) {
const list = this.__reactDebuggerListeners[type];
const index = list.findIndex(item => item.original === listener);
if (index !== -1) {
const { wrapped } = list[index];
list.splice(index, 1);
return originalRemoveEventListener.call(this, type, wrapped, options);
}
}
return originalRemoveEventListener.call(this, type, listener, options);
};|
|
||
| // Collect stdout — resolve as soon as ANY data comes back | ||
| child.stdout.on("data", (chunk) => { | ||
| stdout += chunk.toString(); | ||
| if (!responded) { | ||
| responded = true; | ||
| const elapsed = performance.now() - start; | ||
| child.kill("SIGTERM"); | ||
| resolve(elapsed); | ||
| } | ||
| }); | ||
|
|
||
| child.stderr.on("data", () => {}); // suppress stderr noise | ||
|
|
||
| // Send initialize handshake — standard MCP protocol first message | ||
| const initMsg = JSON.stringify({ | ||
| jsonrpc: "2.0", | ||
| id: 1, | ||
| method: "initialize", | ||
| params: { | ||
| protocolVersion: "2024-11-05", | ||
| capabilities: {}, | ||
| clientInfo: { name: "bench", version: "0.0.1" }, | ||
| }, | ||
| }) + "\n"; | ||
|
|
||
| child.stdin.write(initMsg); | ||
|
|
||
| child.on("error", (err) => { | ||
| if (!responded) reject(err); | ||
| }); | ||
|
|
||
| child.on("close", (code) => { | ||
| if (!responded) { | ||
| reject(new Error(`Process exited with code ${code} without responding`)); | ||
| } | ||
| }); | ||
|
|
||
| // 15s timeout — generous for cold npx starts | ||
| setTimeout(() => { | ||
| if (!responded) { | ||
| child.kill("SIGTERM"); | ||
| reject(new Error("Timeout after 15000ms")); | ||
| } | ||
| }, 15000); |
There was a problem hiding this comment.
The 15-second timeout timer created in measureRound is never cleared when the child process successfully responds or exits. Since the benchmark runs sequentially for up to 100 runs, this leaves many active timers on the Node.js event loop, preventing the process from exiting cleanly and immediately upon completion. We should store the timeout ID and clear it in a cleanup function when the promise resolves or rejects.
const timer = setTimeout(() => {
if (!responded) {
child.kill("SIGTERM");
reject(new Error("Timeout after 15000ms"));
}
}, 15000);
const cleanup = () => {
clearTimeout(timer);
};
// Collect stdout — resolve as soon as ANY data comes back
child.stdout.on("data", (chunk) => {
stdout += chunk.toString();
if (!responded) {
responded = true;
cleanup();
const elapsed = performance.now() - start;
child.kill("SIGTERM");
resolve(elapsed);
}
});
child.stderr.on("data", () => {}); // suppress stderr noise
// Send initialize handshake — standard MCP protocol first message
const initMsg = JSON.stringify({
jsonrpc: "2.0",
id: 1,
method: "initialize",
params: {
protocolVersion: "2024-11-05",
capabilities: {},
clientInfo: { name: "bench", version: "0.0.1" },
},
}) + "\n";
child.stdin.write(initMsg);
child.on("error", (err) => {
if (!responded) {
cleanup();
reject(err);
}
});
child.on("close", (code) => {
if (!responded) {
cleanup();
reject(new Error(`Process exited with code ${code} without responding`));
}
});| const lines = execSync("where react-debugger", { encoding: "utf8" }) | ||
| .trim() | ||
| .split("\n") | ||
| .map(l => l.trim()); |
There was a problem hiding this comment.
The where command is Windows-specific. On macOS/Linux, where is not available, causing execSync("where react-debugger") to always throw an error and fall back to npx even if react-debugger is globally installed. We should use platform() === "win32" ? "where" : "which" to support global binary resolution on Unix-like systems.
| const lines = execSync("where react-debugger", { encoding: "utf8" }) | |
| .trim() | |
| .split("\n") | |
| .map(l => l.trim()); | |
| const cmd = platform() === "win32" ? "where" : "which"; | |
| const lines = execSync(`${cmd} react-debugger`, { encoding: "utf8" }) | |
| .trim() | |
| .split("\n") | |
| .map(l => l.trim()); |
| if (type === "object") { | ||
| const obj = value; | ||
| if (obj.$$typeof) return "[React Element]"; | ||
| if (obj instanceof HTMLElement) return `[${obj.tagName}]`; |
There was a problem hiding this comment.
Using obj instanceof HTMLElement can fail if the object comes from another iframe or realm (where HTMLElement is a different constructor). This causes the serializer to fall back to full object serialization, leading to massive performance overhead or depth-limit errors. We should use obj.nodeType === 1 for cross-realm safe HTML element detection.
| if (obj instanceof HTMLElement) return `[${obj.tagName}]`; | |
| if (obj.nodeType === 1) return `[${obj.tagName || obj.nodeName}]`; |
hoainho
left a comment
hoainho
left a comment
There was a problem hiding this comment.
Review: ⚠️ Needs cleanup
Thanks for the thorough Windows investigation and the benchmark script! However, this PR includes a significant amount of unrelated code that needs to be addressed.
What looks good
scripts/bench-mcp-latency.mjs— Well-structured, cross-platform benchmark runner with proper Windowsspawnfixes (EINVAL + ENOENT). Theshell: trueand.cmdresolution are correct fixes for Windows.evidence/phase0-latency-windows-notes.md— Clear investigation writeup with environment details, root causes, and reproduction steps.evidence/phase0-latency-windows.json— Proper metadata format for run results.
What needs fixing
Unrelated mcp/ Chrome extension included (15 files, ~18k lines):
The PR description only mentions the benchmark script and evidence files, but the diff includes a complete Chrome DevTools extension under mcp/ (background.js, content.js, devtools.js, panel.js, inject.js, popup.js, manifest.json, icons, styles — 15 files). This is an entirely separate feature not described in the PR body.
Please:
- Remove the
mcp/directory from this PR - If this is intentional work, please submit it as a separate PR with a proper description
- Keep only the
evidence/,scripts/bench-mcp-latency.mjsand any files directly related to the Windows latency benchmark (issue #11)
|
Done! Here's what I fixed:
That was accidentally committed from a local experiment — not related to issue #11. Removed it entirely; this PR now contains only scripts/bench-mcp-latency.mjs and the evidence/ files. resolveBin() was using where unconditionally, which is Windows-only and throws on macOS/Linux. Now uses platform() === "win32" ? "where" : "which" so global binary detection works on all platforms. The setTimeout in measureRound() was never cleared on successful runs, leaving active timers on the event loop and preventing the process from exiting cleanly. Stored the timer ID, added a cleanup() helper, and called it in all three exit paths — stdout data, error, and close. platform was imported twice (once from "os", once from "node:os") and spawn/execSync were split across two import lines. Consolidated into single clean imports at the top. |
2 participants
Summary
Windows validation for MCP latency benchmark — addresses #11.
What I found
After installing
@nhonh/react-debugger@2.1.2globally and runningthe benchmark, I discovered that the
mcpsubcommand is an interactiveinstaller (creates a local
/mcpdirectory), not a stdio JSON-RPCserver. The CLI only exposes
-v,-h, and-yflags — there is no--stdiotransport available in the current published version.This means Phase A (the MCP stdio server) has not been published to
npm yet, so latency data cannot be collected at this time.
Windows-specific bugs found and fixed in the script
spawn EINVAL.cmdfiles needshell: trueon Windowsshell: trueto spawn optionsspawn ENOENT.cmdwrappers — plain path failsresolveBin()now prefers.cmdpath fromwhereoutputThese bugs are Windows-only and would not appear on macOS/Linux.
Files added
scripts/bench-mcp-latency.mjs— reusable benchmark runner, works onWindows + Linux, auto-detects OS and writes the correct output file
evidence/phase0-latency-windows.json— run metadata + environment infoevidence/phase0-latency-windows-notes.md— full investigation writeupReady to re-run
Once
react-debugger mcp --stdio(or equivalent) is available in apublished version, re-running:
on Windows will produce valid
evidence/phase0-latency-windows.jsontiming data automatically. No script changes needed.
Environment