Skip to content

fix(mcp): Error handling when server is not enabled in MCP Apis (#8757)#8766

Merged
preetriti1 merged 1 commit intohotfix/v5.262from
priti/cpcp
Jan 30, 2026
Merged

fix(mcp): Error handling when server is not enabled in MCP Apis (#8757)#8766
preetriti1 merged 1 commit intohotfix/v5.262from
priti/cpcp

Conversation

@preetriti1
Copy link
Contributor

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

Error handling in listing mcp servers make sure UX is not frozen when API throws and users can continue editing.
Styling updates based on designs.
Added logging for unexpected error and gracefully handling it in UI.
Updated the property name to enable mcp servers in hostconfig correctly which was causing issues in new app.

Impact of Change

  • Users: End-users will no longer see indefinite loading when MCP servers are disabled (good).
  • Developers: hostConfig property rename (enabled -> enable) is the bug fix which was causing test page to not load first created servers in new app correctly. This was only a test page issue and not a breaking change.
  • System: The new logging call will capture any errors being thrown while loading blade but will continue to load UX components so users can continue editing.

Test Plan

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

Contributors

@kewear

Screenshots/Videos

Co-authored-by: Priti Sambandam <psamband@microsoft.com>
@github-actions
Copy link

github-actions bot commented Jan 30, 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: fix(mcp): Error handling when server is not enabled in MCP Apis (#8757)
  • Issue: Minor stylistic/clarity issues: "Apis" should be capitalized as "APIs"; the trailing (#8757) is unnecessary/redundant in the PR title and may confuse the reader. The title can be more specific about the outcome (e.g., avoiding UI freeze) and the change (error handling + logging + hostConfig rename).
  • Recommendation: Use a concise, descriptive title without referencing another PR number. Example suggestions:
    • fix(mcp): Gracefully handle MCP servers-disabled error and avoid UI freeze
    • fix(mcp): Handle MCP server-not-enabled error, add logging, and fix hostConfig property

Commit Type

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

Risk Level

  • The PR has the risk:medium label and the PR body also selects Medium. These match and are appropriate given the hostConfig property renaming and behavior changes to MCP server handling.

What & Why

  • Current: "Error handling in listing mcp servers make sure UX is not frozen when API throws and users can continue editing. Styling updates based on designs. Added logging for unexpected error and gracefully handling it in UI. Updated the property name to enable mcp servers in hostconfig correctly which was causing issues in new app."
  • Issue: Good overall. Consider making the hostConfig renaming detail explicit and aligned with the code change (property path changed and field name changed from enabled to enable).
  • Recommendation: Expand one sentence to explicitly call out the code change: e.g., "Updated hostConfig property usage: moved to hostConfig.properties.extensions.workflow.McpServerEndpoints and renamed flag from enabled -> enable to match runtime schema."

Impact of Change

  • Impact description is present and generally accurate.
  • Recommendation: Add a short note about the hard-coded test App ID and location change found in McpServer.tsx (if that file is intended only for local/dev testing): either mark as test/dev artifact or remove/parameterize it. Example: Note: McpServer.tsx contains a hardcoded appId and location used for dev/test; please confirm this is intentional.

Test Plan

  • Unit tests are present in the diff (libs/designer/src/lib/core/mcp/utils/test/queries.spec.tsx) and cover the error handling, success cases, defaulting of enabled, and query key normalization. This aligns with the PR claim of unit tests updated.
  • E2E tests are not present (PR marks E2E as not updated). Manual testing is checked in the PR; acceptable for this change but consider adding an E2E test if this behavior is critical in production flows.

Contributors

  • @kewear is mentioned. Good to credit contributors.

⚠️ Screenshots/Videos

  • Not provided. This is fine for non-visual or small styling changes; if UI behavior changed significantly (e.g., layout/padding differences), consider adding a screenshot to help reviewers validate visual impact.

Summary Table

Section Status Recommendation
Title ⚠️ Remove (#8757), fix capitalization, and make more descriptive.
Commit Type No change needed.
Risk Level No change needed (risk:medium).
What & Why Clarify hostConfig rename and exact property path.
Impact of Change Add note about hard-coded appId/location in McpServer.tsx.
Test Plan Unit tests present; consider E2E for critical flows.
Contributors No change needed.
Screenshots/Videos ⚠️ Optional: add if UI change is significant.

Summary:
The PR meets the repository's PR body template and includes unit tests that cover the updated error handling behavior. Labels match the selected risk level. I recommend small improvements to the PR title and brief clarifications in the What & Why and Impact sections (see recommendations above). Also confirm whether the hard-coded appId/location in McpServer.tsx is intentional (dev/test only) or should be removed/parameterized.

Please update the PR title and optionally the PR body per the suggestions, then remove the needs-pr-update label if no further changes to the PR description are necessary. Thank you for the thorough test coverage and clear description!


Last updated: Fri, 30 Jan 2026 18:18:35 GMT

@preetriti1 preetriti1 added the risk:medium Medium risk change with potential impact label Jan 30, 2026
@github-actions
Copy link

📊 Coverage check completed. See workflow run for details.

@preetriti1 preetriti1 merged commit e24545a into hotfix/v5.262 Jan 30, 2026
10 of 11 checks passed
@preetriti1 preetriti1 deleted the priti/cpcp branch January 30, 2026 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-validated risk:medium Medium risk change with potential impact

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants