Skip to content

fix unnecessary re-render which causes performance issue#12028

Open
ruanyl wants to merge 1 commit into
opensearch-project:mainfrom
ruanyl:fix-performance-isse
Open

fix unnecessary re-render which causes performance issue#12028
ruanyl wants to merge 1 commit into
opensearch-project:mainfrom
ruanyl:fix-performance-isse

Conversation

@ruanyl
Copy link
Copy Markdown
Member

@ruanyl ruanyl commented May 21, 2026

Description

Fixes chart re-rendering oscillation when switching chart types (e.g., line → bar) for queries with multiple numerical columns in the same axis type (e.g., | stats AVG(bytes) as avg_bytes, MAX(bytes) as max_bytes by span(timestamp, 1h)). The chart would flash continuously because the axes mapping alternated between two states (y/color fields swapping).

Root cause: Two issues combined after #11975 moved AxesSelectPanel to persist across chart type changes:

  1. Format mismatch in setAxesMapping: getAxesMappingByRule returns string values ("max_bytes") while convertMappingsToStrings returns arrays (["max_bytes"]). isEqual("str", ["str"]) is false, causing spurious visConfig$ re-emissions.
  2. Stale effect trigger in AxesSelectPanel: The useEffect that syncs selections to the builder had chartType and remainingMappings in its dependency array, causing it to fire on chart type changes with a stale currentSelections whose key insertion order differed — feeding columns to getAxesMappingByRule in a different order and producing a swapped mapping.

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

Signed-off-by: Yulong Ruan <ruanyl@amazon.com>
@github-actions
Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 No relevant tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review

Stale Ref

chartTypeRef.current is updated on every render but the useEffect at line 127 does not include chartTypeRef in its dependency array. If chartType changes and the effect runs due to other dependencies changing, chartTypeRef.current will reflect the new chartType even though chartType itself did not trigger the effect. This can cause findRuleByAxesMapping to be called with a chartType that does not match the currentSelections state, potentially producing incorrect rule matches.

const chartTypeRef = useRef(chartType);
chartTypeRef.current = chartType;

@github-actions
Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Handle empty arrays in normalization

The normalization logic doesn't handle empty arrays consistently. An empty array []
is truthy and will be included in the normalized result, but the comparison in
setAxesMapping may not behave as expected. Consider explicitly checking for
non-empty arrays.

src/plugins/explore/public/components/visualizations/visualization_builder.ts [397-408]

 private normalizeMapping(
   mapping: AxisFieldNameMappings | undefined
 ): Record<string, string[]> | undefined {
   if (!mapping) return undefined;
   const normalized: Record<string, string[]> = {};
   for (const [key, value] of Object.entries(mapping)) {
-    if (value) {
+    if (value && (Array.isArray(value) ? value.length > 0 : true)) {
       normalized[key] = Array.isArray(value) ? value : [value];
     }
   }
   return normalized;
 }
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies that empty arrays are truthy and will be included in normalized results. However, whether this is a bug depends on the intended behavior, which isn't clear from the PR context. The fix is reasonable but may not be necessary.

Low
Update ref in useEffect hook

The ref is being updated on every render, which defeats the purpose of using a ref.
If chartType changes frequently, this approach won't prevent re-renders. Consider
adding chartType back to the dependency array or use a different memoization
strategy.

src/plugins/explore/public/components/visualizations/style_panel/axes/axes_selector.tsx [71-72]

 const chartTypeRef = useRef(chartType);
-chartTypeRef.current = chartType;
+useEffect(() => {
+  chartTypeRef.current = chartType;
+}, [chartType]);
Suggestion importance[1-10]: 3

__

Why: While wrapping the ref update in useEffect is technically more idiomatic, updating chartTypeRef.current directly on every render is a valid pattern and doesn't cause issues. The suggestion provides marginal improvement in code style.

Low

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.

1 participant