Skip to content

Conversation

@waleedlatif1
Copy link
Collaborator

Summary

  • reuse sessionID for consecutive MCP tool calls
  • fix dynamic args clearing
  • fix refreshing tools on save

Type of Change

  • Bug fix

Testing

Tested manually

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel
Copy link

vercel bot commented Dec 2, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Preview Comments Updated (UTC)
docs Skipped Skipped Dec 2, 2025 8:37pm

@waleedlatif1 waleedlatif1 marked this pull request as ready for review December 2, 2025 20:30
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Dec 2, 2025

Greptile Overview

Greptile Summary

This PR implements session ID reuse for consecutive MCP tool calls, fixes dynamic parameter clearing, and improves autofill prevention in form inputs.

Key changes:

  • Added session caching in McpService to reuse session IDs across consecutive tool calls, with fallback to fresh connections on session errors
  • Fixed updateParameter logic to properly remove parameters when cleared (empty string now deletes the key rather than setting empty value)
  • Added browser autofill prevention using readonly-on-focus pattern and hidden dummy inputs
  • Added serverId to create server mutation response

Issues found:

  • Math.random() generates unstable input names on every render, which could cause form state problems
  • Session cache has no cleanup mechanism for deleted/disabled servers, leading to potential memory leaks
  • Session error detection logic is overly broad and could match unrelated errors
  • Unsafe type casting in getSessionId() bypasses TypeScript type safety

Confidence Score: 2/5

  • This PR introduces logical bugs that need to be fixed before merging
  • The session restoration logic is well-intentioned but has critical issues: random input names will cause instability, session cache lacks cleanup leading to memory leaks, and overly broad error detection could trigger incorrect fallback behavior. The dynamic args clearing fix is correct.
  • Pay close attention to apps/sim/lib/mcp/service.ts (session cache cleanup) and mcp-dynamic-args.tsx (Math.random() in input names)

Important Files Changed

File Analysis

Filename Score Overview
apps/sim/lib/mcp/client.ts 3/5 Added sessionID parameter to constructor and getSessionId() method with unsafe type casting
apps/sim/lib/mcp/service.ts 2/5 Implemented session caching and restoration with overly broad error detection and missing cleanup logic
apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/mcp-dynamic-args/mcp-dynamic-args.tsx 2/5 Fixed empty value clearing logic, added autofill prevention, but uses Math.random() for input names causing instability

Sequence Diagram

sequenceDiagram
    participant User
    participant Component as MCP Dynamic Args
    participant Service as MCP Service
    participant Client as MCP Client
    participant Transport as StreamableHTTPTransport
    participant Server as MCP Server

    User->>Component: Enter tool parameter
    Component->>Component: updateParameter()
    Note over Component: Check if value empty<br/>Clear param if empty<br/>Otherwise update args

    User->>Service: Execute MCP tool
    Service->>Service: getCachedSessionId(serverId)
    alt Session ID cached
        Service->>Client: new McpClient(config, policy, cachedSessionId)
        Client->>Transport: new StreamableHTTPTransport(url, {sessionId})
        Client->>Server: connect() with cached session
        alt Session valid
            Server-->>Client: Connected
            Client->>Service: getSessionId()
            Service->>Service: cacheSessionId(serverId, sessionId)
        else Session invalid/expired
            Server-->>Client: Session error
            Service->>Service: isSessionError() checks message
            Service->>Service: clearCachedSessionId(serverId)
            Service->>Client: new McpClient(config, policy) [fresh]
            Client->>Transport: new StreamableHTTPTransport(url)
            Client->>Server: connect() without session
            Server-->>Client: Connected
            Client->>Service: getSessionId()
            Service->>Service: cacheSessionId(serverId, newSessionId)
        end
    else No cached session
        Service->>Client: new McpClient(config, policy)
        Client->>Transport: new StreamableHTTPTransport(url)
        Client->>Server: connect()
        Server-->>Client: Connected
        Client->>Service: getSessionId()
        Service->>Service: cacheSessionId(serverId, sessionId)
    end
    
    Service->>Client: callTool(toolCall)
    Client->>Server: Execute tool
    Server-->>Client: Tool result
    Client-->>Service: Tool result
    Service->>Client: disconnect()
    Service-->>User: Tool result
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

4 files reviewed, 5 comments

Edit Code Review Agent Settings | Greptile

@waleedlatif1 waleedlatif1 merged commit 3b4f227 into staging Dec 2, 2025
3 checks passed
@waleedlatif1 waleedlatif1 deleted the fix/mcp branch December 2, 2025 20:38
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