Skip to content

Shared code architecture#1027

Open
BobDickinson wants to merge 152 commits intomodelcontextprotocol:v1.5/mainfrom
BobDickinson:tui-integration-design
Open

Shared code architecture#1027
BobDickinson wants to merge 152 commits intomodelcontextprotocol:v1.5/mainfrom
BobDickinson:tui-integration-design

Conversation

@BobDickinson
Copy link
Contributor

@BobDickinson BobDickinson commented Jan 20, 2026

This draft PR introduces a shared code architecture that enables code reuse across the MCP Inspector's CLI, TUI, and web client interfaces. This addresses the feature drift and maintenance burden caused by the CLI and web client maintaining separate implementations, with immediate code sharing across the CLI and TUI, and planned support for the web client (v2).

What's Included

1. TUI Integration

  • Integrated the TUI from mcp-inspect as a first-class workspace
  • TUI is accessible via mcp-inspector --tui flag
  • Currently experimental; feature gaps include OAuth, completions, elicitation, and sampling

2. Shared Package (@modelcontextprotocol/inspector-shared)

  • Created internal workspace package for shared MCP client code
  • Includes InspectorClient - a comprehensive wrapper around the MCP SDK Client that manages:
    • Client and transport lifecycle
    • Message tracking and stderr logging
    • Server data management (tools, resources, prompts)
    • Event-driven updates via EventTarget (cross-platform compatible)
    • High-level MCP method wrappers with automatic parameter conversion

3. CLI Migration

  • Migrated CLI to use InspectorClient from shared package
  • Consolidated CLI helper functions into InspectorClient methods
  • Removed duplicate code (CLI transport implementation, cli/src/client/ directory, etc)
  • All CLI tests passing

4. Shared React Code

  • useInspectorClient hook for reactive state management
  • Works in both TUI and v2 web client (future)
  • Subscribes to EventTarget events and provides reactive React state

Architecture

The shared package includes:

  • shared/mcp/ - MCP client/server interaction (InspectorClient, transport creation, config loading)
  • shared/react/ - Shared React code (hooks, components, etc.)
  • shared/json/ - JSON utilities for parameter conversion
  • shared/test/ - Test fixtures and harness servers

Both CLI and TUI now use the same InspectorClient implementation, ensuring consistent behavior and eliminating code duplication (both apps are now thin UX-only implementations).

Next Steps

Phase 4: TUI Feature Gap Implementation (Planned)

  • Address TUI feature gaps (OAuth, completions, elicitation, sampling) by extending InspectorClient
  • This dual-purpose approach: brings TUI to feature parity while preparing InspectorClient for v2 web client integration
  • When complete, InspectorClient will be very close to ready for full v2 web client support

Phase 5: v2 Web Client Integration (Planned)

  • Integrate InspectorClient with the v2 web client once Phase 4 is complete and v2 web client is ready
  • Update InspectorClient and other shared code as required to support v2 while maintaining CLI and TUI compatability

Documentation

See docs/shared-code-architecture.md (included in this PR) for:

  • Complete architecture overview
  • InspectorClient capabilities and API
  • Integration history and current usage patterns
  • Detailed web client integration plan

Testing

  • All CLI tests passing
  • TUI tested with test harness servers
  • TypeScript Project References ensure correct build ordering
  • CI workflow updated to build shared package first

This is a draft PR for review. Feedback welcome on the architecture and integration approach.

@BobDickinson
Copy link
Contributor Author

Another element that I probably should have implemented in this PR is that we can now build a comprehensive test suite around the shared InspectorClient that will provide very good coverage of the functional elements of the Inspector, so that all we really need to test in the TUI or web client is the UX part.

@BobDickinson
Copy link
Contributor Author

I merged main and resolved conflicts. All projects build and all tests pass, CI tests pass. Should be ready to review/test.

@cliffhall
Copy link
Member

@claude /review

@claude
Copy link
Contributor

claude bot commented Feb 12, 2026

Claude Code is working…

I'll analyze this and get back to you.

View job run

@BobDickinson
Copy link
Contributor Author

Per the suggestion from @olaservo - I think the idea of moving this code to a v1.5 branch makes sense. It will make it easier for people to review, test, and collaborate on the code.

If we do that, I'd want to make a couple of changes on my branch first.

  • Rename shared to core - I named it shared instead of core because I thought there might be some situation where it needed to coexist with the v2 core, but I think core is the proper dir/package name and this is probably the time to make that change (whether or not this evolves into the v2 core).
  • Remove client and server projects and tooling - I left these in place also with the idea that web would coexist with them if merged to main, but if this v1.5 test/proto branch is meant to show a possible future where web based on InspectorClient is the Inspector UX, then I'd prefer to clean up the branch now (and also remove the --web launcher option and just make the cli launch web by defailt like it uses to launch client/server).

It may or may not also make sense to resolve any major issues from the pending Claude review before we do this.

Anyway, if the branch plan is agreed, let me know and I'll make the above changes to my local branch and let you know when it's ready to be merged to this repo.

After the branch is created and populated, we can close this PR.

cc: @cliffhall

@BobDickinson
Copy link
Contributor Author

@cliffhall - The Claude review failed. I'm not sure why it fails on this PR and not others. There are some open issues that may be relevant: anthropics/claude-code-action#223, which may be a duplicate of anthropics/claude-code-action#46.

@cliffhall
Copy link
Member

cliffhall commented Feb 14, 2026

Per the suggestion from @olaservo - I think the idea of moving this code to a v1.5 branch makes sense. It will make it easier for people to review, test, and collaborate on the code.

Agreed.

If we do that, I'd want to make a couple of changes on my branch first.

The proposed changes sound good.

It may or may not also make sense to resolve any major issues from the pending Claude review before we do this.

I think Claude may just be having difficulty with all the commits that went into it. Could be a single squash commit would make it easier.

Anyway, if the branch plan is agreed, let me know and I'll make the above changes to my local branch and let you know when it's ready to be merged to this repo.

I think that's good. We can treat 1.5 not as a deviation from what we have now but a whole new entity that's a stepping stone toward 2.0. We can review it and test the heck out of it against all the things and get confidence in it that way.

I added a v1.5/main branch you can create a new PR against with the above suggested changes (i.e., removing client and server, etc)

…stry), all tests actually validate what they say they test.
… up process termination (possible leak on Windows), beefed up http server cleanup (close all connections), removed unused hasValidJsonOutput, reduced CLI timeout to give it breathing room with vitest timeout.
…truth (all UX now reflects InspectorClient state directly for prompts, resources, tools, messages, and stdio transport log events).
… utilize shared MCP functionality. Enhanced build scripts to include shared, upgraded React and TypeScript dependencies across all workspaces, and implemented Project References for improved type resolution and build order.
…ng direct Client usage. Removed transport-related code and updated logging level handling. Enhanced environment configuration management. Cleaned up unused imports and streamlined argument parsing.
…rations, consolidating client logic and removing deprecated client utility files. Updated argument parsing and logging configurations, ensuring consistent behavior across CLI and TUI. Enhanced documentation to reflect changes in architecture and functionality.
…r, enabling cross-platform event handling for both browser and Node.js. Update related documentation and React hook to utilize new event system, ensuring compatibility and improved state management across TUI and web clients.
…. The shared package is now built before the CLI, ensuring proper dependencies are in place. This change enhances the build process and maintains consistency across the project.
@BobDickinson
Copy link
Contributor Author

I addressed the following concerns from the Claude review:

  • 1. Hardcoded client version in InspectorClient constructor
  • 2. any types on transport fields and repeated params: any
  • 3. useInspectorClient uses any[] for lists
  • 5. CI workflow doesn't trigger on core/** changes
  • 8. Minor: @deprecated on InspectorClientLogger type

I agree that InspectorClient is very large (the test module is even larger). We should look at decomposing it if there are clear ways to do it that provide advantages around separation of concerns, testability, etc (my suspicion is that will be the case). I'm not in favor of breaking it up just because it's large. In the world of AI, the agents and models often perform better with larger, less decomposed implementations. Anyway, I suspect there is a decomposition that probably makes sense, so I'll start investigating some strategies and rationale for this.

@BobDickinson
Copy link
Contributor Author

I worked up a design for implementing sub-managers (as recommended) and added the design doc to this PR as docs/inspector-client-sub-managers.md. After this PR is merged, I'll make a new PR with an implementation of that and we can use that as a place to have the discussion about how to make InspectorClient more manageable.

…d content compatibility warning" fix (PR 1098)
…e Code) to "streamable-http". Added support for custom headers in config from file (mcpServers).
…me flakiness) and smart test wait helpers (cause of the rest of them). Implemented a test:repeat target that will run the entire test suite repeatedly. Verified 50 test runs with no failures.
Copy link
Member

@cliffhall cliffhall left a comment

Choose a reason for hiding this comment

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

Haven't done a deep review, just a few things I see testing with the Everything Server.

  1. When you clear the notes in the Server Notifications section, they all reappear when the next notification comes in.
notes.mov
  1. When running both the Everything Server's trigger-sampling-request and trigger-sampling-request-async tools, it should automatically take us to the sampling screen and when we submit the form on that screen, it should take us back to the tool screen automatically.
Screen.Recording.2026-02-28.at.2.47.11.PM.mov

It should operate like the trigger-elicitation-request-async tool works:

Screen.Recording.2026-02-28.at.2.51.00.PM.mov

Copy link
Member

@cliffhall cliffhall left a comment

Choose a reason for hiding this comment

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

When I run npm run start it no. longer shows the url with the authtoken on the query string and auto-launches . It should.

Image

It does the right thing when you npm run dev

Screenshot 2026-02-28 at 3 28 18 PM

Copy link
Member

@cliffhall cliffhall left a comment

Choose a reason for hiding this comment

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

Having issues testing the OAuth.

When running with the latest version (0.21.1) ...
Image

It works fine when hitting the demo server in the TS SDK with:
tsx --watch src/examples/server/simpleStreamableHttp.ts --oauth

Screen.Recording.2026-02-28.at.3.49.18.PM.mov

When I try this PR:

Screen.Recording.2026-02-28.at.3.53.44.PM.mov

… as is more common). Also, now we automatically start auth when auth required (instead of just bringing up the auth ux and making the user push a button). This should be at parity with how v1 works.
@BobDickinson
Copy link
Contributor Author

Fixed:

  • Notification clearing bug (also history clearing)
  • Sampling flow (auto switch to sampling tab, then back to tools tab)
  • Prod launcher now opens browser
  • Bug on oauth when 401 happens on initialize (versus connect)
  • Auto auth on connect (as v1 does)
  • Renamed top level test folder to test-servers

Copy link
Member

@cliffhall cliffhall left a comment

Choose a reason for hiding this comment

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

@BobDickinson Would you mind adding a run of the linter just before or after the prettier check? There are a couple of linter complaints.

Image Image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants