Skip to content

Conversation

@preetriti1
Copy link
Contributor

@preetriti1 preetriti1 commented Jan 23, 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

Adding UI wizard to list existing mcp servers in standard logic app as first class experience. This also allows to use existing workflows or add new workflows. Authentication for MCP servers can also be setup through this experience.
This is much needed experience needed for users to view their current servers in standard apps.

Design link - https://www.figma.com/design/Qm4OobAv8qyiCLziYo6w6n/LA--MCP-Server-Experience?node-id=250-156508&p=f&t=DgBYlwpBQEhzDirw-0

Impact of Change

  • Users: New portal experience to view/edit/create mcp servers in app
  • Developers: New hooks are added, used only existing client service to make ad-hoc calls to service apis, did not warrant a new client service. Added inline updates to query cache without he need to refetch data on server updates. Also added code to invalidate queries on create scenarios. Generic functions in resource service is heavily used for crud operations in the app
  • System: There are direct updates to host.json and mcpservers.json files in this UI with small interactions, this may or may not restart the app. The user needs Contributor permission to make any successful updates in this UI like any other write blade on app. This change is not different from other blades because calls to deployworkflowartifacts also happen there and changes to app or restarts are expected.

There are some minor refactoring changes due to which new files are added in the changelist like McpWizard.tsx, will not be writing the tests for that file in this PR. Also changes to styles.ts should not be needing any test coverage.

Test Plan

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

No tests required for css styling in styles.ts

Contributors

@bonicaayala @kewear @DevArjun23 @ecfan

Screenshots/Videos

Loading the page for LA with existing servers.
mcpinla

Using authentication
onlyauth

Loading app without any servers configured
firstload

@preetriti1 preetriti1 added the risk:medium Medium risk change with potential impact label Jan 23, 2026
Copilot AI review requested due to automatic review settings January 23, 2026 12:22
@github-actions
Copy link

github-actions bot commented Jan 23, 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(mcp): Add UI to manage MCP servers in Logic App blade
  • Issue: None — title is concise, follows conventional commit style and clearly states the feature and scope (mcp / Logic App blade).
  • Recommendation: Keep as-is. If you want to be even more explicit for readers, you can include a short note that this adds a standalone preview route and designer integration (example: feat(mcp): Add UI to manage MCP servers (standalone + designer integration)).

Commit Type

  • Properly selected (feature). Only one box is checked which is correct.
  • Note: If the PR contains multiple independent changes in the same branch in future, consider splitting into smaller PRs (you already mention some refactor files were added which is fine here).

Risk Level

  • The PR body selects High and the repository label risk:high is present.
  • Assessment: The label matches the PR body. Advised risk level (based on code diff): High — the change touches host configuration, deployWorkflowArtifacts flows, host.json writes, and many core/designer/shared libraries and runtime interactions, and therefore deserves careful review and validation.

What & Why

  • Current: "Adding UI wizard to list existing mcp servers in standard logic app as first class experience... Authentication for MCP servers can also be setup through this experience..."
  • Issue: None — the section explains what was added and links to design. It is clear and includes the rationale.
  • Recommendation: Consider adding a brief bullet about where the new entry points are exposed (standalone route /mcpserver, McpServer panel, and designer integration points) to make the impact more explicit for reviewers.

Impact of Change

  • The PR body contains Users/Developers/System sections and mentions host.json and mcpservers.json updates and app restart possibility. Good coverage.
  • Recommendation:
    • Users: Keep as stated — new UI to view/edit/create MCP servers. Consider adding a small note that generated API keys are shown only once (you already show text in i18n).
    • Developers: Add an explicit note that the PR introduces hostConfig writing and uses ResourceService.executeResourceAction (deployWorkflowArtifacts) so calls that mutate app host config are present and will require reviewers to verify permissions and error handling.
    • System: Add a reminder that saving host.json might restart an app and appropriate telemetry or user messaging is advisable.

Test Plan

  • Current claim: Unit tests added/updated, Manual testing completed.
  • Assessment: Verified in the diff — there are numerous unit tests added across the new components and utilities (many test files). Good.
  • Recommendation: Please ensure that tests cover the hostConfig update scenarios and error paths in updateMcpServers and updateAuthSettings. Those functions write host.json/mcpservers.json and call deployWorkflowArtifacts; consider a test/mocking strategy that covers success and failure branches (you already added extensive tests in many files but double-check coverage for these mutation flows).

Contributors

  • The PR lists contributors (@bonicaayala @kewear @DevArjun23 @ecfan). This is good — keep the list up to date.
  • Recommendation: none required. If there are other reviewers/PMs/designers who provided feedback, consider tagging them as well.

Screenshots/Videos

  • Screenshots are attached in the PR body and illustrate the experience. Good.
  • Recommendation: none required.

Summary Table

Section Status Recommendation
Title Good: keep as-is or optionally mention route/designer integration.
Commit Type Correct (feature).
Risk Level Correctly set to High and label present.
What & Why Good; consider listing entry points (routes/components).
Impact of Change Add explicit note about host.json writes, app restart implications, and permission scope.
Test Plan Tests added — ensure hostConfig mutation/error branches are covered.
Contributors Good (list present).
Screenshots/Videos Good (images attached).

Final notes and required actions

  • PASS: This PR passes title and body template checks.
  • Risk: Advised risk level from the code diff is High and matches what you declared. I have no change to recommend for the PR-level risk label.

Important code observations found in the diff (action items — please address before merge):

  • Hardcoded test resource id: apps/Standalone/src/mcp/app/McpServer.tsx contains a hardcoded appId constant (a full subscription/resourceGroup/site resource id). This looks like a hard-coded test stub and must be removed or replaced with the actual resource id coming from the resourceDetails / props in production code. Leaving a hardcoded id in production code is a significant issue and increases risk.
    • Recommendation: Use resourceDetails (already computed) or the ArmParser on the real resource ID passed in environment; remove the literal test GUID and subscription from the committed code.
  • Window.alert / console.log in production component: McpServer.tsx uses window.alert and console.log for user feedback. These are not acceptable in production UI code — replace with toaster/toast notifications or the app's standard notification/toast component. Remove console.log statements or use the LoggerService if needed.
  • Secrets and API keys: There are flows around generating API keys (generateKeys) which call hostruntime endpoints and return API key in response headers. Make sure: keys are NOT stored in logs, not stored in any accessible storage; the UI already says keys shown only once — keep that and ensure telemetry does not log keys. Confirm tests for generateKeys cover success/failure and header extraction (you have tests for this — good).
  • saveWorkflowStandard/updateMcpServers: Those functions now accept and write hostConfig. Make sure the tests cover the scenarios that enable/disable McpServerEndpoints.enabled and the code path that may trigger host restarts. Also ensure required permissions are documented: PR body calls out Contributor permission, which is good — ensure any UX indicates the required role to users.
  • Add unit tests for the critical mutation functions: updateMcpServers, updateAuthSettings, generateKeys (I see unit tests added for many of those — please verify they are included in CI). If there are files intentionally left without tests (you mentioned McpWizard.tsx), justify that in the PR description (you already noted not writing tests for one file; add a reason and a follow-up issue/PR to add them later).
  • Remove any temporary placeholders (e.g., Provider store={mcpStore} usage — ensure mcpStore is imported/defined or the Provider uses the right store) and verify no stray test-only constants were committed.

Please update the PR if you can to address the above code issues and add a short note in the PR body describing fixes or follow-up items for anything you cannot change in this PR (for example, if McpWizard tests are intentionally deferred, link a tracking issue).

Thank you — once the small code hygiene issues above are resolved (hardcoded appId, window.alert usage, console.log removal, and confirming mutation tests are present in CI), the PR looks well-formed and ready for a full code review/merge.

Please update the PR body or code with the recommended fixes above, then re-submit. Thank you for the thorough submission and tests — great coverage overall!


Last updated: Fri, 23 Jan 2026 18:41:42 GMT

@preetriti1 preetriti1 changed the title feat(mcp): Adding new experience for adding/listing mcp servers in logic app blade feat(mcp): Add UI to manage MCP servers in Logic App blade Jan 23, 2026
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 adds a comprehensive UI experience for managing MCP (Model Context Protocol) servers in Azure Logic Apps' standard SKU. The implementation allows users to view, create, edit, and configure authentication for MCP servers through a new wizard-based interface.

Changes:

  • Adds MCP server management UI wizard with CRUD operations
  • Implements authentication configuration (API key, OAuth, anonymous)
  • Adds API key generation functionality with expiration options
  • Moves validation functions from helper.ts to dedicated server.ts file
  • Includes comprehensive unit test coverage for new components
  • Adds new query hooks for fetching MCP servers and eligible workflows

Reviewed changes

Copilot reviewed 44 out of 45 changed files in this pull request and generated 11 comments.

Show a summary per file
File Description
libs/logic-apps-shared/src/utils/src/lib/models/mcp.ts New McpServer interface definition
libs/designer/src/lib/core/mcp/utils/server.ts Server validation and authentication management functions
libs/designer/src/lib/core/mcp/utils/queries.ts React Query hooks for MCP server data
libs/designer/src/lib/ui/mcp/wizard/mcpservers.tsx Main wizard component for server management
libs/designer/src/lib/ui/mcp/servers/*.tsx Server list, authentication, and modal components
libs/designer/src/lib/ui/mcp/panel/server/*.tsx Panel components for creating/editing servers and generating keys
libs/designer/src/lib/ui/mcp/servers/test/*.spec.tsx Comprehensive unit tests for server components
libs/designer/src/lib/ui/panel/connectionsPanel/createConnection/*.tsx Formatting changes only (quote style, line breaks)
apps/Standalone/src/designer/app/AzureLogicAppsDesigner/Services/WorkflowAndArtifacts.tsx Integration with workflow save functionality
apps/Standalone/src/mcp/app/McpServer.tsx New standalone test harness for MCP server UI

@github-actions
Copy link

📊 Coverage check completed. See workflow run for details.

@preetriti1 preetriti1 added risk:high High risk change requiring careful review and removed risk:medium Medium risk change with potential impact labels Jan 23, 2026
@github-actions
Copy link

📊 Coverage check completed. See workflow run for details.

1 similar comment
@github-actions
Copy link

📊 Coverage check completed. See workflow run for details.

@github-actions
Copy link

📊 Coverage check completed. See workflow run for details.

@preetriti1 preetriti1 enabled auto-merge (squash) January 23, 2026 19:35
@ecfan
Copy link
Member

ecfan commented Jan 23, 2026

#assign:ecfan

@preetriti1 preetriti1 merged commit 38025fb into main Jan 23, 2026
13 checks passed
@preetriti1 preetriti1 deleted the priti/lamcp branch January 23, 2026 19:46
@ecfan
Copy link
Member

ecfan commented Jan 23, 2026

@preetriti1: I was going to review the strings in this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-validated risk:high High risk change requiring careful review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants