Skip to content

Fix race condition between app and browser on npm command#1616

Merged
prathmeshpatel merged 3 commits intomainfrom
fix-npm-browser-race
Mar 14, 2026
Merged

Fix race condition between app and browser on npm command#1616
prathmeshpatel merged 3 commits intomainfrom
fix-npm-browser-race

Conversation

@prathmeshpatel
Copy link
Collaborator

@prathmeshpatel prathmeshpatel commented Mar 14, 2026

Summary

Replaced the 2-second delay with a proper connection check that waits for the server to actually accept connections before attempting to open the browser. Added waitForServerReady() function that polls the server port with exponential backoff and a 30-second timeout to ensure the application is fully ready before launching the browser

On npx @mcpjam/inspector@latest:
Screenshot 2026-03-14 at 4.24.40 PM.png

@chelojimenez
Copy link
Contributor

chelojimenez commented Mar 14, 2026

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

Copy link
Collaborator Author

prathmeshpatel commented Mar 14, 2026

@prathmeshpatel prathmeshpatel added the bug Something isn't working label Mar 14, 2026 — with Graphite App
@prathmeshpatel prathmeshpatel marked this pull request as ready for review March 14, 2026 23:25
@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Mar 14, 2026
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 14, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 96d0a3ac-ef94-410e-9e17-62520176ba52

📥 Commits

Reviewing files that changed from the base of the PR and between 38b3a26 and 661cc13.

📒 Files selected for processing (3)
  • mcpjam-inspector/bin/start.js
  • mcpjam-inspector/client/src/hooks/__tests__/use-chat-session.minimal-mode.test.tsx
  • mcpjam-inspector/server/routes/web/__tests__/chat-v2.guest.test.ts

Walkthrough

The pull request introduces server readiness polling to the startup sequence in start.js, adding a waitForServerReady helper function that attempts TCP connections with configurable timeout. The startup flow now waits for server confirmation before opening the browser, logging a warning if the server fails to respond within 30 seconds. Two test files receive formatting adjustments without functional modifications: test assertions in use-chat-session.minimal-mode.test.tsx are rewrapped across lines, and the mkdtempSync call in chat-v2.guest.test.ts is reformatted for readability.

📝 Coding Plan
  • Generate coding plan for human review comments

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@chelojimenez chelojimenez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: Fix race condition between app and browser on npm command

Approve — this correctly fixes the root cause. The hardcoded 2-second delay was insufficient on slower machines, causing the sandboxed iframe to fail with chrome-error://chromewebdata/ before the server was ready.

What's good

  • TCP-level check via net.createConnection is the right approach — lightweight and doesn't depend on HTTP routes
  • Proper cleanup with settled flag to prevent double-resolution
  • Graceful degradation (warns user to visit URL manually if server doesn't start in 30s)
  • Re-checks cancelled after awaiting readiness

Minor suggestions

  1. Description says "exponential backoff" but it's a fixed 200ms interval. This is actually better for this use case (open browser ASAP), just a description nitpick.

  2. Host mismatch on IPv6 systems: The server binds to 127.0.0.1 (server/index.ts:27), but the readiness check uses host which could be localhost in dev mode. On IPv6-preferring systems, localhost may resolve to ::1, causing the TCP check to fail even though the server is running on 127.0.0.1. Consider always polling 127.0.0.1 for the readiness check regardless of the display URL.

  3. Unrelated formatting changes in the test files — not a blocker, but ideally a separate commit.

None of these are blocking. #2 is worth considering to avoid a different "works for some but not others" scenario.

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Mar 14, 2026
@prathmeshpatel
Copy link
Collaborator Author

  1. Host mismatch on IPv6 systems: The server binds to 127.0.0.1 (server/index.ts:27), but the readiness check uses host which could be localhost in dev mode. On IPv6-preferring systems, localhost may resolve to ::1, causing the TCP check to fail even though the server is running on 127.0.0.1. Consider always polling 127.0.0.1 for the readiness check regardless of the display URL.

Production is unaffected. When ENVIRONMENT is not set (the default for bin/start.js), both the binding and the readiness check use 127.0.0.1. The mismatch only occurs when ENVIRONMENT=dev is explicitly set.

@prathmeshpatel prathmeshpatel merged commit b25c464 into main Mar 14, 2026
4 of 6 checks passed
@prathmeshpatel prathmeshpatel deleted the fix-npm-browser-race branch March 14, 2026 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working lgtm This PR has been approved by a maintainer size:S This PR changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants