[Explore Vis]add value mapping for remaining chart type #10923
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
opensearch-changeset-bot bot
added a commit
to Qxisylolo/OpenSearch-Dashboards
that referenced
this pull request
Nov 17, 2025
Qxisylolo
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #10923 +/- ##
========================================
Coverage 60.79% 60.80%
========================================
Files 4532 4535 +3
Lines 122275 122579 +304
Branches 20501 20637 +136
========================================
+ Hits 74337 74533 +196
- Misses 42697 42760 +63
- Partials 5241 5286 +45
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Qxisylolo
changed the title
WalkthroughThis PR introduces value mapping functionality for pie chart visualizations and extends it across bar, heatmap, histogram, scatter, and state_timeline charts. It replaces boolean threshold color flags with a Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Qxisylolo <[email protected]>
Signed-off-by: Qxisylolo <[email protected]>
Signed-off-by: Qxisylolo <[email protected]>
Signed-off-by: Qxisylolo <[email protected]>
Signed-off-by: Qxisylolo <[email protected]>
Signed-off-by: Qxisylolo <[email protected]>
Qxisylolo
force-pushed
the
feat/valuemapping_pie
branch
from
eabe907 to
0a87590
Compare
Signed-off-by: Qxisylolo <[email protected]>
Signed-off-by: Qxisylolo <[email protected]>
Qxisylolo
force-pushed
the
feat/valuemapping_pie
branch
from
5b3e606 to
ba1184a
Compare
Signed-off-by: Qxisylolo <[email protected]>
Qxisylolo
changed the title
Signed-off-by: Qxisylolo <[email protected]>
Qxisylolo
requested review from
AMoo-Miki,
abbyhu2000,
ananzh,
ashwin-pc,
joshuarrrr,
kavilla,
manasvinibs,
zengyan-amazon and
zhongnansu
as code owners
Qxisylolo
requested review from
BionIT,
Flyingliuhub,
LDrago27,
Maosaic,
SuZhou-Joe,
TackAdam,
ZilongX,
angle943,
bandinib-amzn,
curq,
d-buckner,
huyaboo,
joshuali925,
mengweieric,
paulstn,
ps48,
raintygao,
ruanyl,
ruchidh,
sejli,
sumukhswamy,
virajsanghvi,
wanglam,
xinruiba and
zhyuanqi
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/plugins/explore/public/components/visualizations/table/table_vis.tsx (1)
283-294: Add optional chaining for nested dependency access.Line 293 accesses
styleOptions?.valueMappingOptions.valueMappingswithout optional chaining onvalueMappingOptions. IfvalueMappingOptionsis undefined, this could cause issues in the dependency array.Apply this diff:
- styleOptions?.valueMappingOptions.valueMappings, + styleOptions?.valueMappingOptions?.valueMappings,src/plugins/explore/public/components/visualizations/scatter/scatter_exclusive_vis_options.test.tsx (1)
122-134: Fix incorrect comment about switch count.Line 132 comment mentions "useThresholdcolor" which no longer exists as a separate switch. The ColorModeOptionSelect uses a combobox (select dropdown), not a switch.
Apply this diff:
// Check for specific controls expect(screen.getAllByRole('combobox')).toHaveLength(2); // Shape selector - expect(screen.getAllByRole('switch')).toHaveLength(1); // Filled switch and useThresholdcolor + expect(screen.getAllByRole('switch')).toHaveLength(1); // Filled switch expect(screen.getByRole('spinbutton')).toBeInTheDocument(); // Angle input
🧹 Nitpick comments (24)
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/07/rule_matching_vis_histogram.spec.js (1)
63-65: Add explicit wait after color mode selection for consistency.The color mode selection immediately proceeds to canvas comparison (line 67) without an explicit wait, while other style changes in this test include
cy.wait(1000)(lines 75, 84). This inconsistency could lead to test flakiness if the threshold color rendering is asynchronous.Apply this diff to add a wait for consistency:
cy.getElementByTestId('colorModeOptionsSelection') .should('be.visible') .select('Use Threshold Color'); +cy.wait(1000); // compare with new canvas cy.get('canvas.marks').then((canvas) => {src/plugins/explore/public/components/visualizations/heatmap/heatmap_chart_utils.test.ts (1)
331-345: Consider enhancing test coverage for value mapping.The test verifies basic value mapping behavior but could be more comprehensive:
- The test passes an empty object
{}for styles but tests the value mapping path - this works but doesn't test interaction withcolorModeOption.- Consider adding assertions for the
scaleproperty returned bydecideScale()andlegend.labelExprfromgenerateLabelExpr().- The
rangeMappingsparameter (6th argument) is not passed, sodecideScalereceivesundefinedfor that parameter.Consider adding a more complete test that exercises the full value mapping path:
it('able to handle value mapping', () => { const markLayer = JSON.parse(JSON.stringify(baseMarkLayer)); + const styles = { + ...defaultHeatmapChartStyles, + colorModeOption: 'useValueMapping', + } as HeatmapChartStyle; enhanceStyle( markLayer, - ({} as unknown) as HeatmapChartStyle, + styles, transformedData, colorField, true, - [{ type: 'value', value: '10' }] + [{ type: 'value', value: '10', color: '#ff0000' }], + [{ type: 'range', range: { min: 0, max: 100 }, color: '#00ff00' }] ); expect(markLayer.encoding.color.field).toBe('mappingValue'); expect(markLayer.encoding.color.type).toBe('nominal'); expect(markLayer.encoding.color.scale.type).toBeUndefined(); + // Verify legend label expression is set + expect(markLayer.encoding.color.legend.labelExpr).toBeDefined(); });src/plugins/explore/public/components/visualizations/utils/utils.ts (1)
322-353: Remove unused generic type parameter.The generic type
<T>is declared but never used in the function. This appears to be copied from the similargetThresholdByValue<T>function above, which also doesn't use its generic. Consider removing it for clarity.-export function getValueMappingByValue<T>( +export function getValueMappingByValue( value: any, valueMappings: ValueMapping[] = [] ): ValueMapping | undefined {Also worth noting: the existing
getThresholdByValue<T>at line 300 has the same unused generic pattern - this could be cleaned up in a follow-up.src/plugins/explore/public/components/visualizations/state_timeline/to_expression.test.ts (1)
197-269: Consider splitting this test into separate test cases.This test case validates two distinct scenarios within a single
itblock:
- Lines 204-245: Value mapping with
colorModeOption: 'useValueMapping'- Lines 247-268: No value mapping with
colorModeOption: 'none'Splitting these improves test isolation and makes failures easier to diagnose.
- it('should able to handle value mappings in numerical state timeline', () => { + it('should handle value mappings when colorModeOption is useValueMapping', () => { const mockAxisColumnMappings: AxisColumnMappings = { [AxisRole.COLOR]: mockNumericalColumns[0], [AxisRole.Y]: mockCateColumns[0], [AxisRole.X]: mockTimeColumns[0], }; const result = createNumericalStateTimeline( // ... first test case only ); // ... assertions for useValueMapping + }); + + it('should use original field when colorModeOption is none', () => { + const mockAxisColumnMappings: AxisColumnMappings = { + [AxisRole.COLOR]: mockNumericalColumns[0], + [AxisRole.Y]: mockCateColumns[0], + [AxisRole.X]: mockTimeColumns[0], + }; - const result2 = createNumericalStateTimeline( + const result = createNumericalStateTimeline( // ... second test case ); - const markLayer2 = result2.layer[0]; + const markLayer = result.layer[0]; // ... assertions for none mode });src/plugins/explore/public/components/visualizations/histogram/to_expression.ts (1)
66-66: Consider handling additional color modes.The current implementation only checks for
colorModeOption === 'useThresholdColor', but theColorModeOptiontype includes'useValueMapping'and'highlightValueMapping'modes. If histogram visualizations should support value mapping (as indicated by the PR objectives), the color encoding logic may need to be extended.Verify whether histogram charts should support value mapping:
#!/bin/bash # Description: Check if histogram config includes valueMappingOptions and if other visualizations handle all color modes # Check histogram config for valueMappingOptions rg -n --type=ts 'valueMappingOptions' src/plugins/explore/public/components/visualizations/histogram/histogram_vis_config.ts # Check how bar visualization handles different color modes as reference rg -n -A10 --type=ts "colorModeOption.*useValueMapping" src/plugins/explore/public/components/visualizations/bar/to_expression.tsAlso applies to: 150-150
src/plugins/explore/public/components/visualizations/types.ts (1)
344-348: LGTM! Consider adding JSDoc documentation.The
ColorModeOptiontype is well-defined with clear, descriptive values. To improve maintainability, consider adding JSDoc comments explaining when each mode should be used:+/** + * Color mode options for visualizations + * - 'none': No special coloring applied + * - 'useThresholdColor': Apply colors based on threshold values + * - 'useValueMapping': Apply colors based on value mappings + * - 'highlightValueMapping': Highlight data based on value mappings + */ export type ColorModeOption = | 'useValueMapping' | 'highlightValueMapping' | 'none' | 'useThresholdColor';src/plugins/explore/public/components/visualizations/pie/pie_vis_options.test.tsx (1)
270-278: LGTM! Consider adding a more comprehensive test.The test correctly verifies that value mapping updates trigger
onStyleChangewith the expected structure. For better coverage, consider adding a test that verifies adding actual value mappings with specific values (not just an empty array).src/plugins/explore/public/components/visualizations/style_panel/value_mapping/value_mapping_utils.test.ts (1)
530-537: Test data clarity: misleading displayText.The value mapping at line 534 has
value: '40'butdisplayText: 'Thirty', which is confusing. While this doesn't affect test correctness, consider using consistent naming for clarity.{ type: 'value', value: '40', - displayText: 'Thirty', + displayText: 'Forty', color: '#00ff00', },src/plugins/explore/public/components/visualizations/bar/bar_chart_utils.test.ts (1)
133-141: Consider more complete mock data for ValueMapping objects.The test mocks for
valueMappingsandvalidRangesomit several optional fields (likeid,type,displayText). While the tests work, using more complete mock data would better reflect real-world usage and catch potential issues with missing fields.Example for lines 135 and 155:
- const valueMappings = [{ value: 'test', color: '#ff0000' }]; + const valueMappings = [{ + id: '1', + type: 'value' as const, + value: 'test', + displayText: 'Test', + color: '#ff0000' + }];Also applies to: 153-168
src/plugins/explore/public/components/visualizations/pie/to_expression.ts (1)
49-51: Simplify the canUseValueMapping condition.The boolean condition could be more readable by extracting it or adding a comment explaining when value mapping is enabled.
Consider this refactor for clarity:
+ // Value mapping is active when valid mappings exist and not explicitly disabled const canUseValueMapping = ((validRanges && validRanges.length > 0) || (validValues && validValues.length > 0)) && styleOptions.colorModeOption !== 'none';src/plugins/explore/public/components/visualizations/scatter/scatter_exclusive_vis_options.test.tsx (1)
55-73: Consider using data-test-subj for more robust tests.The tests rely on array indices to select elements (e.g.,
getAllByRole('combobox')[1]), which makes them brittle when the UI structure changes. The component already usesdata-test-subjattributes (e.g., "pointFilledSwitch" on line 47).Consider using data-test-subj selectors for the shape selector:
// In the component (scatter_exclusive_vis_options.tsx), add: <EuiSelect compressed data-test-subj="scatterShapeSelect" // ... rest of props /> // In the test: const shapeSelector = screen.getByTestId('scatterShapeSelect');This would make tests more resilient to UI reordering.
src/plugins/explore/public/components/visualizations/bar/bar_vis_options.tsx (1)
130-132: Consider removing the deprecatedonUseThresholdColorChangehandler.The
onUseThresholdColorChangecallback is passed without its correspondinguseThresholdColorvalue prop. SincecolorModeOptionis now the preferred approach (as indicated by the deprecation comments inbar_vis_config.ts), this handler appears to be dead code.- onUseThresholdColorChange={(useThresholdColor) => - updateStyleOption('useThresholdColor', useThresholdColor) - }src/plugins/explore/public/components/visualizations/pie/pie_exclusive_vis_options.tsx (1)
7-7: Unused import:EuiSelect.
EuiSelectis imported but not used in this component. The color mode selection is handled byColorModeOptionSelect.-import { EuiFormRow, EuiButtonGroup, EuiSwitch, EuiSelect } from '@elastic/eui'; +import { EuiFormRow, EuiButtonGroup, EuiSwitch } from '@elastic/eui';src/plugins/explore/public/components/visualizations/heatmap/heatmap_exclusive_vis_options.test.tsx (2)
51-51: Test selector relies on DOM order.Using
getAllByRole('switch')[0]is fragile as it depends on render order. If the component's structure changes, this test could break silently or target the wrong element.Consider adding
data-test-subjto the reverse schema switch in the component for more reliable selection:- const reverse = screen.getAllByRole('switch')[0]; + const reverse = screen.getByTestId('reverseColorSchemaSwitch');
61-63: Fragile selector using array index.Similar to the switch selector, using
getAllByRole('combobox')[1]couples the test to DOM order. If adata-test-subjattribute exists on the color schema select, prefer usinggetByTestId.src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_utils.test.ts (2)
300-341: mergeNumericalDataCore “missing fields” / “filter ranges” tests mostly solid, but consider checking all returnsThe new tests for:
- returning original data when
timestampField/rangeFieldare missing, and- filtering range mappings based on actual data values,
look correct and line up with
mergeNumericalDataCore’s return shape. To make them slightly stronger you might also destructure and assert the third tuple element (validValues) is[]in the “missing fields” case.
376-423: Clarify mapping type in “handle both value mappings and range mappings”In this test,
valueMappingsis defined withtype: 'range'but uses thevaluefield ({ type: 'range', value: '15', ... }). TodaymergeNumericalDataCoreonly looks atvalue, so the test passes, but semantically this object is a value mapping and would normally usetype: 'value'(consistent with how other callers splitvalueMappingsvsrangeMappingsbased ontype).Switching this test mapping to
type: 'value'would better reflect real usage without changing behavior.- const valueMappings: ValueMapping[] = [{ type: 'range', value: '15', displayText: 'low' }]; + const valueMappings: ValueMapping[] = [{ type: 'value', value: '15', displayText: 'low' }];src/plugins/explore/public/components/visualizations/histogram/histogram_vis_config.ts (1)
20-22: Histogram style config is aligned with the new color‑mode/value‑mapping modelAdding
valueMappingOptionsandcolorModeOptiontoHistogramChartStyleOptions, plus:
- deprecating
useThresholdColor, and- redefining
HistogramChartStyleto treatcolorModeOptionseparately,brings histograms in line with the shared ColorModeOption/value‑mapping API. The default styles correctly seed
valueMappingOptions.valueMappingsas an empty array andcolorModeOption: 'none'. You might optionally remove the commented‑out oldHistogramChartStylealias to avoid confusion, but functionally this looks good.Also applies to: 48-54, 56-62, 129-133
src/plugins/explore/public/components/visualizations/state_timeline/to_expression.ts (3)
155-163: Empty object in tooltip array may cause rendering issues.When
colorModeOption === 'none', this creates an empty object{}in the tooltip array. Depending on the Vega-Lite implementation, this could cause unexpected behavior or be silently ignored.Consider filtering out the empty case entirely:
- { - ...(styleOptions?.colorModeOption === 'none' - ? { - field: rangeField, - type: 'nominal', - title: rangeFieldColumn.name, - } - : {}), - }, + ...(styleOptions?.colorModeOption === 'none' + ? [{ + field: rangeField, + type: 'nominal', + title: rangeFieldColumn.name, + }] + : []),
191-191: Inconsistent null check operators in Vega filter expressions.Line 191 uses
!= null(loose equality) while lines 369, 553, and 761 use!== null(strict equality). For consistency and to avoid potential bugs with Vega expression evaluation:
- Line 191:
{ filter: 'datum.displayText != null' }- Lines 369, 553, 761:
{ filter: 'datum.displayText !== null' }Standardize on one approach across all filter expressions.
Also applies to: 369-369, 553-553, 761-761
405-590: Consider extracting shared logic to reduce duplication.The new
createSingleCategoricalStateTimelineandcreateSingleNumericalStateTimelinefunctions share significant structural similarity withcreateCategoricalStateTimelineandcreateNumericalStateTimeline. Key differences are:
- Use of
fakeYAxisfield for single-row display- Absence of a Y-axis grouping field
Consider extracting common patterns (bar layer encoding, text layer, transform generation) into shared helper functions to improve maintainability.
Also applies to: 592-796
src/plugins/explore/public/components/visualizations/bar/bar_chart_utils.ts (2)
93-94: Typo in parameter name:disbaleAggregationTypeThe parameter name has a typo - should be
disableAggregationType.- aggregationType: AggregationType | undefined, - disbaleAggregationType: boolean = false + aggregationType: AggregationType | undefined, + disableAggregationType: boolean = false
198-223: Consider explicitly excludinguseThresholdColormode.The function returns early for
colorModeOption === 'none', butuseThresholdColormode should be handled bybuildThresholdColorEncodinginstead. To prevent accidental misuse and make intent explicit:if ( styleOptions?.colorModeOption === 'none' || + styleOptions?.colorModeOption === 'useThresholdColor' || (!rangeMappings?.length && !valueMappings?.length) ) return [];src/plugins/explore/public/components/visualizations/heatmap/heatmap_chart_utils.ts (1)
11-12: Consolidate imports from the same module.Both imports are from the same module and can be combined into a single import statement.
-import { generateTransformLayer } from '../style_panel/value_mapping/value_mapping_utils'; -import { decideScale, generateLabelExpr } from '../style_panel/value_mapping/value_mapping_utils'; +import { generateTransformLayer, decideScale, generateLabelExpr } from '../style_panel/value_mapping/value_mapping_utils';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (57)
changelogs/fragments/10923.yml(1 hunks)cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/06/rule_matching_vis_bar.spec.js(2 hunks)cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/07/rule_matching_vis_histogram.spec.js(1 hunks)src/plugins/explore/public/components/visualizations/bar/bar_chart_utils.test.ts(2 hunks)src/plugins/explore/public/components/visualizations/bar/bar_chart_utils.ts(7 hunks)src/plugins/explore/public/components/visualizations/bar/bar_exclusive_vis_options.tsx(4 hunks)src/plugins/explore/public/components/visualizations/bar/bar_vis_config.test.ts(1 hunks)src/plugins/explore/public/components/visualizations/bar/bar_vis_config.ts(3 hunks)src/plugins/explore/public/components/visualizations/bar/bar_vis_options.tsx(4 hunks)src/plugins/explore/public/components/visualizations/bar/to_expression.test.ts(5 hunks)src/plugins/explore/public/components/visualizations/bar/to_expression.ts(13 hunks)src/plugins/explore/public/components/visualizations/heatmap/heatmap_chart_utils.test.ts(5 hunks)src/plugins/explore/public/components/visualizations/heatmap/heatmap_chart_utils.ts(5 hunks)src/plugins/explore/public/components/visualizations/heatmap/heatmap_exclusive_vis_options.test.tsx(6 hunks)src/plugins/explore/public/components/visualizations/heatmap/heatmap_exclusive_vis_options.tsx(9 hunks)src/plugins/explore/public/components/visualizations/heatmap/heatmap_vis_config.test.ts(1 hunks)src/plugins/explore/public/components/visualizations/heatmap/heatmap_vis_config.ts(4 hunks)src/plugins/explore/public/components/visualizations/heatmap/heatmap_vis_options.tsx(3 hunks)src/plugins/explore/public/components/visualizations/heatmap/to_expression.test.ts(2 hunks)src/plugins/explore/public/components/visualizations/heatmap/to_expression.ts(5 hunks)src/plugins/explore/public/components/visualizations/histogram/histogram_vis_config.test.ts(1 hunks)src/plugins/explore/public/components/visualizations/histogram/histogram_vis_config.ts(3 hunks)src/plugins/explore/public/components/visualizations/histogram/histogram_vis_options.tsx(3 hunks)src/plugins/explore/public/components/visualizations/histogram/to_expression.ts(2 hunks)src/plugins/explore/public/components/visualizations/pie/pie_exclusive_vis_options.tsx(2 hunks)src/plugins/explore/public/components/visualizations/pie/pie_vis_config.ts(3 hunks)src/plugins/explore/public/components/visualizations/pie/pie_vis_options.test.tsx(4 hunks)src/plugins/explore/public/components/visualizations/pie/pie_vis_options.tsx(2 hunks)src/plugins/explore/public/components/visualizations/pie/to_expression.test.ts(2 hunks)src/plugins/explore/public/components/visualizations/pie/to_expression.ts(3 hunks)src/plugins/explore/public/components/visualizations/rule_repository.ts(3 hunks)src/plugins/explore/public/components/visualizations/scatter/scatter_exclusive_vis_options.test.tsx(5 hunks)src/plugins/explore/public/components/visualizations/scatter/scatter_exclusive_vis_options.tsx(2 hunks)src/plugins/explore/public/components/visualizations/scatter/scatter_vis_config.ts(3 hunks)src/plugins/explore/public/components/visualizations/scatter/scatter_vis_options.tsx(3 hunks)src/plugins/explore/public/components/visualizations/scatter/to_expression.test.ts(2 hunks)src/plugins/explore/public/components/visualizations/scatter/to_expression.ts(11 hunks)src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_config.ts(4 hunks)src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_exclusive_vis_options.tsx(3 hunks)src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_utils.test.ts(12 hunks)src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_utils.ts(10 hunks)src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_vis_options.tsx(1 hunks)src/plugins/explore/public/components/visualizations/state_timeline/to_expression.test.ts(11 hunks)src/plugins/explore/public/components/visualizations/state_timeline/to_expression.ts(16 hunks)src/plugins/explore/public/components/visualizations/style_panel/value_mapping/filter_options_select.test.tsx(1 hunks)src/plugins/explore/public/components/visualizations/style_panel/value_mapping/filter_options_select.tsx(1 hunks)src/plugins/explore/public/components/visualizations/style_panel/value_mapping/value_mapping_utils.test.ts(1 hunks)src/plugins/explore/public/components/visualizations/style_panel/value_mapping/value_mapping_utils.ts(1 hunks)src/plugins/explore/public/components/visualizations/table/table_vis.tsx(4 hunks)src/plugins/explore/public/components/visualizations/table/table_vis_config.test.ts(1 hunks)src/plugins/explore/public/components/visualizations/table/table_vis_config.ts(3 hunks)src/plugins/explore/public/components/visualizations/table/table_vis_options.tsx(5 hunks)src/plugins/explore/public/components/visualizations/types.ts(1 hunks)src/plugins/explore/public/components/visualizations/utils/calculation.ts(2 hunks)src/plugins/explore/public/components/visualizations/utils/utils.ts(2 hunks)src/plugins/explore/public/components/visualizations/visualization_builder_utils.test.ts(1 hunks)src/plugins/explore/public/components/visualizations/visualization_builder_utils.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (39)
src/plugins/explore/public/components/visualizations/pie/pie_vis_options.tsx (2)
src/plugins/explore/public/components/visualizations/style_panel/value_mapping/value_mapping_panel.tsx (1)
ValueMappingPanel(18-39)src/plugins/explore/public/components/visualizations/pie/pie_exclusive_vis_options.tsx (1)
PieExclusiveVisOptions(22-122)
src/plugins/explore/public/components/visualizations/bar/bar_exclusive_vis_options.tsx (2)
src/plugins/explore/public/components/visualizations/types.ts (1)
ColorModeOption(344-348)src/plugins/explore/public/components/visualizations/style_panel/value_mapping/filter_options_select.tsx (1)
ColorModeOptionSelect(11-71)
src/plugins/explore/public/components/visualizations/heatmap/heatmap_chart_utils.test.ts (2)
src/plugins/explore/public/components/visualizations/heatmap/heatmap_chart_utils.ts (1)
enhanceStyle(102-155)src/plugins/explore/public/components/visualizations/heatmap/heatmap_vis_config.ts (1)
HeatmapChartStyle(76-79)
src/plugins/explore/public/components/visualizations/style_panel/value_mapping/filter_options_select.tsx (1)
src/plugins/explore/public/components/visualizations/types.ts (1)
ColorModeOption(344-348)
src/plugins/explore/public/components/visualizations/pie/pie_vis_options.test.tsx (1)
src/plugins/explore/public/components/visualizations/pie/pie_vis_options.tsx (1)
PieVisStyleControls(20-101)
src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_exclusive_vis_options.tsx (3)
src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_config.ts (1)
StateTimeLineChartStyle(56-59)src/plugins/explore/public/components/visualizations/types.ts (1)
ColorModeOption(344-348)src/plugins/explore/public/components/visualizations/style_panel/value_mapping/filter_options_select.tsx (1)
ColorModeOptionSelect(11-71)
src/plugins/explore/public/components/visualizations/table/table_vis_config.ts (1)
src/plugins/explore/public/components/visualizations/types.ts (3)
Threshold(220-223)ColorModeOption(344-348)ValueMappingOptions(324-326)
src/plugins/explore/public/components/visualizations/pie/to_expression.ts (1)
src/plugins/explore/public/components/visualizations/style_panel/value_mapping/value_mapping_utils.ts (4)
processData(125-224)decideScale(12-32)generateLabelExpr(80-101)generateTransformLayer(45-78)
src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_utils.test.ts (2)
src/plugins/explore/public/components/visualizations/types.ts (1)
ValueMapping(315-322)src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_utils.ts (2)
mergeCategoricalData(125-184)mergeNumericalDataCore(268-383)
src/plugins/explore/public/components/visualizations/heatmap/to_expression.ts (3)
src/plugins/explore/public/components/visualizations/style_panel/value_mapping/value_mapping_utils.ts (1)
processData(125-224)src/plugins/explore/public/components/visualizations/heatmap/heatmap_chart_utils.ts (2)
enhanceStyle(102-155)addTransform(72-100)src/plugins/explore/public/components/visualizations/types.ts (1)
VEGASCHEMA(281-281)
src/plugins/explore/public/components/visualizations/pie/pie_exclusive_vis_options.tsx (3)
src/plugins/explore/public/components/visualizations/pie/pie_vis_config.ts (1)
PieChartStyle(45-48)src/plugins/explore/public/components/visualizations/types.ts (1)
ColorModeOption(344-348)src/plugins/explore/public/components/visualizations/style_panel/value_mapping/filter_options_select.tsx (1)
ColorModeOptionSelect(11-71)
src/plugins/explore/public/components/visualizations/utils/utils.ts (1)
src/plugins/explore/public/components/visualizations/types.ts (1)
ValueMapping(315-322)
src/plugins/explore/public/components/visualizations/table/table_vis.tsx (4)
src/plugins/explore/public/components/visualizations/utils/utils.ts (1)
getValueMappingByValue(322-353)src/plugins/explore/public/components/visualizations/theme/color_utils.ts (1)
resolveColor(77-91)src/plugins/explore/public/components/visualizations/theme/default_colors.ts (1)
DEFAULT_GREY(76-76)src/plugins/explore/public/components/visualizations/table/cell_value.tsx (1)
CellValue(26-125)
src/plugins/explore/public/components/visualizations/histogram/histogram_vis_config.ts (1)
src/plugins/explore/public/components/visualizations/types.ts (2)
ValueMappingOptions(324-326)ColorModeOption(344-348)
src/plugins/explore/public/components/visualizations/heatmap/heatmap_exclusive_vis_options.tsx (3)
src/plugins/explore/public/components/visualizations/heatmap/heatmap_vis_config.ts (1)
HeatmapChartStyle(76-79)src/plugins/explore/public/components/visualizations/types.ts (1)
ColorModeOption(344-348)src/plugins/explore/public/components/visualizations/style_panel/value_mapping/filter_options_select.tsx (1)
ColorModeOptionSelect(11-71)
src/plugins/explore/public/components/visualizations/scatter/to_expression.ts (2)
src/plugins/explore/public/components/visualizations/style_panel/value_mapping/value_mapping_utils.ts (3)
processData(125-224)generateTransformLayer(45-78)generateLabelExpr(80-101)src/plugins/explore/public/components/visualizations/bar/bar_chart_utils.ts (3)
buildThresholdColorEncoding(142-196)buildValueMappingColorEncoding(198-223)buildCombinedScale(225-256)
src/plugins/explore/public/components/visualizations/bar/bar_chart_utils.test.ts (2)
src/plugins/explore/public/components/visualizations/bar/bar_vis_config.ts (1)
BarChartStyle(70-76)src/plugins/explore/public/components/visualizations/bar/bar_chart_utils.ts (2)
buildValueMappingColorEncoding(198-223)buildCombinedScale(225-256)
src/plugins/explore/public/components/visualizations/bar/bar_vis_config.ts (1)
src/plugins/explore/public/components/visualizations/types.ts (2)
ValueMappingOptions(324-326)ColorModeOption(344-348)
src/plugins/explore/public/components/visualizations/style_panel/value_mapping/value_mapping_utils.ts (5)
src/plugins/explore/public/components/visualizations/types.ts (3)
ColorModeOption(344-348)ValueMapping(315-322)Threshold(220-223)src/plugins/explore/public/components/visualizations/theme/color_utils.ts (2)
resolveColor(77-91)getCategoryNextColor(72-74)src/plugins/explore/public/components/visualizations/theme/default_colors.ts (1)
DEFAULT_GREY(76-76)src/plugins/explore/public/components/visualizations/utils/calculation.ts (2)
CalculationMethod(11-23)calculateValue(40-105)src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_utils.ts (1)
convertThresholdsToValueMappings(111-120)
src/plugins/explore/public/components/visualizations/bar/to_expression.ts (4)
src/plugins/explore/public/components/visualizations/utils/calculation.ts (1)
transformBucketToCalculationMethod(25-33)src/plugins/explore/public/components/visualizations/style_panel/value_mapping/value_mapping_utils.ts (3)
processData(125-224)generateTransformLayer(45-78)generateLabelExpr(80-101)src/plugins/explore/public/components/visualizations/bar/bar_chart_utils.ts (5)
buildThresholdColorEncoding(142-196)buildValueMappingColorEncoding(198-223)buildEncoding(89-113)buildTooltipEncoding(115-140)buildCombinedScale(225-256)src/plugins/explore/public/components/visualizations/utils/utils.ts (2)
getSchemaByAxis(210-223)applyAxisStyling(22-89)
src/plugins/explore/public/components/visualizations/scatter/scatter_exclusive_vis_options.test.tsx (1)
src/plugins/explore/public/components/visualizations/scatter/scatter_exclusive_vis_options.tsx (2)
ScatterVisOptionsProps(16-22)ScatterExclusiveVisOptions(24-110)
src/plugins/explore/public/components/visualizations/scatter/scatter_vis_options.tsx (2)
src/plugins/explore/public/components/visualizations/style_panel/value_mapping/value_mapping_panel.tsx (1)
ValueMappingPanel(18-39)src/plugins/explore/public/components/visualizations/scatter/scatter_exclusive_vis_options.tsx (1)
ScatterExclusiveVisOptions(24-110)
src/plugins/explore/public/components/visualizations/bar/to_expression.test.ts (2)
src/plugins/explore/public/components/visualizations/bar/bar_vis_config.ts (2)
BarChartStyle(70-76)defaultBarChartStyles(78-155)src/plugins/explore/public/components/visualizations/bar/to_expression.ts (1)
createDoubleNumericalBarChart(720-834)
src/plugins/explore/public/components/visualizations/scatter/scatter_vis_config.ts (1)
src/plugins/explore/public/components/visualizations/types.ts (3)
ThresholdOptions(275-279)ValueMappingOptions(324-326)ColorModeOption(344-348)
src/plugins/explore/public/components/visualizations/visualization_builder_utils.ts (7)
src/plugins/explore/public/components/visualizations/style_panel/threshold/threshold_utils.ts (2)
transformToThreshold(185-204)Colors(99-183)src/plugins/explore/public/components/visualizations/utils/use_visualization_types.ts (1)
StyleOptions(56-68)src/plugins/explore/public/components/visualizations/bar/bar_vis_config.ts (1)
BarChartStyleOptions(29-68)src/plugins/explore/public/components/visualizations/theme/default_colors.ts (1)
getColors(9-74)src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_config.ts (1)
StateTimeLineChartStyleOptions(32-54)src/plugins/explore/public/components/visualizations/scatter/scatter_vis_config.ts (1)
ScatterChartStyleOptions(31-52)src/plugins/explore/public/components/visualizations/histogram/histogram_vis_config.ts (1)
HistogramChartStyleOptions(27-54)
src/plugins/explore/public/components/visualizations/table/table_vis_options.tsx (3)
src/plugins/explore/public/components/visualizations/types.ts (1)
ColorModeOption(344-348)src/plugins/explore/public/components/visualizations/style_panel/value_mapping/filter_options_select.tsx (1)
ColorModeOptionSelect(11-71)src/plugins/explore/public/components/visualizations/style_panel/value_mapping/value_mapping_panel.tsx (1)
ValueMappingPanel(18-39)
src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_config.ts (1)
src/plugins/explore/public/components/visualizations/types.ts (2)
ThresholdOptions(275-279)ColorModeOption(344-348)
src/plugins/explore/public/components/visualizations/style_panel/value_mapping/filter_options_select.test.tsx (2)
src/plugins/explore/public/components/visualizations/style_panel/value_mapping/filter_options_select.tsx (1)
ColorModeOptionSelect(11-71)src/plugins/explore/public/components/visualizations/types.ts (1)
ColorModeOption(344-348)
src/plugins/explore/public/components/visualizations/state_timeline/to_expression.ts (3)
src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_utils.ts (2)
convertThresholdsToValueMappings(111-120)mergeNumericalDataCore(268-383)src/plugins/explore/public/components/visualizations/style_panel/value_mapping/value_mapping_utils.ts (4)
convertThresholdsToValueMappings(226-235)decideScale(12-32)generateLabelExpr(80-101)generateTransformLayer(45-78)src/plugins/explore/public/components/visualizations/utils/utils.ts (2)
applyAxisStyling(22-89)getSwappedAxisRole(173-208)
src/plugins/explore/public/components/visualizations/pie/pie_vis_config.ts (1)
src/plugins/explore/public/components/visualizations/types.ts (2)
ValueMappingOptions(324-326)ColorModeOption(344-348)
src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_utils.ts (1)
src/plugins/explore/public/components/visualizations/types.ts (3)
RangeValue(216-219)ColorModeOption(344-348)ValueMapping(315-322)
src/plugins/explore/public/components/visualizations/heatmap/heatmap_vis_config.ts (1)
src/plugins/explore/public/components/visualizations/types.ts (3)
ThresholdOptions(275-279)ValueMappingOptions(324-326)ColorModeOption(344-348)
src/plugins/explore/public/components/visualizations/scatter/scatter_exclusive_vis_options.tsx (3)
src/plugins/explore/public/components/visualizations/scatter/scatter_vis_config.ts (1)
ScatterChartStyle(54-60)src/plugins/explore/public/components/visualizations/types.ts (1)
ColorModeOption(344-348)src/plugins/explore/public/components/visualizations/style_panel/value_mapping/filter_options_select.tsx (1)
ColorModeOptionSelect(11-71)
src/plugins/explore/public/components/visualizations/heatmap/heatmap_exclusive_vis_options.test.tsx (2)
src/plugins/explore/public/components/visualizations/types.ts (1)
ColorModeOption(344-348)src/plugins/explore/public/components/visualizations/heatmap/heatmap_exclusive_vis_options.tsx (1)
HeatmapExclusiveVisOptions(29-187)
src/plugins/explore/public/components/visualizations/bar/bar_vis_options.tsx (2)
src/plugins/explore/public/components/visualizations/style_panel/value_mapping/value_mapping_panel.tsx (1)
ValueMappingPanel(18-39)src/plugins/explore/public/components/visualizations/bar/bar_exclusive_vis_options.tsx (1)
BarExclusiveVisOptions(45-224)
src/plugins/explore/public/components/visualizations/histogram/histogram_vis_options.tsx (1)
src/plugins/explore/public/components/visualizations/style_panel/value_mapping/value_mapping_panel.tsx (1)
ValueMappingPanel(18-39)
src/plugins/explore/public/components/visualizations/heatmap/heatmap_vis_options.tsx (2)
src/plugins/explore/public/components/visualizations/style_panel/value_mapping/value_mapping_panel.tsx (1)
ValueMappingPanel(18-39)src/plugins/explore/public/components/visualizations/heatmap/heatmap_exclusive_vis_options.tsx (1)
HeatmapExclusiveVisOptions(29-187)
src/plugins/explore/public/components/visualizations/bar/bar_chart_utils.ts (4)
src/plugins/explore/public/components/visualizations/types.ts (2)
VisColumn(62-69)ValueMapping(315-322)src/plugins/explore/public/components/visualizations/bar/bar_vis_config.ts (1)
BarChartStyle(70-76)src/plugins/explore/public/components/visualizations/style_panel/value_mapping/value_mapping_utils.ts (2)
decideScale(12-32)generateLabelExpr(80-101)src/plugins/explore/public/components/visualizations/theme/color_utils.ts (2)
getCategoryNextColor(72-74)resolveColor(77-91)
src/plugins/explore/public/components/visualizations/heatmap/heatmap_chart_utils.ts (3)
src/plugins/explore/public/components/visualizations/heatmap/heatmap_vis_config.ts (1)
HeatmapChartStyle(76-79)src/plugins/explore/public/components/visualizations/types.ts (1)
ValueMapping(315-322)src/plugins/explore/public/components/visualizations/style_panel/value_mapping/value_mapping_utils.ts (3)
generateTransformLayer(45-78)decideScale(12-32)generateLabelExpr(80-101)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (67)
- GitHub Check: Run functional tests on Windows (ciGroup13)
- GitHub Check: Run functional tests on Windows (ciGroup8)
- GitHub Check: Run functional tests on Windows (ciGroup6)
- GitHub Check: Run functional tests on Windows (ciGroup5)
- GitHub Check: Run functional tests on Windows (ciGroup2)
- GitHub Check: Run functional tests on Windows (ciGroup4)
- GitHub Check: Run functional tests on Windows (ciGroup7)
- GitHub Check: Run functional tests on Windows (ciGroup3)
- GitHub Check: Run functional tests on Windows (ciGroup12)
- GitHub Check: Run functional tests on Linux (ciGroup11)
- GitHub Check: Run functional tests on Windows (ciGroup9)
- GitHub Check: Run functional tests on Linux (ciGroup12)
- GitHub Check: Run functional tests on Windows (ciGroup11)
- GitHub Check: Run functional tests on Windows (ciGroup10)
- GitHub Check: Run functional tests on Linux (ciGroup13)
- GitHub Check: Run functional tests on Linux (ciGroup8)
- GitHub Check: Run functional tests on Windows (ciGroup1)
- GitHub Check: Run functional tests on Linux (ciGroup10)
- GitHub Check: Run functional tests on Linux (ciGroup6)
- GitHub Check: Run cypress tests (osd:ciGroup17Explore)
- GitHub Check: Run functional tests on Linux (ciGroup4)
- GitHub Check: Run functional tests on Linux (ciGroup9)
- GitHub Check: Run functional tests on Linux (ciGroup5)
- GitHub Check: Run functional tests on Linux (ciGroup3)
- GitHub Check: Run functional tests on Linux (ciGroup7)
- GitHub Check: Run cypress tests (osd:ciGroup12Explore)
- GitHub Check: Run cypress tests (osd:ciGroup13Explore)
- GitHub Check: Run cypress tests (osd:ciGroup16Explore)
- GitHub Check: Run cypress tests (osd:ciGroup10Explore)
- GitHub Check: Run cypress tests (osd:ciGroup15)
- GitHub Check: Run cypress tests (osd:ciGroup14Explore)
- GitHub Check: Run functional tests on Linux (ciGroup2)
- GitHub Check: Run cypress tests (osd:ciGroup15Explore)
- GitHub Check: Run functional tests on Linux (ciGroup1)
- GitHub Check: Run cypress tests (osd:ciGroup14)
- GitHub Check: Run cypress tests (osd:ciGroup9)
- GitHub Check: Run cypress tests (osd:ciGroup11)
- GitHub Check: Run cypress tests (osd:ciGroup13)
- GitHub Check: Run plugin functional tests on Windows
- GitHub Check: Run cypress tests (osd:ciGroup5)
- GitHub Check: Run cypress tests (osd:ciGroup12)
- GitHub Check: Run cypress tests (osd:ciGroup10Fast)
- GitHub Check: Run cypress tests (osd:ciGroup8)
- GitHub Check: Run cypress tests (osd:ciGroup10Slow)
- GitHub Check: Run cypress tests (osd:ciGroup3)
- GitHub Check: Run cypress tests (osd:ciGroup1)
- GitHub Check: Run cypress tests (osd:ciGroup6)
- GitHub Check: Run cypress tests (osd:ciGroup4)
- GitHub Check: Run cypress tests (osd:ciGroup7)
- GitHub Check: Run cypress tests (osd:ciGroup2)
- GitHub Check: Build and Verify on Windows (ciGroup4)
- GitHub Check: Build min release artifacts on Linux x64
- GitHub Check: Build min release artifacts on Linux ARM64
- GitHub Check: Build min release artifacts on macOS ARM64
- GitHub Check: Build min release artifacts on macOS x64
- GitHub Check: Build and Verify on Windows (ciGroup2)
- GitHub Check: Build min release artifacts on Windows x64
- GitHub Check: Build and Verify on Windows (ciGroup3)
- GitHub Check: Build and Verify on Linux (ciGroup2)
- GitHub Check: Build and Verify on Windows (ciGroup1)
- GitHub Check: Build and Verify on Linux (ciGroup1)
- GitHub Check: Build and Verify on Linux (ciGroup4)
- GitHub Check: Run plugin functional tests on Linux
- GitHub Check: Build and Verify on Linux (ciGroup3)
- GitHub Check: Lint and validate
- GitHub Check: bundle-analyzer
- GitHub Check: lighthouse
Comment on lines
+1
to
+2
| feat: | ||
| - Add value mapping for pie ([#10923](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/10923)) No newline at end of file |
There was a problem hiding this comment.
Changelog entry incomplete - missing heatmap mention.
According to the PR objectives and the code changes, this PR adds value mapping support for both pie and heatmap chart types. The changelog should reflect both additions.
feat:
-- Add value mapping for pie ([#10923](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/10923))
+- Add value mapping for pie and heatmap ([#10923](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/10923))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| feat: | |
| - Add value mapping for pie ([#10923](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/10923)) | |
| feat: | |
| - Add value mapping for pie and heatmap ([#10923](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/10923)) |
🤖 Prompt for AI Agents
In changelogs/fragments/10923.yml around lines 1 to 2, the changelog only
mentions adding value mapping for pie but omits heatmap; update the fragment to
include both chart types by adding a parallel entry for heatmap (e.g., "Add
value mapping for pie and heatmap") or add a separate line for heatmap so the
changelog accurately reflects both additions from the PR.
Comment on lines
+247
to
+250
| const colors = [ | ||
| ...(categorical2Options ?? []).map((c, i) => getCategoryNextColor(i)), | ||
| ...items.map((m, i) => resolveColor(m.color) || getCategoryNextColor(i)), | ||
| ]; |
There was a problem hiding this comment.
Color index collision when mapping has no explicit color.
When a mapping item lacks an explicit color, getCategoryNextColor(i) uses the item's index within the items array (starting from 0), which can produce duplicate colors with the categorical options. The index should be offset by the number of categorical options.
const colors = [
...(categorical2Options ?? []).map((c, i) => getCategoryNextColor(i)),
- ...items.map((m, i) => resolveColor(m.color) || getCategoryNextColor(i)),
+ ...items.map((m, i) => resolveColor(m.color) || getCategoryNextColor((categorical2Options?.length ?? 0) + i)),
];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const colors = [ | |
| ...(categorical2Options ?? []).map((c, i) => getCategoryNextColor(i)), | |
| ...items.map((m, i) => resolveColor(m.color) || getCategoryNextColor(i)), | |
| ]; | |
| const colors = [ | |
| ...(categorical2Options ?? []).map((c, i) => getCategoryNextColor(i)), | |
| ...items.map((m, i) => resolveColor(m.color) || getCategoryNextColor((categorical2Options?.length ?? 0) + i)), | |
| ]; |
🤖 Prompt for AI Agents
In src/plugins/explore/public/components/visualizations/bar/bar_chart_utils.ts
around lines 247-250, the color array construction can produce duplicate colors
because getCategoryNextColor(i) for items uses the item's index without
accounting for preceding categorical2Options entries; change the items mapping
to offset the index by the number of categorical options (use an offset =
(categorical2Options ?? []).length and call getCategoryNextColor(i + offset)) so
items without explicit color pick the next non-colliding category color; ensure
the code handles categorical2Options being undefined (treat length as 0).
| styleOptions.colorModeOption === 'useThresholdColor' | ||
| ? buildThresholdColorEncoding(yAxis, styleOptions, true) | ||
| : buildValueMappingColorEncoding(styleOptions, validValues, validRanges); | ||
| // const colorEncodingLayer = buildThresholdColorEncoding(yAxis, styleOptions); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove commented-out code.
Lines 770 and 787 contain commented-out code that should be removed to keep the codebase clean.
- // const colorEncodingLayer = buildThresholdColorEncoding(yAxis, styleOptions);
-
...
- // aggregate: styles?.bucket?.aggregationType,Also applies to: 787-787
🤖 Prompt for AI Agents
In src/plugins/explore/public/components/visualizations/bar/to_expression.ts
around lines 770 and 787, remove the two commented-out lines (the unused calls
to buildThresholdColorEncoding) to clean up dead code; simply delete those
commented lines and ensure surrounding formatting/indentation remains correct
and no trailing commas or syntax issues are introduced.
Comment on lines
+140
to
+154
| if (canUseValueMapping) { | ||
| markLayer.encoding.color.field = 'mappingValue'; | ||
| markLayer.encoding.color.type = 'nominal'; | ||
| markLayer.encoding.color.scale = decideScale( | ||
| styles?.colorModeOption, | ||
| rangeMappings, | ||
| valueMappings | ||
| ); | ||
| delete markLayer.encoding.color.scale.type; | ||
| markLayer.encoding.color.legend.labelExpr = generateLabelExpr( | ||
| rangeMappings, | ||
| valueMappings, | ||
| styles?.colorModeOption | ||
| ); | ||
| } |
There was a problem hiding this comment.
Potential null reference when setting legend.labelExpr.
If styles.addLegend is false, markLayer.encoding.color.legend could be null, causing a runtime error on Line 149 when attempting to set labelExpr.
if (canUseValueMapping) {
markLayer.encoding.color.field = 'mappingValue';
markLayer.encoding.color.type = 'nominal';
markLayer.encoding.color.scale = decideScale(
styles?.colorModeOption,
rangeMappings,
valueMappings
);
delete markLayer.encoding.color.scale.type;
- markLayer.encoding.color.legend.labelExpr = generateLabelExpr(
- rangeMappings,
- valueMappings,
- styles?.colorModeOption
- );
+ if (markLayer.encoding.color.legend) {
+ markLayer.encoding.color.legend.labelExpr = generateLabelExpr(
+ rangeMappings,
+ valueMappings,
+ styles?.colorModeOption
+ );
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (canUseValueMapping) { | |
| markLayer.encoding.color.field = 'mappingValue'; | |
| markLayer.encoding.color.type = 'nominal'; | |
| markLayer.encoding.color.scale = decideScale( | |
| styles?.colorModeOption, | |
| rangeMappings, | |
| valueMappings | |
| ); | |
| delete markLayer.encoding.color.scale.type; | |
| markLayer.encoding.color.legend.labelExpr = generateLabelExpr( | |
| rangeMappings, | |
| valueMappings, | |
| styles?.colorModeOption | |
| ); | |
| } | |
| if (canUseValueMapping) { | |
| markLayer.encoding.color.field = 'mappingValue'; | |
| markLayer.encoding.color.type = 'nominal'; | |
| markLayer.encoding.color.scale = decideScale( | |
| styles?.colorModeOption, | |
| rangeMappings, | |
| valueMappings | |
| ); | |
| delete markLayer.encoding.color.scale.type; | |
| if (markLayer.encoding.color.legend) { | |
| markLayer.encoding.color.legend.labelExpr = generateLabelExpr( | |
| rangeMappings, | |
| valueMappings, | |
| styles?.colorModeOption | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
src/plugins/explore/public/components/visualizations/heatmap/heatmap_chart_utils.ts
around lines 140-154, the code assumes markLayer.encoding.color.legend is
present which can be null when styles.addLegend is false; guard against this by
checking styles.addLegend (or markLayer.encoding.color.legend) before assigning
legend.labelExpr and if needed instantiate markLayer.encoding.color.legend = {}
(or an object with required properties) so the assignment is safe; ensure the
guard wraps the generateLabelExpr assignment (and any other legend property
writes) to prevent null-reference runtime errors.
Comment on lines
+119
to
+121
| colorModeOption={styleOptions?.colorModeOption} | ||
| onColorModeOptionChange={(option) => updateStyleOption('colorModeOption', option)} | ||
| hasDate={true} |
There was a problem hiding this comment.
hasDate is hardcoded to true, which will always disable value mapping for histograms.
In bar_vis_options.tsx (line 43), hasDate is dynamically computed based on axis schema. Since hasDate is passed to BarExclusiveVisOptions where it controls disableValueMapping={hasDate}, hardcoding it to true will always disable value mapping for histogram charts, contradicting this PR's intent.
Consider computing hasDate dynamically:
+ const hasDate = axes.some((axis) => axis?.schema === VisFieldType.Date);
+
// The mapping object will be an empty object if no fields are selected on the axes selector.Then update line 121:
- hasDate={true}
+ hasDate={hasDate}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
src/plugins/explore/public/components/visualizations/histogram/histogram_vis_options.tsx
around lines 119 to 121, hasDate is hardcoded to true which always disables
value mapping; compute hasDate dynamically from the axis schema (like in
bar_vis_options.tsx) by checking whether series?.[0]?.x.scale?.type === 'date'
or inspecting the x-axis schema for a date field, then pass that computed
boolean to the component (replace hasDate={true} with hasDate={computedHasDate})
so value mapping is only disabled when the x axis is actually a date.
Comment on lines
+78
to
+84
| it('should use validValues when validRanges is not empty', () => { | ||
| const result = decideScale(undefined, undefined, mockValidValues); | ||
|
|
||
| expect(result).toEqual({ | ||
| domain: ['A', 'B'], | ||
| range: ['#ff0000', '#00ff00'], | ||
| }); |
There was a problem hiding this comment.
Clarify test description.
The test description states "when validRanges is not empty" but the test passes undefined for validRanges (line 79). This is confusing.
Consider updating to:
- it('should use validValues when validRanges is not empty', () => {
+ it('should use validValues when validRanges is empty or undefined', () => {
const result = decideScale(undefined, undefined, mockValidValues);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('should use validValues when validRanges is not empty', () => { | |
| const result = decideScale(undefined, undefined, mockValidValues); | |
| expect(result).toEqual({ | |
| domain: ['A', 'B'], | |
| range: ['#ff0000', '#00ff00'], | |
| }); | |
| it('should use validValues when validRanges is empty or undefined', () => { | |
| const result = decideScale(undefined, undefined, mockValidValues); | |
| expect(result).toEqual({ | |
| domain: ['A', 'B'], | |
| range: ['#ff0000', '#00ff00'], | |
| }); |
🤖 Prompt for AI Agents
In
src/plugins/explore/public/components/visualizations/style_panel/value_mapping/value_mapping_utils.test.ts
around lines 78 to 84, the test title incorrectly says "when validRanges is not
empty" while the call passes undefined for validRanges; update the test
description to accurately reflect the scenario (e.g., "should use validValues
when validRanges is undefined" or "should use validValues when validRanges is
not provided") so the test name matches the inputs used.
Comment on lines
+88
to
+100
| const mappings = [ | ||
| ...(validValues?.map((m) => `'${m.value}': '${m.displayText || m.value}'`) ?? []), | ||
| ...(validRanges?.map((m) => { | ||
| const key = `[${m.range?.min},${m.range?.max ?? '∞'})`; | ||
| return `'${key}': '${m.displayText || key}'`; | ||
| }) ?? []), | ||
| ]; | ||
|
|
||
| if (colorModeOption === 'highlightValueMapping') { | ||
| mappings.push("null: 'unmatched'"); | ||
| } | ||
|
|
||
| return `{${mappings.join(', ')}}[datum.label] || datum.label`; |
There was a problem hiding this comment.
Values containing quotes may break the generated expression.
If m.value or m.displayText contains single quotes, the generated expression string will be malformed. Consider escaping quotes in user-provided values.
+const escapeQuotes = (str: string) => str.replace(/'/g, "\\'");
+
const mappings = [
- ...(validValues?.map((m) => `'${m.value}': '${m.displayText || m.value}'`) ?? []),
+ ...(validValues?.map((m) => `'${escapeQuotes(m.value ?? '')}': '${escapeQuotes(m.displayText || m.value || '')}'`) ?? []),
...(validRanges?.map((m) => {
const key = `[${m.range?.min},${m.range?.max ?? '∞'})`;
- return `'${key}': '${m.displayText || key}'`;
+ return `'${key}': '${escapeQuotes(m.displayText || key)}'`;
}) ?? []),
];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const mappings = [ | |
| ...(validValues?.map((m) => `'${m.value}': '${m.displayText || m.value}'`) ?? []), | |
| ...(validRanges?.map((m) => { | |
| const key = `[${m.range?.min},${m.range?.max ?? '∞'})`; | |
| return `'${key}': '${m.displayText || key}'`; | |
| }) ?? []), | |
| ]; | |
| if (colorModeOption === 'highlightValueMapping') { | |
| mappings.push("null: 'unmatched'"); | |
| } | |
| return `{${mappings.join(', ')}}[datum.label] || datum.label`; | |
| const escapeQuotes = (str: string) => str.replace(/'/g, "\\'"); | |
| const mappings = [ | |
| ...(validValues?.map((m) => `'${escapeQuotes(m.value ?? '')}': '${escapeQuotes(m.displayText || m.value || '')}'`) ?? []), | |
| ...(validRanges?.map((m) => { | |
| const key = `[${m.range?.min},${m.range?.max ?? '∞'})`; | |
| return `'${key}': '${escapeQuotes(m.displayText || key)}'`; | |
| }) ?? []), | |
| ]; | |
| if (colorModeOption === 'highlightValueMapping') { | |
| mappings.push("null: 'unmatched'"); | |
| } | |
| return `{${mappings.join(', ')}}[datum.label] || datum.label`; |
🤖 Prompt for AI Agents
In
src/plugins/explore/public/components/visualizations/style_panel/value_mapping/value_mapping_utils.ts
around lines 88-100, the generated expression uses single-quoted user strings
which will break if m.value, m.displayText or the range key contain single
quotes or backslashes; update the generation to safely escape single quotes and
backslashes (or prefer producing values via JSON.stringify) when building each
mapping key and displayText so the resulting '{...}[datum.label] || datum.label'
expression remains valid; ensure range keys are escaped the same way and the
"null: 'unmatched'" entry remains syntactically correct.
Comment on lines
+112
to
+121
| const generateValue = (mapping: ValueMapping) => { | ||
| return useRangeMappings | ||
| ? `'[${mapping?.range?.min},${mapping?.range?.max ?? '∞'})'` | ||
| : mapping.value; | ||
| }; | ||
|
|
||
| return mappings?.map((v, i) => ({ | ||
| test: `datum['${field}']=== ${generateValue(v)}`, | ||
| value: resolveColor(v.color) || getCategoryNextColor(i), | ||
| })); |
There was a problem hiding this comment.
Inconsistent quoting in test expression for value mappings.
For range mappings, generateValue returns a quoted string (e.g., '[0,∞)'), but for value mappings, it returns the raw value without quotes. This results in malformed test expressions for value mappings.
const generateValue = (mapping: ValueMapping) => {
return useRangeMappings
? `'[${mapping?.range?.min},${mapping?.range?.max ?? '∞'})'`
- : mapping.value;
+ : `'${mapping.value}'`;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const generateValue = (mapping: ValueMapping) => { | |
| return useRangeMappings | |
| ? `'[${mapping?.range?.min},${mapping?.range?.max ?? '∞'})'` | |
| : mapping.value; | |
| }; | |
| return mappings?.map((v, i) => ({ | |
| test: `datum['${field}']=== ${generateValue(v)}`, | |
| value: resolveColor(v.color) || getCategoryNextColor(i), | |
| })); | |
| const generateValue = (mapping: ValueMapping) => { | |
| return useRangeMappings | |
| ? `'[${mapping?.range?.min},${mapping?.range?.max ?? '∞'})'` | |
| : `'${mapping.value}'`; | |
| }; | |
| return mappings?.map((v, i) => ({ | |
| test: `datum['${field}']=== ${generateValue(v)}`, | |
| value: resolveColor(v.color) || getCategoryNextColor(i), | |
| })); |
🤖 Prompt for AI Agents
In
src/plugins/explore/public/components/visualizations/style_panel/value_mapping/value_mapping_utils.ts
around lines 112 to 121, the test expression is malformed because generateValue
returns a quoted string for range mappings but raw unquoted values for simple
value mappings; update generateValue to return a properly escaped JS literal for
any mapping (e.g., use JSON.stringify for string values, keep numbers/booleans
as-is, and preserve the range string already wrapped) so the produced test
string datum['field']===<value> is always syntactically correct, ensuring
strings are quoted and other primitives are not.
Comment on lines
+172
to
+176
| const numericalOptions = Array.from(new Set(newRecord.map((t) => t[numericalColumn!]))); | ||
|
|
||
| const categorical2Options = categoricalColumn2 | ||
| ? Array.from(new Set(newRecord.map((t) => t[categoricalColumn2!]))) | ||
| : null; |
There was a problem hiding this comment.
Potential undefined access on numericalColumn.
Lines 172-176 use numericalColumn! (non-null assertion) outside the block that checks if numericalColumn exists (Line 154). If numericalColumn is undefined, accessing t[numericalColumn!] will use undefined as a key.
- const numericalOptions = Array.from(new Set(newRecord.map((t) => t[numericalColumn!])));
+ const numericalOptions = numericalColumn
+ ? Array.from(new Set(newRecord.map((t) => t[numericalColumn])))
+ : [];
- const categorical2Options = categoricalColumn2
+ const categorical2Options = categoricalColumn2 && numericalColumn
? Array.from(new Set(newRecord.map((t) => t[categoricalColumn2!])))
: null;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const numericalOptions = Array.from(new Set(newRecord.map((t) => t[numericalColumn!]))); | |
| const categorical2Options = categoricalColumn2 | |
| ? Array.from(new Set(newRecord.map((t) => t[categoricalColumn2!]))) | |
| : null; | |
| const numericalOptions = numericalColumn | |
| ? Array.from(new Set(newRecord.map((t) => t[numericalColumn]))) | |
| : []; | |
| const categorical2Options = categoricalColumn2 && numericalColumn | |
| ? Array.from(new Set(newRecord.map((t) => t[categoricalColumn2!]))) | |
| : null; |
🤖 Prompt for AI Agents
In
src/plugins/explore/public/components/visualizations/style_panel/value_mapping/value_mapping_utils.ts
around lines 172 to 176, the code uses numericalColumn! without ensuring it
exists; change the logic to only compute numericalOptions when numericalColumn
is defined (mirror the categoricalColumn2 pattern) — e.g., if numericalColumn is
truthy, build Array.from(new Set(newRecord.map(t => t[numericalColumn]))),
otherwise set numericalOptions to null (or an empty array depending on expected
consumers); remove the non-null assertion and add a conditional guard so
accessing t[numericalColumn] cannot occur when numericalColumn is undefined.
Comment on lines
+247
to
+265
| let displayText; | ||
| if ( | ||
| columnCellType !== 'auto' && | ||
| styleOptions?.valueMappingOptions?.valueMappings && | ||
| styleOptions?.valueMappingOptions?.valueMappings.length > 0 && | ||
| (styleOptions.colorModeOption === 'highlightValueMapping' || | ||
| styleOptions.colorModeOption === 'useValueMapping') | ||
| ) { | ||
| const mapping = getValueMappingByValue( | ||
| cellValue, | ||
| styleOptions.valueMappingOptions.valueMappings | ||
| ); | ||
| if (mapping) { | ||
| color = resolveColor(mapping.color); | ||
| displayText = mapping?.displayText; | ||
| } else { | ||
| color = DEFAULT_GREY; | ||
| } | ||
| } |
There was a problem hiding this comment.
Clarify fallback behavior for unmapped values.
When value mapping is active but no mapping matches a cell value, the code falls back to DEFAULT_GREY (line 263). This behavior makes sense for 'highlightValueMapping' mode (unmapped values should be neutral), but for 'useValueMapping' mode, consider whether unmapped values should:
- Use the default cell color (no special coloring)
- Show a different indicator that they're unmapped
- Continue using grey
The current implementation treats both modes identically, which may not align with user expectations.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This pr adds value mapping for remaining charts
Screenshot
pie-value
2025-11-17.14.33.53.mp4
pie-range
2025-11-17.14.34.36.1.mp4
Testing the changes
Changelog
Check List
yarn test:jestyarn test:jest_integrationSummary by CodeRabbit
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.