diff --git a/.gitignore b/.gitignore index 80e0c1837..07105a9e1 100644 --- a/.gitignore +++ b/.gitignore @@ -39,3 +39,5 @@ src/web/apps/dist/ .work/reports/*.md !.work/specs/TEMPLATE.md !.work/**/.gitkeep + +.worktrees/ diff --git a/package.json b/package.json index 30c6f125b..1e24d2130 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "name": "@tableau/mcp-server", "description": "Helping agents see and understand data.", - "version": "3.5.2", + "version": "3.5.3", "repository": { "type": "git", "url": "git+https://github.com/tableau/tableau-mcp.git" diff --git a/src/server/oauth/scopes.ts b/src/server/oauth/scopes.ts index 853751dae..fed868f59 100644 --- a/src/server/oauth/scopes.ts +++ b/src/server/oauth/scopes.ts @@ -297,6 +297,11 @@ const toolScopeMap: Record< mcp: [], api: new Set(), }, + // MCP-app event telemetry relay: no Tableau REST API calls, no content scope required. + 'record-event': { + mcp: [], + api: new Set(), + }, // Dispatches on `kind` to ts-events, site-content, job-performance (raw VDS) or stale-content // (server-side anti-join). Union of the scopes required by all four kinds. 'query-admin-insights': { @@ -362,6 +367,7 @@ async function getEnabledToolNames(): Promise> { // human-gesture confirm steps for their preview tools and only exist when the iframe can render. if (!mcpAppsEnabled) { enabledTools.delete('get-embed-token'); + enabledTools.delete('record-event'); enabledTools.delete('confirm-update-cloud-extract-refresh-task'); enabledTools.delete('confirm-delete-content'); } diff --git a/src/server/passthroughAuthMiddleware.test.ts b/src/server/passthroughAuthMiddleware.test.ts index 7efc703bd..34af70fff 100644 --- a/src/server/passthroughAuthMiddleware.test.ts +++ b/src/server/passthroughAuthMiddleware.test.ts @@ -21,6 +21,9 @@ const TOOLS_WITHOUT_API_SCOPES_WITH_PASSTHROUGH_GUARD: ReadonlyArray { diff --git a/src/telemetry/productTelemetry/telemetryForwarder.ts b/src/telemetry/productTelemetry/telemetryForwarder.ts index 798016692..064ffdef9 100644 --- a/src/telemetry/productTelemetry/telemetryForwarder.ts +++ b/src/telemetry/productTelemetry/telemetryForwarder.ts @@ -7,7 +7,7 @@ type PropertiesType = { [key: string]: ValidPropertyValueType }; const DEFAULT_HOST_NAME = 'External'; const SERVICE_NAME = 'tableau-mcp'; -export type TelemetryEventType = 'tool_call'; +export type TelemetryEventType = 'tool_call' | 'tableau_mcp_event'; export type ProductTelemetryBase = { endpoint: string; diff --git a/src/tools/web/recordEvent/recordEvent.test.ts b/src/tools/web/recordEvent/recordEvent.test.ts new file mode 100644 index 000000000..c05f0f5c7 --- /dev/null +++ b/src/tools/web/recordEvent/recordEvent.test.ts @@ -0,0 +1,123 @@ +import { CallToolResult } from '@modelcontextprotocol/sdk/types.js'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { z } from 'zod'; + +import { WebMcpServer } from '../../../server.web.js'; +import { Provider } from '../../../utils/provider.js'; +import { getMockRequestHandlerExtra } from '../toolContext.mock.js'; +import { getRecordEventTool } from './recordEvent.js'; + +// Mock getProductTelemetry so we can assert on the forwarder's send(). Note that +// WebTool.logAndExecute also emits an automatic 'tool_call' event through the same +// forwarder, so the spy is called for both 'tool_call' and 'tableau_mcp_event'. +vi.mock('../../../telemetry/productTelemetry/telemetryForwarder.js', async (importOriginal) => { + const actual = + await importOriginal< + typeof import('../../../telemetry/productTelemetry/telemetryForwarder.js') + >(); + return { ...actual, getProductTelemetry: vi.fn() }; +}); + +import { getProductTelemetry } from '../../../telemetry/productTelemetry/telemetryForwarder.js'; + +type Extra = ReturnType; + +describe('getRecordEventTool', () => { + let sendSpy: ReturnType; + + beforeEach(() => { + sendSpy = vi.fn(); + vi.mocked(getProductTelemetry).mockReturnValue({ send: sendSpy } as never); + }); + + afterEach(() => { + vi.clearAllMocks(); + }); + + it('should create a tool instance with correct properties', async () => { + const tool = getRecordEventTool(new WebMcpServer()); + const annotations = await Provider.from(tool.annotations); + expect(tool.name).toBe('record-event'); + expect(annotations?.readOnlyHint).toBe(true); + expect(annotations?.openWorldHint).toBe(false); + }); + + it('should set visibility to app-only', () => { + const tool = getRecordEventTool(new WebMcpServer()); + expect(tool.meta?.ui?.visibility).toEqual(['app']); + }); + + it('sends an tableau_mcp_event event with the event_type, message and server context', async () => { + const extra = getMockRequestHandlerExtra(); + const result = await getToolResult(extra, { event_type: 'PARSE_ERROR', message: 'bad json' }); + + expect(result.isError).toBe(false); + expect(sendSpy).toHaveBeenCalledWith( + 'tableau_mcp_event', + expect.objectContaining({ + event_type: 'PARSE_ERROR', + message: 'bad json', + podname: extra.config.server, + is_hyperforce: extra.config.isHyperforce, + }), + ); + }); + + it('defaults message to empty string when omitted', async () => { + const extra = getMockRequestHandlerExtra(); + await getToolResult(extra, { event_type: 'EMBED_LOAD_ERROR', message: undefined }); + + expect(sendSpy).toHaveBeenCalledWith( + 'tableau_mcp_event', + expect.objectContaining({ event_type: 'EMBED_LOAD_ERROR', message: '' }), + ); + }); + + it('accepts SCREAMING_SNAKE_CASE event_type values', async () => { + const schema = z.object( + await Provider.from(getRecordEventTool(new WebMcpServer()).paramsSchema), + ); + for (const event_type of [ + 'TOOL_ERROR', + 'PARSE_ERROR', + 'AUTH_ERROR', + 'EMBED_LOAD_ERROR', + 'MCP_APP_CLICKED', + ]) { + expect(schema.safeParse({ event_type }).success).toBe(true); + } + }); + + it('rejects event_type that is too long or not SCREAMING_SNAKE_CASE', async () => { + const schema = z.object( + await Provider.from(getRecordEventTool(new WebMcpServer()).paramsSchema), + ); + expect(schema.safeParse({ event_type: 'A'.repeat(65) }).success).toBe(false); // too long + expect(schema.safeParse({ event_type: 'tool_error' }).success).toBe(false); // lowercase + expect(schema.safeParse({ event_type: 'TOOL ERROR' }).success).toBe(false); // spaces + expect(schema.safeParse({ event_type: '1TOOL_ERROR' }).success).toBe(false); // leading digit + expect(schema.safeParse({ event_type: '_TOOL_ERROR' }).success).toBe(false); // leading underscore + }); + + it('truncates message longer than 1024 characters in the forwarded event', async () => { + const extra = getMockRequestHandlerExtra(); + const longMessage = 'x'.repeat(2000); + await getToolResult(extra, { event_type: 'TOOL_ERROR', message: longMessage }); + + const sentMessage = sendSpy.mock.calls.find((c) => c[0] === 'tableau_mcp_event')?.[1]?.message; + expect(sentMessage).toBe('x'.repeat(1024)); + expect(sentMessage.length).toBe(1024); + }); +}); + +async function getToolResult( + extra: Extra, + args: { event_type: string; message?: string | undefined }, +): Promise { + const tool = getRecordEventTool(new WebMcpServer()); + const callback = await Provider.from(tool.callback); + // Mirror the MCP framework: params are validated/transformed against paramsSchema before the + // callback runs, so route args through the schema here (this is where message truncation happens). + const parsedArgs = z.object(await Provider.from(tool.paramsSchema)).parse(args); + return await callback({ event_type: parsedArgs.event_type, message: parsedArgs.message }, extra); +} diff --git a/src/tools/web/recordEvent/recordEvent.ts b/src/tools/web/recordEvent/recordEvent.ts new file mode 100644 index 000000000..693a58e6b --- /dev/null +++ b/src/tools/web/recordEvent/recordEvent.ts @@ -0,0 +1,86 @@ +import { CallToolResult } from '@modelcontextprotocol/sdk/types.js'; +import { Ok } from 'ts-results-es'; +import { z } from 'zod'; + +import { getFeatureGate } from '../../../features/init.js'; +import { WebMcpServer } from '../../../server.web.js'; +import { getProductTelemetry } from '../../../telemetry/productTelemetry/telemetryForwarder.js'; +import { WebTool } from '../tool.js'; + +// Starting field set — the final app-supplied schema is expected to grow later. +const paramsSchema = { + // Bounded free-form string rather than a hard enum: the event-type set is intentionally + // app-extensible (see above), so we reject malformed values but not unknown-yet-valid ones. + event_type: z + .string() + .max(64) + .regex(/^[A-Z][A-Z0-9_]*$/, 'event_type must be SCREAMING_SNAKE_CASE (e.g. TOOL_ERROR).') + .describe( + 'The event type for product telemetry, e.g. TOOL_ERROR, PARSE_ERROR, AUTH_ERROR, EMBED_LOAD_ERROR, MCP_APP_CLICKED.', + ), + // Optional free-text detail: truncate rather than reject so an over-long message never fails + // the telemetry call (mirrors the length cap in src/telemetry/clientDisplayName.ts). + message: z + .string() + .transform((s) => s.slice(0, 1024)) + .optional() + .describe('Optional detail or context for the event.'), +}; + +/** + * Records a product-telemetry event from the MCP app UI (errors, user actions, etc.). + * Called by the app (never the model) via app.callServerTool. Mirrors the + * server-side 'tool_call' telemetry pattern, enriching the event with request + * context the browser bundle does not have. + */ +export const getRecordEventTool = (server: WebMcpServer): WebTool => { + const recordEventTool = new WebTool({ + server, + name: 'record-event', + description: + 'Records a product-telemetry event from the MCP app UI (errors, user actions, etc.). This tool is only visible to the app, never the model. It takes an event type and optional detail, forwards a telemetry event, and returns immediately.', + paramsSchema, + annotations: { + title: 'Record Event', + readOnlyHint: true, + destructiveHint: false, + idempotentHint: true, + openWorldHint: false, + }, + meta: { + ui: { + visibility: ['app'], // Only visible to the app, not the model + }, + }, + disabled: !getFeatureGate().isFeatureEnabled('mcp-apps'), + callback: async (args, extra): Promise => { + return recordEventTool.logAndExecute<{ recorded: true }>({ + extra, + args, + callback: async () => { + const { config } = extra; + + const productTelemetryForwarder = getProductTelemetry( + config.productTelemetryEndpoint, + config.productTelemetryEnabled, + config.server, + ); + + productTelemetryForwarder.send('tableau_mcp_event', { + event_type: args.event_type, + message: args.message ?? '', + site_luid: extra.getSiteLuid(), + user_luid: extra.getUserLuid(), + podname: config.server, + is_hyperforce: config.isHyperforce, + }); + + return Ok({ recorded: true as const }); + }, + constrainSuccessResult: (result) => ({ type: 'success', result }), + }); + }, + }); + + return recordEventTool; +}; diff --git a/src/tools/web/tool.test.ts b/src/tools/web/tool.test.ts index 41ff31cdb..faffc3586 100644 --- a/src/tools/web/tool.test.ts +++ b/src/tools/web/tool.test.ts @@ -230,6 +230,7 @@ describe('Tool', () => { is_hyperforce: false, success: true, error_code: '', + error_message: '', }), ); }); @@ -252,6 +253,7 @@ describe('Tool', () => { is_hyperforce: false, success: false, error_code: '500', + error_message: 'requestId: 2, error: Callback failed', }), ); }); @@ -582,6 +584,16 @@ describe('Tool', () => { const parsed = JSON.parse(result.content[0].text); expect(parsed.data).toEqual(rawApiData.toString()); expect(parsed.warning).toContain('Expected string, received object'); + + // The passthrough result carries the full API payload but is isError: false, so it must + // NOT leak into telemetry's error_message (keyed off isError, not the false `success`). + expect(mockTelemetrySend).toHaveBeenCalledWith( + 'tool_call', + expect.objectContaining({ + success: false, + error_message: '', + }), + ); }); it('should return isError: false with validation warning for discriminatedUnion schema errors', async () => { diff --git a/src/tools/web/tool.ts b/src/tools/web/tool.ts index 7567d0404..685e7fa19 100644 --- a/src/tools/web/tool.ts +++ b/src/tools/web/tool.ts @@ -12,6 +12,7 @@ import { } from '../../telemetry/clientDisplayName.js'; import { getTelemetryProvider } from '../../telemetry/init.js'; import { getProductTelemetry } from '../../telemetry/productTelemetry/telemetryForwarder.js'; +import { extractToolErrorMessage } from '../../utils/extractToolErrorMessage.js'; import { getExceptionMessage } from '../../utils/getExceptionMessage.js'; import { getHttpStatus } from '../../utils/getHttpStatus.js'; import { LogAndExecuteParams, Tool, ToolParams } from '../tool.js'; @@ -162,7 +163,7 @@ export class WebTool extends T let success = false; let errorCode = ''; // HTTP status category: "4xx", "5xx", or empty for successful calls - let toolResult: CallToolResult; + let toolResult: CallToolResult | undefined; try { const result = await callback(); @@ -232,6 +233,10 @@ export class WebTool extends T is_hyperforce: config.isHyperforce, success, error_code: errorCode, + // Only populated for genuine error results (isError: true). The ZodiosValidationError + // passthrough returns isError: false with the full API payload, so keying off isError + // (not !success) keeps successful response data out of telemetry. + error_message: toolResult?.isError ? extractToolErrorMessage(toolResult) : '', oauth_client_id: sanitizeClientIdForTelemetry(oauthClientId), oauth_client_display_name: getClientDisplayName(oauthClientId) ?? sanitizeClientIdForTelemetry(oauthClientId), diff --git a/src/tools/web/toolName.ts b/src/tools/web/toolName.ts index 9081aad43..2b42ae1bb 100644 --- a/src/tools/web/toolName.ts +++ b/src/tools/web/toolName.ts @@ -14,6 +14,7 @@ export const webToolNames = [ 'get-datasource-metadata', 'resolve-datasource-luid', 'get-embed-token', + 'record-event', 'get-workbook', 'get-view', 'get-flow', @@ -52,6 +53,7 @@ export const webToolGroupNames = [ 'jobs', 'users', 'token-management', + 'mcp-apps', 'admin-insights', 'content', ] as const; @@ -94,7 +96,8 @@ export const webToolGroups = { ], jobs: ['list-jobs'], users: ['list-users', 'update-user'], - 'token-management': ['get-embed-token', 'revoke-access-token', 'reset-consent'], + 'token-management': ['revoke-access-token', 'reset-consent'], + 'mcp-apps': ['get-embed-token', 'record-event'], 'admin-insights': ['query-admin-insights'], content: ['delete-content', 'confirm-delete-content'], } as const satisfies Record>; diff --git a/src/tools/web/tools.ts b/src/tools/web/tools.ts index ec92a01d3..ac839a76f 100644 --- a/src/tools/web/tools.ts +++ b/src/tools/web/tools.ts @@ -22,6 +22,7 @@ import { getListPulseMetricsFromMetricDefinitionIdTool } from './pulse/listMetri import { getListPulseMetricsFromMetricIdsTool } from './pulse/listMetricsFromMetricIds/listPulseMetricsFromMetricIds.js'; import { getListPulseMetricSubscriptionsTool } from './pulse/listMetricSubscriptions/listPulseMetricSubscriptions.js'; import { getQueryDatasourceTool } from './queryDatasource/queryDatasource.js'; +import { getRecordEventTool } from './recordEvent/recordEvent.js'; import { getResetConsentTool } from './resetConsent/resetConsent.js'; import { getRevokeAccessTokenTool } from './revokeAccessToken/revokeAccessToken.js'; import { getListUsersTool } from './users/listUsers.js'; @@ -39,6 +40,7 @@ import { getListWorkbooksTool } from './workbooks/listWorkbooks.js'; export const webToolFactories = [ getGetDatasourceMetadataTool, getEmbedTokenTool, + getRecordEventTool, getListDatasourcesTool, getResolveDatasourceLuidTool, getListExtractRefreshTasksTool, diff --git a/src/utils/extractToolErrorMessage.test.ts b/src/utils/extractToolErrorMessage.test.ts new file mode 100644 index 000000000..7546662ba --- /dev/null +++ b/src/utils/extractToolErrorMessage.test.ts @@ -0,0 +1,62 @@ +import type { CallToolResult } from '@modelcontextprotocol/sdk/types.js'; + +import { extractToolErrorMessage } from './extractToolErrorMessage.js'; + +describe('extractToolErrorMessage', () => { + it('returns the text of a single text content block', () => { + const result: CallToolResult = { + isError: true, + content: [{ type: 'text', text: 'Request failed with status code 404' }], + }; + + expect(extractToolErrorMessage(result)).toBe('Request failed with status code 404'); + }); + + it('joins multiple text blocks with a space', () => { + const result: CallToolResult = { + isError: true, + content: [ + { type: 'text', text: 'first' }, + { type: 'text', text: 'second' }, + ], + }; + + expect(extractToolErrorMessage(result)).toBe('first second'); + }); + + it('ignores non-text content blocks', () => { + const result = { + isError: true, + content: [ + { type: 'image', data: 'abc', mimeType: 'image/png' }, + { type: 'text', text: 'only this' }, + ], + } as unknown as CallToolResult; + + expect(extractToolErrorMessage(result)).toBe('only this'); + }); + + it('returns an empty string when there are no text blocks', () => { + const result = { + isError: true, + content: [{ type: 'image', data: 'abc', mimeType: 'image/png' }], + } as unknown as CallToolResult; + + expect(extractToolErrorMessage(result)).toBe(''); + }); + + it('returns an empty string when the text is empty or whitespace only', () => { + const result: CallToolResult = { + isError: true, + content: [{ type: 'text', text: ' ' }], + }; + + expect(extractToolErrorMessage(result)).toBe(''); + }); + + it('returns an empty string when content is not an array', () => { + const result = { isError: true, content: undefined } as unknown as CallToolResult; + + expect(extractToolErrorMessage(result)).toBe(''); + }); +}); diff --git a/src/utils/extractToolErrorMessage.ts b/src/utils/extractToolErrorMessage.ts new file mode 100644 index 000000000..0d1383ad4 --- /dev/null +++ b/src/utils/extractToolErrorMessage.ts @@ -0,0 +1,19 @@ +import type { CallToolResult } from '@modelcontextprotocol/sdk/types.js'; + +/** + * Extracts a human-readable error message from a failed tool result's text + * content, so it can be forwarded as telemetry detail. Joins all text blocks + * and returns an empty string when the result carries no usable text. + */ +export function extractToolErrorMessage(result: CallToolResult): string { + const content = result.content; + if (!Array.isArray(content)) { + return ''; + } + + return content + .filter((item): item is { type: 'text'; text: string } => item?.type === 'text') + .map((item) => item.text) + .join(' ') + .trim(); +} diff --git a/src/web/apps/src/embed/handleToolResult.test.ts b/src/web/apps/src/embed/handleToolResult.test.ts index d92d459a2..35c227f7b 100644 --- a/src/web/apps/src/embed/handleToolResult.test.ts +++ b/src/web/apps/src/embed/handleToolResult.test.ts @@ -12,7 +12,9 @@ vi.mock('./getEmbedTokenToolClient.js'); vi.mock('./embedTableauViz.js'); vi.mock('./loadTableauEmbeddingApi.js'); vi.mock('./openInTableauLink.js'); +vi.mock('../shared/recordEventClient.js'); +import { recordEvent } from '../shared/recordEventClient.js'; import { embedTableauViz } from './embedTableauViz.js'; import { callGetEmbedTokenTool } from './getEmbedTokenToolClient.js'; import { loadTableauEmbeddingApi } from './loadTableauEmbeddingApi.js'; @@ -341,4 +343,27 @@ describe('handleToolResult', () => { // Assert setupOpenInTableauLink WAS called expect(vi.mocked(setupOpenInTableauLink)).toHaveBeenCalledTimes(1); }); + + it('reports telemetry with the tool error message when a tool error occurs', async () => { + const errorResult: CallToolResult = { + isError: true, + content: [{ type: 'text', text: 'Tool execution failed' }], + }; + + await handleToolResult(mockApp, errorResult); + await new Promise((r) => setTimeout(r, 0)); + + expect(vi.mocked(recordEvent)).toHaveBeenCalledWith( + mockApp, + 'TOOL_ERROR', + 'Tool execution failed', + ); + }); + + it('reports telemetry with undefined cause when the tool result is null', async () => { + await handleToolResult(mockApp, null as any); + await new Promise((r) => setTimeout(r, 0)); + + expect(vi.mocked(recordEvent)).toHaveBeenCalledWith(mockApp, 'TOOL_ERROR', undefined); + }); }); diff --git a/src/web/apps/src/embed/handleToolResult.ts b/src/web/apps/src/embed/handleToolResult.ts index 83e73de82..7b4c47817 100644 --- a/src/web/apps/src/embed/handleToolResult.ts +++ b/src/web/apps/src/embed/handleToolResult.ts @@ -2,6 +2,7 @@ import type { App } from '@modelcontextprotocol/ext-apps'; import type { CallToolResult } from '@modelcontextprotocol/sdk/types.js'; import { z } from 'zod'; +import { extractToolErrorMessage } from '../../../../utils/extractToolErrorMessage.js'; import { showError } from '../shared/showError.js'; import { embedTableauViz } from './embedTableauViz.js'; import { callGetEmbedTokenTool } from './getEmbedTokenToolClient.js'; @@ -43,7 +44,8 @@ export function extractUrlObjectFromResult(result: CallToolResult): string { */ export async function handleToolResult(app: App, result: CallToolResult): Promise { if (!result || result.isError) { - showError('TOOL_ERROR'); + const cause = result ? extractToolErrorMessage(result) : undefined; + showError('TOOL_ERROR', cause, app); return; } @@ -52,7 +54,7 @@ export async function handleToolResult(app: App, result: CallToolResult): Promis try { viewUrl = extractUrlObjectFromResult(result); } catch (e) { - showError('PARSE_ERROR', e); + showError('PARSE_ERROR', e, app); return; } @@ -60,7 +62,7 @@ export async function handleToolResult(app: App, result: CallToolResult): Promis try { await loadTableauEmbeddingApi(viewUrl); } catch (e) { - showError('EMBED_LOAD_ERROR', e); + showError('EMBED_LOAD_ERROR', e, app); return; } @@ -69,12 +71,12 @@ export async function handleToolResult(app: App, result: CallToolResult): Promis try { token = await callGetEmbedTokenTool(app); } catch (e) { - showError('AUTH_ERROR', e); + showError('AUTH_ERROR', e, app); return; } // Auth failure (runtime) - handled by onError callback - embedTableauViz(viewUrl, token, () => showError('AUTH_ERROR')); + embedTableauViz(viewUrl, token, () => showError('AUTH_ERROR', undefined, app)); const main = document.querySelector('.main'); if (main) { diff --git a/src/web/apps/src/embed/openInTableauLink.test.ts b/src/web/apps/src/embed/openInTableauLink.test.ts index b900b3f3b..172a68a69 100644 --- a/src/web/apps/src/embed/openInTableauLink.test.ts +++ b/src/web/apps/src/embed/openInTableauLink.test.ts @@ -4,6 +4,9 @@ import type { App } from '@modelcontextprotocol/ext-apps'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +vi.mock('../shared/recordEventClient.js'); + +import { recordEvent } from '../shared/recordEventClient.js'; import { setupOpenInTableauLink } from './openInTableauLink.js'; describe('setupOpenInTableauLink', () => { @@ -111,6 +114,23 @@ describe('setupOpenInTableauLink', () => { expect(preventDefaultSpy).toHaveBeenCalled(); }); + it('should call recordEvent with MCP_APP_CLICKED when link is clicked', async () => { + const url = 'https://tableau.example.com/views/workbook/view'; + + setupOpenInTableauLink(mockApp, url, container); + + const linkElement = container.querySelector('#openInTableauLink') as HTMLAnchorElement; + expect(linkElement).not.toBeNull(); + + // Click the link + linkElement.click(); + + // Wait for async handler + await new Promise((resolve) => setTimeout(resolve, 0)); + + expect(vi.mocked(recordEvent)).toHaveBeenCalledWith(mockApp, 'MCP_APP_CLICKED', url); + }); + it('should show inline error when openLink returns isError true', async () => { const url = 'https://tableau.example.com/views/workbook/view'; mockApp.openLink = vi.fn().mockResolvedValue({ isError: true }); diff --git a/src/web/apps/src/embed/openInTableauLink.ts b/src/web/apps/src/embed/openInTableauLink.ts index bc3a14532..c3e0f481c 100644 --- a/src/web/apps/src/embed/openInTableauLink.ts +++ b/src/web/apps/src/embed/openInTableauLink.ts @@ -1,5 +1,7 @@ import type { App } from '@modelcontextprotocol/ext-apps'; +import { recordEvent } from '../shared/recordEventClient.js'; + /** * Shows an inline error message when the link fails to open. * @@ -60,6 +62,7 @@ export function setupOpenInTableauLink(app: App, url: string, container: HTMLEle // Set onclick handler to use host-mediated link opening link.onclick = async (e) => { e.preventDefault(); + recordEvent(app, 'MCP_APP_CLICKED', url); try { const result = await app.openLink({ url }); diff --git a/src/web/apps/src/hitl/handleConfirmResult.test.ts b/src/web/apps/src/hitl/handleConfirmResult.test.ts index 34975494a..b5dfa5fcb 100644 --- a/src/web/apps/src/hitl/handleConfirmResult.test.ts +++ b/src/web/apps/src/hitl/handleConfirmResult.test.ts @@ -56,7 +56,7 @@ describe('handleConfirmResult', () => { it('shows error UI when tool returns error result (isError: true)', () => { handleConfirmResult(mockApp, { isError: true, content: [{ type: 'text', text: 'boom' }] }); - expect(vi.mocked(showError)).toHaveBeenCalledWith('TOOL_ERROR'); + expect(vi.mocked(showError)).toHaveBeenCalledWith('TOOL_ERROR', 'boom', mockApp); expect(vi.mocked(renderDeleteWorkbookConfirm)).not.toHaveBeenCalled(); }); @@ -65,7 +65,7 @@ describe('handleConfirmResult', () => { handleConfirmResult(mockApp, null as any); expect(vi.mocked(showError)).toHaveBeenCalledTimes(2); - expect(vi.mocked(showError)).toHaveBeenCalledWith('TOOL_ERROR'); + expect(vi.mocked(showError)).toHaveBeenCalledWith('TOOL_ERROR', undefined, mockApp); }); it('routes a delete-workbook confirm result to renderDeleteWorkbookConfirm', () => { @@ -113,7 +113,7 @@ describe('handleConfirmResult', () => { it('shows error UI when no known confirm-panel shape matches', () => { handleConfirmResult(mockApp, okResult); - expect(vi.mocked(showError)).toHaveBeenCalledWith('TOOL_ERROR'); + expect(vi.mocked(showError)).toHaveBeenCalledWith('TOOL_ERROR', undefined, mockApp); expect(vi.mocked(renderDeleteWorkbookConfirm)).not.toHaveBeenCalled(); expect(vi.mocked(renderDeleteDatasourceConfirm)).not.toHaveBeenCalled(); expect(vi.mocked(renderDeleteExtractRefreshTaskConfirm)).not.toHaveBeenCalled(); diff --git a/src/web/apps/src/hitl/handleConfirmResult.ts b/src/web/apps/src/hitl/handleConfirmResult.ts index 7c7775cf6..f1ed3a6cb 100644 --- a/src/web/apps/src/hitl/handleConfirmResult.ts +++ b/src/web/apps/src/hitl/handleConfirmResult.ts @@ -1,6 +1,7 @@ import type { App } from '@modelcontextprotocol/ext-apps'; import type { CallToolResult } from '@modelcontextprotocol/sdk/types.js'; +import { extractToolErrorMessage } from '../../../../utils/extractToolErrorMessage.js'; import { showError } from '../shared/showError.js'; import { isDeleteDatasourceConfirmResult, @@ -29,7 +30,8 @@ import { */ export function handleConfirmResult(app: App, result: CallToolResult): void { if (!result || result.isError) { - showError('TOOL_ERROR'); + const cause = result ? extractToolErrorMessage(result) : undefined; + showError('TOOL_ERROR', cause, app); return; } @@ -51,5 +53,5 @@ export function handleConfirmResult(app: App, result: CallToolResult): void { } // No known confirm-panel shape matched — surface the error UI rather than silently doing nothing. - showError('TOOL_ERROR'); + showError('TOOL_ERROR', undefined, app); } diff --git a/src/web/apps/src/shared/mcp-app.css b/src/web/apps/src/shared/mcp-app.css index 85cc2eae0..2bf32117d 100644 --- a/src/web/apps/src/shared/mcp-app.css +++ b/src/web/apps/src/shared/mcp-app.css @@ -11,6 +11,10 @@ body { font-family: system-ui, -apple-system, sans-serif; } +body:has(.mcp-app-error) { + background-color: #FFFFFF; +} + .main { width: 100%; margin: 0; diff --git a/src/web/apps/src/shared/recordEventClient.test.ts b/src/web/apps/src/shared/recordEventClient.test.ts new file mode 100644 index 000000000..d8a6068df --- /dev/null +++ b/src/web/apps/src/shared/recordEventClient.test.ts @@ -0,0 +1,64 @@ +import type { App } from '@modelcontextprotocol/ext-apps'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { recordEvent } from './recordEventClient.js'; + +describe('recordEvent', () => { + let mockApp: App; + let callServerTool: ReturnType; + + beforeEach(() => { + callServerTool = vi.fn().mockResolvedValue({}); + mockApp = { + getHostCapabilities: vi.fn().mockReturnValue({ serverTools: {} }), + callServerTool, + } as unknown as App; + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('calls the record-event tool with event_type and Error message', () => { + recordEvent(mockApp, 'PARSE_ERROR', new Error('bad json')); + + expect(callServerTool).toHaveBeenCalledWith({ + name: 'record-event', + arguments: { event_type: 'PARSE_ERROR', message: 'bad json' }, + }); + }); + + it('omits message when there is no detail', () => { + recordEvent(mockApp, 'TOOL_ERROR'); + + expect(callServerTool).toHaveBeenCalledWith({ + name: 'record-event', + arguments: { event_type: 'TOOL_ERROR' }, + }); + }); + + it('does not call the tool when the host lacks serverTools capability', () => { + (mockApp.getHostCapabilities as ReturnType).mockReturnValue({}); + + recordEvent(mockApp, 'AUTH_ERROR'); + + expect(callServerTool).not.toHaveBeenCalled(); + }); + + it('does not throw when callServerTool rejects (fire-and-forget)', async () => { + callServerTool.mockRejectedValue(new Error('transport failed')); + + expect(() => recordEvent(mockApp, 'EMBED_LOAD_ERROR')).not.toThrow(); + // Let the rejected promise settle so the internal .catch runs. + await new Promise((r) => setTimeout(r, 0)); + }); + + it('does not throw when getHostCapabilities throws', () => { + (mockApp.getHostCapabilities as ReturnType).mockImplementation(() => { + throw new Error('not connected'); + }); + + expect(() => recordEvent(mockApp, 'TOOL_ERROR')).not.toThrow(); + expect(callServerTool).not.toHaveBeenCalled(); + }); +}); diff --git a/src/web/apps/src/shared/recordEventClient.ts b/src/web/apps/src/shared/recordEventClient.ts new file mode 100644 index 000000000..fb0644951 --- /dev/null +++ b/src/web/apps/src/shared/recordEventClient.ts @@ -0,0 +1,36 @@ +import type { App } from '@modelcontextprotocol/ext-apps'; + +/** + * Best-effort telemetry reporter for MCP app events (errors, user actions, etc.). + * Calls the app-only `record-event` server tool via the host proxy. Fire-and-forget: + * it never awaits, never throws, and silently no-ops when the host cannot + * proxy server tools. Telemetry must never block the UI. + * + * @param app - The MCP App instance. + * @param eventType - The event type (e.g. 'TOOL_ERROR', 'MCP_APP_CLICKED'). + * @param detail - Optional detail context (error message, URL, etc.). + */ +export function recordEvent(app: App, eventType: string, detail?: unknown): void { + try { + if (!app.getHostCapabilities()?.serverTools) { + return; + } + + const message = toMessage(detail); + const args = + message !== undefined ? { event_type: eventType, message } : { event_type: eventType }; + + void app.callServerTool({ name: 'record-event', arguments: args }).catch(() => { + // Best-effort telemetry: swallow transport failures. + }); + } catch { + // Never let telemetry reporting break the UI. + } +} + +function toMessage(detail: unknown): string | undefined { + if (detail === undefined || detail === null) { + return undefined; + } + return detail instanceof Error ? detail.message : String(detail); +} diff --git a/src/web/apps/src/shared/showError.test.ts b/src/web/apps/src/shared/showError.test.ts index 373d4baa8..9ab18d214 100644 --- a/src/web/apps/src/shared/showError.test.ts +++ b/src/web/apps/src/shared/showError.test.ts @@ -1,8 +1,11 @@ /** * @vitest-environment jsdom */ +import type { App } from '@modelcontextprotocol/ext-apps'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +vi.mock('./recordEventClient.js'); +import { recordEvent } from './recordEventClient.js'; import { showError } from './showError.js'; describe('showError', () => { @@ -125,4 +128,29 @@ describe('showError', () => { expect(document.querySelector('.mcp-app-error')).toBeNull(); }); + + it('reports telemetry with scenario and cause when app is provided', () => { + const app = {} as unknown as App; + const cause = new Error('JSON parse failed'); + + showError('PARSE_ERROR', cause, app); + + expect(vi.mocked(recordEvent)).toHaveBeenCalledWith(app, 'PARSE_ERROR', cause); + }); + + it('does not report telemetry when app is not provided', () => { + showError('TOOL_ERROR'); + + expect(vi.mocked(recordEvent)).not.toHaveBeenCalled(); + }); + + it('reports telemetry even when the container is missing', () => { + document.body.replaceChildren(); + const app = {} as unknown as App; + + showError('EMBED_LOAD_ERROR', undefined, app); + + expect(vi.mocked(recordEvent)).toHaveBeenCalledWith(app, 'EMBED_LOAD_ERROR', undefined); + expect(document.querySelector('.mcp-app-error')).toBeNull(); + }); }); diff --git a/src/web/apps/src/shared/showError.ts b/src/web/apps/src/shared/showError.ts index c0fbab24c..a820225b7 100644 --- a/src/web/apps/src/shared/showError.ts +++ b/src/web/apps/src/shared/showError.ts @@ -1,4 +1,7 @@ +import type { App } from '@modelcontextprotocol/ext-apps'; + import DISCONNECTED_SVG from './assets/disconnected.svg?raw'; +import { recordEvent } from './recordEventClient.js'; import { TABLEAU_VIZ_CONTAINER_ID } from './vizContainer.js'; export type Scenario = 'TOOL_ERROR' | 'PARSE_ERROR' | 'AUTH_ERROR' | 'EMBED_LOAD_ERROR'; @@ -28,15 +31,20 @@ const ERROR_UI: Record = { * Shows an error message in the tableau viz container * @param scenario - The error scenario to display * @param cause - Optional error that caused this scenario + * @param app - Optional MCP App instance for telemetry reporting */ -export function showError(scenario: Scenario, cause?: unknown): void { +export function showError(scenario: Scenario, cause?: unknown, app?: App): void { + // Report telemetry first (best-effort), so errors are recorded even when the + // container is missing and the error UI cannot be rendered. + if (app) { + recordEvent(app, scenario, cause); + } + const container = document.getElementById(TABLEAU_VIZ_CONTAINER_ID); if (!container) { return; } - console.error(ERROR_UI[scenario].logCode, cause); - const errorElement = document.createElement('div'); errorElement.className = 'mcp-app-error'; errorElement.setAttribute('role', 'alert');