Skip to content

Conversation

@anthony0t
Copy link
Contributor

@anthony0t anthony0t commented Jan 16, 2026

Commit Type

  • feature - New functionality
  • fix - Bug fix
  • refactor - Code restructuring without behavior change
  • perf - Performance improvement
  • docs - Documentation update
  • test - Test-related changes
  • chore - Maintenance/tooling

Risk Level

  • Low - Minor changes, limited scope
  • Medium - Moderate changes, some user impact
  • High - Major changes, significant user/system impact

What & Why

port v1 code to v2 for adding mcp servers redux action. This is pure new code specific to mcp server support and doesn't impact existing scenarios.

Impact of Change

  • Users:
    No impact. new code path is not used yet but will be executed once we enabled UI for adding MCP servers
  • Developers:
  • System:

Test Plan

  • Unit tests added/updated
  • E2E tests added/updated
  • Manual testing completed
  • Tested in: local

Contributors

Screenshots/Videos

Copilot AI review requested due to automatic review settings January 16, 2026 00:44
@anthony0t anthony0t added the risk:low Low risk change with minimal impact label Jan 16, 2026
@github-actions
Copy link

github-actions bot commented Jan 16, 2026

🤖 AI PR Validation Report

PR Review Results

Thank you for your submission! Here's detailed feedback on your PR title and body compliance:

PR Title

  • Current: feat(designerv2): port v1 add MCP servers logic (addMcpServer, connection handling)
  • Issue: Title is generally clear and follows conventional commit style. Minor nit: punctuation/capitalization and a slightly more specific scope of change could improve discoverability.
  • Recommendation: Keep the conventional commit prefix. Example improvement: feat(designerv2): port v1 add MCP server logic (addMcpServer, connection handling) or feat(designerv2): port v1 add MCP servers logic — addMcpServer & connection handling

Commit Type

  • Properly selected (feature).
  • Only one commit type selected, which is correct.

⚠️ Risk Level

  • Assessment: The PR is labeled and marked in the body as Low risk. However, the code diff reveals changes to core workflow actions: adding addMcpServer flow, modifying addOperation and initializeOperationDetails, and altering connection selection/setup logic. These touch node addition and connection setup behavior and introduce new native MCP handling — this has broader impact surface than a small, isolated change.
  • Recommendation: Please update the PR label and the Risk Level section in the PR body to Medium (and add a short justification). Example: Medium - Changes workflow node creation and connection setup logic; affects connection initialization and node manifest handling.

What & Why

  • Current: "port v1 code to v2 for adding mcp servers redux action. This is pure new code specific to mcp server support and doesn't impact existing scenarios."
  • Issue: Concise and present. However, the claim "doesn't impact existing scenarios" contradicts the risk assessment because the diff changes addOperation and initializeOperationDetails pathing for some operations and connection handling.
  • Recommendation: Expand slightly to explain which code paths are new vs changed and call out the new native MCP handling. Example: "Port v1 addMcpServer code to v2. Adds flow for managed/native MCP clients and extends connection initialization (trySetDefaultConnectionForNode) to accept a preferred connectionId. Existing normal operation flows are intended to be unchanged except where explicit MCP handling is added."

⚠️ Impact of Change

  • The Impact section is missing developer and system details and should be filled in.
  • Recommendation: Provide short bullets describing the impact. Example suggestions to put in the body:
    • Users: No immediate UI changes until the UI flag is enabled; once enabled, users can add MCP servers via the new flow.
    • Developers: Adds addMcpServer action, extends addOperation/initializeOperationDetails signatures (new connectionId param), and modifies connection selection behavior (preferredConnectionId). Reviewers should be aware of the new public function signatures and side effects.
    • System: May affect connection initialization flow and node initialization performance; consider monitoring/telemetry around connection setup and node add operations after merge.

Test Plan

  • Assessment: Unit tests were added (new test file at libs/designer-v2/src/lib/core/actions/bjsworkflow/test/add.spec.ts) — good.
  • Note: E2E tests were not added. If this flow will be enabled in UI soon, consider adding an E2E or integration test to cover the end-to-end addMcpServer + connection setup scenario. At minimum, reference how manual test steps exercise the new flow.

⚠️ Contributors

  • Current: blank.
  • Recommendation: Not required, but please add contributors (PMs, designers, reviewers) if applicable to help credit work.

Screenshots/Videos

  • Assessment: Not applicable (non-visual change). No screenshots required.

Summary Table

Section Status Recommendation
Title Minor wording/capitalization improvement suggested
Commit Type No change needed
Risk Level ⚠️ Update to Medium and adjust label risk:lowrisk:medium
What & Why Expand a sentence to clarify which paths change vs are new
Impact of Change ⚠️ Fill in Users / Developers / System bullets (see recommendation)
Test Plan Unit tests present. Consider adding E2E/integration if UI will enable soon
Contributors ⚠️ Optional: add contributors where applicable
Screenshots/Videos Not required for this change

Final notes

  • Overall: The PR title and body largely follow the required template and the unit tests are present — passing the checklist. I recommend updating the risk label to risk:medium (both the PR label and the Risk Level section in the body) because the changes modify core add-operation and connection initialization flows. Also expand the Impact of Change section (developers/system) and optionally add an E2E or integration test if this change will be enabled in the UI soon.

Please update the PR description/labels as recommended and re-submit. Thanks for the thorough work and for including unit tests!


Last updated: Fri, 16 Jan 2026 00:51:57 GMT

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR ports MCP (Model Context Protocol) server management code from v1 to v2, specifically adding the ability to handle managed MCP client operations with preferred connection selection. The new code path is inactive until UI is enabled.

Changes:

  • Added support for managed MCP client operations with custom parameter handling
  • Introduced preferred connection selection for MCP servers
  • Restructured node addition logic to handle MCP servers separately from agent tools

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
libs/designer-v2/src/lib/core/actions/bjsworkflow/add.ts Core implementation for MCP server addition, including managed MCP client handling and preferred connection ID support
libs/designer-v2/src/lib/core/actions/bjsworkflow/test/add.spec.ts Comprehensive unit tests for preferred connection selection logic

@anthony0t anthony0t changed the title feat(designerv2): portal v1 code to v2 for adding mcp servers feat(designerv2): port v1 add MCP servers logic (addMcpServer, connection handling) Jan 16, 2026
@Azure Azure deleted a comment from Copilot AI Jan 16, 2026
@Azure Azure deleted a comment from Copilot AI Jan 16, 2026
@Azure Azure deleted a comment from Copilot AI Jan 16, 2026
@Azure Azure deleted a comment from Copilot AI Jan 16, 2026
@github-actions
Copy link

📊 Coverage check completed. See workflow run for details.

@anthony0t anthony0t merged commit 5f0fd1f into main Jan 16, 2026
24 of 27 checks passed
@anthony0t anthony0t deleted the tonytang/add branch January 16, 2026 05:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-validated risk:low Low risk change with minimal impact

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants