Skip to content

Add hint for hidden AE data sources in legacy viz data source selectors - #12470

Open
yubonluo wants to merge 2 commits into
opensearch-project:mainfrom
yubonluo:add-prompt-for-AE
Open

Add hint for hidden AE data sources in legacy viz data source selectors#12470
yubonluo wants to merge 2 commits into
opensearch-project:mainfrom
yubonluo:add-prompt-for-AE

Conversation

@yubonluo

Copy link
Copy Markdown
Collaborator

Description

Legacy visualization types (Visualize/aggregation-based, TSVB, Controls, and VisBuilder) only support DSL queries. Index patterns and data sources backed by Optimized engine (AnalyticEngine type) data sources are therefore filtered out of their data source / index pattern selectors. Today this filtering is silent — users with only Optimized engine sources see an empty or shortened list with no explanation, which looks like a bug.

This PR surfaces a short, contextual note wherever such sources are hidden, telling the user why they don't appear. The note is shown only when Optimized engine sources actually exist and were hidden, so users without any such sources see no change.

Issues Resolved

Screenshot

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

@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit 8154e72)

Here are some key observations to aid the review process:

🧪 No relevant tests
🔒 No security concerns identified
✅ No TODO sections
🔀 Multiple PR themes

Sub-PR theme: Add Optimized engine hint to input_control_vis editors

Relevant files:

  • src/plugins/input_control_vis/public/components/editor/index_pattern_select_form_row.tsx
  • src/plugins/input_control_vis/public/components/editor/list_control_editor.tsx
  • src/plugins/input_control_vis/public/components/editor/range_control_editor.tsx

Sub-PR theme: Add Optimized engine footer note to VisBuilder data source selector

Relevant files:

  • src/plugins/vis_builder/public/application/components/data_source_select.tsx
  • src/plugins/vis_builder/public/application/components/searchable_dropdown.tsx
  • src/plugins/vis_builder/public/application/utils/use/use_index_pattern.tsx

Sub-PR theme: Add Optimized engine callout to visualization wizard and TSVB

Relevant files:

  • src/plugins/visualizations/public/wizard/search_selection/search_selection.tsx
  • src/plugins/vis_type_timeseries/public/application/components/index_pattern.js

⚡ Recommended focus areas for review

State update during render

The memoized dataSourceFilter calls setHasAnalyticEngine(true) from within the filter callback. If DataSourceSelector invokes this filter synchronously during its render phase (as filter predicates commonly are), it will trigger a setState during another component's render, causing a React warning ("Cannot update a component while rendering a different component") and potential re-render loops. Consider deferring the state update via useEffect or computing the flag outside the render path.

const dataSourceFilter = useMemo(() => {
  const baseFilter = createDataSourceFilterForAnalyticEngine();
  return (dataSource) => {
    const allowed = baseFilter(dataSource);
    if (!allowed) {
      setHasAnalyticEngine(true);
    }
    return allowed;
  };
}, []);
setState during render

setError(...) is being called directly in the function body of the hook (outside useEffect) when the condition is met. This triggers a state update during render, which React will warn about and can cause extra renders. This existed before but the condition was modified in this PR; consider moving the error-setting logic into an effect.

let foundSelected: IndexPattern | undefined;
if (!loading && !error && indexId) {
  foundSelected = indexPatterns.filter((p) => p.id === indexId)[0];
  // If the selected index pattern was filtered out (e.g., it's an AnalyticEngine data source),
  // don't treat it as an error - it will be handled by selecting the first available pattern.
  // When the list is empty *because* the only sources are Optimized engine (AnalyticEngine),
  // skip the generic error so the dropdown can show a tailored empty-state message instead.
  if (foundSelected === undefined && indexPatterns.length === 0 && !hasAnalyticEngine) {
    setError(new Error('No index patterns available'));
  }
}
Incorrect hasAnalyticEngine check

setHasAnalyticEngine(patterns.length > indexPatternIds.size) compares the total saved-object patterns count to the deduplicated ID set size. If indexPatternList contains any duplicates or if patterns and indexPatternList are derived from different sources with unrelated counts, this comparison may yield false positives/negatives. Using indexPatternList.length (or comparing the filtered array length) would be more robust.

setIndexPatterns(patterns.filter((i) => indexPatternIds.has(i.id)));
setHasAnalyticEngine(patterns.length > indexPatternIds.size);

@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to 8154e72

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Avoid state updates during render phase

Calling setHasAnalyticEngine(true) inside a filter function called during render can
trigger a state update during rendering, potentially causing React warnings ("Cannot
update a component while rendering a different component") or unnecessary
re-renders. Defer the state update via a microtask/effect, or track the flag with a
ref and sync via useEffect.

src/plugins/vis_type_timeseries/public/application/components/index_pattern.js [143-153]

 const [hasAnalyticEngine, setHasAnalyticEngine] = useState(false);
 const dataSourceFilter = useMemo(() => {
   const baseFilter = createDataSourceFilterForAnalyticEngine();
   return (dataSource) => {
     const allowed = baseFilter(dataSource);
     if (!allowed) {
-      setHasAnalyticEngine(true);
+      Promise.resolve().then(() => setHasAnalyticEngine(true));
     }
     return allowed;
   };
 }, []);
Suggestion importance[1-10]: 7

__

Why: Valid concern - the filter may be invoked during render of the DataSourceSelector, and calling setHasAnalyticEngine synchronously could produce React warnings. Deferring via microtask mitigates this.

Medium
Do not set state during render

Calling setError during render will cause a re-render loop or React warnings about
setting state during rendering. This state update should be moved into the useEffect
where indexPatterns are loaded, not executed on every render pass.

src/plugins/vis_builder/public/application/utils/use/use_index_pattern.tsx [22-31]

 let foundSelected: IndexPattern | undefined;
 if (!loading && !error && indexId) {
   foundSelected = indexPatterns.filter((p) => p.id === indexId)[0];
-  // If the selected index pattern was filtered out (e.g., it's an AnalyticEngine data source),
-  // don't treat it as an error - it will be handled by selecting the first available pattern.
-  // When the list is empty *because* the only sources are Optimized engine (AnalyticEngine),
-  // skip the generic error so the dropdown can show a tailored empty-state message instead.
-  if (foundSelected === undefined && indexPatterns.length === 0 && !hasAnalyticEngine) {
-    setError(new Error('No index patterns available'));
-  }
 }
Suggestion importance[1-10]: 6

__

Why: The setError call during render is a pre-existing pattern (not introduced in this PR), but it's a valid observation. However, the proposed fix removes the error handling entirely without moving it to useEffect, making it incomplete.

Low
General
Guard setState against unmounted component

getIndexPatternSelect is called after componentDidMount, but the class guards other
async completions using this.isMounted. Without that guard here, setState may be
invoked after unmount, causing memory leaks/warnings. Add an if (!this.isMounted)
return; check before setState.

src/plugins/input_control_vis/public/components/editor/list_control_editor.tsx [129-139]

 const allIndexPatterns = await data.indexPatterns.getCache();
 const indexPatternList = await data.indexPatterns.getCache({
   excludeEngineTypes: UNSUPPORTED_ENGINE_TYPES,
 });
 const allowedIndexPatternIds = new Set(indexPatternList?.map((i) => i.id) || []);
 
+if (!this.isMounted) return;
 this.setState({
   IndexPatternSelect: data.ui.IndexPatternSelect,
   allowedIndexPatternIds,
   hasAnalyticEngine: (allIndexPatterns?.length ?? 0) > (indexPatternList?.length ?? 0),
 });
Suggestion importance[1-10]: 5

__

Why: Reasonable defensive suggestion to prevent setState after unmount, consistent with the existing pattern in the class using this.isMounted. Impact is minor as this is only invoked once at mount.

Low

Previous suggestions

Suggestions up to commit 8154e72
CategorySuggestion                                                                                                                                    Impact
Possible issue
Avoid setState-during-render in filter callback

Calling setHasAnalyticEngine(true) inside a filter callback triggered by the child
DataSourceSelector during its render can cause a "setState during render" warning or
infinite re-renders, since the parent re-renders and passes a new state that may
re-invoke the filter. Defer the state update via a microtask/setTimeout, or guard it
to only fire when the value actually changes.

src/plugins/vis_type_timeseries/public/application/components/index_pattern.js [143-153]

 const [hasAnalyticEngine, setHasAnalyticEngine] = useState(false);
 const dataSourceFilter = useMemo(() => {
   const baseFilter = createDataSourceFilterForAnalyticEngine();
   return (dataSource) => {
     const allowed = baseFilter(dataSource);
     if (!allowed) {
-      setHasAnalyticEngine(true);
+      setHasAnalyticEngine((prev) => (prev ? prev : true));
     }
     return allowed;
   };
 }, []);
Suggestion importance[1-10]: 8

__

Why: Calling setHasAnalyticEngine(true) inside a filter invoked during a child render can cause React warnings or re-render loops; the suggestion correctly identifies a real potential issue, though the proposed fix only partially mitigates it.

Medium
Prevent setState loop during render

Calling setError unconditionally during render will trigger a re-render loop because
the error state is set on every render where the condition holds. Guard the call so
it only runs when the error is not already set, or move this side effect into a
useEffect.

src/plugins/vis_builder/public/application/utils/use/use_index_pattern.tsx [22-31]

 let foundSelected: IndexPattern | undefined;
 if (!loading && !error && indexId) {
   foundSelected = indexPatterns.filter((p) => p.id === indexId)[0];
   // If the selected index pattern was filtered out (e.g., it's an AnalyticEngine data source),
   // don't treat it as an error - it will be handled by selecting the first available pattern.
   // When the list is empty *because* the only sources are Optimized engine (AnalyticEngine),
   // skip the generic error so the dropdown can show a tailored empty-state message instead.
-  if (foundSelected === undefined && indexPatterns.length === 0 && !hasAnalyticEngine) {
+  if (foundSelected === undefined && indexPatterns.length === 0 && !hasAnalyticEngine && !error) {
     setError(new Error('No index patterns available'));
   }
 }
Suggestion importance[1-10]: 5

__

Why: The existing guard !error already prevents the loop, so the additional check is redundant; however, moving the side effect to useEffect would be a legitimate improvement.

Low
General
Guard setState against unmounted component

setState is called after two awaits without checking whether the component is still
mounted. Since componentWillUnmount toggles isMounted, guard the state update to
avoid the "setState on unmounted component" warning and potential memory leaks.

src/plugins/input_control_vis/public/components/editor/list_control_editor.tsx [129-139]

 const allIndexPatterns = await data.indexPatterns.getCache();
 const indexPatternList = await data.indexPatterns.getCache({
   excludeEngineTypes: UNSUPPORTED_ENGINE_TYPES,
 });
 const allowedIndexPatternIds = new Set(indexPatternList?.map((i) => i.id) || []);
+
+if (!this.isMounted) {
+  return;
+}
 
 this.setState({
   IndexPatternSelect: data.ui.IndexPatternSelect,
   allowedIndexPatternIds,
   hasAnalyticEngine: (allIndexPatterns?.length ?? 0) > (indexPatternList?.length ?? 0),
 });
Suggestion importance[1-10]: 5

__

Why: Valid concern about setState on unmounted component, but the impact is limited to a warning; also isMounted is a private field so the pattern is already partially handled elsewhere.

Low
Suggestions up to commit 53b2c3c
CategorySuggestion                                                                                                                                    Impact
Possible issue
Avoid setState during render in filter

Calling setHasAnalyticEngine(true) inside the filter function invoked during render
by DataSourceSelector can cause a "setState during render" warning and potentially
infinite re-renders. Schedule the state update asynchronously (e.g., via setTimeout
or queueMicrotask) and guard against redundant updates, or compute this via an
effect that inspects the data source list separately.

src/plugins/vis_type_timeseries/public/application/components/index_pattern.js [143-153]

 const [hasAnalyticEngine, setHasAnalyticEngine] = useState(false);
 const dataSourceFilter = useMemo(() => {
   const baseFilter = createDataSourceFilterForAnalyticEngine();
   return (dataSource) => {
     const allowed = baseFilter(dataSource);
     if (!allowed) {
-      setHasAnalyticEngine(true);
+      setTimeout(() => setHasAnalyticEngine((prev) => (prev ? prev : true)), 0);
     }
     return allowed;
   };
 }, []);
Suggestion importance[1-10]: 8

__

Why: Calling setHasAnalyticEngine inside a filter function called during render can cause React warnings and potential infinite re-render loops. This is a valid concern, though the suggested setTimeout fix is a workaround rather than an ideal solution.

Medium
Avoid setState during render body

Calling setError during render body (outside useEffect) causes an immediate
re-render and can lead to render-phase state updates and warnings. Move this
error-setting logic into an effect or into the async handleUpdate where the state is
already being computed.

src/plugins/vis_builder/public/application/utils/use/use_index_pattern.tsx [22-31]

 let foundSelected: IndexPattern | undefined;
 if (!loading && !error && indexId) {
   foundSelected = indexPatterns.filter((p) => p.id === indexId)[0];
-  // If the selected index pattern was filtered out (e.g., it's an AnalyticEngine data source),
-  // don't treat it as an error - it will be handled by selecting the first available pattern.
-  // When the list is empty *because* the only sources are Optimized engine (AnalyticEngine),
-  // skip the generic error so the dropdown can show a tailored empty-state message instead.
-  if (foundSelected === undefined && indexPatterns.length === 0 && !hasAnalyticEngine) {
+}
+useEffect(() => {
+  if (!loading && !error && indexId && !foundSelected && indexPatterns.length === 0 && !hasAnalyticEngine) {
     setError(new Error('No index patterns available'));
   }
-}
+}, [loading, error, indexId, foundSelected, indexPatterns.length, hasAnalyticEngine]);
Suggestion importance[1-10]: 6

__

Why: Correctly identifies that setError is called in the render body, which can trigger React warnings. However, this pattern existed before the PR and isn't newly introduced by these changes.

Low
General
Guard setState after async in mount

getIndexPatternSelect is invoked in componentDidMount but the setState call does not
check this.isMounted, which can trigger the "setState on unmounted component"
warning if the component unmounts before the async cache calls resolve. Guard the
setState call with the existing isMounted flag.

src/plugins/input_control_vis/public/components/editor/list_control_editor.tsx [129-141]

 const [allIndexPatterns, indexPatternList] = await Promise.all([
   data.indexPatterns.getCache(),
   data.indexPatterns.getCache({
     excludeEngineTypes: UNSUPPORTED_ENGINE_TYPES,
   }),
 ]);
 const allowedIndexPatternIds = new Set(indexPatternList?.map((i) => i.id) || []);
 
+if (!this.isMounted) return;
 this.setState({
   IndexPatternSelect: data.ui.IndexPatternSelect,
   allowedIndexPatternIds,
   hasAnalyticEngine: (allIndexPatterns?.length ?? 0) > (indexPatternList?.length ?? 0),
 });
Suggestion importance[1-10]: 4

__

Why: Valid defensive programming concern about setState on unmounted component, but this pre-existing pattern isn't newly introduced. Also, this.isMounted is defined but only set to true in componentDidMount; the suggestion doesn't fully address the lifecycle setup.

Low

@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

✅ All unit and integration tests passing

🔗 Workflow run · commit 8154e72e7b290fe9e5a1b0267d05606f0a1947fc

ruanyl
ruanyl previously approved these changes Jul 28, 2026
Comment thread src/plugins/visualizations/public/wizard/search_selection/search_selection.tsx Outdated
wanglam
wanglam previously approved these changes Jul 28, 2026
Signed-off-by: yubo luo <yubonluo@amazon.com>
@yubonluo
yubonluo dismissed stale reviews from wanglam and ruanyl via 8154e72 July 28, 2026 07:17
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 8154e72

1 similar comment
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 8154e72

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.

3 participants