Skip to content

[Chat]Add dashboard context-aware and enable auto-vis - #12424

Open
Qxisylolo wants to merge 4 commits into
opensearch-project:mainfrom
Qxisylolo:fix/add_text2vis
Open

[Chat]Add dashboard context-aware and enable auto-vis#12424
Qxisylolo wants to merge 4 commits into
opensearch-project:mainfrom
Qxisylolo:fix/add_text2vis

Conversation

@Qxisylolo

@Qxisylolo Qxisylolo commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Description

This PR adds an AI-driven visualization capability to the chat assistant. Users can describe what they want in natural language (e.g. "show the average ticket price per airport"), and the assistant generates a PPL query, picks a suitable chart type, renders a live chart preview inside the chat window, and offers an "Open in Editor" link that deep-links into the full visualization editor.

The core new tool is auto_create_visualization. And it relies on opensearch-project/opensearch-mcp-server-py#257.

The intended tool-call workflow (encoded in the tool description) is:
Index mapping tool → look up timeFieldName
ppl_execute → run the query, return the result column schema
auto_create_visualization → resolve chart config from columns, render preview, provide editor link

Besides, this pr adds:

  1. fetch_panel_data tool , access panel explore embeddable data
  2. Add dashboard page context and screenshot configuration, revert changes in [Chat] Add include screenshot in chatbot for dashboards page #11287

Screenshot

  1. ask for dashboard panel data
2026-07-21.09.17.38.mov
  1. text2v and add to dashboard
2026-07-21.13.51.13.mov

Testing the changes

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Commits are signed per the DCO using --signoff

Signed-off-by: Qxisylolo <qianxisy@amazon.com>
Signed-off-by: Qxisylolo <qianxisy@amazon.com>
@github-actions

github-actions Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit 8f9fe80)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 Security concerns

Potential cross-context data exposure:
The fetch_panel_data tool exposes a process-wide store of panel row data keyed only by savedObjectId, with no check that the caller/assistant should have access to that panel. If any panel data was ever loaded in the current browser session, the assistant tool can retrieve it by ID, even outside the current dashboard/workspace context. Consider scoping access to panels currently rendered in the active page context.

✅ No TODO sections
🔀 Multiple PR themes

Sub-PR theme: Add fetch_panel_data tool and PanelDataService

Relevant files:

  • src/plugins/explore/public/embeddable/panel_data_service.ts
  • src/plugins/explore/public/embeddable/panel_data_service.test.ts
  • src/plugins/explore/public/embeddable/index.ts
  • src/plugins/explore/public/embeddable/explore_embeddable.tsx
  • src/plugins/explore/public/embeddable/explore_embeddable.test.tsx

Sub-PR theme: Add auto_create_visualization tool and explicit timeRange in PPL search

Relevant files:

  • src/plugins/explore/public/components/visualizations/actions/auto_visualization_action.tsx
  • src/plugins/explore/public/components/visualizations/actions/auto_visualization_action.test.tsx
  • src/plugins/explore/public/components/visualizations/actions/utils.ts
  • src/plugins/explore/public/plugin.ts
  • src/plugins/data/common/search/search_source/types.ts
  • src/plugins/query_enhancements/public/search/ppl_search_interceptor.ts

Sub-PR theme: Register dashboard page context and screenshot configuration

Relevant files:

  • src/plugins/dashboard/public/application/components/dashboard_editor.tsx
  • src/plugins/dashboard/public/application/index.tsx
  • src/plugins/dashboard/opensearch_dashboards.json

⚡ Recommended focus areas for review

Open redirect risk

openVisualizationEditor sets window.location.href directly using an editorPath derived from tool arguments (query, indexName, potentialChartType, timeRange, etc.). Although the path is prefixed with the app base and encoded via rison/encodeURIComponent, if an attacker (or a malicious LLM output) manages to inject a path that breaks out of the intended URL structure, this could be exploited. Verify that editorPath cannot introduce a scheme or // prefix that would escape the app base, and consider validating/normalizing the final URL before assigning to window.location.href.

function openVisualizationEditor(core: CoreStart, editorPath: string) {
  const visEditorAppBase = core.http.basePath.prepend(`/app/${VISUALIZATION_EDITOR_APP_ID}`);
  if (window.location.pathname.startsWith(visEditorAppBase)) {
    // When already on the vis editor app, navigateToApp only calls history.push without
    // remounting the page, so the new URL params are ignored by the already-initialized
    // component. Use window.location.href instead to force a full navigation that
    // unmounts and remounts the app.
    window.location.href = `${visEditorAppBase}${editorPath}`;
  } else {
    core.application.navigateToApp(VISUALIZATION_EDITOR_APP_ID, { path: editorPath });
  }
}
Data leakage across workspaces

PanelDataService is a process-wide singleton keyed only by savedObjectId, and the fetch_panel_data tool description states it can be called by the assistant when the user asks for specific data. Any chat/assistant session in the same browser could request panel data for any saved object ID that has ever been rendered (even from a different dashboard/workspace previously visited). There is no authorization check tying the request back to the currently displayed panel/context. Consider validating that the requested savedObjectId corresponds to a panel currently visible/allowed in the active dashboard context.

  handler: async (args: { savedObjectId: string }) => {
    const entry = this.store.get(args.savedObjectId);
    if (!entry) {
      return {
        success: false,
        message: `No data available for panel ${args.savedObjectId}. The panel may not be loaded yet.`,
      };
    }

    const formattedRows = entry.rows.map((hit: any) => hit._source || hit.fields || hit);
    return {
      success: true,
      panelTitle: entry.panelTitle,
      savedObjectId: args.savedObjectId,
      rowCount: formattedRows.length,
      rows: formattedRows,
    };
  },
});
Uncached dataset lookup may fail

executePPLQuery calls data.query.queryString.getDatasetService().cacheDataset(dataset, ...) and then immediately data.dataViews.get(dataset.id). The dataset is built manually in buildPreparedQuery with an id like ${datasourceId}_${indexName} and is not created as a saved object. If cacheDataset fails silently or dataViews.get doesn't resolve the transient id, the chart preview will error. Consider handling the failure path explicitly or resolving a data view through the same code path used by the explore app.

async function executePPLQuery(
  preparedQueryObject: PreparedQuery,
  core: CoreStart,
  data: DataPublicPluginStart,
  timeRange?: TimeRange,
  abortSignal?: AbortSignal
): Promise<VisData> {
  const uiSettings = core.uiSettings;
  const dataset = preparedQueryObject.dataset;

  await data.query.queryString.getDatasetService().cacheDataset(
    dataset,
    {
      uiSettings: core.uiSettings,
      savedObjects: core.savedObjects,
      notifications: core.notifications,
      http: core.http,
      data,
    },
    false
  );
  const dataView = await data.dataViews.get(dataset.id);

  const size = uiSettings.get(SAMPLE_SIZE_SETTING);
  const searchSource = await data.search.searchSource.create();

  searchSource.setFields({
    index: dataView,
    size,
    query: preparedQueryObject,
    highlightAll: false,
    version: false,

    // Pass absolute time range so the PPL search interceptor injects
    // it into the query instead of using the global timefilter.
    ...(timeRange && dataset.timeFieldName ? { timeRange } : {}),
  });

  const languageConfig = data.query.queryString
    .getLanguageService()
    .getLanguage(preparedQueryObject.language);

  // Execute query
  const rawResults = await searchSource.fetch({
    abortSignal,
    withLongNumeralsSupport: await uiSettings.get('data:withLongNumerals'),
    ...(languageConfig?.fields?.formatter ? { formatter: languageConfig.fields.formatter } : {}),
  });

  const rawResultsHit = rawResults.hits?.hits ?? [];
  const fieldSchema = searchSource.getDataFrame()?.schema ?? [];
  return normalizeResultRows(rawResultsHit, fieldSchema);
}
Unbounded memory growth

setPanelData stores full row arrays keyed by savedObjectId and only removes entries when an embeddable is destroyed. For dashboards with many/large panels, or when embeddables are recreated without destroy running (e.g., page navigations that skip destroy), the map can retain large result sets indefinitely. Consider capping row count/size per entry or clearing on route changes.

setPanelData(savedObjectId: string, entry: PanelDataEntry) {
  this.store.set(savedObjectId, entry);
  this.registerTool();
}

removePanelData(savedObjectId: string) {
  this.store.delete(savedObjectId);
}

@github-actions

github-actions Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to 8f9fe80

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Use consistent optional chaining on cleanup

The cleanup function uses chat?.screenshot.configure (non-optional access on
screenshot), which will throw a TypeError if chat exists but screenshot is undefined
at unmount time. Use optional chaining consistently to match the setup call above.

src/plugins/dashboard/public/application/components/dashboard_editor.tsx [62-72]

 useEffect(() => {
   chat?.screenshot?.configure({
     enabled: true,
     title: i18n.translate('dashboard.editor.chat.addScreenshot', {
       defaultMessage: 'Add dashboard screenshot',
     }),
   });
   return () => {
-    chat?.screenshot.configure({ enabled: false });
+    chat?.screenshot?.configure({ enabled: false });
   };
 }, [chat]);
Suggestion importance[1-10]: 7

__

Why: Valid catch: the cleanup uses chat?.screenshot.configure while setup uses chat?.screenshot?.configure, which could throw if screenshot is undefined at unmount. Small but correct defensive fix.

Medium
Guard singleton reset when uninitialized

PanelDataService.getInstance() throws if init() was never called (e.g., when
contextProvider plugin is not present, since init is only invoked inside the if
(plugins.contextProvider) block in start()). Guard the reset to avoid throwing
during plugin stop.

src/plugins/explore/public/plugin.ts [853-862]

 public stop() {
     Object.values(this.stopUrlTrackingCallbackByApp).forEach((callback) => callback());
     if (this.editorStopUrlTracking) {
       this.editorStopUrlTracking();
     }
     this.unregisterPPLExecuteQueryAction?.();
     this.unregisterVisualizationTools?.();
     // cleanup shared panel-data store + fetch_panel_data tool.
-    PanelDataService.getInstance().reset();
+    try {
+      PanelDataService.getInstance().reset();
+    } catch {
+      // PanelDataService was never initialized (no contextProvider plugin) — safe to ignore.
+    }
   }
Suggestion importance[1-10]: 7

__

Why: Legitimate concern: PanelDataService.init is only called when contextProvider is present, but stop() unconditionally calls getInstance().reset(), which throws if not initialized. Guarding prevents runtime errors during plugin stop.

Medium
General
Locate user additional message robustly

Only inspecting additionalMessages[0] will miss user messages that appear at other
indices (e.g., after a system message) and will also mutate the messages sent to the
assistant if the same additionalMessages array is passed to
subscribeToMessageStream, causing the injected context/image to be duplicated.
Consider filtering the first user entry explicitly and/or removing it from
messagesToSend to prevent duplicate content in the outgoing payload.

src/plugins/chat/public/components/chat_window.tsx [442-460]

-const userAdditionalMessage =
-  additionalMessages[0] && additionalMessages[0]?.role === 'user'
-    ? additionalMessages[0]
-    : null;
+const userAdditionalMessage = additionalMessages.find((m) => m?.role === 'user') ?? null;
 
 const additionalContent = userAdditionalMessage
   ? Array.isArray(userAdditionalMessage.content)
     ? userAdditionalMessage.content
     : [{ type: 'text' as const, text: String(userAdditionalMessage.content) }]
   : [];
 
-// add additional msg such as image or context to message list
 const userMsg: UserMessage = userAdditionalMessage
   ? {
       id: chatService.generateMessageId(),
       role: 'user',
       content: [...additionalContent, { type: 'text' as const, text: messageContent.trim() }],
     }
   : chatService.getUserMessage(messageContent);
Suggestion importance[1-10]: 5

__

Why: Reasonable concern about only checking additionalMessages[0] and possible duplication in messagesToSend, though the impact depends on unseen callers. Moderate improvement in robustness.

Low
Guard partial time range in URL param

The test asserts _g=... appears without a leading & ( _g=${encodeURIComponent(...)}
), but the code prepends &. Given the URL is built as
#/?_v=...&_eq=...${gParam}${cParam}, the leading & here is correct in production,
but the test in auto_visualization_action.test.tsx will fail. Align the test
expectation or verify the concatenation; also ensure _g param is only appended when
both from and to are non-empty to avoid producing _g for partial ranges.

src/plugins/explore/public/components/visualizations/actions/auto_visualization_action.tsx [87-89]

-const gParam = timeRange
-  ? `&_g=${encodeURIComponent(rison.encode({ time: { from: timeRange.from, to: timeRange.to } }))}`
-  : '';
+const gParam =
+  timeRange && timeRange.from && timeRange.to
+    ? `&_g=${encodeURIComponent(rison.encode({ time: { from: timeRange.from, to: timeRange.to } }))}`
+    : '';
Suggestion importance[1-10]: 3

__

Why: The test claim is incorrect (the test expects _g= as substring, so leading & is fine). The partial-range guard is a marginal defensive improvement but not a real bug given getAbsoluteTimeRange already checks bounds.

Low

Previous suggestions

Suggestions up to commit 67b702e
CategorySuggestion                                                                                                                                    Impact
Possible issue
Guard reset against uninitialized singleton

PanelDataService.getInstance() throws if init() was never called (e.g. when
plugins.contextProvider was absent during start). This can crash stop(). Guard the
reset call so shutdown remains safe when the service was not initialized.

src/plugins/explore/public/plugin.ts [858-861]

 this.unregisterPPLExecuteQueryAction?.();
 this.unregisterVisualizationTools?.();
 // cleanup shared panel-data store + fetch_panel_data tool.
-PanelDataService.getInstance().reset();
+try {
+  PanelDataService.getInstance().reset();
+} catch {
+  // PanelDataService was never initialized; nothing to reset.
+}
Suggestion importance[1-10]: 7

__

Why: Correctly identifies that PanelDataService.getInstance() throws when not initialized (when contextProvider is absent), which could crash plugin stop(). Reasonable defensive fix.

Medium
General
Guard against malformed visualization JSON

JSON.parse will throw and abort the whole "Ask AI" flow if
savedExplore.visualization is malformed or not valid JSON. Wrap it in try/catch so
the assistant still receives the screenshot and basic context when parsing fails.

src/plugins/explore/public/actions/ask_ai_embeddable_action.tsx [109-112]

 // Parse visualization config for axes mapping and transformations
-const visualizationConfig = JSON.parse(visEmbeddable.savedExplore.visualization || '{}');
+let visualizationConfig: any = {};
+try {
+  visualizationConfig = JSON.parse(visEmbeddable.savedExplore.visualization || '{}');
+} catch {
+  visualizationConfig = {};
+}
 const axesMapping = visualizationConfig.axesMapping;
 const chartType = visualizationConfig.chartType;
 const dataTransformations = visualizationConfig.dataTransformations;
Suggestion importance[1-10]: 6

__

Why: Valid concern; a malformed visualization JSON would throw and abort the Ask AI flow. Wrapping in try/catch improves robustness.

Low
Refetch preview when inputs change

Disabling the exhaustive-deps rule and using [] means the preview will never refetch
if query, visConfig, or timeRange change (e.g. when the tool result updates).
Include these dependencies (or a stable derived key) so the effect re-runs on
meaningful changes.

src/plugins/explore/public/components/visualizations/actions/auto_visualization_action.tsx [367-384]

 useEffect(() => {
   let cancelled = false;
   const abortController = new AbortController();
   executePPLQuery(query, core, data, timeRange, abortController.signal)
     .then((result) => {
       if (!cancelled) setVisData(result);
     })
     .catch((e) => {
       if (!cancelled && !abortController.signal.aborted) {
         setError(e instanceof Error ? e.message : 'Failed to load chart data');
       }
     });
   return () => {
     cancelled = true;
     abortController.abort();
   };
-  // eslint-disable-next-line react-hooks/exhaustive-deps
-}, []);
+}, [query, timeRange, core, data]);
Suggestion importance[1-10]: 5

__

Why: Valid concern about stale preview if inputs change, though the ChartPreview is likely mounted fresh per tool result. Moderate improvement for correctness.

Low
Include all additional user messages, not first

Using only additionalMessages[0] silently discards any subsequent additional user
messages and can pick up a non-user role at index 0 unpredictably. Iterate over all
user-role entries in additionalMessages to preserve every additional content block,
or explicitly document that only the first is honored.

src/plugins/chat/public/components/chat_window.tsx [443-452]

-const userAdditionalMessage =
-  additionalMessages[0] && additionalMessages[0]?.role === 'user'
-    ? additionalMessages[0]
-    : null;
+const userAdditionalMessages = additionalMessages.filter(
+  (m) => m && m.role === 'user'
+);
 
-const additionalContent = userAdditionalMessage
-  ? Array.isArray(userAdditionalMessage.content)
-    ? userAdditionalMessage.content
-    : [{ type: 'text' as const, text: String(userAdditionalMessage.content) }]
-  : [];
+const additionalContent = userAdditionalMessages.flatMap((m) =>
+  Array.isArray(m.content)
+    ? m.content
+    : [{ type: 'text' as const, text: String(m.content) }]
+);
Suggestion importance[1-10]: 4

__

Why: The suggestion is reasonable but the current code appears intentional (only handles first additional user message). The impact is minor and behavior may be by design.

Low
Suggestions up to commit 98fce72
CategorySuggestion                                                                                                                                    Impact
Possible issue
Guard JSON.parse against malformed data

JSON.parse will throw if savedExplore.visualization contains malformed JSON,
aborting the entire "Ask AI" flow. Wrap the parse in try/catch and fall back to an
empty object so context extraction stays resilient to bad saved-object data.

src/plugins/explore/public/actions/ask_ai_embeddable_action.tsx [109-112]

 // Parse visualization config for axes mapping and transformations
-const visualizationConfig = JSON.parse(visEmbeddable.savedExplore.visualization || '{}');
+let visualizationConfig: any = {};
+try {
+  visualizationConfig = JSON.parse(visEmbeddable.savedExplore.visualization || '{}');
+} catch {
+  visualizationConfig = {};
+}
 const axesMapping = visualizationConfig.axesMapping;
 const chartType = visualizationConfig.chartType;
 const dataTransformations = visualizationConfig.dataTransformations;
Suggestion importance[1-10]: 6

__

Why: Legitimate defensive concern: unhandled JSON.parse on a saved-object field could throw and break the Ask AI flow. Wrapping in try/catch adds resilience.

Low
General
Handle no compatible charts gracefully

When potentialChartType is provided but candidates is empty (no compatible charts at
all), the error message will show an empty list [], which is not actionable for the
LLM. Consider falling back to the table visualization in that case, or improving the
message to indicate no compatible charts exist for the columns.

src/plugins/explore/public/components/visualizations/actions/auto_visualization_action.tsx [157-171]

 if (potentialChartType) {
     const matched = candidates.find(
       (c) => c.chartType.toLowerCase() === potentialChartType.toLowerCase()
     );
     if (matched) {
       return { chartType: matched.chartType, axesMapping: matched.axesMapping };
     }
-    // chart is incompatible with the columns.
-    // the throw reaches the llm.
+    if (candidates.length === 0) {
+      return { chartType: 'table', axesMapping: {} };
+    }
     throw new Error(
       `Chart type "${potentialChartType}" is not compatible with the query result columns. ` +
         `Compatible chart types: [${candidates.map((c) => c.chartType).join(', ')}]. ` +
         `Please adjust ppl query.`
     );
   }
Suggestion importance[1-10]: 5

__

Why: Valid edge-case improvement: when potentialChartType is provided but no candidates exist, the error message shows an empty list which is unhelpful. Falling back to table or improving the message is a reasonable minor enhancement.

Low
Avoid resetting shared singleton prematurely

PanelDataService.reset() is called from ExplorePlugin.stop(), but it operates on a
process-wide singleton. If multiple plugin instances (or a re-mount) share this
singleton, resetting on one stop will clear another plugin's live panel data and
unregister the tool from underneath it. Consider tracking a refcount or scoping the
service to the plugin instance instead of a global singleton.

src/plugins/explore/public/embeddable/panel_data_service.ts [43-46]

 reset() {
+    // Only reset when no panels are still publishing to avoid clobbering
+    // shared state used by other consumers of the singleton.
+    if (this.store.size === 0) {
+      this.unregisterTool();
+      return;
+    }
     this.store.clear();
     this.unregisterTool();
   }
Suggestion importance[1-10]: 5

__

Why: Valid concern about resetting a global singleton from stop() in a multi-instance scenario, though the proposed fix is only partial and the real-world impact may be low.

Low
Find user message anywhere in additionalMessages

Only the first element of additionalMessages is inspected for a user message; if the
caller passes multiple additional messages or a user message at a non-zero index,
its content will be silently dropped. Iterate to find any user message (or merge all
of them) so no context is lost.

src/plugins/chat/public/components/chat_window.tsx [443-446]

 const userAdditionalMessage =
-  additionalMessages[0] && additionalMessages[0]?.role === 'user'
-    ? additionalMessages[0]
-    : null;
+  additionalMessages.find((m) => m?.role === 'user') ?? null;
Suggestion importance[1-10]: 4

__

Why: Reasonable observation, but without knowing the caller contract it's unclear whether user messages can appear at other indices. May be intentional behavior.

Low
Suggestions up to commit 4da9a01
CategorySuggestion                                                                                                                                    Impact
Possible issue
Guard against non-array message content

The code casts userAdditionalMessage.content without handling the case where it is a
string, which would throw a runtime error when spreading. Guard against string
content by wrapping it into a text block, or explicitly check
Array.isArray(userAdditionalMessage.content) before spreading. Also use
setTimelineSynced here to remain consistent with the new synced updater pattern.

src/plugins/chat/public/components/chat_window.tsx [443-460]

 const userAdditionalMessage =
   additionalMessages[0] && additionalMessages[0]?.role === 'user'
     ? additionalMessages[0]
     : null;
 
-// add additional msg such as image or context to message list
+const additionalContent = userAdditionalMessage
+  ? Array.isArray(userAdditionalMessage.content)
+    ? userAdditionalMessage.content
+    : [{ type: 'text' as const, text: String(userAdditionalMessage.content) }]
+  : [];
+
 const userMsg: UserMessage = userAdditionalMessage
   ? {
       id: chatService.generateMessageId(),
       role: 'user',
       content: [
-        ...(userAdditionalMessage.content as Exclude<UserMessage['content'], string>),
+        ...additionalContent,
         { type: 'text' as const, text: messageContent.trim() },
       ],
     }
   : chatService.getUserMessage(messageContent);
 
-setTimeline((prev) => [...prev, userMsg]);
+setTimelineSynced((prev) => [...prev, userMsg]);
Suggestion importance[1-10]: 6

__

Why: Valid concern: UserMessage['content'] could be a string, and spreading it would fail. Also suggests using setTimelineSynced for consistency, which is reasonable given the PR's new pattern.

Low
Handle malformed visualization JSON safely

JSON.parse will throw if visEmbeddable.savedExplore.visualization contains malformed
JSON, breaking the entire "Ask AI" flow. Wrap the parse in a try/catch and fall back
to an empty object so the action still succeeds without the extra context.

src/plugins/explore/public/actions/ask_ai_embeddable_action.tsx [120]

 // Parse visualization config for axes mapping and transformations
-const visualizationConfig = JSON.parse(visEmbeddable.savedExplore.visualization || '{}');
+let visualizationConfig: any = {};
+try {
+  visualizationConfig = JSON.parse(visEmbeddable.savedExplore.visualization || '{}');
+} catch {
+  visualizationConfig = {};
+}
Suggestion importance[1-10]: 5

__

Why: Reasonable defensive programming suggestion, though the visualization field is typically stored as valid JSON. Impact is moderate.

Low
General
Avoid deleting shared panel data prematurely

Multiple embeddable instances may share the same savedExplore.id (e.g., the same
saved visualization used twice in a dashboard). Deleting on destroy of one instance
will remove data still needed by another. Consider reference counting entries, or
only deleting when no other embeddable of the same id remains.

src/plugins/explore/public/embeddable/explore_embeddable.tsx [790-792]

 if (this.savedExplore.id) {
-  panelDataStore.delete(this.savedExplore.id);
+  // Only delete if this embeddable owned the current entry to avoid removing
+  // data still in use by another embeddable sharing the same savedExplore.id.
+  const entry = panelDataStore.get(this.savedExplore.id);
+  if (entry && entry.panelTitle === (this.panelTitle || this.savedExplore.title)) {
+    panelDataStore.delete(this.savedExplore.id);
+  }
 }
Suggestion importance[1-10]: 6

__

Why: Legitimate concern about multiple embeddables sharing the same savedExplore.id. However, the proposed panelTitle-based check is fragile; reference counting would be a better solution.

Low
Guard JSON parsing of page contexts

JSON.parse inside the .find predicate will throw if any page context has a malformed
string value, aborting the entire lookup and breaking editor URL building. Wrap
parsing in try/catch (as is done elsewhere in this PR) so malformed entries are
skipped rather than crashing.

src/plugins/explore/public/components/visualizations/actions/auto_visualization_action.tsx [219-222]

 function getCurrentDashboardId(contextProvider?: ContextProviderStart): string | undefined {
   const pageContexts = contextProvider?.getAssistantContextStore().getContextsByCategory('page');
   const dashboardContext = pageContexts?.find((ctx) => {
-    const value = typeof ctx.value === 'string' ? JSON.parse(ctx.value) : ctx.value;
-    return value?.appId === 'dashboards';
+    try {
+      const value = typeof ctx.value === 'string' ? JSON.parse(ctx.value) : ctx.value;
+      return value?.appId === 'dashboards';
+    } catch {
+      return false;
+    }
   });
Suggestion importance[1-10]: 6

__

Why: Valid concern: unlike similar code in chat_service.ts which wraps JSON.parse in try/catch, this new function does not, and could crash on malformed context values.

Low

@opensearch-project opensearch-project deleted a comment from github-actions Bot Jul 21, 2026
@opensearch-project opensearch-project deleted a comment from github-actions Bot Jul 21, 2026
@github-actions

github-actions Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 8f9fe80.

PathLineSeverityDescription
src/core/public/chat/types.ts13mediumNew `hiddenInConversation` flag on both TextInputContent and BinaryInputContent creates infrastructure to send arbitrary content to the AI backend without it being visible in the chat UI. This transparency gap could be abused in future changes to exfiltrate context (user data, credentials, page state) to the LLM silently. The current use case (visualization metadata) is benign, but the mechanism itself is a foothold for covert data injection.
src/plugins/explore/public/actions/ask_ai_embeddable_action.tsx147mediumA text block containing visualization internals (query string, applied filters, axes mapping, data transformations, saved object ID, index name) is sent to the AI with `hiddenInConversation: true`, meaning users cannot see what metadata about their dashboards is being forwarded to the LLM backend. If the query or filters contain sensitive field values they may be transmitted without the user's awareness.
src/plugins/explore/public/embeddable/panel_data_service.ts78lowPanelDataService registers a global `fetch_panel_data` AI tool that exposes raw hit rows (`_source`, `fields`, or bare objects) from every loaded dashboard panel to the LLM. Any panel whose saved-object ID the AI knows can be read in full. The tool description discourages unnecessary calls, but enforcement relies on LLM compliance rather than an authorization check.

The table above displays the top 10 most important findings.

Total: 3 | Critical: 0 | High: 0 | Medium: 2 | Low: 1


Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.


⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

@Qxisylolo Qxisylolo changed the title [WIP]Add dashboard context-aware and enable auto-vis [Chat]Add dashboard context-aware and enable auto-vis Jul 21, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 98fce72

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 67b702e

Signed-off-by: Qxisylolo <qianxisy@amazon.com>
Signed-off-by: Qxisylolo <qianxisy@amazon.com>
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 8f9fe80

@github-actions

github-actions Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

✅ All unit and integration tests passing

🔗 Workflow run · commit 8f9fe809cddc9f106f80305a8e43bd989f416630

export interface TextInputContent {
type: 'text';
text: string;
hiddenInConversation?: boolean;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this property be correctly persisted in ML's agentic memory?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, only if hiddenInConversation is true

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Qxisylolo Qxisylolo Jul 28, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean giving the content that needs to be hidden a name and then filtering it out in the chat input? The main reason I added hiddenInConversation is to pass visualization context to the chatbot without displaying that context in the chat input.

? {
id: chatService.generateMessageId(),
role: 'user',
content: [...additionalContent, { type: 'text' as const, text: messageContent.trim() }],

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{ type: 'text' as const, text: messageContent.trim() }, shall we use chatService.getUserMessage(messageContent)?

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants