Skip to content

Commit e24545a

Browse files
preetriti1Priti Sambandam
andauthored
fix(mcp): Error handling when server is not enabled in MCP Apis (#8757) (#8766)
Co-authored-by: Priti Sambandam <psamband@microsoft.com>
1 parent 5f4c34d commit e24545a

File tree

6 files changed

+262
-20
lines changed

6 files changed

+262
-20
lines changed

apps/Standalone/src/designer/app/AzureLogicAppsDesigner/Services/WorkflowAndArtifacts.tsx

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -848,7 +848,7 @@ export const updateMcpServers = async (
848848
let updatedHostConfig: any;
849849
const queryClient = getReactQueryClient();
850850

851-
if (!areAllServersDeleted && !hostConfig?.extensions?.workflow?.McpServerEndpoints?.enabled) {
851+
if (!areAllServersDeleted && !hostConfig?.properties.extensions?.workflow?.McpServerEndpoints?.enable) {
852852
updatedHostConfig = {
853853
...(hostConfig.properties ?? {}),
854854
extensions: {
@@ -857,12 +857,12 @@ export const updateMcpServers = async (
857857
...(hostConfig.properties.extensions?.workflow ?? {}),
858858
McpServerEndpoints: {
859859
...(hostConfig.properties.extensions?.workflow?.McpServerEndpoints ?? {}),
860-
enabled: true,
860+
enable: true,
861861
},
862862
},
863863
},
864864
};
865-
} else if (areAllServersDeleted && hostConfig?.extensions?.workflow?.McpServerEndpoints?.enabled === true) {
865+
} else if (areAllServersDeleted && hostConfig?.properties.extensions?.workflow?.McpServerEndpoints?.enable === true) {
866866
updatedHostConfig = {
867867
...(hostConfig.properties ?? {}),
868868
extensions: {
@@ -871,7 +871,7 @@ export const updateMcpServers = async (
871871
...(hostConfig.properties.extensions?.workflow ?? {}),
872872
McpServerEndpoints: {
873873
...(hostConfig.properties.extensions?.workflow?.McpServerEndpoints ?? {}),
874-
enabled: false,
874+
enable: false,
875875
},
876876
},
877877
},

apps/Standalone/src/mcp/app/McpServer.tsx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,14 +17,14 @@ export const McpServer = () => {
1717
theme: state.workflowLoader.theme,
1818
}));
1919

20-
const appId = '/subscriptions/f34b22a3-2202-4fb1-b040-1332bd928c84/resourceGroups/TestACSRG/providers/Microsoft.Web/sites/prititemplates';
20+
const appId = '/subscriptions/f34b22a3-2202-4fb1-b040-1332bd928c84/resourceGroups/TestACSRG/providers/Microsoft.Web/sites/pritimcpserver';
2121
const resourceDetails = useMemo(() => {
2222
const parser = new ArmParser(appId);
2323
return {
2424
subscriptionId: parser.subscriptionId || '',
2525
resourceGroup: parser.resourceGroup || '',
2626
logicAppName: parser.resourceName || '',
27-
location: 'westus3',
27+
location: 'westus',
2828
};
2929
}, []);
3030

Lines changed: 223 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,223 @@
1+
/**
2+
* @vitest-environment jsdom
3+
*/
4+
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
5+
import { renderHook, waitFor } from '@testing-library/react';
6+
import { QueryClient, QueryClientProvider } from '@tanstack/react-query';
7+
import type { ReactNode } from 'react';
8+
import { useAllMcpServers } from '../queries';
9+
import type { McpServer } from '@microsoft/logic-apps-shared';
10+
11+
// Mock external dependencies
12+
vi.mock('@microsoft/logic-apps-shared', async (importOriginal) => {
13+
const actual = await importOriginal<typeof import('@microsoft/logic-apps-shared')>();
14+
return {
15+
...actual,
16+
ResourceService: vi.fn(),
17+
LoggerService: vi.fn(() => ({
18+
log: vi.fn(),
19+
})),
20+
};
21+
});
22+
23+
describe('useAllMcpServers', () => {
24+
let mockResourceService: any;
25+
let mockExecuteResourceAction: any;
26+
let queryClient: QueryClient;
27+
28+
const createWrapper = () => {
29+
return ({ children }: { children: ReactNode }) => <QueryClientProvider client={queryClient}>{children}</QueryClientProvider>;
30+
};
31+
32+
beforeEach(async () => {
33+
// Create a new QueryClient for each test to avoid cache issues
34+
queryClient = new QueryClient({
35+
defaultOptions: {
36+
queries: {
37+
retry: false,
38+
},
39+
},
40+
});
41+
42+
// Import mocked functions dynamically
43+
const shared = await import('@microsoft/logic-apps-shared');
44+
mockResourceService = shared.ResourceService as any;
45+
46+
mockExecuteResourceAction = vi.fn();
47+
mockResourceService.mockReturnValue({
48+
executeResourceAction: mockExecuteResourceAction,
49+
});
50+
});
51+
52+
afterEach(() => {
53+
vi.clearAllMocks();
54+
queryClient.clear();
55+
});
56+
57+
describe('successful responses', () => {
58+
it('should return MCP servers sorted by name', async () => {
59+
const mockServers: McpServer[] = [
60+
{ name: 'zebra-server', description: 'Zebra', enabled: true, tools: [] },
61+
{ name: 'alpha-server', description: 'Alpha', enabled: true, tools: [] },
62+
{ name: 'middle-server', description: 'Middle', enabled: false, tools: [] },
63+
];
64+
65+
mockExecuteResourceAction.mockResolvedValue({ value: mockServers });
66+
67+
const { result } = renderHook(() => useAllMcpServers('/subscriptions/sub1/resourceGroups/rg1/providers/Microsoft.Web/sites/myApp'), {
68+
wrapper: createWrapper(),
69+
});
70+
71+
await waitFor(() => expect(result.current.isSuccess).toBe(true));
72+
73+
expect(result.current.data).toHaveLength(3);
74+
expect(result.current.data?.[0].name).toBe('alpha-server');
75+
expect(result.current.data?.[1].name).toBe('middle-server');
76+
expect(result.current.data?.[2].name).toBe('zebra-server');
77+
});
78+
79+
it('should default enabled to true when undefined', async () => {
80+
const mockServers = [
81+
{ name: 'server1', description: 'Server 1' },
82+
{ name: 'server2', description: 'Server 2', enabled: false },
83+
{ name: 'server3', description: 'Server 3', enabled: true },
84+
];
85+
86+
mockExecuteResourceAction.mockResolvedValue({ value: mockServers });
87+
88+
const { result } = renderHook(() => useAllMcpServers('/subscriptions/sub1/resourceGroups/rg1/providers/Microsoft.Web/sites/myApp'), {
89+
wrapper: createWrapper(),
90+
});
91+
92+
await waitFor(() => expect(result.current.isSuccess).toBe(true));
93+
94+
const server1 = result.current.data?.find((s) => s.name === 'server1');
95+
const server2 = result.current.data?.find((s) => s.name === 'server2');
96+
const server3 = result.current.data?.find((s) => s.name === 'server3');
97+
98+
expect(server1?.enabled).toBe(true);
99+
expect(server2?.enabled).toBe(false);
100+
expect(server3?.enabled).toBe(true);
101+
});
102+
103+
it('should return empty array when response value is null', async () => {
104+
mockExecuteResourceAction.mockResolvedValue({ value: null });
105+
106+
const { result } = renderHook(() => useAllMcpServers('/subscriptions/sub1/resourceGroups/rg1/providers/Microsoft.Web/sites/myApp'), {
107+
wrapper: createWrapper(),
108+
});
109+
110+
await waitFor(() => expect(result.current.isSuccess).toBe(true));
111+
112+
expect(result.current.data).toEqual([]);
113+
});
114+
115+
it('should return empty array when response value is undefined', async () => {
116+
mockExecuteResourceAction.mockResolvedValue({});
117+
118+
const { result } = renderHook(() => useAllMcpServers('/subscriptions/sub1/resourceGroups/rg1/providers/Microsoft.Web/sites/myApp'), {
119+
wrapper: createWrapper(),
120+
});
121+
122+
await waitFor(() => expect(result.current.isSuccess).toBe(true));
123+
124+
expect(result.current.data).toEqual([]);
125+
});
126+
});
127+
128+
describe('error handling', () => {
129+
it('should return empty array when McpServerNotEnabled error occurs', async () => {
130+
mockExecuteResourceAction.mockRejectedValue({
131+
error: { code: 'McpServerNotEnabled' },
132+
});
133+
134+
const { result } = renderHook(() => useAllMcpServers('/subscriptions/sub1/resourceGroups/rg1/providers/Microsoft.Web/sites/myApp'), {
135+
wrapper: createWrapper(),
136+
});
137+
138+
await waitFor(() => expect(result.current.isSuccess).toBe(true));
139+
140+
expect(result.current.data).toEqual([]);
141+
});
142+
143+
it('should return empty array and log error for other errors', async () => {
144+
const shared = await import('@microsoft/logic-apps-shared');
145+
const mockLog = vi.fn();
146+
(shared.LoggerService as any).mockReturnValue({ log: mockLog });
147+
148+
mockExecuteResourceAction.mockRejectedValue({
149+
error: { code: 'SomeOtherError', message: 'Something went wrong' },
150+
});
151+
152+
const { result } = renderHook(() => useAllMcpServers('/subscriptions/sub1/resourceGroups/rg1/providers/Microsoft.Web/sites/myApp'), {
153+
wrapper: createWrapper(),
154+
});
155+
156+
await waitFor(() => expect(result.current.isSuccess).toBe(true));
157+
158+
expect(result.current.data).toEqual([]);
159+
expect(mockLog).toHaveBeenCalledWith(
160+
expect.objectContaining({
161+
level: shared.LogEntryLevel.Error,
162+
area: 'McpServer.listServers',
163+
})
164+
);
165+
});
166+
});
167+
168+
describe('query configuration', () => {
169+
it('should not execute query when siteResourceId is empty', async () => {
170+
const { result } = renderHook(() => useAllMcpServers(''), {
171+
wrapper: createWrapper(),
172+
});
173+
174+
// Query should not be loading or fetching when disabled
175+
expect(result.current.isFetching).toBe(false);
176+
expect(mockExecuteResourceAction).not.toHaveBeenCalled();
177+
});
178+
179+
it('should use lowercase siteResourceId in query key', async () => {
180+
mockExecuteResourceAction.mockResolvedValue({ value: [] });
181+
182+
const { result: result1 } = renderHook(
183+
() => useAllMcpServers('/subscriptions/SUB1/resourceGroups/RG1/providers/Microsoft.Web/sites/MYAPP'),
184+
{
185+
wrapper: createWrapper(),
186+
}
187+
);
188+
189+
await waitFor(() => expect(result1.current.isSuccess).toBe(true));
190+
191+
// The second call with different casing should use cached result
192+
const { result: result2 } = renderHook(
193+
() => useAllMcpServers('/subscriptions/sub1/resourceGroups/rg1/providers/Microsoft.Web/sites/myapp'),
194+
{
195+
wrapper: createWrapper(),
196+
}
197+
);
198+
199+
await waitFor(() => expect(result2.current.isSuccess).toBe(true));
200+
201+
// Should only be called once due to cache hit with normalized key
202+
expect(mockExecuteResourceAction).toHaveBeenCalledTimes(1);
203+
});
204+
205+
it('should call executeResourceAction with correct parameters', async () => {
206+
mockExecuteResourceAction.mockResolvedValue({ value: [] });
207+
208+
const siteResourceId = '/subscriptions/sub1/resourceGroups/rg1/providers/Microsoft.Web/sites/myApp';
209+
210+
const { result } = renderHook(() => useAllMcpServers(siteResourceId), {
211+
wrapper: createWrapper(),
212+
});
213+
214+
await waitFor(() => expect(result.current.isSuccess).toBe(true));
215+
216+
expect(mockExecuteResourceAction).toHaveBeenCalledWith(
217+
`${siteResourceId}/hostruntime/runtime/webhooks/workflow/api/management/listMcpServers`,
218+
'POST',
219+
{ 'api-version': '2024-11-01' }
220+
);
221+
});
222+
});
223+
});

libs/designer/src/lib/core/mcp/utils/queries.ts

Lines changed: 31 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@ import {
55
equals,
66
type McpServer,
77
getObjectPropertyValue,
8+
LoggerService,
9+
LogEntryLevel,
810
} from '@microsoft/logic-apps-shared';
911
import { useQuery, type UseQueryResult } from '@tanstack/react-query';
1012
import { useAzureConnectorsLazyQuery } from '../../../core/queries/browse';
@@ -26,18 +28,35 @@ export const useAllMcpServers = (siteResourceId: string) => {
2628
return useQuery({
2729
queryKey: ['mcpservers', siteResourceId.toLowerCase()],
2830
queryFn: async (): Promise<McpServer[]> => {
29-
const response: any = await ResourceService().executeResourceAction(
30-
`${siteResourceId}/hostruntime/runtime/webhooks/workflow/api/management/listMcpServers`,
31-
'POST',
32-
{ 'api-version': '2024-11-01' }
33-
);
34-
35-
return (response.value ?? [])
36-
.map((server: McpServer) => ({
37-
...server,
38-
enabled: server.enabled === undefined ? true : server.enabled,
39-
}))
40-
.sort((a: McpServer, b: McpServer) => a.name.localeCompare(b.name));
31+
try {
32+
const response: any = await ResourceService().executeResourceAction(
33+
`${siteResourceId}/hostruntime/runtime/webhooks/workflow/api/management/listMcpServers`,
34+
'POST',
35+
{ 'api-version': '2024-11-01' }
36+
);
37+
38+
return (response.value ?? [])
39+
.map((server: McpServer) => ({
40+
...server,
41+
enabled: server.enabled === undefined ? true : server.enabled,
42+
}))
43+
.sort((a: McpServer, b: McpServer) => a.name.localeCompare(b.name));
44+
} catch (errorResponse: any) {
45+
const error = errorResponse?.error || {};
46+
47+
if (equals(error.code, 'McpServerNotEnabled')) {
48+
return [];
49+
}
50+
51+
// For now log the error and return empty list
52+
LoggerService().log({
53+
level: LogEntryLevel.Error,
54+
area: 'McpServer.listServers',
55+
error,
56+
message: `Error while fetching mcp servers for the app: ${siteResourceId}`,
57+
});
58+
return [];
59+
}
4160
},
4261
enabled: !!siteResourceId,
4362
...queryOpts,

libs/designer/src/lib/ui/mcp/servers/styles.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ export const useMcpServerStyles = makeStyles({
8181
serverHeaderActions: { display: 'flex', alignItems: 'center', gap: '6px' },
8282
serverHeaderButtons: { minWidth: '50px', padding: '0 4px' },
8383
serverHeaderDivider: { padding: '0 8px' },
84-
serverDescription: { paddingLeft: '28px', maxWidth: '778px', paddingTop: '8px' },
84+
serverDescription: { paddingLeft: '28px', maxWidth: '778px', paddingTop: '8px', display: 'block' },
8585
serverField: { paddingTop: '10px', paddingLeft: '28px' },
8686
serverContent: { paddingTop: '16px', paddingLeft: '16px' },
8787
});

libs/designer/src/lib/ui/mcp/wizard/styles.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -157,7 +157,7 @@ export const useMcpServerWizardStyles = makeStyles({
157157
flex: 1,
158158
alignItems: 'center',
159159
justifyContent: 'center',
160-
paddingTop: '30%',
160+
paddingTop: '10%',
161161
},
162162

163163
loadingText: {

0 commit comments

Comments
 (0)