Skip to content

PR for team review #2

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

Open
wants to merge 32 commits into
base: jerome/pr-for-review
Choose a base branch
from
Open

PR for team review #2

wants to merge 32 commits into from

Conversation

jerome3o-anthropic
Copy link
Member

@jerome3o-anthropic jerome3o-anthropic commented Jun 30, 2025

Over the last week or so I've been hacking on this client to get it to a v0 state - now I'd like to get some 👀 on it before we open source it.

This PR is just an easy way for team members to leave comments / ask for changes etc.

The readme for this project is here

jerome3o-anthropic and others added 29 commits June 10, 2025 17:37
## Features
- 🔧 InferenceProvider interface with pluggable architecture
- 🔑 OpenRouter API key and OAuth PKCE authentication providers
- 🛠️ Tool calling support with automatic agent loop execution
- 📱 Interactive test UI with model selection and chat interface
- 🎯 Provider-level filtering to tool-capable models only
- ⚛️ React context with proper state management for UI reactivity

## Components
- Core types and interfaces in src/types/inference.ts
- OpenRouter client with shared utilities for both auth methods
- Two separate providers (composition over inheritance):
  - OpenRouterApiProvider for API key authentication
  - OpenRouterOAuthProvider for OAuth PKCE flow
- React InferenceContext with useInference hook
- Interactive test UI with tool calling demonstration
- OAuth callback routing with proper namespacing

## Tool Calling
- 3 test tools: weather, calculator, time
- Automatic tool execution and agent loop
- Visual tool call display with arguments and results
- Error handling for malformed tool calls

## Bug Fixes
- Fixed model selection UI reactivity issue
- Fixed OAuth popup race condition causing "cancelled" errors
- Proper cleanup of event listeners and intervals

## Documentation
- Comprehensive interface design document
- Implementation strategy and key design decisions
- Testing approach and security considerations

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
- Add MCPOAuthProvider implementing OAuthClientProvider interface
- Implement popup-based OAuth flow with message passing
- Add automatic connection retry after OAuth completion
- Support client registration persistence per server URL
- Add proper error suppression for expected OAuth UnauthorizedErrors
- Create comprehensive OAuth implementation documentation
- Clean up debug logging with emoji status indicators

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
- Add conversation management with persistence and multi-conversation support
- Create agent loop hook with test tools and MCP tool integration
- Implement full chat UI with message display, tool call visualization
- Add inline authentication flow for inference providers
- Support both API key and OAuth authentication methods
- Add conversation sidebar with conversation list and management
- Integrate MCP status display for debugging
- Create message input with proper disabled states and tooltips

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
- Fix ChatInterface to use context's reactive isAuthenticated value
- Fix useAgentLoop to use context's reactive isAuthenticated value
- Ensure consistent authentication state checking across the application
- Add isAuthenticated to useCallback dependencies for proper reactivity

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
- Document conversation system architecture and data models
- Explain agent loop implementation and tool integration
- Cover state management, authentication flow, and UI components
- Provide development guidelines and troubleshooting guide
- Include performance, security, and testing considerations

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
- Change tool name separator from dot to double underscore (server__tool)
- Update tool discovery to use server__tool_name format
- Update tool execution routing to parse double underscore separator
- Fix OpenRouter API error: tool names must match pattern '^[a-zA-Z0-9_-]{1,64}'

This resolves the silent failure when MCP tools were present in conversations.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
- Update MCP tool naming from server.tool to server__tool format
- Add tool naming requirements section with OpenRouter API constraints
- Update troubleshooting guide with tool naming validation errors
- Add specific troubleshooting for agent loop hanging with MCP tools

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
- Add OpenRouter API key persistence to localStorage with auto-restore
- Add automatic provider restoration on app startup for seamless login
- Fix MCP OAuth token persistence by preserving connection IDs across refreshes
- Add manual reconnect button for failed/disconnected MCP servers
- Add auto-reconnect for MCP servers on app startup
- Maintain backward compatibility with old connection storage format

Resolves auth token loss on page refresh for both inference and MCP providers.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
## Features
- Add proactive health check monitoring for MCP connections (30s intervals)
- Implement automatic reconnection on health check failures
- Add proper cleanup of health check intervals on disconnect

## Fixes
- Resolve OAuth type compatibility issues with MCP SDK
- Fix naming collision between InferenceProvider class and React component
- Remove unused React imports across all components (new JSX transform)
- Clean up unused type imports and variables
- Remove unused handleOAuthAuthentication function

## Technical Details
- Health checks use lightweight listTools() calls to verify connection state
- Automatic reconnection triggers when health checks fail
- Maintains existing exponential backoff retry logic
- All TypeScript compilation errors resolved

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
## Features Added
- **Unique server name validation**: Prevent duplicate MCP server names to avoid tool routing conflicts
- **Quick-add server examples**: Added example server buttons for "Everything" and "Test Server" configurations
- **Conversation model selector**: Added dropdown to change AI models directly in chat interface
- **Fixed nav bar height**: Conversation widget now properly accounts for navigation bar height

## User Experience Improvements
- Server name uniqueness prevents tool naming conflicts and confusion
- Quick-add buttons make it easier to get started with common MCP servers
- Model selector allows switching between AI models without leaving conversation
- Proper height calculation eliminates layout issues with conversation interface

## Technical Details
- Server name validation happens before calling addMcpServer API
- Model selector integrates with existing inference context
- Height calculation uses CSS calc(100vh - 4rem) for nav bar compensation
- All changes maintain backward compatibility

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
## Problem
MCP server names with invalid characters (spaces, special chars, etc.)
caused tool routing failures when prefixed to tool names, as OpenRouter
API requires tool names to match pattern ^[a-zA-Z0-9_-]{1,64}$

## Solution
- **Added normalizeServerName utility**: Converts invalid characters to underscores
- **Centralized normalization logic**: Shared utility prevents code duplication
- **Fixed tool routing**: Agent loop now matches normalized server names correctly
- **Updated tool discovery**: MCP tools use normalized prefixes consistently

## Examples
- "My Weather Service\!" → "My_Weather_Service"
- "Test@Server#123" → "Test_Server_123"
- "Server...with...dots" → "Server_with_dots"
- "  spaces_and__double__underscores  " → "spaces_and_double_underscores"

## Technical Details
- Created /src/utils/mcpUtils.ts for shared normalization function
- Updated MCPConnectionManager to use normalized prefixes in tool names
- Fixed agent loop to match connections using normalized names
- Maintains original server names in UI while using normalized names for API

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
## Features
- **Sticky model selection**: Chosen model persists across browser sessions
- **Automatic restoration**: Saved model is restored when loading models
- **Validation**: Invalid saved models are automatically cleared
- **Provider switching**: Model selection is reset when switching providers

## Implementation Details
- Model ID saved to localStorage on selection
- Model restored during loadModels() if still available
- Saved selection cleared when:
  - Switching inference providers
  - Logging out
  - Model no longer exists in available models

## User Experience
- Users no longer need to reselect their preferred model after refresh
- Model selector maintains state across page reloads
- Graceful handling when saved model becomes unavailable

## Technical Notes
- Uses 'selected_model_id' localStorage key
- Integrates with existing InferenceContext state management
- Maintains backward compatibility with existing provider logic

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
## Problem
After page refresh, the model selector would be empty even though the
authentication provider was restored and models were loaded successfully.
Users had to manually click "reload models" to populate the selector.

## Root Cause
The context state wasn't being properly updated after models were loaded
during provider restoration. The provider had the models, but React
components weren't re-rendering to show them.

## Solution
- **Added forced re-render**: Added extra `setAuthStateVersion()` call after
  model loading to trigger React context update
- **Auto-load models on restore**: Provider restoration now automatically
  loads models and restores saved model selection
- **Improved error handling**: Better error handling during model restoration

## Technical Details
- Modified `restoreProviderWithModels()` to force context re-render
- Ensured model loading happens immediately when provider is restored
- Maintained existing model persistence functionality
- Cleaned up debugging logs

## User Experience
- ✅ **Seamless model selection**: Model selector is populated immediately after refresh
- ✅ **Persistent choice**: Previously selected model is automatically restored
- ✅ **No manual intervention**: No need to click "reload models" button

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
## Problem
When starting an agent loop in one conversation and switching to another,
the "assistant thinking" UI would incorrectly appear in the second
conversation, even though it was idle.

## Root Cause
The `isLoading` state in ChatInterface was global/shared across all
conversations. When conversation A started generating, `isLoading` was
set to `true`. Switching to conversation B would still show the thinking
UI because `isGenerating = agentLoopState?.isRunning || isLoading`
evaluated to `true` due to the shared loading state.

## Solution
- **Conversation-specific loading**: Changed from single `isLoading` boolean
  to `loadingConversations` Set tracking which conversations are loading
- **Isolated state**: Each conversation now has independent loading state
- **Proper cleanup**: Loading state is properly added/removed per conversation

## Technical Changes
- Replaced `useState<boolean>` with `useState<Set<string>>`
- Updated `handleSendMessage` to add/remove conversation IDs from Set
- Modified `isGenerating` calculation to check current conversation only

## User Experience
- ✅ **Isolated UI state**: Thinking UI only appears for actually running conversations
- ✅ **No cross-conversation interference**: Switching conversations shows correct state
- ✅ **Maintained functionality**: All existing behavior preserved

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
## Changes
- **Removed navigation tabs**: Eliminated inference and MCP provider test tabs
- **Simplified App component**: Now directly shows ConversationApp without navigation
- **Full-screen layout**: ConversationApp now uses full viewport height (h-screen)
- **Cleaned up imports**: Removed unused test component imports
- **Deleted test components**: Removed InferenceTest.tsx and MCPTest.tsx files

## Benefits
- ✅ **Streamlined UX**: Single-purpose interface focused on conversations
- ✅ **Smaller bundle**: Reduced JS bundle size by ~25KB and CSS by ~3KB
- ✅ **Cleaner codebase**: Removed complexity of tab navigation and test interfaces
- ✅ **Better focus**: All functionality accessible through conversation interface

## Integration Points Preserved
- MCP server management available through conversation sidebar MCP status
- Model selection integrated into conversation header
- All authentication flows still work through conversation interface
- OAuth callbacks remain functional

The conversation interface now provides access to all functionality:
- Model selection in chat header
- MCP server status/management in sidebar
- Test tools still available through agent loop
- Full authentication and provider management

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
## Features
- **Logout button**: Added "Log Out" button at bottom of conversation sidebar
- **Confirmation dialog**: Prompts user to confirm before logging out
- **Conditional display**: Only shows when user is authenticated with a provider
- **Proper styling**: Matches sidebar design with subtle gray styling

## Implementation
- Integrated with existing InferenceContext for logout functionality
- Uses `clearProvider()` to handle complete logout process
- Added border separator between conversations and logout button
- Positioned at bottom of sidebar with fixed placement

## User Experience
- ✅ **Easy access**: Logout button always visible when authenticated
- ✅ **Safety confirmation**: Prevents accidental logout with confirm dialog
- ✅ **Clean design**: Subtle styling that doesn't distract from conversations
- ✅ **Complete logout**: Clears all authentication state and persisted data

## Technical Details
- Leverages existing clearProvider() functionality
- Automatically clears API keys, OAuth tokens, and selected models
- Maintains responsive design and dark mode compatibility
- No impact on conversation data (conversations persist locally)

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
## Problem
The logout button only cleared inference provider authentication, leaving
MCP OAuth tokens and server connections active. Users remained authenticated
to MCP servers after logging out.

## Solution
- **Clear MCP OAuth tokens**: Remove OAuth tokens and state for all MCP connections
- **Disconnect all servers**: Remove all MCP server connections on logout
- **Complete cleanup**: Comprehensive logout that clears all authentication state
- **Enhanced confirmation**: Updated dialog to inform users about MCP disconnection

## Technical Implementation
- Added MCP context integration to access connections and removal methods
- Clear OAuth tokens from localStorage for each OAuth-enabled connection
- Remove all MCP servers to properly disconnect and clean up connections
- Maintained existing inference provider logout functionality

## User Experience
- ✅ **Complete logout**: All authentication state cleared (inference + MCP)
- ✅ **Clean slate**: No residual authentication data after logout
- ✅ **Clear communication**: User informed about full scope of logout action
- ✅ **Security**: No orphaned authentication tokens left behind

## Data Cleared on Logout
- Inference provider API keys and OAuth tokens
- MCP OAuth tokens and authentication state
- Model selection preferences
- All active MCP server connections

