ref: refactor MCP-related functionality centralizing common utilities#9059
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes refactor MCP server handler logic by moving all internal implementations from Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant MCP_Handler (mcp.py/mcp_projects.py)
participant MCP_Support
Client->>Router: HTTP Request (e.g., list resources, call tool)
Router->>MCP_Handler: Route to handler
MCP_Handler->>MCP_Support: Call centralized handler (e.g., handle_list_resources)
MCP_Support-->>MCP_Handler: Result (resources, tool output, etc.)
MCP_Handler-->>Router: Return response
Router-->>Client: Response
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
edwinjosechittilappilly
changed the title
edwinjosechittilappilly
requested review from
erichare,
jordanrfrazier,
lucaseduoli and
phact
|
Known Issues: MCP With Cursor Install not working, same experience as Main. |
|
Next Step I am planning to have a Test suite for MCP! |
edwinjosechittilappilly
changed the title
edwinjosechittilappilly
changed the title
edwinjosechittilappilly
removed request for
erichare,
jordanrfrazier,
lucaseduoli and
phact
|
Testing Ongoing! |
dosubot
Bot
added
size:XL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/backend/base/langflow/api/v1/mcp_support.py (3)
91-96: Simplify conditional assignment with ternary operator.The if-else block can be simplified using a ternary operator as suggested by static analysis.
- # Build query based on whether project_id is provided - if project_id: - # Filter flows by project (folder_id) - flows_query = select(Flow).where(Flow.folder_id == project_id) - else: - # Get all flows - flows_query = select(Flow) + # Build query based on whether project_id is provided + flows_query = ( + select(Flow).where(Flow.folder_id == project_id) + if project_id + else select(Flow) + )
136-137: Remove unnecessary variable assignment.The variable
twois only used once and adds no clarity.- # The flow_id and filename should be the last two parts - two = 2 - if len(path_parts) < two: + # The flow_id and filename should be the last two parts + if len(path_parts) < 2:
323-332: Consider extracting unique name generation logic.The logic for generating unique names with numeric suffixes is complex and could be extracted to a helper function for better readability and reusability.
Would you like me to create a helper function to handle the unique name generation with numeric suffixes? This would improve code readability and make the logic reusable.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/backend/base/langflow/api/v1/mcp.py(2 hunks)src/backend/base/langflow/api/v1/mcp_projects.py(3 hunks)src/backend/base/langflow/api/v1/mcp_support.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/backend/**/*.py
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .cursor/rules/backend_development.mdc
🧠 Learnings (1)
src/backend/base/langflow/api/v1/mcp.py (4)
Learnt from: CR
PR: langflow-ai/langflow#0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-06-30T14:41:58.849Z
Learning: Applies to {src/backend/tests/**/*.py,tests/**/*.py} : Use 'anyio' and 'aiofiles' for async file operations in tests.
Learnt from: CR
PR: langflow-ai/langflow#0
File: .cursor/rules/backend_development.mdc:0-0
Timestamp: 2025-06-30T14:39:17.464Z
Learning: Applies to src/backend/base/langflow/components/**/*.py : Use asyncio.Queue for non-blocking queue operations in async components and handle timeouts appropriately
Learnt from: CR
PR: langflow-ai/langflow#0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-06-30T14:41:58.849Z
Learning: Applies to {src/backend/tests/**/*.py,tests/**/*.py} : Be aware of ContextVar propagation in async tests and test both direct event loop execution and 'asyncio.to_thread' scenarios.
Learnt from: CR
PR: langflow-ai/langflow#0
File: .cursor/rules/backend_development.mdc:0-0
Timestamp: 2025-06-30T14:39:17.464Z
Learning: Context variables may not propagate correctly in asyncio.to_thread; test both patterns
🪛 GitHub Check: Ruff Style Check (3.13)
src/backend/base/langflow/api/v1/mcp_support.py
[failure] 283-283: Ruff (FBT002)
src/backend/base/langflow/api/v1/mcp_support.py:283:46: FBT002 Boolean default positional argument in function definition
[failure] 163-163: Ruff (FBT002)
src/backend/base/langflow/api/v1/mcp_support.py:163:58: FBT002 Boolean default positional argument in function definition
[failure] 91-96: Ruff (SIM108)
src/backend/base/langflow/api/v1/mcp_support.py:91:13: SIM108 Use ternary operator flows_query = select(Flow).where(Flow.folder_id == project_id) if project_id else select(Flow) instead of if-else-block
🪛 GitHub Actions: Ruff Style Check
src/backend/base/langflow/api/v1/mcp_support.py
[error] 91-91: Ruff SIM108: Use ternary operator flows_query = select(Flow).where(Flow.folder_id == project_id) if project_id else select(Flow) instead of if-else-block.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Check PyPI Version Update
🔇 Additional comments (8)
src/backend/base/langflow/api/v1/mcp_projects.py (4)
24-31: LGTM! Clean import organization.The imports from the centralized
mcp_support.pymodule are well-organized and include all the necessary utilities for MCP operations.
45-47: Good use of context variables for project-specific state.The introduction of
current_project_ctxappropriately manages project-specific state alongside the user context.
653-655: Clean delegation to centralized handler.The refactored handler correctly delegates to the centralized
handle_list_toolsfunction with appropriate parameters for project-specific filtering.
662-675: Excellent refactoring of MCP handlers.All project-specific handlers now properly delegate to their centralized counterparts in
mcp_support.py, passing the project ID and other necessary parameters. This significantly reduces code duplication and improves maintainability.src/backend/base/langflow/api/v1/mcp.py (1)
46-67: Clean delegation pattern for global MCP handlers.All global MCP handlers have been successfully refactored to delegate to the centralized implementations in
mcp_support.py. The handlers correctly omit the project_id parameter for global operations.src/backend/base/langflow/api/v1/mcp_support.py (3)
1-4: Good module documentation.The module docstring clearly explains its purpose as the centralized source for MCP functionality.
197-201: Excellent progress notification implementation.The progress notification logic properly checks for both the configuration flag and the presence of a progress token before sending notifications. The async task management with proper cancellation handling is well implemented.
238-255: Well-designed result deduplication logic.The use of a set to track processed texts ensures no duplicate results are returned, and the handling of different result types (messages vs other values) is comprehensive.
|
|
||
| def get_mcp_config(): | ||
| return MCPConfig() | ||
| # Common MCP utilities now imported from mcp_projects.py |
There was a problem hiding this comment.
Fix incorrect comment.
The comment incorrectly states that utilities are imported from mcp_projects.py, but they are actually imported from mcp_support.py.
-# Common MCP utilities now imported from mcp_projects.py
+# Common MCP utilities now imported from mcp_support.py📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Common MCP utilities now imported from mcp_projects.py | |
| # Common MCP utilities now imported from mcp_support.py |
🤖 Prompt for AI Agents
In src/backend/base/langflow/api/v1/mcp.py at line 23, the comment incorrectly
states that common MCP utilities are imported from mcp_projects.py. Update the
comment to correctly reflect that these utilities are imported from
mcp_support.py.
| async def handle_call_tool( | ||
| name: str, arguments: dict, server, project_id=None, is_action=False | ||
| ) -> list[types.TextContent]: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use keyword-only arguments for boolean parameters.
Boolean positional arguments can be confusing at call sites. Consider making these parameters keyword-only for better clarity.
async def handle_call_tool(
- name: str, arguments: dict, server, project_id=None, is_action=False
+ name: str, arguments: dict, server, *, project_id=None, is_action=False
) -> list[types.TextContent]:-async def handle_list_tools(project_id=None, mcp_enabled_only=False):
+async def handle_list_tools(*, project_id=None, mcp_enabled_only=False):Also applies to: 283-283
🧰 Tools
🪛 GitHub Check: Ruff Style Check (3.13)
[failure] 163-163: Ruff (FBT002)
src/backend/base/langflow/api/v1/mcp_support.py:163:58: FBT002 Boolean default positional argument in function definition
🤖 Prompt for AI Agents
In src/backend/base/langflow/api/v1/mcp_support.py at lines 162-164 and also
line 283, the boolean parameter is_action should be made keyword-only to improve
clarity at call sites. Modify the function definitions to place a * before
is_action so that it can only be passed as a keyword argument, preventing
confusion from positional boolean arguments.
There was a problem hiding this comment.
Can you explain why should we do that way?
There was a problem hiding this comment.
is it for fixing the ruff error?
There was a problem hiding this comment.
@edwinjosechittilappilly Great question! Let me explain why keyword-only boolean parameters are considered a best practice in Python:
The Problem with Positional Boolean Arguments
When boolean parameters can be passed positionally, it creates unclear code at call sites. For example:
# Current (confusing):
result = handle_call_tool("my_tool", {}, server, None, True)
# What does True mean? is_action? Some other flag?
# With multiple booleans, it gets worse:
tools = handle_list_tools(project_123, True)
# Is True for mcp_enabled_only? Hard to tell without checking the function signatureBenefits of Keyword-Only Arguments
1. Self-Documenting Code
# Much clearer with keyword-only:
result = handle_call_tool("my_tool", {}, server, is_action=True)
tools = handle_list_tools(project_id=project_123, mcp_enabled_only=True)2. Prevents Mistakes
# Easy to accidentally swap boolean values:
handle_call_tool("tool", {}, server, True, False) # Which is which?
# Keyword-only prevents this:
handle_call_tool("tool", {}, server, project_id=None, is_action=True) # Clear intent3. Better Maintainability
If you need to add more parameters or reorder them later, keyword-only parameters are much safer and less likely to break existing code.
4. Python Best Practice
This aligns with PEP 570 and general Python style guidelines that recommend keyword-only arguments for flags and optional parameters to improve code clarity.
The * forces callers to be explicit about their intent, making the code more readable and maintainable. It's a small change that significantly improves code quality!
There was a problem hiding this comment.
@edwinjosechittilappilly Yes, the suggestion does fix the Ruff error FBT002, but the error exists for good reasons related to code clarity and maintainability.
The Ruff rule FBT002 flags boolean default positional arguments because they create confusing code at call sites. For example:
Confusing (current):
# What does False mean here?
result = await handle_call_tool("my_tool", {}, server, None, False)Clear (with keyword-only):
# Much clearer what False refers to
result = await handle_call_tool("my_tool", {}, server, project_id=None, is_action=False)The keyword-only approach (* before the parameter) forces callers to be explicit about what the boolean argument represents, making the code more readable and less error-prone. This is especially important in functions with multiple parameters where the boolean's meaning isn't obvious from position alone.
So while it does fix the Ruff error, the underlying motivation is improving code quality and preventing bugs that can arise from unclear boolean arguments.
/| /|
( :v: )
|(_)|
-"-"-
Updated function calls in mcp_projects.py to use explicit keyword arguments for clarity. Refactored mcp_support.py to use more concise query assignment and added keyword-only arguments to handle_call_tool and handle_list_tools for improved code readability and maintainability.
|
@coderabbitai check the production readiness of the changes made. |
edwinjosechittilappilly
requested review from
jordanrfrazier,
lucaseduoli and
phact
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@edwinjosechittilappilly I'll analyze the production readiness of your MCP refactoring changes. Let me examine the code structure, error handling, and potential issues. Based on my analysis, here's my assessment of the production readiness of your MCP refactoring changes: 🟢 Strengths (Production Ready)
🟡 Areas Requiring Attention
🔴 Production Concerns
📋 Recommendations for Production Readiness
🎯 Next Steps
The refactoring is well-structured and follows good practices, but the missing test coverage and known issues need resolution before production deployment. 🏁 Script executed: Length of output: 10920 🏁 Script executed: Length of output: 5481 🏁 Script executed: Length of output: 6877 🏁 Script executed: Length of output: 5208 Based on my comprehensive analysis of the codebase, here's my detailed production readiness assessment: 🟢 Production-Ready Aspects1. Error Handling & Logging✅ Excellent implementation
2. Database Operations✅ Well-structured
3. Security Practices✅ Good foundation
4. Resource Management✅ Proper cleanup
5. Code Structure✅ Well-organized
🟡 Areas Needing Attention1. Input Validation
2. Configuration Management
3. Error Boundaries
🔴 Critical Production Blockers1. Missing Test Coverage
2. Known Functional Issues
3. Performance Considerations
📋 Production Readiness ChecklistCritical (Must Fix)
High Priority (Should Fix)
Medium Priority (Nice to Have)
🎯 Final Verdict❌ NOT READY FOR PRODUCTION - Critical blockers must be resolved Blocking Issues:
Recommendation:
Estimated Timeline to Production Ready: 1-2 weeks with focused effort on testing and bug fixes. The refactoring itself is well-executed and follows good practices, but the lack of test coverage and known issues make it unsuitable for production deployment at this time. 🧠 Learnings used |
|
|
||
| # Create a context variable to store the current user | ||
| current_user_ctx: ContextVar[User] = ContextVar("current_user_ctx") | ||
| # Context variable now imported from mcp_projects.py |
| from langflow.services.storage.utils import build_content_type_from_extension | ||
| from langflow.services.deps import get_settings_service, session_scope | ||
|
|
||
| # Common MCP utilities now imported from mcp_support.py |
There was a problem hiding this comment.
can remove all these comments
| @@ -0,0 +1,348 @@ | |||
| """Common MCP handler functions shared between mcp.py and mcp_projects.py. | |||
There was a problem hiding this comment.
maybe mcp_utils.py instead of support
There was a problem hiding this comment.
We have a mcp.utils.py in a different folder, but happy to rename.
|
|
||
| # Build full URL from settings | ||
| host = getattr(settings_service.settings, "host", "localhost") | ||
| port = getattr(settings_service.settings, "port", 3000) |
There was a problem hiding this comment.
Why is this port hardcoded?
There was a problem hiding this comment.
thats the default value when the value is not available.Let me cross check this with the previous implementations.
There was a problem hiding this comment.
It was the Port of the FE.
There was a problem hiding this comment.
The frontend port is only used in dev, though.
There was a problem hiding this comment.
On checking the current main found that this was utilised to frame the earlier versions of MCP URL.
Currently this is not used just kept for reverse compatibility , ideally in long run we need to deprecate the API in mcp.py
| msg = f"Error in listing tools: {e!s} from flow: {base_name}" | ||
| logger.warning(msg) | ||
| continue | ||
| except Exception as e: |
There was a problem hiding this comment.
This is fine for now, but we should be aware that catching a generic exception in such a large block isn’t ideal. Better to use more targeted try-except blocks where possible.
There was a problem hiding this comment.
Sure Adding as ToDo, post refactor changes
ogabrielluiz
changed the title
Renamed mcp_support.py to mcp_utils.py for clarity and updated all relevant import statements in mcp.py and mcp_projects.py to reflect the new module name.
…/langflow into mcp-ref-common-codes
…langflow-ai#9059) * reactor to have common mcp codes in mcp_support * [autofix.ci] apply automated fixes * Refactor MCP API argument passing and function signatures Updated function calls in mcp_projects.py to use explicit keyword arguments for clarity. Refactored mcp_support.py to use more concise query assignment and added keyword-only arguments to handle_call_tool and handle_list_tools for improved code readability and maintainability. * Rename mcp_support.py to mcp_utils.py and update imports Renamed mcp_support.py to mcp_utils.py for clarity and updated all relevant import statements in mcp.py and mcp_projects.py to reflect the new module name. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
3 participants
Purpose: Simplify the code base and a optimize the resuable elements in mcp.py and mcp_projects.py. This ensure better maintanence of the codebase.
This pull request refactors the MCP-related functionality in the backend by centralizing common utilities into a new module,
mcp_support.py. It simplifies the codebase by removing redundant implementations and consolidating logic for handling MCP server operations. Additionally, it introduces global handlers for MCP server operations to streamline functionality.Refactoring and Code Simplification:
handle_call_tool,handle_list_resources,handle_list_tools, andhandle_read_resource, are now imported frommcp_support.pyinstead of being defined redundantly in multiple files. (src/backend/base/langflow/api/v1/mcp.py, [1];src/backend/base/langflow/api/v1/mcp_projects.py, [2]handle_call_tool,handle_list_resources,handle_list_tools,handle_read_resource) and replaced them with global handlers that delegate to the centralized utilities. (src/backend/base/langflow/api/v1/mcp.py, src/backend/base/langflow/api/v1/mcp.pyL97-R67)Improved Code Organization:
current_project_ctx, to manage project-specific state. (src/backend/base/langflow/api/v1/mcp_projects.py, src/backend/base/langflow/api/v1/mcp_projects.pyL26-R49)base64,uuid4, and others to streamline the code and improve readability. (src/backend/base/langflow/api/v1/mcp.py, [1];src/backend/base/langflow/api/v1/mcp_projects.py, [2]These changes enhance maintainability by reducing duplication and improving modularity, making the MCP server logic easier to extend and debug.
Summary by CodeRabbit
Refactor
New Features
Chores