refactor(explore): colocate visualization rules with chart configs and simplify registry

[ruanyl]

Description

Refactors the Explore plugin's visualization system to give each chart type ownership of its own matching rules and render logic, replacing the centralized rule_repository with colocated VisRule definitions inside each chart config file.

Motivation

The previous architecture maintained a large centralized rule_repository.ts (~1,000 lines) that mapped column types to chart types and defined rendering logic separately from the chart configurations. This created tight coupling, made it hard to reason about individual chart types, and required updating multiple files when adding or modifying a visualization.

Key changes

New VisRule interface — each chart config now defines its own rules with priority, mappings (axis-role-to-field-type), and a render function:

interface VisRule<T extends ChartType> {
  priority: number;
  mappings: AxisTypeMapping[];
  render: (props: VisRenderProps<T>) => React.ReactNode;
}

VisualizationRegistry simplified — instead of maintaining separate rule lists, the registry derives available chart types and performs rule matching directly from registered

Issues Resolved

Screenshot

Testing the changes

Changelog

• refactor: colocate visualization rules with chart configs and simplify registry

Check List

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

[github-actions]

Failed to generate code suggestions for PR

[github-actions]

Failed to generate code suggestions for PR

[github-actions]

Failed to generate code suggestions for PR

[github-actions]

Failed to generate code suggestions for PR

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit d7f01b0)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Colocate VisRule definitions inside each chart config file Relevant files: src/plugins/explore/public/components/visualizations/area/area_vis_config.tsx src/plugins/explore/public/components/visualizations/bar/bar_vis_config.tsx src/plugins/explore/public/components/visualizations/line/line_vis_config.tsx src/plugins/explore/public/components/visualizations/metric/metric_vis_config.tsx src/plugins/explore/public/components/visualizations/scatter/scatter_vis_config.tsx src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_config.tsx Sub-PR theme: Simplify VisualizationRegistry and introduce VisualizationRegistryService Relevant files: src/plugins/explore/public/components/visualizations/visualization_registry.ts src/plugins/explore/public/components/visualizations/visualization_registry.test.ts src/plugins/explore/public/services/visualization_registry_service.ts src/plugins/explore/public/services/visualization_registry_service.test.ts Sub-PR theme: Refactor to_expression function signatures to use axisColumnMappings Relevant files: src/plugins/explore/public/components/visualizations/bar/to_expression.ts src/plugins/explore/public/components/visualizations/bar/to_expression.test.ts src/plugins/explore/public/components/visualizations/area/to_expression.ts src/plugins/explore/public/components/visualizations/area/to_expression.test.ts src/plugins/explore/public/components/visualizations/line/to_expression.ts src/plugins/explore/public/components/visualizations/line/to_expression.test.ts src/plugins/explore/public/components/visualizations/utils/utils.ts src/plugins/explore/public/components/visualizations/utils/utils.test.ts

⚡ Recommended focus areas for review Duplicate Column Definitions The numCol, catCol, and dateCol column definitions are duplicated verbatim in both the findRulesByColumns and findRuleByAxesMapping describe blocks. These should be extracted to a shared scope to avoid maintenance issues and potential inconsistencies. const numCol: VisColumn = { id: 1, name: 'value', schema: VisFieldType.Numerical, column: 'value', validValuesCount: 1, uniqueValuesCount: 1, }; const catCol: VisColumn = { id: 2, name: 'category', schema: VisFieldType.Categorical, column: 'category', validValuesCount: 1, uniqueValuesCount: 1, }; const dateCol: VisColumn = { id: 3, name: 'timestamp', schema: VisFieldType.Date, column: 'timestamp', validValuesCount: 1, uniqueValuesCount: 1, }; it('should return exact matches when column counts match rule mappings', () => { const rule = makeRule(100, [ { [AxisRole.X]: { type: VisFieldType.Categorical }, [AxisRole.Y]: { type: VisFieldType.Numerical }, }, ]); registry.registerVisualization(makeVisType('bar', 'Bar', [rule])); const result = registry.findRulesByColumns([numCol], [catCol], []); expect(result.exact).toHaveLength(1); expect(result.exact[0].visType).toBe('bar'); expect(result.exact[0].rules).toContain(rule); }); it('should return compatible matches when columns are a superset', () => { const rule = makeRule(100, [ { [AxisRole.Y]: { type: VisFieldType.Numerical }, }, ]); registry.registerVisualization(makeVisType('metric', 'Metric', [rule])); // Providing extra categorical column — compatible but not exact const result = registry.findRulesByColumns([numCol], [catCol], []); expect(result.all).toHaveLength(1); expect(result.exact).toHaveLength(0); }); it('should filter by chartType when provided', () => { const barRule = makeRule(100, [{ [AxisRole.Y]: { type: VisFieldType.Numerical } }]); const lineRule = makeRule(100, [{ [AxisRole.Y]: { type: VisFieldType.Numerical } }]); registry.registerVisualization(makeVisType('bar', 'Bar', [barRule])); registry.registerVisualization(makeVisType('line', 'Line', [lineRule])); const result = registry.findRulesByColumns([numCol], [], [], 'bar'); expect(result.exact).toHaveLength(1); expect(result.exact[0].visType).toBe('bar'); }); it('should return empty results when no rules match', () => { const rule = makeRule(100, [{ [AxisRole.Y]: { type: VisFieldType.Date } }]); registry.registerVisualization(makeVisType('line', 'Line', [rule])); const result = registry.findRulesByColumns([numCol], [], []); expect(result.all).toHaveLength(0); expect(result.exact).toHaveLength(0); }); }); describe('findBestMatch', () => { const numCol: VisColumn = { id: 1, name: 'value', schema: VisFieldType.Numerical, column: 'value', validValuesCount: 1, uniqueValuesCount: 1, }; it('should return null when no rules match', () => { registry.registerVisualization(makeVisType('line', 'Line', [])); expect(registry.findBestMatch([numCol], [], [])).toBeNull(); }); it('should select the highest priority rule', () => { const lowPriority = makeRule(50, [{ [AxisRole.Y]: { type: VisFieldType.Numerical } }]); const highPriority = makeRule(100, [{ [AxisRole.Y]: { type: VisFieldType.Numerical } }]); registry.registerVisualization(makeVisType('bar', 'Bar', [lowPriority, highPriority])); const result = registry.findBestMatch([numCol], [], []); expect(result?.rule).toBe(highPriority); expect(result?.chartType).toBe('bar'); }); it('should select highest priority across visualization types', () => { const barRule = makeRule(50, [{ [AxisRole.Y]: { type: VisFieldType.Numerical } }]); const lineRule = makeRule(100, [{ [AxisRole.Y]: { type: VisFieldType.Numerical } }]); registry.registerVisualization(makeVisType('bar', 'Bar', [barRule])); registry.registerVisualization(makeVisType('line', 'Line', [lineRule])); const result = registry.findBestMatch([numCol], [], []); expect(result?.rule).toBe(lineRule); expect(result?.chartType).toBe('line'); }); it('should filter by chartType when provided', () => { const barRule = makeRule(50, [{ [AxisRole.Y]: { type: VisFieldType.Numerical } }]); const lineRule = makeRule(100, [{ [AxisRole.Y]: { type: VisFieldType.Numerical } }]); registry.registerVisualization(makeVisType('bar', 'Bar', [barRule])); registry.registerVisualization(makeVisType('line', 'Line', [lineRule])); const result = registry.findBestMatch([numCol], [], [], 'bar'); expect(result?.rule).toBe(barRule); expect(result?.chartType).toBe('bar'); }); }); describe('getAxesMappingByRule', () => { it('should map columns to axis roles based on rule mapping', () => { const rule = makeRule(100, [ { [AxisRole.X]: { type: VisFieldType.Categorical }, [AxisRole.Y]: { type: VisFieldType.Numerical }, }, ]); const numCol: VisColumn = { id: 1, name: 'count', schema: VisFieldType.Numerical, column: 'count', validValuesCount: 1, uniqueValuesCount: 1, }; const catCol: VisColumn = { id: 2, name: 'category', schema: VisFieldType.Categorical, column: 'category', validValuesCount: 1, uniqueValuesCount: 1, }; const result = registry.getAxesMappingByRule(rule, [numCol], [catCol], []); expect(result).toEqual({ [AxisRole.X]: 'category', [AxisRole.Y]: 'count', }); }); it('should return empty object when columns are insufficient', () => { const rule = makeRule(100, [ { [AxisRole.X]: { type: VisFieldType.Date }, [AxisRole.Y]: { type: VisFieldType.Numerical }, }, ]); const result = registry.getAxesMappingByRule(rule, [], [], []); expect(result).toEqual({}); }); }); describe('findRuleByAxesMapping', () => { const numCol: VisColumn = { id: 1, name: 'value', schema: VisFieldType.Numerical, column: 'value', validValuesCount: 1, uniqueValuesCount: 1, }; const catCol: VisColumn = { id: 2, name: 'category', schema: VisFieldType.Categorical, column: 'category', validValuesCount: 1, uniqueValuesCount: 1, }; const dateCol: VisColumn = { id: 3, name: 'timestamp', schema: VisFieldType.Date, column: 'timestamp', validValuesCount: 1, uniqueValuesCount: 1, }; const allColumns = [numCol, catCol, dateCol]; Duplicate Rule Logic Two pairs of rules share identical render implementations but differ only in the COLOR axis field type (Categorical vs Numerical). For example, the rules at priority 80 and 60 for createMultiAreaChart, and similarly for createFacetedMultiAreaChart and createStackedAreaChart. This duplication could be consolidated or documented to clarify the intended distinction. { priority: 80, mappings: [ { [AxisRole.X]: { type: VisFieldType.Date }, [AxisRole.Y]: { type: VisFieldType.Numerical }, [AxisRole.COLOR]: { type: VisFieldType.Categorical }, }, ], render(props) { const spec = createMultiAreaChart( props.transformedData, props.styleOptions, props.axisColumnMappings, props.timeRange ); return <EchartsRender spec={spec} onSelectTimeRange={props.onSelectTimeRange} />; }, }, { priority: 60, mappings: [ { [AxisRole.X]: { type: VisFieldType.Date }, [AxisRole.Y]: { type: VisFieldType.Numerical }, [AxisRole.COLOR]: { type: VisFieldType.Numerical }, }, ], render(props) { const spec = createMultiAreaChart( props.transformedData, props.styleOptions, props.axisColumnMappings, props.timeRange ); return <EchartsRender spec={spec} onSelectTimeRange={props.onSelectTimeRange} />; }, }, { priority: 80, mappings: [ { [AxisRole.X]: { type: VisFieldType.Date }, [AxisRole.Y]: { type: VisFieldType.Numerical }, [AxisRole.COLOR]: { type: VisFieldType.Categorical }, [AxisRole.FACET]: { type: VisFieldType.Categorical }, }, ], render(props) { const spec = createFacetedMultiAreaChart( props.transformedData, props.styleOptions, props.axisColumnMappings, props.timeRange ); return <EchartsRender spec={spec} onSelectTimeRange={props.onSelectTimeRange} />; }, }, { priority: 60, mappings: [ { [AxisRole.X]: { type: VisFieldType.Date }, [AxisRole.Y]: { type: VisFieldType.Numerical }, [AxisRole.COLOR]: { type: VisFieldType.Numerical }, [AxisRole.FACET]: { type: VisFieldType.Categorical }, }, ], render(props) { const spec = createFacetedMultiAreaChart( props.transformedData, props.styleOptions, props.axisColumnMappings, props.timeRange ); return <EchartsRender spec={spec} onSelectTimeRange={props.onSelectTimeRange} />; }, }, { priority: 20, mappings: [ { [AxisRole.X]: { type: VisFieldType.Categorical }, [AxisRole.Y]: { type: VisFieldType.Numerical }, }, ], render(props) { const spec = createCategoryAreaChart( props.transformedData, props.styleOptions, props.axisColumnMappings ); return <EchartsRender spec={spec} onSelectTimeRange={props.onSelectTimeRange} />; }, }, { priority: 60, mappings: [ { [AxisRole.X]: { type: VisFieldType.Categorical }, [AxisRole.Y]: { type: VisFieldType.Numerical }, [AxisRole.COLOR]: { type: VisFieldType.Categorical }, }, ], render(props) { const spec = createStackedAreaChart( props.transformedData, props.styleOptions, props.axisColumnMappings ); return <EchartsRender spec={spec} onSelectTimeRange={props.onSelectTimeRange} />; }, }, { priority: 60, mappings: [ { [AxisRole.X]: { type: VisFieldType.Categorical }, [AxisRole.Y]: { type: VisFieldType.Numerical }, [AxisRole.COLOR]: { type: VisFieldType.Numerical }, }, ], render(props) { const spec = createStackedAreaChart( props.transformedData, props.styleOptions, props.axisColumnMappings ); return <EchartsRender spec={spec} onSelectTimeRange={props.onSelectTimeRange} />; }, }, Side-Effect in Test Module Instantiating new VisualizationRegistryService() at module level (line 12) as a side effect to populate the singleton registry is fragile. If the registry is already populated from a previous test run or if test isolation is needed, this could cause unexpected behavior. Consider using a beforeAll or beforeEach hook instead. new VisualizationRegistryService(); Missing Exact Match Test The findRulesByColumns test for compatible matches (superset case) verifies that result.exact is empty and result.all has one entry, but there is no test verifying that a rule with multiple required axes (e.g., X + Y + COLOR) correctly produces an exact match when all three column types are provided. Edge cases around exact vs. compatible matching logic may be under-tested. it('should return exact matches when column counts match rule mappings', () => { const rule = makeRule(100, [ { [AxisRole.X]: { type: VisFieldType.Categorical }, [AxisRole.Y]: { type: VisFieldType.Numerical }, }, ]); registry.registerVisualization(makeVisType('bar', 'Bar', [rule])); const result = registry.findRulesByColumns([numCol], [catCol], []); expect(result.exact).toHaveLength(1); expect(result.exact[0].visType).toBe('bar'); expect(result.exact[0].rules).toContain(rule); }); it('should return compatible matches when columns are a superset', () => { const rule = makeRule(100, [ { [AxisRole.Y]: { type: VisFieldType.Numerical }, }, ]); registry.registerVisualization(makeVisType('metric', 'Metric', [rule])); // Providing extra categorical column — compatible but not exact const result = registry.findRulesByColumns([numCol], [catCol], []); expect(result.all).toHaveLength(1); expect(result.exact).toHaveLength(0); }); it('should filter by chartType when provided', () => { const barRule = makeRule(100, [{ [AxisRole.Y]: { type: VisFieldType.Numerical } }]); const lineRule = makeRule(100, [{ [AxisRole.Y]: { type: VisFieldType.Numerical } }]); registry.registerVisualization(makeVisType('bar', 'Bar', [barRule])); registry.registerVisualization(makeVisType('line', 'Line', [lineRule])); const result = registry.findRulesByColumns([numCol], [], [], 'bar'); expect(result.exact).toHaveLength(1); expect(result.exact[0].visType).toBe('bar'); }); it('should return empty results when no rules match', () => { const rule = makeRule(100, [{ [AxisRole.Y]: { type: VisFieldType.Date } }]); registry.registerVisualization(makeVisType('line', 'Line', [rule])); const result = registry.findRulesByColumns([numCol], [], []); expect(result.all).toHaveLength(0); expect(result.exact).toHaveLength(0); });

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to d7f01b0

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix type mismatch for timeRange passed to render The timeRange passed to matchedRule.render is searchContext.timeRange, which is of type TimeRange (with from/to as string | ShortDate), but VisRenderProps.timeRange expects { from: string; to: string }. If searchContext.timeRange is undefined or has a different shape, this will silently pass incorrect data. Add a type-safe conversion or guard. src/plugins/explore/public/embeddable/explore_embeddable.tsx [439-447] +const timeRange = searchContext.timeRange + ? { from: String(searchContext.timeRange.from), to: String(searchContext.timeRange.to) } + : undefined; const chartRender = () => matchedRule.render({ transformedData: visualizationData.transformedData, styleOptions: styles || styleOptions, onSelectTimeRange: this.searchProps?.onSelectTimeRange, axisColumnMappings: axesMapping, - timeRange: searchContext.timeRange, + timeRange, }); this.searchProps.chartRender = chartRender; Suggestion importance[1-10]: 6 __ Why: The type mismatch between TimeRange (which may have ShortDate values) and the expected { from: string; to: string } is a real potential issue. The fix correctly adds a type-safe conversion with a guard for undefined.	Low
	Verify correct column identifier used in axis mapping The method stores column.name in the result, but findRuleByAxesMapping looks up columns by col.name to resolve field types. However, getAxesMappingByRule is documented to return an axis-to-column mapping, and the test in visualization_registry.test.ts expects { [AxisRole.X]: 'category', [AxisRole.Y]: 'count' } — which are the name fields. Verify that downstream consumers (e.g., getColumnsByAxesMapping) also use column.name as the key, not column.column, to avoid a mismatch between the stored identifier and the lookup key. src/plugins/explore/public/components/visualizations/visualization_registry.ts [75] -result[axisRole] = column.name; +result[axisRole] = column.column; // use column.column if consumers look up by column identifier Suggestion importance[1-10]: 5 __ Why: The suggestion raises a valid concern about whether column.name or column.column is the correct identifier to store in the axis mapping result. The test expects name values ('category', 'count'), and findRuleByAxesMapping also uses col.name for lookups, so the current code appears consistent. However, the suggestion to verify downstream consumers is worth noting, though the improved_code changes the behavior in a way that may break existing tests.	Low
	Prevent duplicate visualization registrations on multiple instantiations The constructor registers visualizations into the shared singleton visualizationRegistry every time a new VisualizationRegistryService instance is created. This means if the service is instantiated multiple times (e.g., in tests or plugin lifecycle), visualizations will be registered repeatedly, potentially causing duplicates. Consider checking if a visualization is already registered before registering, or guard against multiple registrations. src/plugins/explore/public/services/visualization_registry_service.ts [52-69] constructor() { // TODO: refactor this to not rely on this visualizationRegistry singleton this.registry = visualizationRegistry; - this.registry.registerVisualization([ - createAreaConfig(), - ... - ]); + if (this.registry.getVisualization('area') === undefined) { + this.registry.registerVisualization([ + createAreaConfig(), + createBarConfig(), + createBarGaugeConfig(), + createGaugeConfig(), + createHeatmapConfig(), + createHistogramConfig(), + createLineConfig(), + createMetricConfig(), + createPieConfig(), + createScatterConfig(), + createStateTimelineConfig(), + createTableConfig(), + ]); + } } Suggestion importance[1-10]: 5 __ Why: The concern is valid — registering into a singleton in the constructor can cause duplicates if instantiated multiple times. However, the test file already mocks registerVisualization, and the TODO comment acknowledges this is a known issue. The suggested fix using getVisualization('area') as a guard is fragile and not a proper solution.	Low
	Table visualization missing render rule causes null render The table visualization returns an empty rules array from getRules, which means visualizationRegistry.findRuleByAxesMapping will never find a matching rule for it, and ChartRender will always return null for the table type. The table visualization likely needs at least one rule (possibly a catch-all) to be rendered through the new registry-based flow. src/plugins/explore/public/components/visualizations/table/table_vis_config.ts [57] -getRules: () => [], +getRules: () => [ + { + priority: 0, + mappings: [{}], + render(props) { + // Table rendering is handled separately + return null; + }, + }, +], Suggestion importance[1-10]: 3 __ Why: The getRules: () => [] for the table config means findRuleByAxesMapping will never match, but the table visualization is likely handled separately (e.g., via a dedicated TableVis component path), so this may be intentional. The improved_code suggestion doesn't align well with the existing architecture.	Low
General	Ensure registry state is isolated between tests Instantiating VisualizationRegistryService at module level without assigning it to a variable means it runs once for the entire test file, but the singleton registry state persists across all test suites. If any test spies on or modifies visualizationRegistry (e.g., mockReturnValue(undefined)), it may affect subsequent tests. Consider resetting or restoring the registry state in beforeEach/afterEach to ensure test isolation. src/plugins/explore/public/components/visualizations/visualization_builder.test.ts [11-12] -// Register all built-in visualizations into the singleton registry -new VisualizationRegistryService(); +let registryService: VisualizationRegistryService; +beforeAll(() => { + registryService = new VisualizationRegistryService(); +}); + Suggestion importance[1-10]: 5 __ Why: The concern about singleton registry state persisting across tests is valid, as some tests spy on visualizationRegistry methods with mockReturnValue(undefined). However, those tests already call mockRestore(), and the improved_code uses beforeAll which doesn't fully address the isolation concern mentioned in the suggestion.	Low
	Fix fragile call count assertion in delegation test The test expects mockRegisterVisualization to have been called 2 times after calling setup.registerVisualization(mockVis) once, but the comment says "delegate registerVisualization to the registry". The count of 2 is misleading — it implies the constructor also calls registerVisualization once (for the bulk registration), but this magic number makes the test fragile and unclear. The assertion should be toHaveBeenCalledTimes(1) for the explicit call, or the test should clearly account for the constructor call separately. src/plugins/explore/public/services/visualization_registry_service.test.ts [53-61] it('should delegate registerVisualization to the registry', () => { const setup = service.setup(); const mockVis = createMockVisualization('bar'); + const callsBefore = mockRegisterVisualization.mock.calls.length; setup.registerVisualization(mockVis); - expect(mockRegisterVisualization).toHaveBeenCalledTimes(2); + expect(mockRegisterVisualization).toHaveBeenCalledTimes(callsBefore + 1); expect(mockRegisterVisualization).toHaveBeenCalledWith(mockVis); }); Suggestion importance[1-10]: 5 __ Why: The toHaveBeenCalledTimes(2) assertion is indeed confusing since the constructor also calls registerVisualization once. The suggestion to track calls before and after is a valid improvement for test clarity and robustness.	Low
	Add deterministic tie-breaking in best match selection When iterating over matchedVisRules, the best rule is selected by comparing priorities across all vis types, but bestChartType is only updated when a strictly higher priority is found. If two rules from different vis types share the same highest priority, the first one encountered wins — which is non-deterministic based on Map insertion order. Consider adding a tie-breaking strategy (e.g., prefer the currently active chart type) to make selection deterministic. src/plugins/explore/public/components/visualizations/visualization_registry.ts [161-192] public findBestMatch( numericalColumns: VisColumn[], categoricalColumns: VisColumn[], dateColumns: VisColumn[], chartType?: string ) { const { exact: matchedVisRules } = this.findRulesByColumns( numericalColumns, categoricalColumns, dateColumns, chartType ); let bestRule: VisRule<any> | null = null; let bestChartType: string | null = null; for (const matched of matchedVisRules) { + // Prefer the requested chartType as a tiebreaker + const preferredType = chartType && matched.visType === chartType; for (const r of matched.rules) { - if (!bestRule) { - bestRule = r; - bestChartType = matched.visType; - } else if (r.priority > bestRule?.priority) { + if (!bestRule || r.priority > bestRule.priority || (r.priority === bestRule.priority && preferredType)) { bestRule = r; bestChartType = matched.visType; } } } Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies a non-deterministic tie-breaking issue in findBestMatch when two rules share the same priority across different vis types. However, since chartType is already used to filter via findRulesByColumns, ties across different vis types would only occur when chartType is not specified, making this a minor edge case.	Low
	Remove redundant optional chaining after null check Since data has already been checked for null/undefined above (the if (!data) guard), the optional chaining (?.) on data properties is redundant. More importantly, if data is guaranteed non-null here, using non-optional access makes the intent clearer and avoids masking potential issues where these properties might be unexpectedly undefined. src/plugins/explore/public/components/visualizations/visualization_render.tsx [138-142] const columns = [ - ...(data?.numericalColumns ?? []), - ...(data?.categoricalColumns ?? []), - ...(data?.dateColumns ?? []), + ...(data.numericalColumns ?? []), + ...(data.categoricalColumns ?? []), + ...(data.dateColumns ?? []), ]; Suggestion importance[1-10]: 4 __ Why: After the if (!data) return null guard, data is guaranteed non-null, making the ?. optional chaining on data.numericalColumns, data.categoricalColumns, and data.dateColumns redundant. This is a minor code clarity improvement with no functional impact.	Low
	Avoid side effects at module load time echarts.registerTheme is called at module load time as a side effect. If getColors() depends on runtime context (e.g., theme provider or DOM), calling it at import time may produce incorrect or default colors. Consider deferring theme registration to an explicit initialization function called at the appropriate lifecycle point. src/plugins/explore/public/components/visualizations/theme/default.ts [11] -echarts.registerTheme(DEFAULT_THEME, createEchartsTheme(getColors())); +export function registerDefaultTheme() { + echarts.registerTheme(DEFAULT_THEME, createEchartsTheme(getColors())); +} Suggestion importance[1-10]: 4 __ Why: Calling echarts.registerTheme at module load time is a side effect that could cause issues if getColors() depends on runtime context. However, this pattern existed before the PR (the old code also called echarts.registerTheme at module level), so this is not a regression introduced by this PR.	Low
	Consolidate duplicate render rules with same renderer Two separate rules (priority 60 and 80) both render via createGroupedTimeBarChart but differ only in the COLOR axis type (Categorical vs Numerical). Since findRulesByColumns counts field types independently, a dataset with 1 date + 1 numerical + 1 categorical column could match both rules, and the priority-80 rule (COLOR=Numerical) would never be selected over the priority-60 rule (COLOR=Categorical) for the same column set. Consider consolidating these into a single rule with both mappings, or ensure the priority difference is intentional and documented. src/plugins/explore/public/components/visualizations/bar/bar_vis_config.tsx [164-211] { - priority: 60, + priority: 80, mappings: [ { [AxisRole.X]: { type: VisFieldType.Date }, [AxisRole.Y]: { type: VisFieldType.Numerical }, [AxisRole.COLOR]: { type: VisFieldType.Categorical }, }, - ... - ], - render(props) { - const spec = createGroupedTimeBarChart(...); - ... - }, -}, -{ - priority: 80, - mappings: [ { [AxisRole.X]: { type: VisFieldType.Date }, [AxisRole.Y]: { type: VisFieldType.Numerical }, [AxisRole.COLOR]: { type: VisFieldType.Numerical }, }, - ... + // ... swapped axis variants ], render(props) { - const spec = createGroupedTimeBarChart(...); - ... + const spec = createGroupedTimeBarChart( + props.transformedData, + props.styleOptions, + props.axisColumnMappings, + props.timeRange + ); + return <EchartsRender spec={spec} onSelectTimeRange={props.onSelectTimeRange} />; }, }, Suggestion importance[1-10]: 3 __ Why: The suggestion points out that two rules with different priorities both use createGroupedTimeBarChart, which could cause confusion. However, the different priorities may be intentional to distinguish between categorical and numerical COLOR axes, and consolidating them could change matching behavior. The suggestion is a design consideration rather than a bug fix.	Low
	Document or verify special-cased table chart type handling The table chart type is hardcoded as never disabled (metadata.type !== 'table'), but table may no longer be included in getAvailableChartTypes() since the old code added it separately. If table is now included in the registry, this special-case is still needed but should be documented. If it's not in the registry, the table option will be silently missing from the selector. Verify that table is registered and this special-case is intentional. src/plugins/explore/public/components/visualizations/chart_type_selector.tsx [43-61] -const chartTypeMappedOptions = useMemo(() => { - return visualizationRegistry.getAvailableChartTypes().reduce((acc, metadata) => { - if (!acc[metadata.type]) { - const { all } = visualizationRegistry.findRulesByColumns( - numericalColumns, - categoricalColumns, - dateColumns, - metadata.type - ); - acc[metadata.type] = { - value: metadata.type, - inputDisplay: metadata.name, - iconType: metadata.icon, - disabled: all.length === 0 && metadata.type !== 'table', - }; - } - return acc; - }, {} as Record<string, AvailableChartTypeOption>); -}, [numericalColumns, categoricalColumns, dateColumns]); +acc[metadata.type] = { + value: metadata.type, + inputDisplay: metadata.name, + iconType: metadata.icon, + // 'table' is always enabled as it doesn't require specific column mappings + disabled: all.length === 0 && metadata.type !== 'table', +}; Suggestion importance[1-10]: 3 __ Why: The suggestion only asks to verify/document behavior rather than fix a concrete bug. The improved_code is essentially the same as existing_code with just a comment added, which limits its impact score.	Low
	Verify axis mappings correctness in grouped metric rendering Inside createMultiMetric, createSingleMetric is called with the grouped value data but with the same axisColumnMappings from the outer scope. This is the same behavior as before the refactor, so it's not a regression, but it's worth verifying that axisColumnMappings is correctly scoped for each group's data subset to avoid incorrect metric rendering. src/plugins/explore/public/components/visualizations/metric/to_expression.ts [87] +const result = createSingleMetric(value, styles, axisColumnMappings); - Suggestion importance[1-10]: 2 __ Why: The suggestion asks to verify existing behavior rather than proposing a concrete fix, and the existing_code and improved_code are identical, making this a low-value observation with no actionable change.	Low

---

Previous suggestions

Suggestions up to commit 43aae6c

Suggestions up to commit 9b6e244

Category	Suggestion	Impact
Possible issue	Use consistent column identifier in axes mapping The method stores column.name in the result, but findRuleByAxesMapping looks up columns by col.name when building axisTypeMapping. However, the test for getAxesMappingByRule expects the result to contain column (e.g. 'category', 'count'), which is the column field, not name. If name and column differ, the round-trip through findRuleByAxesMapping will fail to find the column. Use column.column (the field identifier) consistently, or ensure the lookup in findRuleByAxesMapping also uses col.column. src/plugins/explore/public/components/visualizations/visualization_registry.ts [75] -result[axisRole] = column.name; +result[axisRole] = column.column; Suggestion importance[1-10]: 7 __ Why: The test for getAxesMappingByRule expects the result to contain column.column values (e.g., 'category', 'count'), but the code stores column.name. If name and column differ, the round-trip through findRuleByAxesMapping will fail. This is a real inconsistency that could cause bugs.	Medium
	Restore stale column validation before updating visualization The previous code validated that all selected columns still exist in the available columns before proceeding, preventing stale axis mappings from being written back. This validation was removed in the refactor, which means stale or invalid column selections could now trigger updateVisualization with columns that no longer exist in the dataset. The column existence check should be restored. src/plugins/explore/public/components/visualizations/style_panel/axes/axes_selector.tsx [157-178] const normalizedAxesSelections: AxisColumnMappings = {}; +const allColumns = [...numericalColumns, ...categoricalColumns, ...dateColumns]; + +const isValid = Object.values(currentSelections) + .filter(Boolean) + .every((columnName) => allColumns.some((col) => col?.name === columnName?.name)); + +if (!isValid) return; + Object.entries(currentSelections).forEach(([key, value]) => { if (value) { normalizedAxesSelections[key as AxisRole] = value; } }); const ruleToUse = visualizationRegistry.findRuleByAxesMapping( chartType, convertMappingsToStrings(normalizedAxesSelections), - [...numericalColumns, ...categoricalColumns, ...dateColumns] + allColumns ); if (ruleToUse) { updateVisualization({ mappings: normalizedAxesSelections }); } Suggestion importance[1-10]: 7 __ Why: The PR removed the validation that checked whether selected columns still exist in the available dataset columns before calling updateVisualization. This could allow stale or invalid column selections to trigger visualization updates, potentially causing errors or incorrect renders.	Medium
	Prevent duplicate visualization registrations on re-instantiation The constructor registers visualizations into the shared singleton visualizationRegistry every time a new VisualizationRegistryService instance is created. This can cause duplicate registrations if the service is instantiated more than once (e.g., in tests or plugin lifecycle). Consider checking if a visualization is already registered before registering it, or clearing the registry before re-registering. src/plugins/explore/public/services/visualization_registry_service.ts [52-69] constructor() { // TODO: refactor this to not rely on this visualizationRegistry singleton this.registry = visualizationRegistry; - this.registry.registerVisualization([ + const configs = [ createAreaConfig(), - ... - ]); + createBarConfig(), + createBarGaugeConfig(), + createGaugeConfig(), + createHeatmapConfig(), + createHistogramConfig(), + createLineConfig(), + createMetricConfig(), + createPieConfig(), + createScatterConfig(), + createStateTimelineConfig(), + createTableConfig(), + ]; + configs.forEach((config) => { + if (!this.registry.getVisualization(config.type)) { + this.registry.registerVisualization(config); + } + }); } Suggestion importance[1-10]: 6 __ Why: The constructor registers all visualizations into a shared singleton every time a new service instance is created, which can cause duplicate registrations in tests or if the service is instantiated multiple times. The suggestion to guard against duplicates is valid, though the impact depends on how registerVisualization handles duplicates internally.	Low
	Table visualization has no rendering rules defined The table visualization returns an empty rules array from getRules, which means visualizationRegistry.findRuleByAxesMapping will never find a matching rule for it, causing the table chart to never render via the new registry-based rendering path in visualization_render.tsx. The table visualization likely needs at least one rule defined, or the rendering logic needs a special case for table. src/plugins/explore/public/components/visualizations/table/table_vis_config.ts [57] -getRules: () => [], +getRules: () => { + // Table visualization handles its own rendering outside the standard rule-based path + return []; +}, Suggestion importance[1-10]: 6 __ Why: The table visualization returns an empty getRules array, which means findRuleByAxesMapping will return null and the table will never render via the new registry-based path. However, the improved code only adds a comment without actually fixing the issue, so the suggestion identifies a real concern but doesn't provide a proper solution.	Low
	Fix column lookup to match by both name and column field The lookup col.name === field assumes axes mapping values are column names, but getAxesMappingByRule stores column.name while the test data uses column.column as the field identifier. This inconsistency means findRuleByAxesMapping will return undefined when the axes mapping was built using column identifiers. The lookup should match against both col.name and col.column to be robust, or consistently use one identifier throughout. src/plugins/explore/public/components/visualizations/visualization_registry.ts [101-104] for (const [role, field] of Object.entries(axesMapping)) { - const found = allColumns.find((col) => col.name === field); + const found = allColumns.find((col) => col.name === field || col.column === field); if (!found) return undefined; axisTypeMapping[role as AxisRole] = { type: found.schema }; } Suggestion importance[1-10]: 5 __ Why: The lookup inconsistency between col.name and col.column is a valid concern, but the improved code adds a fallback rather than fixing the root cause. This is a defensive workaround that partially addresses the issue raised in suggestion 1.	Low
	Guard against undefined transformed data The ChartRender component contains hooks-like logic (calls to convertStringsToMappings, getAxisConfigByColumnMapping) after early returns, but since this is a functional component, early returns before these calls are fine. However, data.transformedData could be undefined if the data hasn't been transformed yet, which would silently pass undefined to rule.render. Add a guard for data.transformedData before calling rule.render. src/plugins/explore/public/components/visualizations/visualization_render.tsx [152-164] const standardAxes = 'standardAxes' in config.styles ? config.styles.standardAxes : []; const axisColumnMappings = convertStringsToMappings(config?.axesMapping ?? {}, columns); // initialize axis config const allAxisConfig = getAxisConfigByColumnMapping(axisColumnMappings, standardAxes); const styles = { ...config.styles, standardAxes: allAxisConfig }; + +if (!data.transformedData) { + return null; +} return rule.render({ transformedData: data.transformedData, styleOptions: styles, axisColumnMappings, timeRange, onSelectTimeRange, }); Suggestion importance[1-10]: 5 __ Why: data.transformedData could potentially be undefined, and passing it to rule.render without a guard could cause runtime errors. This is a valid defensive check, though the likelihood depends on how VisData is typed and populated.	Low
General	Fix misleading call count assertion in test The test expects mockRegisterVisualization to be called 2 times after calling setup.registerVisualization(mockVis) once, but the comment says it should "delegate registerVisualization to the registry." The count of 2 seems to account for the constructor call, but this makes the assertion fragile and misleading. The test should either assert toHaveBeenCalledTimes(1) for the explicit call (resetting the mock after construction) or clearly document why 2 is expected. src/plugins/explore/public/services/visualization_registry_service.test.ts [53-61] it('should delegate registerVisualization to the registry', () => { const setup = service.setup(); const mockVis = createMockVisualization('bar'); + mockRegisterVisualization.mockClear(); // reset after constructor call setup.registerVisualization(mockVis); - expect(mockRegisterVisualization).toHaveBeenCalledTimes(2); + expect(mockRegisterVisualization).toHaveBeenCalledTimes(1); expect(mockRegisterVisualization).toHaveBeenCalledWith(mockVis); }); Suggestion importance[1-10]: 5 __ Why: The test asserts toHaveBeenCalledTimes(2) which is fragile and misleading since it implicitly depends on the constructor call count. Clearing the mock before the explicit call and asserting toHaveBeenCalledTimes(1) would make the test more robust and readable.	Low
	Avoid shared mutable state leaking between tests Instantiating VisualizationRegistryService at module level without assigning it means the service is created once for the entire test file, but there is no cleanup between tests. If VisualizationRegistryService registers visualizations into a shared singleton visualizationRegistry, tests that mock getVisualization (e.g. mockReturnValue(undefined)) may interfere with other tests. Consider storing the instance and calling a cleanup/reset method in afterEach, or ensure the singleton registry is properly reset between tests. src/plugins/explore/public/components/visualizations/visualization_builder.test.ts [11-12] -// Register all built-in visualizations into the singleton registry -new VisualizationRegistryService(); +let registryService: VisualizationRegistryService; +beforeAll(() => { + // Register all built-in visualizations into the singleton registry + registryService = new VisualizationRegistryService(); +}); + Suggestion importance[1-10]: 4 __ Why: The concern about test isolation is valid since VisualizationRegistryService registers into a shared singleton. However, the improved code only moves instantiation to beforeAll, which doesn't actually solve the cleanup problem between tests that mock getVisualization.	Low
	Consolidate duplicate rules with same render function Two separate rules (priority 60 and 80) both call createGroupedTimeBarChart but differ only in the COLOR axis type (Categorical vs Numerical). Since findRulesByColumns counts field types independently, a mapping with COLOR: Numerical at priority 80 will shadow the priority 60 rule for the same column combination. Consider consolidating these into a single rule with both mappings, or verify the priority difference is intentional and documented. src/plugins/explore/public/components/visualizations/bar/bar_vis_config.tsx [164-211] { - priority: 60, + priority: 80, mappings: [ { [AxisRole.X]: { type: VisFieldType.Date }, [AxisRole.Y]: { type: VisFieldType.Numerical }, [AxisRole.COLOR]: { type: VisFieldType.Categorical }, }, - ... - ], - render(props) { - const spec = createGroupedTimeBarChart(...); - ... - }, -}, -{ - priority: 80, - mappings: [ + { + [AxisRole.X]: { type: VisFieldType.Numerical }, + [AxisRole.Y]: { type: VisFieldType.Date }, + [AxisRole.COLOR]: { type: VisFieldType.Categorical }, + }, { [AxisRole.X]: { type: VisFieldType.Date }, [AxisRole.Y]: { type: VisFieldType.Numerical }, [AxisRole.COLOR]: { type: VisFieldType.Numerical }, }, - ... + { + [AxisRole.X]: { type: VisFieldType.Numerical }, + [AxisRole.Y]: { type: VisFieldType.Date }, + [AxisRole.COLOR]: { type: VisFieldType.Numerical }, + }, ], render(props) { - const spec = createGroupedTimeBarChart(...); - ... + const spec = createGroupedTimeBarChart( + props.transformedData, + props.styleOptions, + props.axisColumnMappings, + props.timeRange + ); + return <EchartsRender spec={spec} onSelectTimeRange={props.onSelectTimeRange} />; }, }, Suggestion importance[1-10]: 3 __ Why: The suggestion to consolidate rules is a valid code quality improvement, but the priority difference between the two rules (60 vs 80) may be intentional to distinguish between categorical and numerical color axes. The improved code changes behavior by merging them at priority 80, which may not be desired.	Low
	Remove hardcoded special-casing for table chart type The table chart type is hardcoded as never disabled (metadata.type !== 'table'), but the table type should now be included in getAvailableChartTypes() via the registry. If table is registered with its own rules, this special-casing may be unnecessary and could mask issues where the table config is missing. Verify that table is properly registered and remove the hardcoded exception if so. src/plugins/explore/public/components/visualizations/chart_type_selector.tsx [43-61] const chartTypeMappedOptions = useMemo(() => { return visualizationRegistry.getAvailableChartTypes().reduce((acc, metadata) => { if (!acc[metadata.type]) { const { all } = visualizationRegistry.findRulesByColumns( numericalColumns, categoricalColumns, dateColumns, metadata.type ); acc[metadata.type] = { value: metadata.type, inputDisplay: metadata.name, iconType: metadata.icon, - disabled: all.length === 0 && metadata.type !== 'table', + disabled: all.length === 0, }; } return acc; }, {} as Record<string, AvailableChartTypeOption>); }, [numericalColumns, categoricalColumns, dateColumns]); Suggestion importance[1-10]: 3 __ Why: The hardcoded metadata.type !== 'table' exception is a minor concern that may be intentional to ensure table is always available regardless of column types. This is a design choice rather than a bug, and the suggestion to remove it could break expected behavior if table has no matching rules for certain data.	Low
	Remove redundant optional chaining after null check Since data has already been checked for null/undefined above (the if (!data) guard), the optional chaining (?.) on data properties is redundant. More importantly, if data is defined but its column arrays are undefined, this could silently produce an empty columns array and cause findRuleByAxesMapping to fail to find a rule. Consider asserting or defaulting these arrays more explicitly. src/plugins/explore/public/components/visualizations/visualization_render.tsx [138-142] const columns = [ - ...(data?.numericalColumns ?? []), - ...(data?.categoricalColumns ?? []), - ...(data?.dateColumns ?? []), + ...(data.numericalColumns ?? []), + ...(data.categoricalColumns ?? []), + ...(data.dateColumns ?? []), ]; Suggestion importance[1-10]: 3 __ Why: After the if (!data) guard, optional chaining on data?.numericalColumns etc. is redundant. While the suggestion is technically correct, this is a minor style issue with low impact since the behavior is identical (both produce [] when the property is undefined).	Low

Suggestions up to commit c4e399f

Category	Suggestion	Impact
Possible issue	Use field identifier instead of display name in axis mapping The getAxesMappingByRule method stores column.name in the result, but findRuleByAxesMapping looks up columns by col.name to build the axisTypeMapping. However, the registry's getAxesMappingByRule is used to produce a mapping that is later consumed by chart rendering, which may expect column.column (the actual field/column identifier) rather than column.name (the display name). If name and column can differ, this could cause mismatches when the mapping is used to look up columns. Verify whether column.column should be stored instead of column.name. src/plugins/explore/public/components/visualizations/visualization_registry.ts [75] -result[axisRole] = column.name; +result[axisRole] = column.column; Suggestion importance[1-10]: 6 __ Why: The suggestion raises a valid concern about whether column.name (display name) or column.column (field identifier) should be stored in the axis mapping result. However, the test in visualization_registry.test.ts expects column.name values (e.g., 'category', 'count'), and findRuleByAxesMapping also looks up by col.name, suggesting the current design is intentional and consistent within this PR.	Low
	Prevent duplicate visualization registrations on multiple instantiations The constructor registers visualizations into the shared singleton visualizationRegistry every time a new VisualizationRegistryService instance is created. This means if the service is instantiated multiple times (e.g., in tests or plugin lifecycle), visualizations will be registered repeatedly, potentially causing duplicates. Consider checking if a visualization is already registered before registering, or guard against multiple registrations. src/plugins/explore/public/services/visualization_registry_service.ts [52-69] constructor() { // TODO: refactor this to not rely on this visualizationRegistry singleton this.registry = visualizationRegistry; - this.registry.registerVisualization([ - createAreaConfig(), - ... - ]); + if (this.registry.getVisualization('line') === undefined) { + this.registry.registerVisualization([ + createAreaConfig(), + createBarConfig(), + createBarGaugeConfig(), + createGaugeConfig(), + createHeatmapConfig(), + createHistogramConfig(), + createLineConfig(), + createMetricConfig(), + createPieConfig(), + createScatterConfig(), + createStateTimelineConfig(), + createTableConfig(), + ]); + } } Suggestion importance[1-10]: 6 __ Why: The constructor registers all visualizations into a shared singleton every time a new service instance is created, which could cause duplicate registrations in tests or if the service is instantiated multiple times. The suggested guard is a reasonable mitigation, though the TODO comment already acknowledges this is a known issue.	Low
	Fix column lookup to handle both name and column identifier The findRuleByAxesMapping method converts axesMapping values (field names) to AxisTypeMapping by looking up columns using col.name. However, if getAxesMappingByRule stores column.name but the actual axes mapping stored in state uses column.column, the lookup allColumns.find((col) => col.name === field) will fail and return undefined, causing the method to return early. Ensure the lookup key matches what is stored in the axes mapping (either both use name or both use column). src/plugins/explore/public/components/visualizations/visualization_registry.ts [107-120] -const found = rules.find((rule) => { - return rule.mappings.some((mapping) => { - const mappingKeys = Object.keys(mapping) as AxisRole[]; - const inputKeys = Object.keys(axisTypeMapping) as AxisRole[]; +const found = allColumns.find((col) => col.column === field || col.name === field); - if (mappingKeys.length !== inputKeys.length) return false; - - return inputKeys.every((key) => { - const mappingEntry = mapping[key]; - const inputEntry = axisTypeMapping[key]; - return mappingEntry && inputEntry && mappingEntry.type === inputEntry.type; - }); - }); -}); - Suggestion importance[1-10]: 4 __ Why: The suggestion points out a potential mismatch between how axes mappings are stored and how columns are looked up. However, within this PR the design appears consistent — getAxesMappingByRule stores column.name and findRuleByAxesMapping looks up by col.name. The improved code changes only the lookup line but doesn't address the broader architectural concern described.	Low
General	Fix misleading call count assertions in service tests The test expects mockRegisterVisualization to have been called 2 times after a single setup.registerVisualization(mockVis) call, but the comment and intent suggest it should be called once for the explicit call. The count of 2 likely accounts for the constructor's bulk registration call, but this makes the assertion fragile and misleading. The assertion should use toHaveBeenCalledTimes(1) for the explicit call, or the test should be restructured to reset the mock after construction. src/plugins/explore/public/services/visualization_registry_service.test.ts [53-61] it('should delegate registerVisualization to the registry', () => { const setup = service.setup(); const mockVis = createMockVisualization('bar'); + mockRegisterVisualization.mockClear(); setup.registerVisualization(mockVis); - expect(mockRegisterVisualization).toHaveBeenCalledTimes(2); + expect(mockRegisterVisualization).toHaveBeenCalledTimes(1); expect(mockRegisterVisualization).toHaveBeenCalledWith(mockVis); }); Suggestion importance[1-10]: 5 __ Why: The test asserts toHaveBeenCalledTimes(2) after a single explicit registerVisualization call, which is confusing because the extra call comes from the constructor. Clearing the mock before the explicit call would make the assertion clearer and more robust.	Low
	Remove redundant deduplication in chart types retrieval Since this.visualizations is a Map<string, VisualizationType> keyed by vis.type, each entry already has a unique type. The deduplication check availableChartTypes.every((t) => t.type !== vis.type) is redundant and adds O(n²) complexity. The map guarantees uniqueness, so you can simplify this to a direct push. src/plugins/explore/public/components/visualizations/visualization_registry.ts [34-46] getAvailableChartTypes() { const availableChartTypes: ChartMetadata[] = []; for (const [, vis] of this.visualizations) { - if (availableChartTypes.every((t) => t.type !== vis.type)) { - availableChartTypes.push({ - type: vis.type, - name: vis.name, - icon: vis.icon ?? '', - }); - } + availableChartTypes.push({ + type: vis.type, + name: vis.name, + icon: vis.icon ?? '', + }); } return availableChartTypes; } Suggestion importance[1-10]: 4 __ Why: The deduplication check is indeed redundant since this.visualizations is a Map keyed by vis.type, guaranteeing uniqueness. Removing it simplifies the code and improves performance, though the impact is minor for typical use cases.	Low
	Move singleton initialization into test lifecycle hook Instantiating VisualizationRegistryService at module level without assigning it to a variable means it runs once for the entire test file, but the side effects (registering visualizations into the singleton) persist across all tests. If other test files also instantiate this service or if tests run in a shared environment, this could cause double-registration or ordering issues. Consider calling this inside a beforeAll block and ensuring cleanup if needed. src/plugins/explore/public/components/visualizations/visualization_builder.test.ts [11-12] -// Register all built-in visualizations into the singleton registry -new VisualizationRegistryService(); +beforeAll(() => { + new VisualizationRegistryService(); +}); Suggestion importance[1-10]: 4 __ Why: Moving the VisualizationRegistryService instantiation into a beforeAll block is a better practice for test isolation and lifecycle management. However, since the registry uses a singleton pattern and tests likely rely on it being populated, the practical impact depends on the test runner's module isolation behavior.	Low
	Remove hardcoded special-casing for table chart type The table chart type is now hardcoded as never disabled (metadata.type !== 'table'), but table should also be returned by getAvailableChartTypes() since createTableConfig() is registered. This special-casing may cause inconsistency if the table config defines its own rules. Consider removing the special case and letting the registry determine availability uniformly, or ensure table always has a matching rule. src/plugins/explore/public/components/visualizations/chart_type_selector.tsx [43-61] const chartTypeMappedOptions = useMemo(() => { return visualizationRegistry.getAvailableChartTypes().reduce((acc, metadata) => { if (!acc[metadata.type]) { const { all } = visualizationRegistry.findRulesByColumns( numericalColumns, categoricalColumns, dateColumns, metadata.type ); acc[metadata.type] = { value: metadata.type, inputDisplay: metadata.name, iconType: metadata.icon, - disabled: all.length === 0 && metadata.type !== 'table', + disabled: all.length === 0, }; } return acc; }, {} as Record<string, AvailableChartTypeOption>); }, [numericalColumns, categoricalColumns, dateColumns]); Suggestion importance[1-10]: 4 __ Why: The table type is hardcoded as never disabled, which is inconsistent with how other chart types are handled. If table is registered with its own rules, the registry should determine its availability uniformly.	Low
	Remove redundant optional chaining after null check Since data has already been checked for null/undefined above (the if (!data) guard), the optional chaining (?.) on data properties is redundant. More importantly, if data is guaranteed non-null here, using non-optional access makes the intent clearer and avoids potential confusion about whether data could still be undefined at this point. src/plugins/explore/public/components/visualizations/visualization_render.tsx [138-142] const columns = [ - ...(data?.numericalColumns ?? []), - ...(data?.categoricalColumns ?? []), - ...(data?.dateColumns ?? []), + ...(data.numericalColumns ?? []), + ...(data.categoricalColumns ?? []), + ...(data.dateColumns ?? []), ]; Suggestion importance[1-10]: 4 __ Why: After the if (!data) guard, data is guaranteed non-null, so the optional chaining (?.) on data.numericalColumns, data.categoricalColumns, and data.dateColumns is redundant. This is a minor code clarity improvement with no functional impact.	Low
	Guard against undefined transformed data data.transformedData could be undefined if the data object doesn't have this property populated, which would silently pass undefined to rule.render. Consider adding a guard or fallback (e.g., data.transformedData ?? []) to prevent downstream rendering errors. src/plugins/explore/public/components/visualizations/visualization_render.tsx [152-164] const standardAxes = 'standardAxes' in config.styles ? config.styles.standardAxes : []; const axisColumnMappings = convertStringsToMappings(config?.axesMapping ?? {}, columns); // initialize axis config const allAxisConfig = getAxisConfigByColumnMapping(axisColumnMappings, standardAxes); const styles = { ...config.styles, standardAxes: allAxisConfig }; return rule.render({ - transformedData: data.transformedData, + transformedData: data.transformedData ?? [], styleOptions: styles, axisColumnMappings, timeRange, onSelectTimeRange, }); Suggestion importance[1-10]: 4 __ Why: Adding a fallback ?? [] for data.transformedData is a defensive measure, but data is already checked for null and transformedData is likely always present in the VisData type. This is a minor robustness improvement with low practical impact.	Low
	Resolve TypeScript error instead of suppressing it A @ts-ignore comment was added to suppress a TypeScript error in the filter range object. This suppresses potentially important type safety checks and may hide a real type mismatch. The underlying type issue should be resolved properly rather than suppressed. src/plugins/explore/public/embeddable/explore_embeddable.tsx [285-288] -// @ts-ignore range: { '*': { mode: 'absolute', Suggestion importance[1-10]: 3 __ Why: Using @ts-ignore suppresses type safety checks and may hide a real type mismatch. The underlying TypeScript error should be resolved properly, but without more context on the type issue, the impact is limited.	Low
	Document intentional empty rules for table config Returning an empty array from getRules means the table visualization will never match any rule and will never render via the registry. If the table chart type has special rendering logic handled elsewhere, this should be documented; otherwise, at least one rule should be defined to handle the table rendering path. src/plugins/explore/public/components/visualizations/table/table_vis_config.ts [57] -getRules: () => [], +getRules: () => { + // Table rendering is handled separately via TableVis component + // and does not use the standard rule-based rendering pipeline + return []; +}, Suggestion importance[1-10]: 1 __ Why: The suggestion only adds a comment to explain the empty getRules array, which is a documentation-only change. The improved_code is functionally equivalent to the existing_code and doesn't address any bug or logic issue.	Low

Suggestions up to commit bda9319

Category	Suggestion	Impact
Possible issue	Fix axis mapping to use column field identifier The method uses column.name to populate the result mapping, but findRuleByAxesMapping later looks up columns by col.name to resolve field types. However, getAxesMappingByRule is documented to return axis-to-column mappings, and the test in visualization_registry.test.ts expects the result to contain column values (e.g., 'category', 'count'), which correspond to col.column not col.name. Using column.name instead of column.column may cause mismatches when the column's display name differs from its field identifier. src/plugins/explore/public/components/visualizations/visualization_registry.ts [48-83] getAxesMappingByRule( rule: VisRule<any>, numericalColumns: VisColumn[], categoricalColumns: VisColumn[], dateColumns: VisColumn[] ) { for (const mapping of rule.mappings) { const result: Record<string, string> = {}; const numCols = [...numericalColumns]; const categoricalCols = [...categoricalColumns]; const dateCols = [...dateColumns]; for (const [axisRole, { type }] of Object.entries(mapping)) { let column: VisColumn | undefined; if (type === VisFieldType.Categorical) { column = categoricalCols.shift(); } if (type === VisFieldType.Numerical) { column = numCols.shift(); } if (type === VisFieldType.Date) { column = dateCols.shift(); } // No available column fits the mapping, cannot create axis mapping for the given columns if (!column) { break; } - result[axisRole] = column.name; + result[axisRole] = column.column; } // Only return if we successfully mapped ALL axes if (Object.keys(result).length === Object.keys(mapping).length) { return result; } } return {}; } Suggestion importance[1-10]: 7 __ Why: The test in visualization_registry.test.ts expects getAxesMappingByRule to return values like 'category' and 'count', which match col.column not col.name. Using column.name instead of column.column could cause mismatches when the display name differs from the field identifier, making this a potentially impactful bug.

[github-actions]

Persistent review updated to latest commit bda9319

[github-actions]

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

[github-actions]

Persistent review updated to latest commit c4e399f

[github-actions]

Persistent review updated to latest commit 43aae6c

[github-actions]

Persistent review updated to latest commit 9b6e244

[github-actions]

Persistent review updated to latest commit d7f01b0

[wanglam]

Nit: Since we will skip the not matched visualizations here. What about just call this.visualizations.get(chartType) here?

[ruanyl]

right, that's a good call, I will tweak in next PR

[wanglam]

Nit: Do we need to throw error when vis type already exists?

[ruanyl]

Also thought about this, will come back to when necessary

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 0.00%. Comparing base (7782a80) to head (d7f01b0).
⚠️ Report is 42 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main   #11589       +/-   ##
===========================================
- Coverage   61.58%        0   -61.59%     
===========================================
  Files        4995        0     -4995     
  Lines      137542        0   -137542     
  Branches    23901        0    -23901     
===========================================
- Hits        84707        0    -84707     
+ Misses      46692        0    -46692     
+ Partials     6143        0     -6143     

Flag	Coverage Δ
Linux_1	?
Linux_2	?
Linux_3	?
Linux_4	?
Linux_5	?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.