Add "Configuration" support in UI for configuring the request timeout (and more things in the future) #204

pulkitsharma07 · 2025-03-22T15:58:38Z

Screen.Recording.2025-03-22.at.8.33.53.PM.mov

Fixes #142, removes support for setting timeout via search params (added as part of #164) , as that may not be intuitive to end users.

Motivation and Context

Same as #142,

How Has This Been Tested?

Tested locally by adding console.log statements when making a request in useConnection to a custom MCP server, was able to see the custom timeout value which was set manually in the UI, otherwise the default timeout of 10 seconds is used.

Breaking Changes

Removes support for setting the request timeout value via search param: timeout

Types of changes

Bug fix (non-breaking change which fixes an issue)
[] New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation update

Checklist

I have read the MCP Documentation
My code follows the repository's style guidelines
New and existing tests pass locally
I have added appropriate error handling
I have added or updated documentation as needed

Additional context

Cloud-Iris · 2025-03-24T04:10:33Z

Yeah it is exactly what we need. Thanks for your work.🌹

cyri113 · 2025-03-26T15:11:13Z

Exactly what I was looking for 👍🏽 please merge!

olaservo

Thanks a lot for submitting this! Had some minor feedback.

olaservo · 2025-03-26T15:53:01Z

client/eslint.config.js

@@ -19,6 +19,7 @@ export default tseslint.config(
    },
    rules: {
      ...reactHooks.configs.recommended.rules,
+      "react-hooks/rules-of-hooks": "off", // Disable hooks dependency order checking


Could you add a little more to this comment to help explain the need to disable this?

I was getting a bunch of eslint warning in Cursor on opening the project for the first time. Particularly for these set of hooks, I believe that is happening because of this condition.

Let me know if I need to configure something in Cursor to avoid the warning.

I've been meaning to fix this. I have a fix working but need to test and will add a separate PR. We don't need to turn off rules of hooks.

The problem is that there is a conditional at the top of the component that short circuits the rest of the component by returning suspense if the path is oauth/callback. All the subsequent hooks complain because hooks are supposed to be run in the same order every time. Sometimes hooks would run, but sometimes we'd render the component as a suspense, bypassing them.

FYI: Fix is to move the conditional to the bottom where the render happens.

I've raised #217 to address.

ack, will fix it and turn it back on this PR.

olaservo · 2025-03-26T16:06:56Z

client/src/lib/constants.ts

 // OAuth-related session storage keys
 export const SESSION_KEYS = {
  CODE_VERIFIER: "mcp_code_verifier",
  SERVER_URL: "mcp_server_url",
  TOKENS: "mcp_tokens",
  CLIENT_INFORMATION: "mcp_client_information",
 } as const;
+
+export const DEFAULT_INSPECTOR_CONFIG: InspectorConfig = {


It could be helpful for future developers to add some clarification in the Readme (or elsewhere) to explain what types of configs belong in InspectorConfig. Assuming my take on this matches your intentions, maybe something like:

# Configuration The MCP Inspector supports two types of configurations: - **Client configurations** apply universally to all MCP server connections and are managed through the Configuration UI. Example: `MCP_SERVER_REQUEST_TIMEOUT` controls timeout duration for MCP server requests. - **Server-dependent settings** are managed through other dedicated UI controls or through the CLI. Examples include `args` and environment variables.

Basically I am trying to set the stage for the incoming PR that adds full config file support for servers.

I have gone through #177 and now I am thinking that we should have just a single config file (and remove the UI based configuration).
So the config file:

configures which MCP server to use

and client specific configurations (like request timeout, authentication, proxy, error handling in the future).

Wdyt ? I guess it depends on how people are using inspector, do they want to change config on the fly (via the UI) or they are okay changing the config file (and restarting inspector).

Personally, I am okay with a config file based approach, with nothing available in the UI. But not sure about other users, need your inputs here. Config file while need a restart and that can get rid of any "state" which MCP inspector had, so maybe we should go into UI direction ?

I think eventual solution would be to use config file as a base config, which is then overridden by the UI.

I think having the UI for client configs is still a good idea based on what I've seen users mention before. Thanks for looking at #177 ! I'd like to get that rolled in soon since I think it will help a lot with more programmatic testing. Lets let other reviewers weigh in in case there are any other strong opinions.

Sure..

In terms of programmatic testing, I guess we will need a config file based approach for inspector config. I guess way forward is to get both PRs merged and then look into further improvements of combining them into a single config file.

client/src/lib/hooks/useConnection.ts

cliffhall

Don't suppress rules of hooks. See comments. Will fully review when PR is ready to go again.

cliffhall · 2025-03-27T16:52:52Z

client/eslint.config.js

@@ -19,6 +19,7 @@ export default tseslint.config(
    },
    rules: {
      ...reactHooks.configs.recommended.rules,
+      "react-hooks/rules-of-hooks": "off", // Disable hooks dependency order checking


I've been meaning to fix this. I have a fix working but need to test and will add a separate PR. We don't need to turn off rules of hooks.

cliffhall · 2025-03-27T17:00:22Z

client/eslint.config.js

@@ -19,6 +19,7 @@ export default tseslint.config(
    },
    rules: {
      ...reactHooks.configs.recommended.rules,
+      "react-hooks/rules-of-hooks": "off", // Disable hooks dependency order checking


The problem is that there is a conditional at the top of the component that short circuits the rest of the component by returning suspense if the path is oauth/callback. All the subsequent hooks complain because hooks are supposed to be run in the same order every time. Sometimes hooks would run, but sometimes we'd render the component as a suspense, bypassing them.

FYI: Fix is to move the conditional to the bottom where the render happens.

I've raised #217 to address.

Add support in UI to configure request timeout

4a23585

pulkitsharma07 changed the title ~~Add "Configuration" support in UI to configuring the request timeouts (and more things in the future)~~ Add "Configuration" support in UI for configuring the request timeout (and more things in the future) Mar 22, 2025

pulkitsharma07 mentioned this pull request Mar 22, 2025

Set longer request times for Inspector client #142

Open

Merge branch 'main' into main

484e282

pulkitsharma07 added 3 commits March 24, 2025 23:30

add configuration types

0656e15

Merge branch 'main' of github.com:pulkitsharma07/inspector

6efdcb6

Merge branch 'main' into main

0e667ac

olaservo requested changes Mar 26, 2025

View reviewed changes

pulkitsharma07 mentioned this pull request Mar 27, 2025

Support overriding proxy server url #165

Open

9 tasks

cliffhall requested changes Mar 27, 2025

View reviewed changes

cliffhall mentioned this pull request Mar 27, 2025

Fix ESlint rules-of-hooks warnings in App.tsx #217

Open

cliffhall added enhancement New feature or request waiting on submitter Waiting for the submitter to provide more info labels Mar 27, 2025

cliffhall mentioned this pull request Mar 27, 2025

Fix ESLint rules-of-hooks warnings #218

Open

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "Configuration" support in UI for configuring the request timeout (and more things in the future) #204

Add "Configuration" support in UI for configuring the request timeout (and more things in the future) #204

pulkitsharma07 commented Mar 22, 2025 •

edited

Loading

Cloud-Iris commented Mar 24, 2025

cyri113 commented Mar 26, 2025

olaservo left a comment

olaservo Mar 26, 2025

pulkitsharma07 Mar 27, 2025

cliffhall Mar 27, 2025

cliffhall Mar 27, 2025 •

edited

Loading

pulkitsharma07 Mar 27, 2025

olaservo Mar 26, 2025

pulkitsharma07 Mar 27, 2025

olaservo Mar 27, 2025

pulkitsharma07 Mar 27, 2025

cliffhall left a comment

cliffhall Mar 27, 2025

cliffhall Mar 27, 2025 •

edited

Loading

Add "Configuration" support in UI for configuring the request timeout (and more things in the future) #204

Are you sure you want to change the base?

Add "Configuration" support in UI for configuring the request timeout (and more things in the future) #204

Conversation

pulkitsharma07 commented Mar 22, 2025 • edited Loading

Motivation and Context

How Has This Been Tested?

Breaking Changes

Types of changes

Checklist

Additional context

Cloud-Iris commented Mar 24, 2025

cyri113 commented Mar 26, 2025

olaservo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cliffhall Mar 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cliffhall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cliffhall Mar 27, 2025 • edited Loading

Choose a reason for hiding this comment

pulkitsharma07 commented Mar 22, 2025 •

edited

Loading

cliffhall Mar 27, 2025 •

edited

Loading

cliffhall Mar 27, 2025 •

edited

Loading