Note: Conversation history remains intact as it's stored separately from auth data.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
- Renamed "Log Out" button to "Reset All Data" to better reflect functionality
- Renamed handleLogout to handleReset for consistency
- Updated confirmation dialog text to say "reset the application"
- Replaced selective localStorage clearing with localStorage.clear() for complete data removal
- Ensures all conversations, authentication, and MCP data is properly cleared

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
- Created tabbed interface with MCP, Conversations, and Inference tabs
- Set MCP tab as default tab
- Added quick connect button for example server with OAuth auth
- Created form to add custom MCP servers with auth type selection
- Implemented server management UI with connect/disconnect/remove actions
- Made sidebar resizable with drag handle (240px-600px range)
- Added comprehensive MCP server status display and tools listing
- Created dedicated Inference tab for OpenRouter connection management

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
- Made MCP tab sections (Summary, Add Servers, Server List) collapsible to save space
- Expanded sidebar width limit from 600px to 80% of window width for better debugging
- Fixed message monitor overflow by establishing proper height constraint chain
- Removed auto-scroll and show/hide toggle for message monitor - always visible now
- Server list now has fixed height (max-h-60) with internal scroll
- Message monitor takes remaining space with proper internal scrolling
- Used h-full chain instead of flex-1 to establish explicit height constraints
- Message monitor now properly scrolls within its container instead of at app level

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
- Fix duplicate React keys by using unique IDs for request/response/error messages
- Show complete JSON data in Request/Response/Error details sections
- Add Message Metadata section showing all message properties
- Remove debug console.log statements

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
- Remove auth type selection from UI - servers will auto-detect OAuth when needed
- Remove auto-reconnect logic and health checks to reduce complexity
- Always initialize OAuth provider for all connections
- Simplify transport selection - always try streamable-http first, then SSE
- Remove unused config options (transport preference, auto-reconnect)
- Add TODO comment about CORS issues for failed fetch errors
- Make authType and transport optional in connection types
- Clean up connection state notifications for better UI updates

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
- Update OAuth callbacks to use query parameters instead of path routing
- Change redirect URIs to use current page URL (works with any base path)
- Add callback type prefix to state parameter (inference: or mcp:)
- Update App.tsx to route OAuth callbacks based on query params
- Add base path configuration to vite.config.ts for GitHub Pages
- Now works with static hosting (GitHub Pages, python http.server, etc)

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
- Update MCP OAuth state expiration in connection.ts
- Update OpenRouter OAuth state expiration in oauth-provider.ts
- Addresses security review feedback to minimize window for potential attacks
- Add InMemoryTransport support for local servers running in the same process
- Update MCPConnectionManager to pass full connection object in callbacks for better context
- Filter out local servers from persistence (they auto-connect on startup)
- Add 'In-Memory' badge in UI to distinguish local servers
- Fix connection state management to properly update connectionsRef
- Auto-connect to available in-memory servers on startup

This enables testing MCP functionality without external servers by running
servers directly in the browser using the InMemoryTransport from the SDK.
- Explain how to create and register in-memory servers
- Provide examples using the Server class and tool() method
- Include best practices and debugging tips
- Document limitations of in-memory servers
- Add complete calculator server example
- Create GitHub Actions workflow for automatic deployment on main branch pushes
- Configure Vite base path for GitHub Pages hosting
- Set up build and deploy pipeline with proper permissions

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

Comment on lines +11 to +34
runs-on: ubuntu-latest

steps:
- name: Checkout
uses: actions/checkout@v4

- name: Setup Node.js
uses: actions/setup-node@v4
with:
node-version: '18'
cache: 'npm'

- name: Install dependencies
run: npm ci

- name: Build
run: npm run build

- name: Upload artifact
uses: actions/upload-pages-artifact@v3
with:
path: ./dist

deploy:

Check warning

Code scanning / CodeQL

Workflow does not contain permissions Medium

Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {contents: read}

Copilot Autofix

AI 4 days ago

To fix the issue, we need to add a permissions block to the build job in the workflow file. The permissions should be set to the least privilege required for the job to function correctly. Since the build job only needs to read the repository contents, we can set contents: read as the permission.

The changes will be made to the .github/workflows/deploy.yml file:

  1. Add a permissions block under the build job.
  2. Set contents: read as the permission for the build job.
Suggested changeset 1
.github/workflows/deploy.yml

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml
--- a/.github/workflows/deploy.yml
+++ b/.github/workflows/deploy.yml
@@ -11,2 +11,4 @@
     runs-on: ubuntu-latest
+    permissions:
+      contents: read
     
EOF
@@ -11,2 +11,4 @@
runs-on: ubuntu-latest
permissions:
contents: read

Copilot is powered by AI and may make mistakes. Always verify output.
- Add @types/node dependency for NodeJS namespace types
- Remove unsupported 'title' property from MCP tool registration
- Add Node.js types to tsconfig.json for proper type resolution
- Ensure build succeeds for GitHub Pages deployment

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
jerome3o-anthropic and others added 2 commits July 4, 2025 15:53
- Set base path to '/' for modelcontextprotocol.github.io root domain
- Remove conditional path logic for cleaner configuration

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
- Restore conditional base path for /example-remote-client/ subdirectory
- Keep current GitHub Pages setup at subdirectory URL

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
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.

1 participant