codeant-ai-for-open-source[bot] commented on code in PR #43483: URL: https://github.com/apache/superset/pull/43483#discussion_r3846303682
########## superset/mcp_service/chart/resources/chart_viewer/src/App.tsx: ########## @@ -0,0 +1,1087 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { + useCallback, + useEffect, + useMemo, + useRef, + useState, + type JSX, + type PointerEvent as ReactPointerEvent, +} from 'react'; +import type { + ChartData, + ChartMeta, + ColorScheme, + DashboardRender, + ViewType, +} from './types'; +import { REQUERY_TOOL_NAME } from './types'; +import { + ChartBridge, + type DisplayMode, + type HostCapabilities, +} from './bridge'; +import { + applyThemeVars, + detectPreferredScheme, + getThemeTokens, + type SupersetThemeTokens, +} from './theme'; +import { + availableViews, + chartDataToEChartsOption, + classifyColumns, + defaultViewForChartType, + describeChart, + isCartesianView, + isEChartsView, + isSubstitutedView, +} from './adapter'; +import { formatByColumn, stripUntrustedMarkers } from './format'; +import { + copyText, + downloadDataUrl, + downloadFile, + exportFilename, + isDownloadRestricted, + toCsv, +} from './export'; +import { EChart, type EChartClickParams } from './components/EChart'; +import type { EChartsType } from './echarts'; +import { BigNumber } from './components/BigNumber'; +import { CopyPanel } from './components/CopyPanel'; +import { DataTable } from './components/DataTable'; +import { DashboardGrid } from './components/DashboardGrid'; +import { Toolbar } from './components/Toolbar'; +import type { ExportAction } from './components/ExportMenu'; +import { EmptyState, ErrorState, LoadingSkeleton } from './components/States'; +import { SAMPLE_CHART_DATA } from './sample-data'; + +const bridge = new ChartBridge(); +const DEFAULT_WIDGET_HEIGHT = 420; +const MAX_WIDGET_HEIGHT = 1200; +const MIN_WIDGET_HEIGHT = 260; +// How long a display-mode switch we requested suppresses host context naming +// the mode we just left. Long enough to cover a transition animation, short +// enough that a host which never confirms cannot mute us for a whole session. +const STALE_DISPLAY_MODE_WINDOW_MS = 2000; + +/** + * Rows included when handing the data to the assistant. The host feeds this + * straight into the model's context, so it is capped well below the result + * size the widget itself can hold. + */ +const SHARE_ROW_LIMIT = 100; + +interface DrillState { + active: boolean; + label: string; +} + +/** + * Resolve a clicked mark back to its source row. Cartesian series are built + * one mark per row so `dataIndex` is the row; pie and scatter reorder or + * collapse rows and carry the original index on the data item instead. + */ +function sourceRowIndex(params: EChartClickParams): number { + return params.rowIndex ?? params.dataIndex; +} + +export function App(): JSX.Element { + const [loading, setLoading] = useState(true); + const [error, setError] = useState<string | null>(null); + const [data, setData] = useState<ChartData | null>(null); + // Composite payload from render_dashboard. Mutually exclusive with `data`: + // one tool result is either a chart or a dashboard, never both. + const [dashboard, setDashboard] = useState<DashboardRender | null>(null); + const [meta, setMeta] = useState<ChartMeta>({}); + const [scheme, setScheme] = useState<ColorScheme>(detectPreferredScheme()); + const [caps, setCaps] = useState<HostCapabilities | null>(null); + const [view, setView] = useState<ViewType>('line'); + const [activeMetrics, setActiveMetrics] = useState<string[]>([]); + const [toast, setToast] = useState<string | null>(null); + const [drill, setDrill] = useState<DrillState>({ active: false, label: '' }); + const [selection, setSelection] = useState<EChartClickParams | null>(null); + const [requestedHeight, setRequestedHeight] = useState(DEFAULT_WIDGET_HEIGHT); + // Tracked so the maximize control can toggle rather than only ever expand. + const [displayMode, setDisplayMode] = useState<DisplayMode>('inline'); + const [restoreHeight, setRestoreHeight] = useState<number | null>(null); + const [copyPanel, setCopyPanel] = useState<{ + title: string; + text: string; + } | null>(null); + const chartInstance = useRef<EChartsType | null>(null); + // The host sandbox decides this once, at load; it cannot change mid-session. + const downloadsBlocked = useMemo(() => isDownloadRestricted(), []); + + const pendingDisplayMode = useRef<DisplayMode | null>(null); + const pendingDisplayModeTimer = useRef<number | null>(null); + + const releaseDisplayModeGuard = useCallback((): void => { + pendingDisplayMode.current = null; + if (pendingDisplayModeTimer.current !== null) { + window.clearTimeout(pendingDisplayModeTimer.current); + pendingDisplayModeTimer.current = null; + } + }, []); + + /** + * Ignore host context naming a mode other than `mode` for a short window. + * + * The guard is time-bound rather than tied to the request's lifetime: it is + * released by the matching notification when one arrives, and by the timer + * when the host never sends one — so a host that stays silent cannot leave + * the widget permanently deaf to its own display-mode updates. + */ + const armDisplayModeGuard = useCallback( + (mode: DisplayMode): void => { + releaseDisplayModeGuard(); + pendingDisplayMode.current = mode; + pendingDisplayModeTimer.current = window.setTimeout(() => { + pendingDisplayMode.current = null; + pendingDisplayModeTimer.current = null; + }, STALE_DISPLAY_MODE_WINDOW_MS); + }, + [releaseDisplayModeGuard], + ); + + const toastTimer = useRef<number | null>(null); + const showToast = useCallback((message: string, ms = 2600): void => { + setToast(message); + if (toastTimer.current !== null) window.clearTimeout(toastTimer.current); + toastTimer.current = window.setTimeout(() => { + setToast(null); + toastTimer.current = null; + }, ms); + }, []); + + // Superset tokens travel with the data, so the widget renders in the + // deployment's own branding rather than hardcoded colors. + const supersetTheme = (data?.theme ?? + dashboard?.theme ?? + null) as SupersetThemeTokens | null; + const theme = useMemo( + () => getThemeTokens(scheme, supersetTheme), + [scheme, supersetTheme], + ); + + // ---- Bridge handshake + data intake ------------------------------------ + useEffect(() => { + let alive = true; + + function intake( + next: ChartData | null, + nextMeta: ChartMeta, + err?: string, + nextDashboard?: DashboardRender | null, + ): void { + if (!alive) return; + if (nextDashboard) { + setDashboard(nextDashboard); + setError(null); + setLoading(false); + window.requestAnimationFrame(() => { + bridge.reportSize(Math.max(window.innerWidth, 320), 720); + }); + return; + } + if (err) { + // A ChartError / isError result (not-found, RBAC, query, OAuth): show + // the styled error state instead of spinning forever. + setError(err); + setLoading(false); + return; + } + if (!next) return; // connected but no data yet — keep waiting + setData(next); + setMeta((m) => ({ ...m, ...nextMeta })); + const defaultView = defaultViewForChartType(next.chart_type, next); + setView(defaultView); + setActiveMetrics(classifyColumns(next).numeric.map((c) => c.name)); + setError(null); + setLoading(false); + window.requestAnimationFrame(() => { + bridge.reportSize( + Math.max(window.innerWidth, 320), + DEFAULT_WIDGET_HEIGHT, + ); + }); + } + + bridge + .initialize() + .then((init) => { + if (!alive) return; + setCaps(init.capabilities); + setScheme(init.context.scheme); + if ( + init.context.displayMode === 'inline' || + init.context.displayMode === 'fullscreen' + ) { + setDisplayMode(init.context.displayMode); + } + if (init.error) { + intake(null, {}, init.error); + } else if (init.dashboard) { + intake(null, init.meta, undefined, init.dashboard); + } else if (init.chartData) { + intake(init.chartData, init.meta); + } else if (init.connected) { + // Connected but no data yet: wait for a tool-result push below. + } else if (init.embedded) { + // Embedded in a host but the handshake failed — NEVER show sample + // data (it would look like the user's real chart). Show an error. + intake(null, {}, 'Could not connect to Superset to load this chart.'); + } else { + // True standalone dev/demo mode (not embedded): sample data is fine. + intake(SAMPLE_CHART_DATA, {}); + } + }) + .catch(() => { + // Unexpected failure resolving the handshake. Only fall back to sample + // data when clearly not embedded; otherwise surface an error. + if (!alive) return; + if (window.self !== window.top) { + intake(null, {}, 'Could not connect to Superset to load this chart.'); + } else { + intake(SAMPLE_CHART_DATA, {}); + } + }); + + const offResult = bridge.onToolResult((d, m, e, dash) => + intake(d, m, e, dash), + ); + const offCtx = bridge.onContextChange((ctx) => { + if (ctx.scheme) setScheme(ctx.scheme); + // The host can change display mode on its own (its own fullscreen + // chrome, or Esc); follow it so our toggle stays in step — unless a + // switch we asked for is still settling, in which case a push naming the + // old mode is stale and would fight the user's click. + if (ctx.displayMode !== 'inline' && ctx.displayMode !== 'fullscreen') { + return; + } + if (pendingDisplayMode.current === null) { + setDisplayMode(ctx.displayMode); + return; + } + if (pendingDisplayMode.current === ctx.displayMode) { + // The switch we asked for has landed; stop filtering. + releaseDisplayModeGuard(); + setDisplayMode(ctx.displayMode); + } + // Otherwise the push describes the mode we just left. Drop it. + }); + + return () => { + alive = false; + offResult(); + offCtx(); + releaseDisplayModeGuard(); + }; + }, [releaseDisplayModeGuard]); + + // Keep chrome + chart theme in sync with the resolved scheme. + useEffect(() => { + applyThemeVars(theme); + }, [theme]); + + // Fall back to OS theme changes when the host does not push a scheme. + useEffect(() => { + if (!window.matchMedia) return undefined; + const mq = window.matchMedia('(prefers-color-scheme: dark)'); + const handler = (e: MediaQueryListEvent): void => + setScheme(e.matches ? 'dark' : 'light'); + mq.addEventListener('change', handler); + return () => mq.removeEventListener('change', handler); + }, []); + + const roles = useMemo(() => (data ? classifyColumns(data) : null), [data]); + const views = useMemo(() => (data ? availableViews(data) : []), [data]); + + const option = useMemo(() => { + if (!data) return {}; + return chartDataToEChartsOption(data, view, { theme, activeMetrics }); + }, [data, view, theme, activeMetrics]); + + // ---- Magic moment: click-to-drill via render_chart_requery ------------- + const canRequery = !!caps && bridge.hasTool(REQUERY_TOOL_NAME); + const canAsk = !!caps?.canUpdateModelContext || !!caps?.canSendMessage; + + const requery = useCallback( + async ( + args: Parameters<ChartBridge['callTool']>[1], + drillLabel: string, + ) => { + if (!data) return; + setLoading(true); + try { + // The tool takes a single `request` model keyed by `identifier`; + // a flat payload is rejected by schema validation. + const next = (await bridge.callTool(REQUERY_TOOL_NAME, { + request: { + identifier: data.chart_id, + ...args, + }, + })) as ChartData; + if (next && Array.isArray(next.columns)) { + setData(next); + setActiveMetrics(classifyColumns(next).numeric.map((c) => c.name)); + setDrill({ + active: true, + label: stripUntrustedMarkers(drillLabel), + }); + } Review Comment: **Suggestion:** Multiple clicks or brush actions can start overlapping re-queries, and each response unconditionally calls `setData`. If an earlier request finishes after a newer interaction, it overwrites the newer result and drill label with stale data. Track a request generation or cancel/ignore superseded requests before applying the response. [race condition] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Rapid point clicks can display stale drill-down data. - ⚠️ Brush zoom results can be overwritten by older clicks. - ⚠️ Drill labels may not match displayed data. ``` </details> [](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5c1d74b99db84a4d8da6136f152adbd2&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=5c1d74b99db84a4d8da6136f152adbd2&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/chart/resources/chart_viewer/src/App.tsx **Line:** 337:350 **Comment:** *Race Condition: Multiple clicks or brush actions can start overlapping re-queries, and each response unconditionally calls `setData`. If an earlier request finishes after a newer interaction, it overwrites the newer result and drill label with stale data. Track a request generation or cancel/ignore superseded requests before applying the response. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43483&comment_hash=54c196434eb1f88da38c09a7ebf0b8bb31d641691c9bcae6226576b1fe7402d4&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43483&comment_hash=54c196434eb1f88da38c09a7ebf0b8bb31d641691c9bcae6226576b1fe7402d4&reaction=dislike'>👎</a> ########## superset/mcp_service/chart/resources/chart_viewer/src/bridge.ts: ########## @@ -0,0 +1,964 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/** + * Thin, well-isolated bridge to the MCP Apps host. The rest of the widget + * depends on this interface — never on the vendor package directly — so we can + * swap the transport (direct postMessage vs. @modelcontextprotocol/ext-apps) + * without touching the UI. + * + * It speaks the documented JSON-RPC-2.0-over-postMessage dialect from the + * MCP Apps spec (2026-01-26): ui/initialize handshake, tool-result / host + * context notifications, tools/call, ui/update-model-context, ui/open-link. + * Outside a host (standalone dev), every call no-ops gracefully. + */ +import type { + ChartData, + ChartMeta, + ColorScheme, + DashboardRender, +} from './types'; + +/** + * Display modes the widget can ask the host to switch between. + * + * `pip` is a floating overlay. The spec defines it as exactly that and says + * nothing about whether it survives the next conversation turn, so the widget + * offers it only where a host advertises it and makes no promise about how + * long it stays. + */ +export type DisplayMode = 'inline' | 'fullscreen' | 'pip'; + +export interface HostContext { + scheme: ColorScheme; + displayMode?: string; + container?: { width?: number; height?: number }; +} + +export interface HostCapabilities { + /** Names of tools the host exposes to the app (visibility: ["app"]). */ + appTools: Set<string>; + /** Whether the host accepts ui/update-model-context. */ + canUpdateModelContext: boolean; + /** Whether the host accepts ui/message follow-ups. */ + canSendMessage: boolean; + /** Whether tools/call is available at all. */ + canCallTools: boolean; + /** Whether the host will open external URLs on our behalf (ui/open-link). */ + canOpenLinks: boolean; + /** Whether the host will save files on our behalf (ui/download-file). */ + canDownloadFile: boolean; +} + +/** + * Everything the host told us at handshake, verbatim. + * + * `deriveCapabilities` guesses at several key spellings (`tools`, `toolCalls`, + * `appTools`, `experimental.appTools`, ...) because the spec does not pin them. + * A host that advertises under a name we do not read leaves every gated + * affordance silently switched off, which is indistinguishable from a broken + * feature. The raw maps are kept so that question can be answered by looking + * rather than by guessing. + */ +export interface HostDiagnostics { + protocolVersion?: string; + /** Exactly what the host sent — no normalisation. */ + hostCapabilities: Record<string, unknown>; + hostContext: Record<string, unknown>; + /** 'null' in a sandboxed iframe without allow-same-origin. */ + origin: string; + embedded: boolean; + /** What we concluded from the above. */ + derived: HostCapabilities; + /** + * Top-level keys the host actually sent, verbatim. + * + * Surfaced in the collapsed summary because that one line has twice now been + * the only diagnostic that made it out of a host — reading the expanded JSON + * depends on a human transcribing it, which kept failing. The key names are + * what identify a spelling mismatch, so they belong where they can be read + * at a glance. + */ + capabilityKeys: string[]; + /** Sandbox permissions the host granted (clipboard-write, etc.), if stated. */ + sandboxPermissions: string[]; + /** + * The last few host-mediated exchanges, request and response together. + * + * Download and open-link have each cost several build/restart/test cycles + * that ended in inferring backwards from a symptom, because the host's + * answer is invisible from outside the iframe. Recording what we sent and + * what came back turns "it does nothing" into a readable fact. + */ + exchanges: HostExchange[]; + /** + * Display modes the host offers (`inline` | `fullscreen` | `pip`). + * + * Surfaced because `pip` — a persistent side panel that survives while the + * conversation continues — is a spec mode the widget does not yet request, + * and whether it is worth building is decided entirely by whether hosts + * advertise it here. + */ + availableDisplayModes: string[]; +} + +/** One request/response pair with the host, for the diagnostics panel. */ +export interface HostExchange { + method: string; + params: unknown; + /** Verbatim result, or the failure if the request never resolved. */ + result?: unknown; + failure?: string; +} + +export interface BridgeInit { + chartData: ChartData | null; + /** Composite payload when the tool was render_dashboard. */ + dashboard?: DashboardRender | null; + meta: ChartMeta; + context: HostContext; + capabilities: HostCapabilities; + diagnostics: HostDiagnostics; + /** True when a real MCP host answered the handshake. */ + connected: boolean; + /** + * True when running inside a host iframe. Distinguishes "embedded but the + * handshake failed" (connected=false, embedded=true → show a connection + * error, NEVER fake data) from "standalone dev" (embedded=false → sample + * data is fine). + */ + embedded: boolean; + /** Error message when the initial tool result was a ChartError / isError. */ + error?: string; +} + +/** App identity sent in the ui/initialize handshake (required by the spec). */ +const APP_INFO = { name: 'superset-chart-viewer', version: '1.0.0' }; + +export type ContextListener = (ctx: Partial<HostContext>) => void; +export type ToolResultListener = ( + data: ChartData | null, + meta: ChartMeta, + error?: string, + dashboard?: DashboardRender | null, +) => void; + +interface PendingCall { + resolve: (value: unknown) => void; + reject: (reason: unknown) => void; +} + +const PROTOCOL_VERSION = '2026-01-26'; + +export class ChartBridge { + private id = 0; + private pending = new Map<number, PendingCall>(); + private contextListeners = new Set<ContextListener>(); + private resultListeners = new Set<ToolResultListener>(); + private capabilities: HostCapabilities = emptyCapabilities(); + /** Modes from HostContext.availableDisplayModes; null when unadvertised. */ + private hostDisplayModes: Set<string> | null = null; + private hostMaxHeight: number | null = null; + private diagnostics: HostDiagnostics = buildDiagnostics( + undefined, + emptyCapabilities(), + false, + ); + + /** + * Raw handshake data, for the in-widget diagnostics panel. + * + * Read directly by the panel rather than threaded through props: it must + * stay available on every render path (loading, error, chart) without + * depending on component state that a failed handshake never populates. + */ + getDiagnostics(): HostDiagnostics { + return this.diagnostics; + } + + private get isEmbedded(): boolean { + return ( + typeof window !== 'undefined' && window.parent && window.parent !== window + ); + } + + /** Perform the ui/initialize handshake. Resolves with host-provided data. */ + async initialize(timeoutMs = 1500): Promise<BridgeInit> { + if (!this.isEmbedded) { + return this.standaloneInit(); + } + window.addEventListener('message', this.onMessage); + + try { + const result = (await this.request( + 'ui/initialize', + { + protocolVersion: PROTOCOL_VERSION, + appInfo: APP_INFO, + appCapabilities: { + availableDisplayModes: ['inline', 'fullscreen', 'pip'], + }, + }, + timeoutMs, + )) as HostInitResult; + + this.capabilities = deriveCapabilities(result); + this.hostDisplayModes = readDisplayModes(result?.hostContext); + this.hostMaxHeight = readMaxHeight(result?.hostContext); + this.diagnostics = buildDiagnostics(result, this.capabilities, true); + this.notify('ui/notifications/initialized', {}); + + const { chartData, dashboard, meta, error } = extractToolResult( + result?.toolResult, + ); + return { + chartData, + dashboard, + meta, + context: parseHostContext(result?.hostContext), + capabilities: this.capabilities, + diagnostics: this.diagnostics, + connected: true, + embedded: true, + error, + }; + } catch { + // Host present but no timely/valid handshake. Do NOT fall back to sample + // data — that would render fake numbers as if they were the user's chart. + // Signal embedded+disconnected so the app shows a connection error. + this.capabilities = emptyCapabilities(); + this.diagnostics = buildDiagnostics(undefined, this.capabilities, true); + return { + chartData: null, + meta: {}, + context: { scheme: detectScheme() }, + capabilities: this.capabilities, + diagnostics: this.diagnostics, + connected: false, + embedded: true, + }; + } + } + + private standaloneInit(): BridgeInit { + this.capabilities = emptyCapabilities(); + this.diagnostics = buildDiagnostics(undefined, this.capabilities, false); + return { + chartData: null, + meta: {}, + context: { scheme: detectScheme() }, + capabilities: this.capabilities, + diagnostics: this.diagnostics, + connected: false, + embedded: false, + }; + } + + getCapabilities(): HostCapabilities { + return this.capabilities; + } + + /** Subscribe to host context changes (theme / display mode / size). */ + onContextChange(fn: ContextListener): () => void { + this.contextListeners.add(fn); + return () => this.contextListeners.delete(fn); + } + + /** Subscribe to late tool-result pushes (host may send data after init). */ + onToolResult(fn: ToolResultListener): () => void { + this.resultListeners.add(fn); + return () => this.resultListeners.delete(fn); + } + + /** Call an app-visible server tool (e.g. render_chart_requery). */ + async callTool<T = unknown>( + name: string, + args: Record<string, unknown>, + ): Promise<T> { + if (!this.isEmbedded || !this.capabilities.canCallTools) { + throw new Error('tools/call unavailable outside a host'); + } + const res = (await this.request('tools/call', { + name, + arguments: args, + })) as { + structuredContent?: unknown; + content?: Array<{ type: string; text?: string }>; + }; + return coerceToolResultData(res) as T; + } + + /** True if the host exposes a given app-visible tool. */ + hasTool(name: string): boolean { + // If the host enumerates app tools, require membership. Otherwise fall back + // to whether tools/call is supported at all (an unknown capability is + // treated as unsupported, so this stays false unless the host advertised + // tool-calling). Drill affordances gate on this and disable cleanly. + return this.capabilities.appTools.size + ? this.capabilities.appTools.has(name) + : this.capabilities.canCallTools; + } + + /** Push a concise context string for the model's next turn ("Ask about this"). */ + async updateModelContext( + text: string, + structured?: Record<string, unknown>, + ): Promise<void> { + if (!this.isEmbedded || !this.capabilities.canUpdateModelContext) return; + try { + await this.request('ui/update-model-context', { + content: [{ type: 'text', text }], + ...(structured ? { structuredContent: structured } : {}), + }); + } catch { + /* best-effort */ + } + } + + /** Send a follow-up user message to the host chat (feature-detected). */ + async sendMessage(text: string): Promise<void> { + if (!this.isEmbedded || !this.capabilities.canSendMessage) return; + try { + await this.request('ui/message', { + role: 'user', + content: [{ type: 'text', text }], + }); + } catch { + /* best-effort */ + } + } + + /** + * Request the host open an external link (deep link to Superset). + * + * Hosts that do not implement ``ui/open-link`` typically leave the request + * unanswered, so this uses a short timeout rather than the default: a click + * must not sit for eight seconds doing nothing. On any failure it falls back + * to opening directly, which works unless the iframe sandbox forbids popups. + * Returns false when the link could not be opened by either route, so the + * caller can offer the URL another way instead of failing silently. + */ + async openLink(url: string, timeoutMs = 4000): Promise<boolean> { + // Ask the host FIRST when it says it can do this. `openLinks` is a + // spec-named capability backed by ui/open-link; going to window.open first + // meant a sandboxed iframe (which blocks it) fell through to a host request + // we then treated as a last resort. The host is the supported route. + if (this.isEmbedded && this.capabilities.canOpenLinks) { + if (await this.requestOk('ui/open-link', { url }, timeoutMs)) return true; + } + // Synchronously, inside the click's user gesture: awaiting anything first + // spends transient activation and gets the popup blocked. + try { + if (typeof window !== 'undefined' && window.open(url, '_blank', 'noopener')) + return true; Review Comment: **Suggestion:** When the host advertises `openLinks` but the host request fails or times out, this `await` completes before `window.open` is attempted. That consumes the click's transient user activation, so the direct popup fallback is commonly blocked even though it would have worked when invoked synchronously from the click handler. Open a synchronously-created popup before awaiting the host response, or use a separate synchronous fallback path. [logic error] <details> <summary><b>Severity Level:</b> Minor 🧹</summary> ```mdx - ⚠️ Explore links fail to open when host handling times out. - ⚠️ Embedded users fall back to copying URLs manually. - ⚠️ The failure affects hosts advertising unavailable link support. ``` </details> [](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9a03b0a2858e4c5d87737d7a95b43b10&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=9a03b0a2858e4c5d87737d7a95b43b10&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/chart/resources/chart_viewer/src/bridge.ts **Line:** 362:369 **Comment:** *Logic Error: When the host advertises `openLinks` but the host request fails or times out, this `await` completes before `window.open` is attempted. That consumes the click's transient user activation, so the direct popup fallback is commonly blocked even though it would have worked when invoked synchronously from the click handler. Open a synchronously-created popup before awaiting the host response, or use a separate synchronous fallback path. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43483&comment_hash=3036db1b68fe9c2cf4044d255d65ee59178542b52e157576b89743966c9e6a0f&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43483&comment_hash=3036db1b68fe9c2cf4044d255d65ee59178542b52e157576b89743966c9e6a0f&reaction=dislike'>👎</a> ########## superset/mcp_service/middleware.py: ########## @@ -696,6 +696,35 @@ async def on_message( return await call_next(context) +# Tools that MUST retain ``outputSchema`` / ``structuredContent`` despite the +# global stripping. MCP Apps widgets read the structured tool result to render an +# interactive UI, so stripping it would break the widget. ``render_chart`` is the +# initial render; ``render_chart_requery`` returns the fresh data the widget +# renders after a drill-down / zoom, so it must be exempt too. +# Overridable via the ``MCP_STRUCTURED_CONTENT_KEEP_TOOLS`` config key. +DEFAULT_STRUCTURED_CONTENT_KEEP_TOOLS: frozenset[str] = frozenset( + {"render_chart", "render_chart_requery"} +) Review Comment: **Suggestion:** The fallback keep-list omits `render_dashboard`, even though the dashboard tool is registered as a structured-content widget and the configured keep-list includes it. Whenever this middleware runs without a Flask application context, `_keep_tools()` returns this default and strips `render_dashboard`'s `output_schema` and `structured_content`, preventing dashboard widgets from rendering. Include all structured-content widget tools in the default set. [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Dashboard MCP Apps lose structured widget rendering. - ❌ `render_dashboard` output schema is removed on fallback. - ⚠️ Dashboard results degrade to non-interactive text responses. ``` </details> [](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7985eb5a31034d34950723bda78af553&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=7985eb5a31034d34950723bda78af553&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/middleware.py **Line:** 705:707 **Comment:** *Api Mismatch: The fallback keep-list omits `render_dashboard`, even though the dashboard tool is registered as a structured-content widget and the configured keep-list includes it. Whenever this middleware runs without a Flask application context, `_keep_tools()` returns this default and strips `render_dashboard`'s `output_schema` and `structured_content`, preventing dashboard widgets from rendering. Include all structured-content widget tools in the default set. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43483&comment_hash=4d4362232508bfc207a2893bf7fb429f1e3f9c718956cd5333bd93eab380f615&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43483&comment_hash=4d4362232508bfc207a2893bf7fb429f1e3f9c718956cd5333bd93eab380f615&reaction=dislike'>👎</a> ########## superset/mcp_service/middleware.py: ########## @@ -696,6 +696,35 @@ async def on_message( return await call_next(context) +# Tools that MUST retain ``outputSchema`` / ``structuredContent`` despite the +# global stripping. MCP Apps widgets read the structured tool result to render an +# interactive UI, so stripping it would break the widget. ``render_chart`` is the +# initial render; ``render_chart_requery`` returns the fresh data the widget +# renders after a drill-down / zoom, so it must be exempt too. +# Overridable via the ``MCP_STRUCTURED_CONTENT_KEEP_TOOLS`` config key. +DEFAULT_STRUCTURED_CONTENT_KEEP_TOOLS: frozenset[str] = frozenset( + {"render_chart", "render_chart_requery"} +) + + +def _schema_shaped_error(exc: Exception, tool_name: str | None) -> dict[str, Any]: + """Build an error payload that satisfies a keep-list tool's ``outputSchema``. + + Keep-list tools return a union (``ChartData | ChartError``), which FastMCP + advertises as ``{"result": {"anyOf": [...]}}`` with ``x-fastmcp-wrap-result``. + Shaping the error as the ``MCPBaseError`` branch — wrapped in ``result`` — + keeps the failure path conformant with what the tool declared. + """ + from superset.mcp_service.common.error_schemas import MCPBaseError + + payload = MCPBaseError( + error_type=type(exc).__name__, + message=str(exc), + ).model_dump(mode="json") Review Comment: **Suggestion:** The structured error path bypasses `_sanitize_error_for_logging` and places `str(exc)` directly into structured content. Exceptions from database or authentication code can contain SQL fragments, connection details, or other sensitive internals, so keep-list tools expose data that the preceding text error path intentionally sanitizes. Sanitize the message before constructing `MCPBaseError`. [security] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Keep-list error responses can expose internal exception text. - ⚠️ MCP clients may receive SQL or connection details. - ⚠️ Dashboard and chart widget failures share this path. ``` </details> [](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1b153817a82d48b4a35722033d79032c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=1b153817a82d48b4a35722033d79032c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/middleware.py **Line:** 720:723 **Comment:** *Security: The structured error path bypasses `_sanitize_error_for_logging` and places `str(exc)` directly into structured content. Exceptions from database or authentication code can contain SQL fragments, connection details, or other sensitive internals, so keep-list tools expose data that the preceding text error path intentionally sanitizes. Sanitize the message before constructing `MCPBaseError`. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43483&comment_hash=250efd050a2d30caecada1261de1d1f4d5620c319420de0273417e0cf61da930&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43483&comment_hash=250efd050a2d30caecada1261de1d1f4d5620c319420de0273417e0cf61da930&reaction=dislike'>👎</a> ########## superset/mcp_service/chart/schemas.py: ########## @@ -632,23 +634,29 @@ def serialize_chart_object(chart: ChartLike | None) -> ChartInfo | None: changed_on_humanized=humanize_timestamp(getattr(chart, "changed_on", None)), created_on=getattr(chart, "created_on", None), created_on_humanized=humanize_timestamp(getattr(chart, "created_on", None)), - uuid=str(getattr(chart, "uuid", "")) - if getattr(chart, "uuid", None) - else None, + uuid=( + str(getattr(chart, "uuid", "")) + if getattr(chart, "uuid", None) + else None + ), deleted_at=getattr(chart, "deleted_at", None), - tags=[ - TagInfo.model_validate(tag, from_attributes=True) - for tag in getattr(chart, "tags", []) - ] - if getattr(chart, "tags", None) - else [], - editors=[ - info - for editor in getattr(chart, "editors", []) - if (info := serialize_subject_object(editor)) is not None - ] - if getattr(chart, "editors", None) - else [], + tags=( + [ + TagInfo.model_validate(tag, from_attributes=True) + for tag in getattr(chart, "tags", []) + ] + if getattr(chart, "tags", None) + else [] + ), + editors=( + [ + info + for editor in getattr(chart, "editors", []) + if (info := serialize_subject_object(editor)) is not None + ] + if getattr(chart, "editors", None) + else [] Review Comment: **Suggestion:** `list_charts` serializes each chart through this function without eager-loading `tags` or `editors`. Accessing both relationships for every item lazily issues additional queries per chart, turning a multi-chart listing into an N+1 query workload and potentially triggering detached-instance failures after the DAO session is closed. Either eager-load these relationships in the list query or avoid loading them when they were not requested. [performance] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ list_charts performs extra queries per returned chart. - ⚠️ Large chart pages increase database latency. - ❌ Detached objects can fail chart-list serialization. ``` </details> [](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0f094ee983db4c988020517efa0e2ba6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=0f094ee983db4c988020517efa0e2ba6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/chart/schemas.py **Line:** 643:658 **Comment:** *Performance: `list_charts` serializes each chart through this function without eager-loading `tags` or `editors`. Accessing both relationships for every item lazily issues additional queries per chart, turning a multi-chart listing into an N+1 query workload and potentially triggering detached-instance failures after the DAO session is closed. Either eager-load these relationships in the list query or avoid loading them when they were not requested. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43483&comment_hash=1840a4d36d52ef2a0fd496e5914d9456e933f6ce4ca60e28e56e7bd46e3a6619&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43483&comment_hash=1840a4d36d52ef2a0fd496e5914d9456e933f6ce4ca60e28e56e7bd46e3a6619&reaction=dislike'>👎</a> ########## superset/mcp_service/chart/tool/get_chart_data.py: ########## @@ -510,6 +586,9 @@ async def get_chart_data( # noqa: C901 # The query_context contains all the information needed to reproduce # the chart's data exactly as shown in the visualization query_context_json = None + # Set only when we fall back to reconstructing the query from + # form_data, which drops Superset's post_processing stage. + fidelity_warning: str | None = None Review Comment: **Suggestion:** The warning is initialized but only populated for the saved-chart path that falls back because `query_context` is absent. The `using_unsaved_state` path immediately builds a query from cached `form_data`, which has the same loss of Superset's saved query/post-processing pipeline, but leaves `fidelity_warning` as `None`; renderers therefore present potentially rearranged or numerically different results without the warning this change adds. Set the warning whenever the query is reconstructed from form data, including the unsaved-state branch. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Unsaved chart renders omit query-fidelity warnings. - ⚠️ MCP widgets may present rearranged chart data without disclosure. - ⚠️ Dashboard cells reuse the same unwarned core path. ``` </details> [](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=456daabbe8db4be9922c60b029a13743&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=456daabbe8db4be9922c60b029a13743&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/chart/tool/get_chart_data.py **Line:** 589:591 **Comment:** *Logic Error: The warning is initialized but only populated for the saved-chart path that falls back because `query_context` is absent. The `using_unsaved_state` path immediately builds a query from cached `form_data`, which has the same loss of Superset's saved query/post-processing pipeline, but leaves `fidelity_warning` as `None`; renderers therefore present potentially rearranged or numerically different results without the warning this change adds. Set the warning whenever the query is reconstructed from form data, including the unsaved-state branch. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43483&comment_hash=5cbfc41a6ed5d322185ec5a42664d050c4c3296cf3f3b0a26d054c311f9b60cd&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43483&comment_hash=5cbfc41a6ed5d322185ec5a42664d050c4c3296cf3f3b0a26d054c311f9b60cd&reaction=dislike'>👎</a> ########## superset/mcp_service/chart/resources/chart_viewer/src/bridge.ts: ########## @@ -0,0 +1,964 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/** + * Thin, well-isolated bridge to the MCP Apps host. The rest of the widget + * depends on this interface — never on the vendor package directly — so we can + * swap the transport (direct postMessage vs. @modelcontextprotocol/ext-apps) + * without touching the UI. + * + * It speaks the documented JSON-RPC-2.0-over-postMessage dialect from the + * MCP Apps spec (2026-01-26): ui/initialize handshake, tool-result / host + * context notifications, tools/call, ui/update-model-context, ui/open-link. + * Outside a host (standalone dev), every call no-ops gracefully. + */ +import type { + ChartData, + ChartMeta, + ColorScheme, + DashboardRender, +} from './types'; + +/** + * Display modes the widget can ask the host to switch between. + * + * `pip` is a floating overlay. The spec defines it as exactly that and says + * nothing about whether it survives the next conversation turn, so the widget + * offers it only where a host advertises it and makes no promise about how + * long it stays. + */ +export type DisplayMode = 'inline' | 'fullscreen' | 'pip'; + +export interface HostContext { + scheme: ColorScheme; + displayMode?: string; + container?: { width?: number; height?: number }; +} + +export interface HostCapabilities { + /** Names of tools the host exposes to the app (visibility: ["app"]). */ + appTools: Set<string>; + /** Whether the host accepts ui/update-model-context. */ + canUpdateModelContext: boolean; + /** Whether the host accepts ui/message follow-ups. */ + canSendMessage: boolean; + /** Whether tools/call is available at all. */ + canCallTools: boolean; + /** Whether the host will open external URLs on our behalf (ui/open-link). */ + canOpenLinks: boolean; + /** Whether the host will save files on our behalf (ui/download-file). */ + canDownloadFile: boolean; +} + +/** + * Everything the host told us at handshake, verbatim. + * + * `deriveCapabilities` guesses at several key spellings (`tools`, `toolCalls`, + * `appTools`, `experimental.appTools`, ...) because the spec does not pin them. + * A host that advertises under a name we do not read leaves every gated + * affordance silently switched off, which is indistinguishable from a broken + * feature. The raw maps are kept so that question can be answered by looking + * rather than by guessing. + */ +export interface HostDiagnostics { + protocolVersion?: string; + /** Exactly what the host sent — no normalisation. */ + hostCapabilities: Record<string, unknown>; + hostContext: Record<string, unknown>; + /** 'null' in a sandboxed iframe without allow-same-origin. */ + origin: string; + embedded: boolean; + /** What we concluded from the above. */ + derived: HostCapabilities; + /** + * Top-level keys the host actually sent, verbatim. + * + * Surfaced in the collapsed summary because that one line has twice now been + * the only diagnostic that made it out of a host — reading the expanded JSON + * depends on a human transcribing it, which kept failing. The key names are + * what identify a spelling mismatch, so they belong where they can be read + * at a glance. + */ + capabilityKeys: string[]; + /** Sandbox permissions the host granted (clipboard-write, etc.), if stated. */ + sandboxPermissions: string[]; + /** + * The last few host-mediated exchanges, request and response together. + * + * Download and open-link have each cost several build/restart/test cycles + * that ended in inferring backwards from a symptom, because the host's + * answer is invisible from outside the iframe. Recording what we sent and + * what came back turns "it does nothing" into a readable fact. + */ + exchanges: HostExchange[]; + /** + * Display modes the host offers (`inline` | `fullscreen` | `pip`). + * + * Surfaced because `pip` — a persistent side panel that survives while the + * conversation continues — is a spec mode the widget does not yet request, + * and whether it is worth building is decided entirely by whether hosts + * advertise it here. + */ + availableDisplayModes: string[]; +} + +/** One request/response pair with the host, for the diagnostics panel. */ +export interface HostExchange { + method: string; + params: unknown; + /** Verbatim result, or the failure if the request never resolved. */ + result?: unknown; + failure?: string; +} + +export interface BridgeInit { + chartData: ChartData | null; + /** Composite payload when the tool was render_dashboard. */ + dashboard?: DashboardRender | null; + meta: ChartMeta; + context: HostContext; + capabilities: HostCapabilities; + diagnostics: HostDiagnostics; + /** True when a real MCP host answered the handshake. */ + connected: boolean; + /** + * True when running inside a host iframe. Distinguishes "embedded but the + * handshake failed" (connected=false, embedded=true → show a connection + * error, NEVER fake data) from "standalone dev" (embedded=false → sample + * data is fine). + */ + embedded: boolean; + /** Error message when the initial tool result was a ChartError / isError. */ + error?: string; +} + +/** App identity sent in the ui/initialize handshake (required by the spec). */ +const APP_INFO = { name: 'superset-chart-viewer', version: '1.0.0' }; + +export type ContextListener = (ctx: Partial<HostContext>) => void; +export type ToolResultListener = ( + data: ChartData | null, + meta: ChartMeta, + error?: string, + dashboard?: DashboardRender | null, +) => void; + +interface PendingCall { + resolve: (value: unknown) => void; + reject: (reason: unknown) => void; +} + +const PROTOCOL_VERSION = '2026-01-26'; + +export class ChartBridge { + private id = 0; + private pending = new Map<number, PendingCall>(); + private contextListeners = new Set<ContextListener>(); + private resultListeners = new Set<ToolResultListener>(); + private capabilities: HostCapabilities = emptyCapabilities(); + /** Modes from HostContext.availableDisplayModes; null when unadvertised. */ + private hostDisplayModes: Set<string> | null = null; + private hostMaxHeight: number | null = null; + private diagnostics: HostDiagnostics = buildDiagnostics( + undefined, + emptyCapabilities(), + false, + ); + + /** + * Raw handshake data, for the in-widget diagnostics panel. + * + * Read directly by the panel rather than threaded through props: it must + * stay available on every render path (loading, error, chart) without + * depending on component state that a failed handshake never populates. + */ + getDiagnostics(): HostDiagnostics { + return this.diagnostics; + } + + private get isEmbedded(): boolean { + return ( + typeof window !== 'undefined' && window.parent && window.parent !== window + ); + } + + /** Perform the ui/initialize handshake. Resolves with host-provided data. */ + async initialize(timeoutMs = 1500): Promise<BridgeInit> { + if (!this.isEmbedded) { + return this.standaloneInit(); + } + window.addEventListener('message', this.onMessage); + + try { + const result = (await this.request( + 'ui/initialize', + { + protocolVersion: PROTOCOL_VERSION, + appInfo: APP_INFO, + appCapabilities: { + availableDisplayModes: ['inline', 'fullscreen', 'pip'], + }, + }, + timeoutMs, + )) as HostInitResult; + + this.capabilities = deriveCapabilities(result); + this.hostDisplayModes = readDisplayModes(result?.hostContext); + this.hostMaxHeight = readMaxHeight(result?.hostContext); + this.diagnostics = buildDiagnostics(result, this.capabilities, true); + this.notify('ui/notifications/initialized', {}); + + const { chartData, dashboard, meta, error } = extractToolResult( + result?.toolResult, + ); + return { + chartData, + dashboard, + meta, + context: parseHostContext(result?.hostContext), + capabilities: this.capabilities, + diagnostics: this.diagnostics, + connected: true, + embedded: true, + error, + }; + } catch { + // Host present but no timely/valid handshake. Do NOT fall back to sample + // data — that would render fake numbers as if they were the user's chart. + // Signal embedded+disconnected so the app shows a connection error. + this.capabilities = emptyCapabilities(); + this.diagnostics = buildDiagnostics(undefined, this.capabilities, true); + return { + chartData: null, + meta: {}, + context: { scheme: detectScheme() }, + capabilities: this.capabilities, + diagnostics: this.diagnostics, + connected: false, + embedded: true, + }; + } + } + + private standaloneInit(): BridgeInit { + this.capabilities = emptyCapabilities(); + this.diagnostics = buildDiagnostics(undefined, this.capabilities, false); + return { + chartData: null, + meta: {}, + context: { scheme: detectScheme() }, + capabilities: this.capabilities, + diagnostics: this.diagnostics, + connected: false, + embedded: false, + }; + } + + getCapabilities(): HostCapabilities { + return this.capabilities; + } + + /** Subscribe to host context changes (theme / display mode / size). */ + onContextChange(fn: ContextListener): () => void { + this.contextListeners.add(fn); + return () => this.contextListeners.delete(fn); + } + + /** Subscribe to late tool-result pushes (host may send data after init). */ + onToolResult(fn: ToolResultListener): () => void { + this.resultListeners.add(fn); + return () => this.resultListeners.delete(fn); + } + + /** Call an app-visible server tool (e.g. render_chart_requery). */ + async callTool<T = unknown>( + name: string, + args: Record<string, unknown>, + ): Promise<T> { + if (!this.isEmbedded || !this.capabilities.canCallTools) { + throw new Error('tools/call unavailable outside a host'); + } + const res = (await this.request('tools/call', { + name, + arguments: args, + })) as { + structuredContent?: unknown; + content?: Array<{ type: string; text?: string }>; + }; + return coerceToolResultData(res) as T; + } + + /** True if the host exposes a given app-visible tool. */ + hasTool(name: string): boolean { + // If the host enumerates app tools, require membership. Otherwise fall back + // to whether tools/call is supported at all (an unknown capability is + // treated as unsupported, so this stays false unless the host advertised + // tool-calling). Drill affordances gate on this and disable cleanly. + return this.capabilities.appTools.size + ? this.capabilities.appTools.has(name) + : this.capabilities.canCallTools; + } + + /** Push a concise context string for the model's next turn ("Ask about this"). */ + async updateModelContext( + text: string, + structured?: Record<string, unknown>, + ): Promise<void> { + if (!this.isEmbedded || !this.capabilities.canUpdateModelContext) return; + try { + await this.request('ui/update-model-context', { + content: [{ type: 'text', text }], + ...(structured ? { structuredContent: structured } : {}), + }); + } catch { + /* best-effort */ + } + } + + /** Send a follow-up user message to the host chat (feature-detected). */ + async sendMessage(text: string): Promise<void> { + if (!this.isEmbedded || !this.capabilities.canSendMessage) return; + try { + await this.request('ui/message', { + role: 'user', + content: [{ type: 'text', text }], + }); + } catch { + /* best-effort */ + } + } + + /** + * Request the host open an external link (deep link to Superset). + * + * Hosts that do not implement ``ui/open-link`` typically leave the request + * unanswered, so this uses a short timeout rather than the default: a click + * must not sit for eight seconds doing nothing. On any failure it falls back + * to opening directly, which works unless the iframe sandbox forbids popups. + * Returns false when the link could not be opened by either route, so the + * caller can offer the URL another way instead of failing silently. + */ + async openLink(url: string, timeoutMs = 4000): Promise<boolean> { + // Ask the host FIRST when it says it can do this. `openLinks` is a + // spec-named capability backed by ui/open-link; going to window.open first + // meant a sandboxed iframe (which blocks it) fell through to a host request + // we then treated as a last resort. The host is the supported route. + if (this.isEmbedded && this.capabilities.canOpenLinks) { + if (await this.requestOk('ui/open-link', { url }, timeoutMs)) return true; + } + // Synchronously, inside the click's user gesture: awaiting anything first + // spends transient activation and gets the popup blocked. + try { + if (typeof window !== 'undefined' && window.open(url, '_blank', 'noopener')) + return true; + } catch { + /* sandboxed without allow-popups */ + } + if (this.isEmbedded && !this.capabilities.canOpenLinks) { + // Unadvertised, but the spec says hosts SHOULD implement it — worth one + // attempt before giving up. + if (await this.requestOk('ui/open-link', { url }, timeoutMs)) return true; + } + return false; + } + + + /** + * Ask the host to save a file for the user. + * + * The spec exists for exactly our situation: "Since MCP Apps run in + * sandboxed iframes where direct downloads are blocked, this provides a + * host-mediated mechanism for file exports." The widget was instead building + * a blob and clicking an anchor — a browser primitive the sandbox blocks + * silently — while the host advertised `downloadFile` the whole time. + * + * Returns false when the host cannot do it or the user declined, so the + * caller can fall back to showing the text instead of claiming a save. + */ + async downloadViaHost( + filename: string, + mimeType: string, + text: string, + // Generous by default: the spec says the host SHOULD confirm with the user + // first, so this waits on a human, not a machine. + timeoutMs = 8000, + ): Promise<boolean> { + if (!this.isEmbedded || !this.capabilities.canDownloadFile) return false; + return this.requestOk( + 'ui/download-file', + { + contents: [ + { + type: 'resource', + resource: { uri: `file:///${filename}`, mimeType, text }, + }, + ], + }, + timeoutMs, + ); + } + + + /** + * Send a request whose result carries `isError`, and record the exchange. + * + * The spec marks refusal with `isError` on the RESULT — the promise still + * resolves. Treating a resolved promise as success is how "Open in Superset" + * reported a link it had opened nothing for, and it is the same defect as + * the display-mode control adopting a mode the host declined. + */ + private async requestOk( + method: string, + params: unknown, + timeoutMs: number, + ): Promise<boolean> { + try { + const result = (await this.request(method, params, timeoutMs)) as + | { isError?: boolean } + | undefined; + const exchange: HostExchange = { method, params, result }; + this.record(exchange); + if (result?.isError === true) { + this.reportFailure(exchange); + return false; + } + return true; + } catch (err) { + const exchange: HostExchange = { method, params, failure: String(err) }; + this.record(exchange); + this.reportFailure(exchange); + return false; + } + } + + private record(exchange: HostExchange): void { + const kept = [...this.diagnostics.exchanges, exchange].slice(-6); + this.diagnostics = { ...this.diagnostics, exchanges: kept }; + } + + /** Operations already reported, so a repeated failure cannot spam context. */ + private reportedFailures = new Set<string>(); + + /** + * Push a failed host operation into the model's context. + * + * Every capability question on this branch — serverTools, the display modes, + * the download and open-link routes — took several round trips for one + * reason: the host's answer is visible only inside the iframe, so diagnosing + * it needed a person to read JSON off a screen and retype it. The host + * advertises `updateModelContext`, so the widget can hand the detail to the + * assistant directly instead. + * + * Failures only, once per operation, and never for the reporting call itself + * — a report that could fail and then report its own failure would loop. + */ + private reportFailure(exchange: HostExchange): void { + // Defensive, not load-bearing: updateModelContext sends via `request` + // rather than `requestOk`, so it cannot reach here today. Kept so that + // routing it through the shared helper later cannot create a report that + // reports its own failure. + if (exchange.method === 'ui/update-model-context') return; + if (!this.capabilities.canUpdateModelContext) return; + if (this.reportedFailures.has(exchange.method)) return; + this.reportedFailures.add(exchange.method); + const detail = exchange.failure + ? `no reply (${exchange.failure})` + : `replied ${safeJson(exchange.result)}`; + void this.updateModelContext( + `Superset chart widget diagnostic: the host declined ${exchange.method}. ` + + `It ${detail}. Sent: ${safeJson(exchange.params)}. ` + + `Developer detail, not something the user asked about.`, + ); + } + + /** Report intrinsic content size so the host can size the iframe. */ + reportSize(width: number, height: number): void { + if (!this.isEmbedded) return; + this.notify('ui/notifications/size-changed', { width, height }); + } + + /** + * The tallest frame the host says it will give us, if it says. + * + * Desktop reports maxHeight 5000 while the widget capped itself at 1200 — + * and since that host offers no fullscreen mode, growing the frame IS the + * maximize feature, so the guess was the ceiling on the control people use + * most. + */ + getHostMaxHeight(): number | null { + return this.hostMaxHeight; + } + + /** Display modes the host advertised, or null if it advertised none. */ + getHostDisplayModes(): Set<string> | null { + return this.hostDisplayModes; + } + + /** + * Whether the host offers a mode, for gating the control that requests it. + * + * A host that advertises nothing is treated as "might support it" — we + * cannot tell, and requesting is harmless. A host that advertises a list + * without this mode is treated as a definite no, so the widget never shows + * a button it knows will do nothing. + */ + supportsDisplayMode(mode: DisplayMode): boolean { + return this.hostDisplayModes ? this.hostDisplayModes.has(mode) : true; + } + + /** + * Ask the host to switch display mode, resolving with the mode the host + * actually applied. + * + * Deliberately NOT a boolean. The spec requires the host to return the + * resulting mode "whether updated or not", and a host that declines a switch + * answers with the mode it is staying in — so `null` (no usable answer) and + * "declined, still fullscreen" are different situations that need different + * handling. Collapsing both to `false` made the widget treat a refusal as + * "host has no display-mode support" and set its own state to the mode it + * had merely *asked* for, which is how the collapse control came to report + * success while the host stayed expanded. + * + * Returns null when the host cannot service the request at all: not embedded, + * the mode is absent from the host's advertised `availableDisplayModes`, or + * the request went unanswered. + */ + async requestDisplayMode( + mode: DisplayMode, + timeoutMs = 1200, + ): Promise<DisplayMode | null> { + if (!this.isEmbedded) return null; + // The spec makes checking this the View's obligation, and it also spares + // the user a timeout's worth of dead button on hosts without mode support. + if (this.hostDisplayModes && !this.hostDisplayModes.has(mode)) return null; + try { + const result = (await this.request( + 'ui/request-display-mode', + { mode }, + timeoutMs, + )) as { mode?: unknown }; + return asDisplayMode(result?.mode); + } catch { + return null; + } + } + + // ---- transport internals ------------------------------------------------- + + private request( + method: string, + params: unknown, + timeoutMs = 8000, + ): Promise<unknown> { + return new Promise((resolve, reject) => { + const id = ++this.id; + this.pending.set(id, { resolve, reject }); + this.post({ jsonrpc: '2.0', id, method, params }); + if (timeoutMs > 0) { + setTimeout(() => { + if (this.pending.has(id)) { + this.pending.delete(id); + reject(new Error(`Timed out waiting for ${method}`)); + } + }, timeoutMs); + } + }); + } + + private notify(method: string, params: unknown): void { + this.post({ jsonrpc: '2.0', method, params }); + } + + private post(msg: unknown): void { + try { + window.parent.postMessage(msg, '*'); + } catch { + /* no-op */ + } + } + + private onMessage = (event: MessageEvent): void => { + const msg = event.data as JsonRpcMessage | undefined; + if (!msg || msg.jsonrpc !== '2.0') return; Review Comment: **Suggestion:** The message handler accepts responses from any `postMessage` sender without checking `event.source` or the expected host origin. Because request IDs are predictable, another frame or window can spoof a response, inject tool-result data, or alter capability and link-operation results. Restrict responses to the initialized parent window and validate the allowed origin where available. [security] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Untrusted frames can inject chart-result notifications. - ⚠️ Spoofed responses can alter displayed chart integrity. - ⚠️ Capability responses can influence link or tool operations. ``` </details> [](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b078be26a7c34ca29c55b21de20533ee&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=b078be26a7c34ca29c55b21de20533ee&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/chart/resources/chart_viewer/src/bridge.ts **Line:** 596:598 **Comment:** *Security: The message handler accepts responses from any `postMessage` sender without checking `event.source` or the expected host origin. Because request IDs are predictable, another frame or window can spoof a response, inject tool-result data, or alter capability and link-operation results. Restrict responses to the initialized parent window and validate the allowed origin where available. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43483&comment_hash=4e166aa6705ec146f4064060cd26a1bb1b2c7ebb0e2a5f4c7ea889d7f18bfe93&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43483&comment_hash=4e166aa6705ec146f4064060cd26a1bb1b2c7ebb0e2a5f4c7ea889d7f18bfe93&reaction=dislike'>👎</a> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
