-
Notifications
You must be signed in to change notification settings - Fork 598
Fix proxy port configuration for non-default SERVER_PORT #555
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🎭 Playwright E2E Test Results✅ 12 passed Details12 tests across 1 suite 📊 View Detailed HTML Report (download artifacts) |
0df3ce3
to
f7cbeab
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One little refactor request.
6424232
to
554a922
Compare
- Update client to read MCP_PROXY_PORT from URL parameters - Update server to include port in displayed URL when non-default - Update start scripts to pass SERVER_PORT to client - Use DEFAULT_MCP_PROXY_LISTEN_PORT constant instead of magic numbers This fixes the issue where the client couldn't connect to the proxy when SERVER_PORT was set to a non-default value.
Address PR feedback by restructuring getClientUrl to always include MCP_PROXY_PORT parameter when using non-default port, regardless of auth status. Also improved code clarity by using params.size check.
554a922
to
9e9419b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-default server port is working great, and if combined withDANGEROUSLY_OMIT_AUTH
, also does the right thing.


There is one more wrinkle. If the user also has MCP_AUTO_OPEN_ENABLED set to false, then they are left with the situation of having no link that will actually open a working inspector. The MCP Inspector is up and running at http://localhost:6274
message isn't enough, it would also need to have ?MCP_PROXY_PORT=7777
appended.

Refactored startProdClient to match startDevClient's logging pattern, ensuring consistent user feedback whether auto-open is enabled or disabled. Also ensures that we log full information on how to open the browser regardless of whether MCP_AUTO_OPEN_ENABLED=false.
Simplifies the startup flow by consolidating browser opening in the client script rather than the start script for prod. We were duplicating a lot of logging, making it easy for logs to go out of sync - this is a proposal to consolidate: - server only logs info about itself - client only logs info about itself To reduce the number of places we print connection information to the user.
dad3696
to
042e7a2
Compare
Addressed in ae1aeb3. On closer look I also found we were logging URLs repeatedly, making it potentially confusing to the user which is the correct place to go, so I made a proposal in 042e7a2 how we could unify all this logging to be cleaner. End result: Without auto-open and auth:
With auto-open & auth:
For comparison - before this change it would look like this (note the duplicated links & info):
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small change below.
But also I now notice that if the Inspector is already running and you start it again, it does not report that the proxy is running and stop. Instead it creates a new token and launches anyway. The new instance works and the old instance stops working.
Here is a video demonstrating it.
Instead of launching the second instance, I would expect the following response, which is what is happening on main
at the moment.
Replace explicit property assignments with shorthand syntax when property names match variable names (e.g., SERVER_PORT: SERVER_PORT becomes just SERVER_PORT).
Interesting - I wasn't able to reproduce this. If I start 2 instances, they conflict as they should: In your video it looks like one of the versions running is 0.14.3 though (the one in puzzlebox?) - I believe that one predates #513 (your video still shows the string We had established here #513 (comment) that the reason no conflict was being detected is the difference between In any case if I check out a separate worktree of 0.14.3 and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 👍
@felixweinberger Yep, that was it. Apologies and thanks for checking. I've been battling a bad RSV type bug all week and my head's still full of cotton. 🤧 |
No problem thanks for the review :) hope you feel better soon! |
Pass proxy port via URL parameters when using non-default SERVER_PORT
Motivation and Context
Fixes the issue raised by @cliffhall in #529 where the client cannot connect to the proxy server when
SERVER_PORT
is set to a non-default value.When the proxy server runs on a non-default port (not 6277), the client doesn't know about it and tries to connect to the default port. This creates a chicken-and-egg problem:
/config
Changes:
getClientUrl()
in start scripts to includeMCP_PROXY_PORT
parameter when non-defaultMCP_PROXY_PORT
from URL parameters and use it for proxy connectionDEFAULT_MCP_PROXY_LISTEN_PORT
constantHow Has This Been Tested?
npm run dev
- worksSERVER_PORT=8888 npm run dev
- works, client connects to port 8888npm run build && SERVER_PORT=9090 npm start
- worksBreaking Changes
None.
Types of changes
Checklist
Additional context