[Explore Vis] migrate state timeline #11136
Conversation
📝 WalkthroughWalkthroughThis PR migrates the state timeline visualization to use ECharts rendering alongside existing Vega-Lite support. It refactors state timeline utilities to introduce a unified data-merging pipeline ( Changes
Sequence DiagramsequenceDiagram
participant Client as Client/Rule
participant Transform as Transform
participant Merge as mergeDataCore
participant Group as groupByMergedLabel
participant Config as ECharts Config
participant Axis as Axis Builder
participant Spec as StateTimeLine Spec
participant Assemble as assembleSpec
participant Render as ECharts Render
Client->>Transform: Raw data
Transform->>Merge: Transformed data
Merge->>Merge: Apply value/range mappings<br/>+ threshold coloring
Merge->>Group: Merged records
Group->>Group: Group by mergedLabel
Group->>Config: Grouped data
Config->>Axis: Base config + state
Axis->>Axis: Compute axis from<br/>axisColumnMappings
Axis->>Spec: Axis configs
Spec->>Spec: Build series with<br/>color/style decisions
Spec->>Assemble: Spec with rendered items
Assemble->>Assemble: Distinguish multi-dataset<br/>+ grid layout
Assemble->>Render: Final ECharts spec
Render->>Client: Rendered visualization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
Qxisylolo
requested review from
AMoo-Miki,
BionIT,
Flyingliuhub,
LDrago27,
Maosaic,
SuZhou-Joe,
ZilongX,
abbyhu2000,
ananzh,
angle943,
ashwin-pc,
bandinib-amzn,
curq,
huyaboo,
joshuali925,
joshuarrrr,
kavilla,
manasvinibs,
mengweieric,
ruanyl,
sejli,
virajsanghvi,
xinruiba,
zengyan-amazon,
zhongnansu and
zhyuanqi
as code owners
Qxisylolo
requested review from
TackAdam,
d-buckner,
paulstn,
ps48,
raintygao,
ruchidh and
sumukhswamy
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: 4
🤖 Fix all issues with AI agents
In @src/plugins/explore/public/components/visualizations/rule_repository.ts:
- Around line 157-165: The new 'state_timeline' branch added to toSpec
references createSingleNumericalStateTimeline but CHART_METADATA.state_timeline
is not included in the chartTypes array used for the "1 Metric & 1 Date" rule;
add CHART_METADATA.state_timeline to the chartTypes array (near where chartTypes
is defined/returned for that rule) so the state_timeline visualization is
suggested, or if omission was intentional, add a clear comment in the chartTypes
definition explaining why state_timeline is excluded; ensure you reference the
same CHART_METADATA key name 'state_timeline' so it matches the toSpec switch
and createSingleNumericalStateTimeline path.
In
@src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_exclusive_vis_options.tsx:
- Around line 19-20: The callback type for onUseValueMappingColorChange uses a
misleading parameter name; update its signature so the parameter is named
useValueMappingColor instead of useThresholdColor (i.e., change
onUseValueMappingColorChange: (useThresholdColor: boolean) => void to
onUseValueMappingColorChange: (useValueMappingColor: boolean) => void) and
update any call sites or usages of onUseValueMappingColorChange to reference the
new parameter name where applicable (ensure consistency in the
state_timeline_exclusive_vis_options component and related handlers).
In
@src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_utils.ts:
- Around line 493-497: The displayText nullish coalescing is a no-op; in the
mergeLabelCombo.push call in state_timeline_utils.ts replace the redundant
`displayText: row[1][displayTextIndex] ?? row[1][displayTextIndex]` with a
meaningful fallback such as `displayText: row[1][displayTextIndex] ??
row[1][mergeLabelIndex]` (or `''` if you prefer empty string) so displayText
uses the displayTextIndex value when present and falls back to the label from
mergeLabelIndex otherwise; update the mergeLabelCombo.push entry that references
mergeLabelIndex, mergedColorIndex, and displayTextIndex accordingly.
In
@src/plugins/explore/public/components/visualizations/state_timeline/to_expression.ts:
- Around line 985-993: The baseSpec for createSingleNumericalStateTimeline is
missing the calculation transform that defines the fakeYAxis field referenced in
the encoding; add a calc transform (same logic as in
createSingleCategoricalStateTimeline) into transformLayer or baseSpec.transform
so it computes fakeYAxis from processedData before layering, ensuring the
transform name/field matches "fakeYAxis" used in encodings of
createSingleNumericalStateTimeline and that transformLayer is used by baseSpec.
🧹 Nitpick comments (6)
src/plugins/explore/public/components/visualizations/utils/data_transformation/filter.ts (3)
23-27: Misleading function name: "filter" uses map internally.The function name
filteris misleading because it usesArray.map()internally rather thanArray.filter(). This doesn't actually remove elements from the array—it transforms them. For row filtering use cases, developers must chain.filter(Boolean)afterward, as mentioned in the documentation.Consider renaming to
transformormapDatato better reflect the actual behavior, or add a more prominent warning in the documentation that this is a transformation function, not a filtering function in the traditional sense.🔎 Proposed rename to clarify intent
-export const filter = (filterConditionFn: (row: Record<string, any>) => Record<string, any>) => ( +export const transform = (transformFn: (row: Record<string, any>) => Record<string, any>) => ( data: Array<Record<string, any>> ): Array<Record<string, any>> => { - return data.map(filterConditionFn); + return data.map(transformFn); };If keeping the name
filter, at minimum update the documentation:/** - * filter data - * @param filterConditionFn - * @returns Function that filter data with fields + * Transform data rows (Note: uses map, not filter - preserves array length) + * @param filterConditionFn Transformation function applied to each row + * @returns Function that transforms data with fields *
23-36: Consider adding generic type parameters for better type safety.Both
filterandselectColumnsuseRecord<string, any>, which loses type information and prevents TypeScript from catching type-related errors. While this provides flexibility for generic data transformation, it reduces type safety.Consider adding generic type parameters to preserve type information:
🔎 Proposed type safety improvement
-export const filter = (filterConditionFn: (row: Record<string, any>) => Record<string, any>) => ( - data: Array<Record<string, any>> -): Array<Record<string, any>> => { +export const filter = <T extends Record<string, any>>( + filterConditionFn: (row: T) => T | null +) => ( + data: T[] +): T[] => { return data.map(filterConditionFn); }; -export const selectColumns = (columns: string[]) => (row: Record<string, any>) => { - const result: Record<string, any> = {}; +export const selectColumns = <T extends Record<string, any>>( + columns: (keyof T)[] +) => (row: T): Partial<T> => { + const result: Partial<T> = {}; columns.forEach((col) => { if (col in row) result[col] = row[col]; }); return result; };This would allow TypeScript to infer and check types at call sites while maintaining the flexible, generic nature of the utilities.
17-20: Clarify row filtering documentation.The row filtering example could better explain why returning
nulland chaining.filter(Boolean)is necessary. The current comment "Remove null entries" is present but could be more prominent given this is a non-obvious pattern.🔎 Suggested documentation improvement
* 2. Filter rows - keep rows that meet a condition: - * For row filtering, combine with filter(Boolean) to remove null entries + * Note: This function uses map(), so it preserves array length. + * Return null for rows to exclude, then chain .filter(Boolean) to remove them. * const rowFilter = filter((row) => row.category ==='A' ? row : null); - * const adults = rowFilter(data).filter(Boolean); // Remove null entries + * const adults = rowFilter(data).filter(Boolean); // Required: removes null entriessrc/plugins/explore/public/components/visualizations/state_timeline/state_timeline_config.ts (1)
49-50: Clarify if this feature is truly temporary.The comment states "Temporarily add this feature," but the PR description presents this as a core feature enabling users to switch between No style, Use Value Mapping Color, and Use Threshold Color. If this is intended to be permanent, the comment should be updated to avoid confusion. If it's truly temporary, consider adding a TODO or tracking issue.
🔎 Suggested comment update if permanent
- // Temporarily add this feature to make a switch between No style, Use Value Mapping Color, Use Threshold Color + // Enable switching between No style, Use Value Mapping Color, and Use Threshold Color useValueMappingColor?: boolean;src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_exclusive_vis_options.tsx (1)
92-104: Consider consistent label casing.The label "Use valueMapping color" uses camelCase for "valueMapping", while the existing label uses "Use threshold color" with lowercase. Consider using consistent casing for better UI consistency:
🔎 Suggested label
- defaultMessage: 'Use valueMapping color', + defaultMessage: 'Use value mapping color',src/plugins/explore/public/components/visualizations/state_timeline/to_expression.ts (1)
129-136: Duplicate asset extraction logic.Lines 133-136 duplicate the
valueMappingsextraction thatprepareAssetsalready provides. Consider usingprepareAssetsfor the Vega-Lite path as well for consistency:🔎 Proposed refactor
- const rangeMappings = styleOptions?.valueMappingOptions?.valueMappings?.filter( - (mapping) => mapping?.type === 'range' - ); - - const valueMappings = styleOptions?.valueMappingOptions?.valueMappings?.filter( - (mapping) => mapping?.type === 'value' - ); - - const disconnectThreshold = - styleOptions?.exclusive?.disconnectValues?.disableMode === DisableMode.Threshold - ? styleOptions?.exclusive?.disconnectValues?.threshold || '1h' - : undefined; - - const connectThreshold = - styleOptions?.exclusive?.connectNullValues?.connectMode === DisableMode.Threshold - ? styleOptions?.exclusive?.connectNullValues?.threshold || '1h' - : undefined; + const { valueMappings, rangeMappings, disconnectThreshold, connectThreshold } = prepareAssets( + styleOptions + );
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
changelogs/fragments/11136.ymlsrc/plugins/explore/public/components/visualizations/bar/bar_chart_utils.tssrc/plugins/explore/public/components/visualizations/line/to_expression.tssrc/plugins/explore/public/components/visualizations/rule_repository.tssrc/plugins/explore/public/components/visualizations/state_timeline/state_timeline_config.tssrc/plugins/explore/public/components/visualizations/state_timeline/state_timeline_exclusive_vis_options.tsxsrc/plugins/explore/public/components/visualizations/state_timeline/state_timeline_utils.test.tssrc/plugins/explore/public/components/visualizations/state_timeline/state_timeline_utils.tssrc/plugins/explore/public/components/visualizations/state_timeline/state_timeline_vis_options.tsxsrc/plugins/explore/public/components/visualizations/state_timeline/to_expression.tssrc/plugins/explore/public/components/visualizations/utils/data_transformation/filter.tssrc/plugins/explore/public/components/visualizations/utils/data_transformation/index.tssrc/plugins/explore/public/components/visualizations/utils/echarts_spec.ts
💤 Files with no reviewable changes (1)
- src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_utils.test.ts
🧰 Additional context used
🧬 Code graph analysis (5)
src/plugins/explore/public/components/visualizations/bar/bar_chart_utils.ts (1)
src/plugins/explore/public/components/visualizations/utils/echarts_spec.ts (1)
EChartsSpecState(72-84)
src/plugins/explore/public/components/visualizations/rule_repository.ts (2)
src/plugins/explore/public/components/visualizations/state_timeline/to_expression.ts (1)
createSingleNumericalStateTimeline(752-996)src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_config.ts (1)
StateTimeLineChartStyle(53-53)
src/plugins/explore/public/components/visualizations/utils/data_transformation/filter.ts (1)
src/plugins/explore/public/components/visualizations/utils/data_transformation/index.ts (2)
filter(20-20)selectColumns(20-20)
src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_utils.ts (5)
src/plugins/explore/public/components/visualizations/types.ts (1)
ValueMapping(310-317)src/plugins/explore/public/components/visualizations/utils/data_transformation/index.ts (1)
TransformFn(13-13)src/plugins/explore/public/components/visualizations/theme/color_utils.ts (1)
resolveColor(77-91)src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_config.ts (1)
StateTimeLineChartStyle(53-53)src/plugins/explore/public/components/visualizations/utils/echarts_spec.ts (2)
BaseChartStyle(30-47)PipelineFn(89-91)
src/plugins/explore/public/components/visualizations/state_timeline/to_expression.ts (9)
src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_config.ts (1)
StateTimeLineChartStyle(53-53)src/plugins/explore/public/components/visualizations/utils/utils.ts (2)
getChartRender(449-456)applyAxisStyling(22-89)src/plugins/explore/public/components/visualizations/types.ts (4)
Threshold(215-218)VisColumn(62-69)AxisColumnMappings(32-32)VEGASCHEMA(276-276)src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_utils.ts (4)
convertThresholdsToValueMappings(107-116)mergeDataCore(122-210)groupByMergedLabel(212-225)createStateTimeLineSpec(476-532)src/plugins/explore/public/components/visualizations/utils/echarts_spec.ts (5)
pipe(96-100)createBaseConfig(122-146)buildAxisConfigs(151-193)assembleSpec(198-253)applyAxisStyling(262-316)src/plugins/explore/public/components/visualizations/utils/data_transformation/index.ts (5)
transform(12-12)filter(20-20)selectColumns(20-20)sortByTime(11-11)convertTo2DArray(10-10)src/plugins/explore/public/components/visualizations/utils/data_transformation/filter.ts (2)
filter(23-27)selectColumns(30-36)src/plugins/explore/public/components/visualizations/theme/color_utils.ts (2)
resolveColor(77-91)getCategoryNextColor(72-74)src/plugins/explore/public/components/visualizations/constants.ts (1)
DEFAULT_OPACITY(68-68)
⏰ 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 Linux (ciGroup5)
- GitHub Check: Run functional tests on Windows (ciGroup11)
- GitHub Check: Run functional tests on Linux (ciGroup10)
- GitHub Check: Run functional tests on Windows (ciGroup13)
- GitHub Check: Run functional tests on Windows (ciGroup4)
- GitHub Check: Run functional tests on Windows (ciGroup6)
- GitHub Check: Run functional tests on Windows (ciGroup10)
- GitHub Check: Run functional tests on Windows (ciGroup1)
- GitHub Check: Run functional tests on Windows (ciGroup3)
- GitHub Check: Run functional tests on Linux (ciGroup11)
- GitHub Check: Run cypress tests (osd:ciGroup2)
- GitHub Check: Run functional tests on Linux (ciGroup7)
- GitHub Check: Run functional tests on Linux (ciGroup1)
- GitHub Check: Run functional tests on Windows (ciGroup8)
- GitHub Check: Run functional tests on Linux (ciGroup9)
- GitHub Check: Run functional tests on Windows (ciGroup9)
- GitHub Check: Run functional tests on Linux (ciGroup12)
- GitHub Check: Run functional tests on Windows (ciGroup5)
- GitHub Check: Run functional tests on Windows (ciGroup2)
- GitHub Check: Run functional tests on Windows (ciGroup7)
- GitHub Check: Run functional tests on Linux (ciGroup8)
- GitHub Check: Run cypress tests (osd:ciGroup15Explore)
- GitHub Check: Run cypress tests (osd:ciGroup16Explore)
- GitHub Check: Run functional tests on Linux (ciGroup6)
- GitHub Check: Run functional tests on Windows (ciGroup12)
- GitHub Check: Run functional tests on Linux (ciGroup13)
- GitHub Check: Run cypress tests (osd:ciGroup7)
- GitHub Check: Run functional tests on Linux (ciGroup4)
- GitHub Check: Run cypress tests (osd:ciGroup9)
- GitHub Check: Run functional tests on Linux (ciGroup3)
- GitHub Check: Run cypress tests (osd:ciGroup17Explore)
- GitHub Check: Run cypress tests (osd:ciGroup15)
- GitHub Check: Run cypress tests (osd:ciGroup13Explore)
- GitHub Check: Run functional tests on Linux (ciGroup2)
- GitHub Check: Run cypress tests (osd:ciGroup14Explore)
- GitHub Check: Run cypress tests (osd:ciGroup13)
- GitHub Check: Run cypress tests (osd:ciGroup6)
- GitHub Check: Run cypress tests (osd:ciGroup10Fast)
- GitHub Check: Run cypress tests (osd:ciGroup11)
- GitHub Check: Run cypress tests (osd:ciGroup10Slow)
- GitHub Check: Run cypress tests (osd:ciGroup12)
- GitHub Check: Run cypress tests (osd:ciGroup1)
- GitHub Check: Run cypress tests (osd:ciGroup14)
- GitHub Check: Run cypress tests (osd:ciGroup12Explore)
- GitHub Check: Run cypress tests (osd:ciGroup8)
- GitHub Check: Run cypress tests (osd:ciGroup10Explore)
- GitHub Check: Run cypress tests (osd:ciGroup4)
- GitHub Check: Run cypress tests (osd:ciGroup3)
- GitHub Check: Run cypress tests (osd:ciGroup5)
- GitHub Check: Build and Verify on Windows (ciGroup3)
- GitHub Check: Build and Verify on Linux (ciGroup3)
- GitHub Check: Build and Verify on Linux (ciGroup2)
- GitHub Check: Build and Verify on Windows (ciGroup2)
- GitHub Check: Build and Verify on Windows (ciGroup1)
- GitHub Check: Build min release artifacts on macOS ARM64
- GitHub Check: Build and Verify on Linux (ciGroup4)
- GitHub Check: Build and Verify on Windows (ciGroup4)
- GitHub Check: Build min release artifacts on Windows x64
- GitHub Check: Build and Verify on Linux (ciGroup1)
- GitHub Check: Build min release artifacts on Linux ARM64
- GitHub Check: Build min release artifacts on macOS x64
- GitHub Check: Lint and validate
- GitHub Check: Build min release artifacts on Linux x64
- GitHub Check: Run plugin functional tests on Linux
- GitHub Check: Run plugin functional tests on Windows
- GitHub Check: lighthouse
- GitHub Check: bundle-analyzer
🔇 Additional comments (20)
src/plugins/explore/public/components/visualizations/line/to_expression.ts (1)
226-226: Good improvement to error message clarity.The updated error message now correctly identifies the chart type as "line-bar chart," matching the function context. This improves debuggability when required fields are missing.
src/plugins/explore/public/components/visualizations/utils/data_transformation/index.ts (1)
19-20: LGTM! Clean re-export of new utilities.The re-export follows the existing module pattern and properly exposes the new filter utilities through the public API.
changelogs/fragments/11136.yml (1)
1-2: LGTM! Changelog entry is properly formatted.The changelog fragment correctly documents the state timeline migration feature with appropriate PR reference.
src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_vis_options.tsx (1)
88-91: LGTM! Props follow established pattern.The new
useValueMappingColorprops are properly wired up and follow the same pattern as the existinguseThresholdColoroption (lines 83-87). The implementation correctly propagates the option through the style panel.src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_config.ts (1)
134-137: Numerical COLOR axis integration verified.The state timeline implementation correctly supports numerical COLOR axes with both value and range mappings. Value mappings are prioritized when both are configured (they're evaluated first in the merged mappings array), and unmatched values fall back to light grey as expected.
src/plugins/explore/public/components/visualizations/rule_repository.ts (1)
51-58: LGTM!The import of
createSingleNumericalStateTimelineis correctly added alongside the other state timeline factory functions.src/plugins/explore/public/components/visualizations/bar/bar_chart_utils.ts (2)
23-23: LGTM!The import of
EChartsSpecStateis correctly added to support the type casting in the facet handling logic.
264-272: Good defensive handling for non-faceted data.This guard correctly handles the case where
createFacetBarSeriesreceives non-multi-series data by delegating tocreateBarSeries. This prevents potential runtime errors when the data structure doesn't match the expected faceted format.src/plugins/explore/public/components/visualizations/utils/echarts_spec.ts (3)
154-156: LGTM!The facet detection logic is now more precise by requiring both the 2D array structure and
axisColumnMappings.facetto be defined. This correctly distinguishes between multi-dataset scenarios (like state-timeline) and true faceted visualizations.
201-227: Good separation of multi-dataset and facet logic.The distinction between
hasMultiDatasetsandhasFacetcorrectly handles the state-timeline case where data may be structured as multiple datasets without being a faceted visualization. The grid layout adjustments are appropriate.
271-271: LGTM!Adding
axisLine: { show: true }as a default is a reasonable enhancement for axis visibility.src/plugins/explore/public/components/visualizations/state_timeline/to_expression.ts (3)
34-54: Good refactor: centralized asset preparation.The
prepareAssetshelper function effectively DRYs up the repeated logic for extracting value mappings, range mappings, and thresholds across the different timeline creation functions.
64-119: LGTM: ECharts rendering path.The echarts branch correctly implements the pipe-based composition pattern with proper data transformation, configuration building, and spec assembly.
752-996: New exportcreateSingleNumericalStateTimelinelooks correct.The new function follows the same dual-path pattern (echarts/vega-lite) as the other timeline creation functions and correctly handles value mappings and range mappings for numerical data.
src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_utils.ts (6)
7-11: LGTM!The new imports for
ValueMapping,StateTimeLineChartStyle,BaseChartStyle,PipelineFn,resolveColor, andTransformFnare correctly added to support the new functionality.
76-105: Good enhancement: color-aware merge records.The
mergeRecordsWithColorfunction now correctly handles bothValueMappingobjects and plain strings, computing appropriate labels and extracting color/displayText from mappings.
122-210: Well-structured core merging logic.The
mergeDataCorefunction provides a clean, configurable merging pipeline that handles both grouped and ungrouped data with support for value and range mappings.
212-225: LGTM: groupByMergedLabel utility.This utility correctly groups data by
mergedLabeland applies the provided transformation function to each group.
382-384: LGTM: decideSeriesStyle.The function correctly returns 'lightgrey' for unmatched labels and resolves the color for matched mappings.
476-532: LGTM: createStateTimeLineSpec pipeline function.The function correctly builds the ECharts custom series specification for state timelines, handling both grouped and single-row scenarios with appropriate axis configuration adjustments.
| case 'state_timeline': | ||
| return createSingleNumericalStateTimeline( | ||
| transformedData, | ||
| numericalColumns, | ||
| categoricalColumns, | ||
| dateColumns, | ||
| styleOptions as StateTimeLineChartStyle, | ||
| axisColumnMappings | ||
| ); |
There was a problem hiding this comment.
Missing state_timeline in chartTypes array.
The state_timeline case is added to toSpec, but CHART_METADATA.state_timeline is not included in the chartTypes array (lines 103-108). This means the state_timeline visualization won't appear as a suggested chart type for the "1 Metric & 1 Date" rule, even though the code path exists.
If this is intentional (e.g., the chart type should only be available via explicit selection), please confirm. Otherwise, consider adding it:
🔎 Proposed fix
chartTypes: [
{ ...CHART_METADATA.line, priority: 100 },
{ ...CHART_METADATA.area, priority: 80 },
{ ...CHART_METADATA.bar, priority: 60 },
{ ...CHART_METADATA.metric, priority: 40 },
+ { ...CHART_METADATA.state_timeline, priority: 20 },
],Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @src/plugins/explore/public/components/visualizations/rule_repository.ts
around lines 157 - 165, The new 'state_timeline' branch added to toSpec
references createSingleNumericalStateTimeline but CHART_METADATA.state_timeline
is not included in the chartTypes array used for the "1 Metric & 1 Date" rule;
add CHART_METADATA.state_timeline to the chartTypes array (near where chartTypes
is defined/returned for that rule) so the state_timeline visualization is
suggested, or if omission was intentional, add a clear comment in the chartTypes
definition explaining why state_timeline is excluded; ensure you reference the
same CHART_METADATA key name 'state_timeline' so it matches the toSpec switch
and createSingleNumericalStateTimeline path.
...ore/public/components/visualizations/state_timeline/state_timeline_exclusive_vis_options.tsx
Outdated
Show resolved
Hide resolved
src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_utils.ts
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #11136 +/- ##
==========================================
- Coverage 60.45% 60.32% -0.14%
==========================================
Files 4603 4618 +15
Lines 126516 127254 +738
Branches 21218 21394 +176
==========================================
+ Hits 76491 76769 +278
- Misses 44594 45047 +453
- Partials 5431 5438 +7
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:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_utils.ts:
- Around line 86-93: The label construction for the variable `label` uses
`valueMapping.value` and `valueMapping.range?.min/max` which are optional per
the `ValueMapping` interface and can yield undefined; update the ternary
branches that handle `valueMapping?.type === 'value'` and `valueMapping?.type
=== 'range'` to defensively coerce undefined to a stable string (e.g., use
String(valueMapping?.value ?? '_unmatched_')) and to render range endpoints with
fallbacks (e.g., min => String(valueMapping?.range?.min ?? '_'), max =>
valueMapping?.range?.max ?? '∞') so `label` never becomes undefined and retains
the existing fallback `_unmatched_` when appropriate.
🧹 Nitpick comments (3)
src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_utils.ts (3)
362-362: Minor grammar issue in comment.The comment reads "last null cannot be connect" but should be "last null cannot be connected".
🔎 Proposed fix
- storeState.buffer.pop(); // last null cannot be connect, pop that to ensure correct length + storeState.buffer.pop(); // last null cannot be connected, pop it to ensure correct length
456-456: TODO: Style display text.There's a TODO comment indicating that display text styling needs attention.
Would you like me to generate a solution for styling the display text, or should this be tracked in a separate issue?
386-474: Consider reducing duplication between render functions.The
renderSingleStateTimeLineItemandrenderItemfunctions share significant logic for rendering rectangles and text. The primary difference is how the y-coordinate is determined.🔎 Possible refactor approach
Consider extracting a shared helper function that accepts a y-coordinate calculator:
const createRenderItem = ( styles: StateTimeLineChartStyle, getY: (api: any, groupField?: string) => number, displayText?: string ) => (params: any, api: any) => { const y = getY(api); const start = api.coord([+new Date(api.value('start')), y]); const end = api.coord([+new Date(api.value('end')), y]); const height = api.size([0, 1])[1] * (styles.exclusive.rowHeight ?? 0.8); // ... rest of shared rendering logic };This would eliminate the duplication while maintaining clarity.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_exclusive_vis_options.tsxsrc/plugins/explore/public/components/visualizations/state_timeline/state_timeline_utils.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_utils.ts (5)
src/plugins/explore/public/components/visualizations/types.ts (1)
ValueMapping(310-317)src/plugins/explore/public/components/visualizations/utils/data_transformation/index.ts (1)
TransformFn(13-13)src/plugins/explore/public/components/visualizations/theme/color_utils.ts (1)
resolveColor(77-91)src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_config.ts (1)
StateTimeLineChartStyle(53-53)src/plugins/explore/public/components/visualizations/utils/echarts_spec.ts (2)
BaseChartStyle(30-47)PipelineFn(89-91)
⏰ 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). (68)
- GitHub Check: Run functional tests on Windows (ciGroup5)
- GitHub Check: Run functional tests on Windows (ciGroup6)
- GitHub Check: Run functional tests on Windows (ciGroup13)
- GitHub Check: Run functional tests on Linux (ciGroup7)
- GitHub Check: Run functional tests on Windows (ciGroup8)
- GitHub Check: Run functional tests on Linux (ciGroup6)
- GitHub Check: Run functional tests on Linux (ciGroup13)
- GitHub Check: Run functional tests on Linux (ciGroup9)
- GitHub Check: Run functional tests on Windows (ciGroup3)
- GitHub Check: Run functional tests on Linux (ciGroup2)
- GitHub Check: Run functional tests on Linux (ciGroup8)
- GitHub Check: Run functional tests on Windows (ciGroup9)
- GitHub Check: Run functional tests on Windows (ciGroup10)
- GitHub Check: Run functional tests on Linux (ciGroup10)
- GitHub Check: Run functional tests on Windows (ciGroup12)
- GitHub Check: Run functional tests on Linux (ciGroup5)
- GitHub Check: Run functional tests on Linux (ciGroup11)
- GitHub Check: Run functional tests on Windows (ciGroup4)
- GitHub Check: Run functional tests on Windows (ciGroup1)
- GitHub Check: Run functional tests on Linux (ciGroup12)
- GitHub Check: Run functional tests on Linux (ciGroup3)
- GitHub Check: Run functional tests on Windows (ciGroup11)
- GitHub Check: Run functional tests on Linux (ciGroup4)
- GitHub Check: Run functional tests on Windows (ciGroup7)
- GitHub Check: Run functional tests on Windows (ciGroup2)
- GitHub Check: Run functional tests on Linux (ciGroup1)
- GitHub Check: Run cypress tests (osd:ciGroup10Fast)
- GitHub Check: Run cypress tests (osd:ciGroup12Explore)
- GitHub Check: Run cypress tests (osd:ciGroup16Explore)
- GitHub Check: Run cypress tests (osd:ciGroup13Explore)
- GitHub Check: Build and Verify on Windows (ciGroup4)
- GitHub Check: Run cypress tests (osd:ciGroup15Explore)
- GitHub Check: Build and Verify on Windows (ciGroup1)
- GitHub Check: Run cypress tests (osd:ciGroup4)
- GitHub Check: Build and Verify on Windows (ciGroup3)
- GitHub Check: Run cypress tests (osd:ciGroup13)
- GitHub Check: Run cypress tests (osd:ciGroup17Explore)
- GitHub Check: Run cypress tests (osd:ciGroup12)
- GitHub Check: Run cypress tests (osd:ciGroup6)
- GitHub Check: Build and Verify on Windows (ciGroup2)
- GitHub Check: Run cypress tests (osd:ciGroup14Explore)
- GitHub Check: Run cypress tests (osd:ciGroup10Explore)
- GitHub Check: Build and Verify on Linux (ciGroup3)
- GitHub Check: Run cypress tests (osd:ciGroup11)
- GitHub Check: Run cypress tests (osd:ciGroup8)
- GitHub Check: Run cypress tests (osd:ciGroup5)
- GitHub Check: Build min release artifacts on Linux ARM64
- GitHub Check: Build and Verify on Linux (ciGroup1)
- GitHub Check: Build min release artifacts on Linux x64
- GitHub Check: Run cypress tests (osd:ciGroup9)
- GitHub Check: Run cypress tests (osd:ciGroup14)
- GitHub Check: Build and Verify on Linux (ciGroup4)
- GitHub Check: Run cypress tests (osd:ciGroup3)
- GitHub Check: Run cypress tests (osd:ciGroup1)
- GitHub Check: Build min release artifacts on macOS ARM64
- GitHub Check: Run cypress tests (osd:ciGroup2)
- GitHub Check: Run cypress tests (osd:ciGroup7)
- GitHub Check: Run cypress tests (osd:ciGroup15)
- GitHub Check: Build min release artifacts on macOS x64
- GitHub Check: Build and Verify on Linux (ciGroup2)
- GitHub Check: Build min release artifacts on Windows x64
- GitHub Check: lighthouse
- GitHub Check: Run cypress tests (osd:ciGroup10Slow)
- GitHub Check: Run plugin functional tests on Linux
- GitHub Check: Lint and validate
- GitHub Check: Run plugin functional tests on Windows
- GitHub Check: bundle-analyzer
- GitHub Check: linkchecker
🔇 Additional comments (6)
src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_exclusive_vis_options.tsx (2)
19-20: LGTM! Parameter naming issue resolved.The callback parameter is now correctly named
useValueMappingColor, matching the prop name and avoiding the previous misleading reference touseThresholdColor.
92-104: Note: Temporary toggle implementation.The comment indicates this toggle is a temporary solution for switching between styling modes. Consider tracking this with a TODO or issue if a more permanent solution is planned.
src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_utils.ts (4)
493-497: LGTM! DisplayText assignment corrected.The redundant nullish coalescing has been removed. The assignment now correctly reads the displayText value without unnecessary fallback logic.
107-116: LGTM! Improved threshold conversion.Using
undefinedinstead ofInfinityfor the final range's max value is more consistent with theValueMappinginterface and cleaner for handling unbounded ranges.
122-210: Well-structured core merging function.The
mergeDataCorefunction centralizes the merging logic with clear separation between grouped and non-grouped paths. The guard clause at line 143 and finalization logic at lines 197-206 properly handle edge cases.
476-532: Well-designed spec creation function.The
createStateTimeLineSpecfunction properly handles both grouped and non-grouped timelines, with appropriate y-axis adjustments and series creation. The pipeline function approach integrates well with the ECharts architecture.
src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_utils.ts
Show resolved
Hide resolved
Signed-off-by: Qxisylolo <qianxisy@amazon.com>
Signed-off-by: Qxisylolo <qianxisy@amazon.com>
Qxisylolo
force-pushed
the
feat/echart_state_timeline
branch
from
4af387f to
6247662
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@src/plugins/explore/public/components/visualizations/state_timeline/to_expression.ts:
- Around line 993-1003: The Vega spec for createSingleNumericalStateTimeline
references the fakeYAxis field in encodings but never injects the corresponding
fakeYAxis transform into the transforms array; add the same fakeYAxis transform
used in createSingleCategoricalStateTimeline to the transformLayer (or include
it directly in baseSpec.transform) so the data pipeline defines fakeYAxis before
layer encoding runs, ensuring transformLayer (used to build baseSpec.transform)
contains the fakeYAxis transform.
🧹 Nitpick comments (5)
src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_exclusive_vis_options.tsx (1)
92-104: Minor label casing inconsistency.The label "Use valueMapping color" uses camelCase (
valueMapping), while the other toggle uses "Use threshold color" (lowercase). Consider using consistent casing like "Use value mapping color" for UI consistency.Also, the comment on line 92 indicates this is a temporary toggle. Consider adding a TODO with a tracking issue if this is meant to be replaced with a different UI pattern (e.g., a single selector with three options).
Suggested label fix
label={i18n.translate('explore.vis.statetimeline.useValueMappingColor', { - defaultMessage: 'Use valueMapping color', + defaultMessage: 'Use value mapping color', })}src/plugins/explore/public/components/visualizations/state_timeline/to_expression.ts (2)
64-121: Verify consistent error handling and field validation.The echarts branch throws an error if required fields are missing (line 74-75), which is good. However, the Vega branch below (lines 123 onwards) doesn't have equivalent validation and could produce incorrect specs if fields are undefined.
Consider adding similar validation to the Vega branch for consistency, or document why the difference exists.
320-323:rangeMappingsnot used in echarts branch for categorical timeline.In
createCategoricalStateTimeline,prepareAssetsreturnsrangeMappingsbut it's not destructured or used in the echarts branch (line 323). This is fine since categorical timelines use value mappings, but the unused destructuring could be cleaned up.- const { valueMappings, disconnectThreshold, connectThreshold } = prepareAssets(styleOptions); + const { valueMappings, rangeMappings: _unused, disconnectThreshold, connectThreshold } = prepareAssets(styleOptions);Or simply ignore
rangeMappingsas currently done - no functional issue.src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_utils.ts (2)
147-161: Potential issue with object identity comparison.The
findMatchfunction returns aValueMappingobject when a match is found. Later, on line 371, the comparisonstoreState.currentValue === currentMappinguses reference equality. If the same logical mapping (e.g., same range) could return different object references across calls, records that should be merged together might be split.Since
mergedMappingsis created once permergeDataCorecall andfindMatchreturns references from this stable array, this should work correctly. However, consider adding a comment clarifying this design decision.
482-486: State mutation detected.Line 486 directly mutates
baseConfig.tooltip.triggerby deleting it. While this may work in the current pipeline structure, mutating shared state can lead to subtle bugs if the same config object is reused.Consider creating a new object instead:
Suggested immutable approach
- delete baseConfig.tooltip.trigger; + const newTooltip = { ...baseConfig.tooltip }; + delete newTooltip.trigger; + const newBaseConfig = { ...baseConfig, tooltip: newTooltip };Then use
newBaseConfigin the state update.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
changelogs/fragments/11136.ymlsrc/plugins/explore/public/components/visualizations/bar/bar_chart_utils.tssrc/plugins/explore/public/components/visualizations/line/to_expression.tssrc/plugins/explore/public/components/visualizations/rule_repository.tssrc/plugins/explore/public/components/visualizations/state_timeline/state_timeline_config.tssrc/plugins/explore/public/components/visualizations/state_timeline/state_timeline_exclusive_vis_options.tsxsrc/plugins/explore/public/components/visualizations/state_timeline/state_timeline_utils.test.tssrc/plugins/explore/public/components/visualizations/state_timeline/state_timeline_utils.tssrc/plugins/explore/public/components/visualizations/state_timeline/state_timeline_vis_options.tsxsrc/plugins/explore/public/components/visualizations/state_timeline/to_expression.test.tssrc/plugins/explore/public/components/visualizations/state_timeline/to_expression.tssrc/plugins/explore/public/components/visualizations/utils/data_transformation/filter.tssrc/plugins/explore/public/components/visualizations/utils/data_transformation/index.tssrc/plugins/explore/public/components/visualizations/utils/echarts_spec.ts
💤 Files with no reviewable changes (1)
- src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_utils.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- changelogs/fragments/11136.yml
- src/plugins/explore/public/components/visualizations/utils/data_transformation/filter.ts
- src/plugins/explore/public/components/visualizations/line/to_expression.ts
- src/plugins/explore/public/components/visualizations/bar/bar_chart_utils.ts
- src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_config.ts
- src/plugins/explore/public/components/visualizations/utils/echarts_spec.ts
🧰 Additional context used
🧬 Code graph analysis (3)
src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_utils.ts (4)
src/plugins/explore/public/components/visualizations/types.ts (1)
ValueMapping(310-317)src/plugins/explore/public/components/visualizations/theme/color_utils.ts (1)
resolveColor(77-91)src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_config.ts (1)
StateTimeLineChartStyle(53-53)src/plugins/explore/public/components/visualizations/utils/echarts_spec.ts (2)
BaseChartStyle(31-49)PipelineFn(91-93)
src/plugins/explore/public/components/visualizations/rule_repository.ts (2)
src/plugins/explore/public/components/visualizations/state_timeline/to_expression.ts (1)
createSingleNumericalStateTimeline(758-1004)src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_config.ts (1)
StateTimeLineChartStyle(53-53)
src/plugins/explore/public/components/visualizations/state_timeline/to_expression.test.ts (1)
src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_config.ts (1)
defaultStateTimeLineChartStyles(55-108)
⏰ 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 (ciGroup10)
- GitHub Check: Run functional tests on Windows (ciGroup11)
- GitHub Check: Run functional tests on Windows (ciGroup12)
- GitHub Check: Run functional tests on Windows (ciGroup2)
- GitHub Check: Run functional tests on Windows (ciGroup13)
- GitHub Check: Run functional tests on Windows (ciGroup8)
- GitHub Check: Run functional tests on Windows (ciGroup7)
- GitHub Check: Run functional tests on Windows (ciGroup9)
- GitHub Check: Run functional tests on Windows (ciGroup6)
- GitHub Check: Run functional tests on Windows (ciGroup5)
- GitHub Check: Run functional tests on Linux (ciGroup1)
- GitHub Check: Run functional tests on Windows (ciGroup4)
- GitHub Check: Run functional tests on Linux (ciGroup5)
- GitHub Check: Run functional tests on Linux (ciGroup10)
- GitHub Check: Run functional tests on Linux (ciGroup13)
- GitHub Check: Run functional tests on Linux (ciGroup8)
- GitHub Check: Run functional tests on Linux (ciGroup4)
- GitHub Check: Run functional tests on Linux (ciGroup6)
- GitHub Check: Run functional tests on Windows (ciGroup1)
- GitHub Check: Run functional tests on Linux (ciGroup12)
- GitHub Check: Run functional tests on Windows (ciGroup3)
- GitHub Check: Run functional tests on Linux (ciGroup2)
- GitHub Check: Run functional tests on Linux (ciGroup9)
- GitHub Check: Run functional tests on Linux (ciGroup11)
- GitHub Check: Run functional tests on Linux (ciGroup7)
- GitHub Check: Run functional tests on Linux (ciGroup3)
- GitHub Check: Run cypress tests (osd:ciGroup16Explore)
- GitHub Check: Build and Verify on Windows (ciGroup4)
- GitHub Check: Run cypress tests (osd:ciGroup13)
- GitHub Check: Run cypress tests (osd:ciGroup14)
- GitHub Check: Build and Verify on Linux (ciGroup3)
- GitHub Check: Build and Verify on Windows (ciGroup3)
- GitHub Check: Build and Verify on Linux (ciGroup1)
- GitHub Check: Build and Verify on Windows (ciGroup1)
- GitHub Check: Build min release artifacts on macOS x64
- GitHub Check: Build and Verify on Linux (ciGroup2)
- GitHub Check: Run cypress tests (osd:ciGroup6)
- GitHub Check: Run cypress tests (osd:ciGroup15)
- GitHub Check: Run cypress tests (osd:ciGroup14Explore)
- GitHub Check: Run cypress tests (osd:ciGroup15Explore)
- GitHub Check: Build and Verify on Windows (ciGroup2)
- GitHub Check: Build and Verify on Linux (ciGroup4)
- GitHub Check: Run cypress tests (osd:ciGroup3)
- GitHub Check: Run cypress tests (osd:ciGroup10Explore)
- GitHub Check: Run cypress tests (osd:ciGroup12Explore)
- GitHub Check: Run cypress tests (osd:ciGroup12)
- GitHub Check: Run cypress tests (osd:ciGroup17Explore)
- GitHub Check: Run cypress tests (osd:ciGroup8)
- GitHub Check: Run cypress tests (osd:ciGroup13Explore)
- GitHub Check: Run cypress tests (osd:ciGroup9)
- GitHub Check: Run cypress tests (osd:ciGroup5)
- GitHub Check: Run cypress tests (osd:ciGroup10Fast)
- GitHub Check: Run cypress tests (osd:ciGroup7)
- GitHub Check: Run cypress tests (osd:ciGroup4)
- GitHub Check: Run cypress tests (osd:ciGroup11)
- GitHub Check: Build min release artifacts on Windows x64
- GitHub Check: Run cypress tests (osd:ciGroup10Slow)
- GitHub Check: Run cypress tests (osd:ciGroup2)
- GitHub Check: Run cypress tests (osd:ciGroup1)
- GitHub Check: Build min release artifacts on Linux ARM64
- GitHub Check: Build min release artifacts on Linux x64
- GitHub Check: Build min release artifacts on macOS ARM64
- GitHub Check: Lint and validate
- GitHub Check: Run plugin functional tests on Windows
- GitHub Check: Run plugin functional tests on Linux
- GitHub Check: bundle-analyzer
- GitHub Check: lighthouse
🔇 Additional comments (13)
src/plugins/explore/public/components/visualizations/utils/data_transformation/index.ts (1)
19-20: LGTM!The new exports for
filterandselectColumnsfollow the established pattern in this barrel file and cleanly expose the new data transformation utilities.src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_vis_options.tsx (1)
88-91: LGTM!The new
useValueMappingColorandonUseValueMappingColorChangeprops are correctly wired, following the same pattern as the existinguseThresholdColorprops.src/plugins/explore/public/components/visualizations/rule_repository.ts (1)
157-165: Verify ifstate_timelineshould be added tochartTypesarray.The
state_timelinecase is handled in the switch statement, butstate_timelineis not listed in thechartTypesarray foroneMetricOneDateRule(lines 103-108). This means users won't be able to select state_timeline for this rule through the UI, even though the code path exists.If this is intentional (e.g., state_timeline is only accessible programmatically or through another mechanism), this is fine. Otherwise, consider adding it to the
chartTypesarray.src/plugins/explore/public/components/visualizations/state_timeline/to_expression.test.ts (3)
33-41: LGTM!The mock for
mergeDataCorecorrectly returns data with the newmergedLabelfield structure, aligning with the refactored implementation.
92-92: LGTM!Setting
useValueMappingColor: trueas the default in test options aligns with the test assertions that expectmergedLabelto be used for color encoding.
150-186: Good test coverage for fallback behavior.This test correctly verifies that when
useValueMappingColorisfalse, the color field falls back to the raw value field (v1) instead ofmergedLabel.src/plugins/explore/public/components/visualizations/state_timeline/to_expression.ts (3)
34-54: Good refactor to extract common logic.The
prepareAssetshelper consolidates the repeated pattern of extracting value mappings, range mappings, and threshold configurations from style options. This reduces duplication across the timeline creation functions.
267-286: Condition logic limits text layer display.The text layer is now suppressed when
useThresholdColoris true (line 268), even ifshowValuesis enabled. This seems intentional (threshold colors may not have meaningful display text), but verify this matches the expected UX.
758-821: NewcreateSingleNumericalStateTimelinefunction follows established patterns.The echarts branch correctly implements the pipe-based composition and follows the same structure as other timeline creation functions. The function is properly exported and integrated with
rule_repository.ts.src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_utils.ts (4)
122-143: Well-structured unified merge function.The
mergeDataCorefunction correctly consolidates the merging logic with configurable mappings and thresholds. The curried pattern allows it to be used in pipe compositions.One consideration: the early return on line 143 returns the original data unchanged, which is appropriate for cases where required fields are missing.
196-206: Off-by-one: final record may be dropped.The finalization logic pops one record from the buffer (line 197) and then merges the remaining buffer. If the buffer has exactly one record,
storeState.buffer.length > 0will be false after the pop, and no merge record is created. This appears intentional to avoid zero-duration records (as noted in line 275-276), but verify this matches expected behavior for single data points.
212-225: Good utility for grouping by merged label.The
groupByMergedLabelfunction correctly groups data rows by theirmergedLabelfield and applies a transform function to each group.
386-427: Custom renderer for single state timeline items.The
renderSingleStateTimeLineItemfunction correctly creates ECharts custom render elements. The height calculation usingapi.size([0, 1])[1] * 5for single items (vs justapi.size([0, 1])[1]for grouped items on line 436) suggests a different visual scaling - verify this is intentional.
| const baseSpec = { | ||
| $schema: VEGASCHEMA, | ||
| title: styleOptions.titleOptions?.show | ||
| ? styleOptions.titleOptions?.titleName || `${categoryName} by ${xAxis?.name}` | ||
| : undefined, | ||
| data: { values: processedData }, | ||
| transform: transformLayer, | ||
| layer: [barLayer, textLayer].filter(Boolean), | ||
| }; | ||
|
|
||
| return baseSpec; |
There was a problem hiding this comment.
Missing fakeYAxis transform in Vega branch.
Unlike createSingleCategoricalStateTimeline (lines 744-751) which adds a fakeYAxis transform to the data, createSingleNumericalStateTimeline references fakeYAxis in the encoding (line 887) but doesn't include the transform in baseSpec.transform (line 999).
This will cause the visualization to fail because fakeYAxis won't exist in the data.
🐛 Proposed fix
const baseSpec = {
$schema: VEGASCHEMA,
title: styleOptions.titleOptions?.show
? styleOptions.titleOptions?.titleName || `${categoryName} by ${xAxis?.name}`
: undefined,
data: { values: processedData },
- transform: transformLayer,
+ transform: [
+ ...(transformLayer || []),
+ // This is a fake field intentionally created to force Vega-Lite to draw an axis.
+ {
+ calculate: "'Response'",
+ as: 'fakeYAxis',
+ },
+ ],
layer: [barLayer, textLayer].filter(Boolean),
};📝 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.
| const baseSpec = { | |
| $schema: VEGASCHEMA, | |
| title: styleOptions.titleOptions?.show | |
| ? styleOptions.titleOptions?.titleName || `${categoryName} by ${xAxis?.name}` | |
| : undefined, | |
| data: { values: processedData }, | |
| transform: transformLayer, | |
| layer: [barLayer, textLayer].filter(Boolean), | |
| }; | |
| return baseSpec; | |
| const baseSpec = { | |
| $schema: VEGASCHEMA, | |
| title: styleOptions.titleOptions?.show | |
| ? styleOptions.titleOptions?.titleName || `${categoryName} by ${xAxis?.name}` | |
| : undefined, | |
| data: { values: processedData }, | |
| transform: [ | |
| ...(transformLayer || []), | |
| // This is a fake field intentionally created to force Vega-Lite to draw an axis. | |
| { | |
| calculate: "'Response'", | |
| as: 'fakeYAxis', | |
| }, | |
| ], | |
| layer: [barLayer, textLayer].filter(Boolean), | |
| }; | |
| return baseSpec; |
🤖 Prompt for AI Agents
In
@src/plugins/explore/public/components/visualizations/state_timeline/to_expression.ts
around lines 993 - 1003, The Vega spec for createSingleNumericalStateTimeline
references the fakeYAxis field in encodings but never injects the corresponding
fakeYAxis transform into the transforms array; add the same fakeYAxis transform
used in createSingleCategoricalStateTimeline to the transformLayer (or include
it directly in baseSpec.transform) so the data pipeline defines fakeYAxis before
layer encoding runs, ensuring transformLayer (used to build baseSpec.transform)
contains the fakeYAxis transform.
| thresholdOptions?: ThresholdOptions; | ||
|
|
||
| // Temporarily add this feature to make a switch between No style, Use Value Mapping Color, Use Threshold Color | ||
| useValueMappingColor?: boolean; |
There was a problem hiding this comment.
what do does it mean Temporarily? Are we going to remove this config in the future?
There was a problem hiding this comment.
If we are going to add color mode selection in the future. but we could keep useValueMappingColor if it is good.
| const thresholdLines = generateThresholdLines(styles?.thresholdOptions, styles?.switchAxes); | ||
|
|
||
| // facet into one chart | ||
| if (!Array.isArray(transformedData?.[0]?.[0])) { |
There was a problem hiding this comment.
Do we need special handle for one chart case? Can we simply create a one grid chart?
There was a problem hiding this comment.
Yes, users can select a facet field that results in only a single group.
| [AxisRole.X]: { type: VisFieldType.Date, index: 0 }, | ||
| [AxisRole.COLOR]: { type: VisFieldType.Categorical, index: 0 }, | ||
| }, | ||
| { |
There was a problem hiding this comment.
Will this affect existing Vega implementation?
There was a problem hiding this comment.
I also create a vega implementation for this one.
| timestampField: string, | ||
| nextData?: string, | ||
| range?: RangeValue | ||
| valueMapping?: ValueMapping | string |
There was a problem hiding this comment.
I think we make value mapping a pure data transformation function similar to pivot, aggregate, what do you think?
There was a problem hiding this comment.
State timeline is a complicated case that needs to transform data with merged label. If we’re going to add value mapping to bar and pie charts etc, we can directly use visualMap, which will save a lot of effort.
| r.range.min <= numberValue && | ||
| (r.range.max ?? Infinity) > numberValue | ||
| )?.range; | ||
| export const mergeDataCore = ({ |
There was a problem hiding this comment.
I'm think of making connect null value and disconnect value two pure data transformation functions similar to agg, pivot. But no blocking, let's get back on this later
| const { transformedData, yAxisConfig, baseConfig } = state; | ||
| const newState = { ...state }; | ||
|
|
||
| delete baseConfig.tooltip.trigger; |
There was a problem hiding this comment.
can we avoid delete? if the baseConfig doesn't work for state timeline, we can create another createBaseConfig function for state timeline
| const mergeLabelCombo: Array<{ label: any; color: any; displayText: any }> = []; | ||
|
|
||
| transformedData?.forEach((row) => { | ||
| const mergeLabelIndex = row[0].indexOf('mergedLabel'); |
There was a problem hiding this comment.
can you add comment to explain what's row[0] and row[1]?
| sortByTime, | ||
| } from '../utils/data_transformation'; | ||
|
|
||
| const prepareAssets = (styleOptions: StateTimeLineChartStyle) => { |
There was a problem hiding this comment.
Does this sound a bit more appropriate?
| const prepareAssets = (styleOptions: StateTimeLineChartStyle) => { | |
| const normalizeConfig = (styleOptions: StateTimeLineChartStyle) => { |
| }; | ||
|
|
||
| // Helper to create column filter | ||
| export const selectColumns = (columns: string[]) => (row: Record<string, any>) => { |
There was a problem hiding this comment.
Name it pick?
| export const selectColumns = (columns: string[]) => (row: Record<string, any>) => { | |
| export const pick = (fields: string[]) => (row: Record<string, any>) => { |
| * const adults = rowFilter(data).filter(Boolean); // Remove null entries | ||
| */ | ||
|
|
||
| export const filter = (filterConditionFn: (row: Record<string, any>) => Record<string, any>) => ( |
There was a problem hiding this comment.
This is not filter, it's just map right?
There was a problem hiding this comment.
yes, what it does here is actual mapping. I named it filter more in terms of functional thinking. will update it
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_utils.ts:
- Around line 147-161: findMatch currently returns mixed types (string |
ValueMapping | undefined), which makes later strict-equality checks in
mergeInAGroup (comparing storeState.currentValue === currentMapping) fragile;
change findMatch to return a single consistent identifier type (e.g.,
ValueMapping | undefined or a stable primitive id like mapping.id) instead of
sometimes returning String(value), update callers in mergeInAGroup to compare
that same identifier (replace currentMapping usage or storeState.currentValue to
that identifier type), and ensure all branches (no-color, range, value) return
the consistent type so strict equality comparisons remain reliable.
🧹 Nitpick comments (6)
src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_utils.ts (2)
122-210: Verify the complex buffer management logic at the end of mergeDataCore.Lines 197-206 handle remaining buffered entries, but the logic is subtle:
- Line 197: Pops
finalDatafrom the buffer- Line 198: Checks if buffer still has entries (length > 0)
- Lines 199-204: Merges using the original
bufferreference, which now excludesfinalDataThis works because the pop modifies the array in place, but it's not immediately obvious. The comment "no need to pass nextTime" on line 196 is helpful but doesn't fully explain the pop operation.
💡 Clarify the buffer handling logic
- // Merge any remaining buffered entries, no need to pass nextTime - const finalData = storeState.buffer.pop(); - if (storeState.buffer.length > 0) { + // Merge any remaining buffered entries (excluding the last point to calculate proper duration) + const lastPoint = storeState.buffer.pop(); + if (storeState.buffer.length > 0) { const rec = mergeRecordsWithColor( buffer, timestampField, - finalData?.[timestampField], + lastPoint?.[timestampField], storeState.currentValue ); merged.push(rec); }
306-380: Review the null value handling logic with connectThreshold.The logic at lines 333-350 handles null/undefined values with connection thresholds. The flow is complex:
- Lines 334-346: If buffer has entries and connectThreshold is set, either continue connecting or push null into buffer
- Line 348: Otherwise flush buffer
- Lines 354-368: After nulls, check if threshold was exceeded and potentially pop the last null
Line 362's comment "last null cannot be connect" has a grammatical error and the pop operation could use more explanation about why it ensures correct length.
📝 Clarify null handling logic with improved comments
if (shouldFlush) { const lastTime = storeState.buffer[storeState.buffer.length - 1][timestampField]; - storeState.buffer.pop(); // last null cannot be connect, pop that to ensure correct length + storeState.buffer.pop(); // Remove the last null value that exceeded the connection threshold flushBuffer(lastTime); }Consider adding a more detailed comment block explaining the null connection strategy.
src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_exclusive_vis_options.tsx (1)
96-97: Consider fixing the label capitalization.Line 97 uses "valueMapping" (camelCase) in the user-facing label. For consistency with line 83 which uses "threshold color" (two words, lowercase), consider changing this to "value mapping color".
📝 Consistent label formatting
label={i18n.translate('explore.vis.statetimeline.useValueMappingColor', { - defaultMessage: 'Use valueMapping color', + defaultMessage: 'Use value mapping color', })}src/plugins/explore/public/components/visualizations/state_timeline/to_expression.ts (3)
28-48: Consider usingnormalizeConfigin Vega-Lite branches too.The
normalizeConfighelper consolidates mapping and threshold extraction, but Vega-Lite branches (e.g., lines 126-142) still use inline logic. Using the helper in both branches would reduce duplication and prevent configuration drift.♻️ Example: Refactor Vega-Lite branch to use normalizeConfig
In
createNumericalStateTimelinearound line 126, replace inline extraction:- const rangeMappings = styleOptions?.valueMappingOptions?.valueMappings?.filter( - (mapping) => mapping?.type === 'range' - ); - - const valueMappings = styleOptions?.valueMappingOptions?.valueMappings?.filter( - (mapping) => mapping?.type === 'value' - ); - - const disconnectThreshold = - styleOptions?.exclusive?.disconnectValues?.disableMode === DisableMode.Threshold - ? styleOptions?.exclusive?.disconnectValues?.threshold || '1h' - : undefined; - - const connectThreshold = - styleOptions?.exclusive?.connectNullValues?.connectMode === DisableMode.Threshold - ? styleOptions?.exclusive?.connectNullValues?.threshold || '1h' - : undefined; + const { valueMappings, rangeMappings, disconnectThreshold, connectThreshold } = normalizeConfig(styleOptions);Apply similar changes to other Vega-Lite branches.
58-116: Consider extracting common ECharts builder pattern.All four builder functions share nearly identical ECharts branches:
- Field extraction and validation
normalizeConfigcall- Identical pipe composition:
transform→createBaseConfig→buildAxisConfigs→createStateTimeLineSpec→assembleSpecExtracting this pattern into a helper (e.g.,
buildEChartsStateTimeline) would reduce duplication and centralize the construction flow.Also applies to: 305-353, 532-579, 763-819
68-69: Consider more specific error messages.The generic "Missing field config" errors don't indicate which field is missing. More specific messages would improve debugging:
💡 Example improvement
if (!groupField || !timeField || !categoryField2) - throw Error('Missing field config for state-timeline chart'); + throw Error( + `Missing required field config for state-timeline chart. ` + + `timeField: ${timeField}, groupField: ${groupField}, categoryField: ${categoryField2}` + );Also applies to: 315-316, 541-542, 772-773
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_config.tssrc/plugins/explore/public/components/visualizations/state_timeline/state_timeline_exclusive_vis_options.tsxsrc/plugins/explore/public/components/visualizations/state_timeline/state_timeline_utils.tssrc/plugins/explore/public/components/visualizations/state_timeline/to_expression.tssrc/plugins/explore/public/components/visualizations/utils/data_transformation/filter.tssrc/plugins/explore/public/components/visualizations/utils/data_transformation/index.tssrc/plugins/explore/public/components/visualizations/utils/echarts_spec.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_utils.ts (5)
src/plugins/explore/public/components/visualizations/types.ts (1)
ValueMapping(310-317)src/plugins/explore/public/components/visualizations/utils/data_transformation/index.ts (1)
TransformFn(13-13)src/plugins/explore/public/components/visualizations/theme/color_utils.ts (1)
resolveColor(77-91)src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_config.ts (1)
StateTimeLineChartStyle(53-53)src/plugins/explore/public/components/visualizations/utils/echarts_spec.ts (2)
BaseChartStyle(31-49)PipelineFn(91-93)
⏰ 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 cypress tests (osd:ciGroup15)
- GitHub Check: Run cypress tests (osd:ciGroup9)
- GitHub Check: Run cypress tests (osd:ciGroup11)
- GitHub Check: Run cypress tests (osd:ciGroup17Explore)
- GitHub Check: Run cypress tests (osd:ciGroup2)
- GitHub Check: Run cypress tests (osd:ciGroup16Explore)
- GitHub Check: Run cypress tests (osd:ciGroup14Explore)
- GitHub Check: Run cypress tests (osd:ciGroup13Explore)
- GitHub Check: Run cypress tests (osd:ciGroup15Explore)
- GitHub Check: Run cypress tests (osd:ciGroup10Slow)
- GitHub Check: Run cypress tests (osd:ciGroup14)
- GitHub Check: Run cypress tests (osd:ciGroup12Explore)
- GitHub Check: Run cypress tests (osd:ciGroup13)
- GitHub Check: Run cypress tests (osd:ciGroup10Explore)
- GitHub Check: Run cypress tests (osd:ciGroup7)
- GitHub Check: Run cypress tests (osd:ciGroup12)
- GitHub Check: Run cypress tests (osd:ciGroup8)
- GitHub Check: Run cypress tests (osd:ciGroup10Fast)
- GitHub Check: Run cypress tests (osd:ciGroup6)
- GitHub Check: Run cypress tests (osd:ciGroup3)
- GitHub Check: Run cypress tests (osd:ciGroup4)
- GitHub Check: Run cypress tests (osd:ciGroup1)
- GitHub Check: Run cypress tests (osd:ciGroup5)
- GitHub Check: Run functional tests on Windows (ciGroup6)
- GitHub Check: Run functional tests on Windows (ciGroup13)
- GitHub Check: Run functional tests on Linux (ciGroup13)
- GitHub Check: Run functional tests on Linux (ciGroup2)
- GitHub Check: Run functional tests on Linux (ciGroup1)
- GitHub Check: Run functional tests on Windows (ciGroup8)
- GitHub Check: Run functional tests on Windows (ciGroup2)
- GitHub Check: Run functional tests on Linux (ciGroup5)
- GitHub Check: Run functional tests on Windows (ciGroup9)
- GitHub Check: Run functional tests on Linux (ciGroup3)
- GitHub Check: Run functional tests on Linux (ciGroup4)
- GitHub Check: Run functional tests on Windows (ciGroup10)
- GitHub Check: Run functional tests on Windows (ciGroup5)
- GitHub Check: Run functional tests on Linux (ciGroup10)
- GitHub Check: Run functional tests on Windows (ciGroup7)
- GitHub Check: Run functional tests on Linux (ciGroup7)
- GitHub Check: Run functional tests on Windows (ciGroup11)
- GitHub Check: Run functional tests on Windows (ciGroup12)
- GitHub Check: Run functional tests on Linux (ciGroup11)
- GitHub Check: Run functional tests on Windows (ciGroup3)
- GitHub Check: Run functional tests on Linux (ciGroup9)
- GitHub Check: Run functional tests on Linux (ciGroup8)
- GitHub Check: Run functional tests on Linux (ciGroup12)
- GitHub Check: Run functional tests on Linux (ciGroup6)
- GitHub Check: Run functional tests on Windows (ciGroup1)
- GitHub Check: Run functional tests on Windows (ciGroup4)
- GitHub Check: Build and Verify on Windows (ciGroup1)
- GitHub Check: Build and Verify on Linux (ciGroup2)
- GitHub Check: Build and Verify on Linux (ciGroup1)
- GitHub Check: Build and Verify on Linux (ciGroup4)
- GitHub Check: Build and Verify on Windows (ciGroup2)
- GitHub Check: Build and Verify on Linux (ciGroup3)
- GitHub Check: Build and Verify on Windows (ciGroup3)
- GitHub Check: Build and Verify on Windows (ciGroup4)
- GitHub Check: Build min release artifacts on macOS x64
- 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 Windows x64
- GitHub Check: Run plugin functional tests on Linux
- GitHub Check: Lint and validate
- GitHub Check: Run plugin functional tests on Windows
- GitHub Check: lighthouse
- GitHub Check: bundle-analyzer
🔇 Additional comments (19)
src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_utils.ts (5)
76-105: LGTM! Clear handling of both string and ValueMapping types.The function correctly:
- Handles both
stringandValueMappingparameter types- Uses
'∞'symbol for display of unbounded ranges- Provides
'_unmatched_'as a sensible fallback label- Only sets
mergedColorwhen valueMapping is an object (line 101)
212-225: LGTM! Clean grouping and transformation helper.The
groupByMergedLabelfunction provides a clean abstraction for grouping data by merged labels and applying transformations to each group.
382-427: LGTM! Consistent ECharts rendering implementations.Both
renderSingleStateTimeLineItemandrenderItemfollow the same pattern:
- Use
decideSeriesStylefor color determination with'_unmatched_'handling- Create rect graphics with proper positioning
- Conditionally render display text when
showValuesis enabled- Apply consistent z-index ordering (z2: 5 for rect, z2: 10 for text)
The only difference is the Y-coordinate calculation (fixed value of 5 vs. dynamic
api.value(groupField)), which correctly handles the presence/absence of grouping.Also applies to: 429-474
476-534: LGTM! Well-structured spec creation with proper axis handling.The function correctly:
- Extracts merged label, color, and display text combinations from transformed data (lines 491-500)
- Adjusts Y-axis configuration when no groupField is present (lines 502-510) - disabling ticks, labels, and gridlines
- Creates custom series with appropriate render functions and styling (lines 512-529)
- Uses
displayText || labelfallback for series names (line 514)
107-116: No action needed—range.max handling is correct.The change to store
undefinedinstead ofInfinityfor the final threshold's max value is intentional and properly handled throughout the codebase. All downstream logic safely convertsundefinedback toInfinitywhen needed for numeric comparisons (line 155), and displays it as '∞' for UI presentation (lines 92, 176, 223, 866, 924). No numeric value assumption is broken.src/plugins/explore/public/components/visualizations/utils/data_transformation/index.ts (1)
19-20: LGTM! Clean re-export of new transformation utilities.The exports for
mapandpickexpand the data transformation API consistently with existing patterns.src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_config.ts (1)
48-50: LGTM! Well-integrated feature addition for value mapping color control.The changes correctly:
- Add the
useValueMappingColoroption to the interface with a clear explanatory comment (lines 48-50)- Initialize it with a sensible default of
false(line 83)- Add a new mapping configuration for time-numerical charts (lines 134-137), supporting the four chart variants mentioned in the PR objectives
The new mapping enables users to visualize numerical data on the color axis without a categorical Y-axis grouping.
Also applies to: 83-83, 134-137
src/plugins/explore/public/components/visualizations/utils/data_transformation/filter.ts (1)
23-36: LGTM! Clean, well-documented transformation utilities.The utilities are well-designed:
mapprovides a functional wrapper for row-wise transformationspickdefensively checks column existence (line 33) before selecting- Comprehensive JSDoc with practical usage examples (lines 6-21)
These will integrate nicely with other transformation functions for building data pipelines.
src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_exclusive_vis_options.tsx (1)
19-20: LGTM! Correct mutual exclusion between color styling options.The implementation correctly enforces that only one color styling mode can be active at a time:
- Line 87:
useThresholdColorswitch is disabled whenuseValueMappingColoris true- Line 101:
useValueMappingColorswitch is disabled whenuseThresholdColoris trueThis provides the three-way choice described in the PR: No style (both off), Use Value Mapping Color, or Use Threshold Color.
Also applies to: 58-59, 87-87, 92-104
src/plugins/explore/public/components/visualizations/utils/echarts_spec.ts (5)
124-139: LGTM: Conditional trigger configuration added.The
addTriggerparameter with a default oftruepreserves backward compatibility while allowing state timeline to disable axis triggers. The conditional logic correctly gates the trigger inclusion.
159-161: LGTM: More robust facet detection.The explicit check for
axisColumnMappings.facet !== undefinedin addition to array structure validation prevents false positives and aligns with the facet handling inassembleSpec.
206-222: LGTM: Multi-dataset support enhanced for state timeline.The distinction between
hasMultiDatasets(array structure) andhasFacet(array structure + facet mapping) correctly accommodates state timeline charts that use multiple datasets without traditional faceting.
227-231: LGTM: Grid layout handles single-facet edge case.The updated condition
!hasFacet || facetNumber <= 1correctly applies simple margins for non-faceted or single-facet scenarios, avoiding unnecessary grid complexity. Also fixes typo "slighly" → "slightly".
276-276: Verify axisLine default doesn't regress state timeline and histogram visualizations.The unconditional
axisLine: { show: true }inbuildAxisConfigsaffects state timeline and histogram chart rendering throughecharts_spec. Confirm these visualizations render correctly with visible axis lines, particularly in edge cases where axes should be hidden.Likely an incorrect or invalid review comment.
src/plugins/explore/public/components/visualizations/state_timeline/to_expression.ts (5)
84-113: LGTM: Pipe composition provides clear data transformation flow.The composition chain (
transform→map(pick(...))→sortByTime→mergeDataCore→groupByMergedLabel→ spec builders) provides a clear, functional data transformation pipeline for ECharts rendering.
755-1002: LGTM: New single numerical timeline builder added.The
createSingleNumericalStateTimelinefunction correctly implements both ECharts and Vega-Lite branches for numerical state timelines. The textLayer filters (lines 977-979) appropriately prevent rendering invalid text positions.
151-163: LGTM: Vega-Lite branch updated for new color mapping options.The Vega-Lite branch correctly integrates
useThresholdColoranduseValueMappingColorflags into themergeDataCorecall and downstream logic, maintaining parity with the new ECharts implementation.
175-177: LGTM: Appropriate use of optional chaining.Optional chaining on
validMappings?.mapandvalueMappings?.mapcorrectly handles cases where mappings are undefined, preventing null reference errors during domain/range construction.Also applies to: 223-225
82-86: No issue found.AxisColumnMappingsis typed asPartial<Record<AxisRole, VisColumn>>, which means the values—when present—are completeVisColumnobjects with a requiredcolumn: stringproperty. The code at line 82 correctly constructsallColumnsfrom these guaranteed-valid VisColumn objects, so there is no risk of undefined values.Likely an incorrect or invalid review comment.
| const findMatch = (value: string) => { | ||
| if (!useThresholdColor && !useValueMappingColor) return String(value); | ||
| return mergedMappings.find((v) => { | ||
| if (v.type === 'range') { | ||
| const numberValue = Number(value); | ||
| return ( | ||
| v.range?.min !== undefined && | ||
| numberValue >= v.range.min && | ||
| numberValue < (v.range.max ?? Infinity) | ||
| ); | ||
| } else { | ||
| return v.value === `${value}`; | ||
| } | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Critical: Object reference comparison bug in value matching logic.
The findMatch function returns a ValueMapping object from mergedMappings.find(). Later, at line 371 in mergeInAGroup, this returned object is compared using strict equality (storeState.currentValue === currentMapping).
Since .find() returns the same object reference from the array, this comparison should work correctly. However, if findMatch returns a string (line 148) or the logic changes to construct new objects, this will break. The mixing of return types (string | ValueMapping | undefined) makes this fragile.
🔍 Recommended refactor for type safety
Consider one of these approaches:
Option 1: Return a consistent identifier
const findMatch = (value: string) => {
if (!useThresholdColor && !useValueMappingColor) return String(value);
- return mergedMappings.find((v) => {
+ const matched = mergedMappings.find((v) => {
if (v.type === 'range') {
const numberValue = Number(value);
return (
v.range?.min !== undefined &&
numberValue >= v.range.min &&
numberValue < (v.range.max ?? Infinity)
);
} else {
return v.value === `${value}`;
}
});
+ // Return a stable identifier for comparison
+ return matched ? matched.id || `${matched.type}-${matched.value || JSON.stringify(matched.range)}` : String(value);
};Option 2: Use deep equality or index-based comparison
At line 371, replace strict equality with a comparison function that handles all return types properly.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
@src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_utils.ts
around lines 147 - 161, findMatch currently returns mixed types (string |
ValueMapping | undefined), which makes later strict-equality checks in
mergeInAGroup (comparing storeState.currentValue === currentMapping) fragile;
change findMatch to return a single consistent identifier type (e.g.,
ValueMapping | undefined or a stable primitive id like mapping.id) instead of
sometimes returning String(value), update callers in mergeInAGroup to compare
that same identifier (replace currentMapping usage or storeState.currentValue to
that identifier type), and ensure all branches (no-color, range, value) return
the consistent type so strict equality comparisons remain reliable.
| * 1. Filter columns - select specific columns from data: | ||
| * const columnFilter = map(pick(['category1', 'category2'])); | ||
| * const result = columnFilter(data); // Only 'category1' and 'category2' columns | ||
| * | ||
| * 2. Filter rows - keep rows that meet a condition: |
There was a problem hiding this comment.
Since the function changed from filter -> map, could you update the comment accordingly? Also the filename, maybe call it common.ts?
There was a problem hiding this comment.
Sure, sorry I missed this one
Signed-off-by: Qxisylolo <qianxisy@amazon.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_exclusive_vis_options.tsx (1)
95-95: Typo indata-test-subjattribute.The test subject identifier has a typo:
showValuesSwtichshould beshowValuesSwitch.Proposed fix
- data-test-subj="showValuesSwtich" + data-test-subj="showValuesSwitch"src/plugins/explore/public/components/visualizations/state_timeline/to_expression.test.ts (3)
136-139: Duplicate assertion.Line 138 duplicates the assertion on line 137. This appears to be a copy-paste error.
Suggested fix
expect(markLayer).toHaveProperty('encoding.x.field', 'timestamp'); expect(markLayer).toHaveProperty('encoding.x.type', 'temporal'); - expect(markLayer).toHaveProperty('encoding.x.type', 'temporal'); expect(markLayer).toHaveProperty('encoding.x2.field', 'end');
182-185: Duplicate assertion (same pattern).Lines 183-184 contain the same duplicate assertion for
encoding.x.typeas the previous test.Suggested fix
expect(markLayer).toHaveProperty('encoding.x.field', 'timestamp'); expect(markLayer).toHaveProperty('encoding.x.type', 'temporal'); - expect(markLayer).toHaveProperty('encoding.x.type', 'temporal'); expect(markLayer).toHaveProperty('encoding.x2.field', 'end');
256-259: Duplicate assertion (same pattern).Lines 257-258 repeat the
encoding.x.typeassertion.Suggested fix
expect(markLayer).toHaveProperty('encoding.x.field', 'timestamp'); expect(markLayer).toHaveProperty('encoding.x.type', 'temporal'); - expect(markLayer).toHaveProperty('encoding.x.type', 'temporal'); expect(markLayer).toHaveProperty('encoding.x2.field', 'end');
🤖 Fix all issues with AI agents
In
@src/plugins/explore/public/components/visualizations/state_timeline/to_expression.ts:
- Line 514: The template literal that builds the chart title
(`\`${categoryName2} by ${yAxis?.name} and ${xAxis?.name} \``) contains an
extraneous trailing space; remove the trailing whitespace from that template
string in to_expression.ts so the generated title doesn't end with a space
(update the template literal where the title is constructed).
- Line 811: The title template currently has an unintended leading space and can
produce double spaces when values are missing; update the construction of the
title (where title is set using colorMapping?.name and axisConfig.xAxis?.name)
to avoid the leading space and to join parts safely (e.g., build an array like
[colorMapping?.name, 'by', axisConfig.xAxis?.name].filter(Boolean).join(' ')) or
otherwise trim/collapse whitespace so the resulting title has no extra leading
or double spaces.
- Line 566: The title string currently includes an unintended leading space and
double spaces (" ${colorMapping?.name} by ..."); update the title construction
(the object property named title where colorMapping and axisConfig.xAxis are
used) to remove the extra leading and duplicate spaces so it reads cleanly
(e.g., interpolate without the leading space and with a single space before "by"
and handle optional name values via the existing optional chaining if needed).
🧹 Nitpick comments (6)
src/plugins/explore/public/components/visualizations/state_timeline/to_expression.ts (6)
28-48: Good extraction of common configuration logic.The
normalizeConfighelper reduces duplication for the echarts paths. However, the Vega-Lite paths increateNumericalStateTimeline(lines 130-146) and other functions still duplicate this logic inline rather than reusing this helper.Consider refactoring the Vega-Lite paths to also use
normalizeConfigfor consistency.
87-87: Inconsistent field reference for sorting.
sortByTimeusesaxisColumnMappings?.x?.columnwhiletimeFieldwas already derived fromaxisConfig.xAxis?.columnat line 61. These should be equivalent, but using the pre-computedtimeFieldwould be more consistent and clearer.♻️ Suggested fix
- sortByTime(axisColumnMappings?.x?.column), + sortByTime(timeField),
130-157: Duplicate configuration extraction in Vega-Lite path.This code block duplicates the logic already extracted into
normalizeConfig(lines 28-48). The echarts path at line 71 usesnormalizeConfig, but the Vega-Lite path here duplicates it manually.♻️ Suggested refactor
- const rangeMappings = styleOptions?.valueMappingOptions?.valueMappings?.filter( - (mapping) => mapping?.type === 'range' - ); - - const valueMappings = styleOptions?.valueMappingOptions?.valueMappings?.filter( - (mapping) => mapping?.type === 'value' - ); - - const disconnectThreshold = - styleOptions?.exclusive?.disconnectValues?.disableMode === DisableMode.Threshold - ? styleOptions?.exclusive?.disconnectValues?.threshold || '1h' - : undefined; - - const connectThreshold = - styleOptions?.exclusive?.connectNullValues?.connectMode === DisableMode.Threshold - ? styleOptions?.exclusive?.connectNullValues?.threshold || '1h' - : undefined; + const { valueMappings, rangeMappings, disconnectThreshold, connectThreshold } = normalizeConfig( + styleOptions + );
827-830:yAxisis destructured but unused.The
yAxisvariable is destructured but not used in this function's Vega-Lite path. Consider removing it to avoid confusion.♻️ Suggested fix
- const { xAxis, xAxisStyle, yAxis, yAxisStyle } = getSwappedAxisRole( + const { xAxis, xAxisStyle, yAxisStyle } = getSwappedAxisRole( styleOptions, axisColumnMappings );
75-80: Consider extracting repeated threshold construction logic.The
completeThresholdconstruction pattern is duplicated across four locations:
- Lines 75-80 (createNumericalStateTimeline echarts)
- Lines 148-153 (createNumericalStateTimeline vega)
- Lines 782-787 (createSingleNumericalStateTimeline echarts)
- Lines 840-845 (createSingleNumericalStateTimeline vega)
Consider extracting this into a helper function within
normalizeConfigor as a separate utility:♻️ Example helper
const buildCompleteThreshold = (thresholdOptions: ThresholdOptions): Threshold[] => [ { value: 0, color: thresholdOptions.baseColor } as Threshold, ...(thresholdOptions.thresholds || []), ];Also applies to: 148-153, 782-787, 840-845
216-221: Duplicated domain generation logic for range mappings.The
reducelogic for building the domain array from range mappings is duplicated betweencreateNumericalStateTimeline(lines 216-221) andcreateSingleNumericalStateTimeline(lines 918-923). Consider extracting this into a shared helper:♻️ Example helper
const buildMappingDomain = (mappings: ValueMapping[]): Array<string | undefined> => mappings.reduce((acc, m) => { acc.push(m.type === 'range' ? `[${m.range?.min},${m?.range?.max ?? '∞'})` : m.value); return acc; }, [] as Array<string | undefined>);Also applies to: 918-923
src/plugins/explore/public/components/visualizations/state_timeline/to_expression.ts
Outdated
Show resolved
Hide resolved
src/plugins/explore/public/components/visualizations/state_timeline/to_expression.ts
Outdated
Show resolved
Hide resolved
src/plugins/explore/public/components/visualizations/state_timeline/to_expression.ts
Outdated
Show resolved
Hide resolved
Qxisylolo
force-pushed
the
feat/echart_state_timeline
branch
from
286c574 to
56ce74d
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
@src/plugins/explore/public/components/visualizations/state_timeline/to_expression.ts:
- Around line 565-567: The title string passed to createBaseConfig contains a
double space between `${colorMapping?.name}` and `by`; update the template in
the createBaseConfig call to use a single space between `${colorMapping?.name}`
and `by` so the title becomes `${colorMapping?.name} by
${axisConfig.xAxis?.name}` (refer to the createBaseConfig invocation and its
title argument where colorMapping?.name and axisConfig.xAxis?.name are used).
- Around line 810-813: The title string passed into createBaseConfig contains a
double space between `${colorMapping?.name}` and `by`; update that template to
use a single space (e.g., `${colorMapping?.name} by ${axisConfig.xAxis?.name}`)
and make the same change for the other identical occurrence in this file; locate
calls to createBaseConfig that build titles using colorMapping and
axisConfig.xAxis and normalize the spacing to a single space.
- Around line 594-603: The Vega-Lite render path currently passes valueMappings
unchanged to mergeDataCore while the ECharts path clears valueMappings when
styleOptions.useThresholdColor is true, causing inconsistent behavior; update
the Vega-Lite branch around the processedData/mergeDataCore call to mirror the
ECharts logic by passing an empty array (or undefined) for valueMappings when
styleOptions.useThresholdColor is true (e.g., compute a local variable like
effectiveValueMappings = styleOptions.useThresholdColor ? [] : valueMappings and
pass that to mergeDataCore) so both render paths behave identically.
- Around line 988-995: The Vega spec misses the transform that adds the
synthetic Y field used by the bar layer: update the baseSpec object to include
the same transform used in createSingleCategoricalStateTimeline that produces
"fakeYAxis" (i.e., a transform/aggregate or window that computes the
constant/index Y value from processedData) so
barLayer.encoding.y.field='fakeYAxis' has a source; modify the baseSpec's
top-level to include a transforms array (or merge into existing transforms) that
creates the fakeYAxis field before the layer is rendered.
🧹 Nitpick comments (2)
src/plugins/explore/public/components/visualizations/utils/data_transformation/common.ts (1)
17-27: Type signature doesn't match documented usage pattern.The JSDoc example (line 19) shows returning
nullfrom the mapper function, but the type signature only allowsRecord<string, any>. Consider updating the type to reflect nullable returns:💡 Suggested type fix
-export const map = (mapConditionFn: (row: Record<string, any>) => Record<string, any>) => ( +export const map = (mapConditionFn: (row: Record<string, any>) => Record<string, any> | null) => ( data: Array<Record<string, any>> -): Array<Record<string, any>> => { +): Array<Record<string, any> | null> => { return data.map(mapConditionFn); };src/plugins/explore/public/components/visualizations/state_timeline/to_expression.ts (1)
130-157: Consider reusingnormalizeConfighelper in Vega-Lite path.The Vega-Lite path (lines 130-146) duplicates the extraction logic that
normalizeConfigalready provides. Using the helper would improve consistency and reduce maintenance burden.💡 Suggested refactor
- const rangeMappings = styleOptions?.valueMappingOptions?.valueMappings?.filter( - (mapping) => mapping?.type === 'range' - ); - - const valueMappings = styleOptions?.valueMappingOptions?.valueMappings?.filter( - (mapping) => mapping?.type === 'value' - ); - - const disconnectThreshold = - styleOptions?.exclusive?.disconnectValues?.disableMode === DisableMode.Threshold - ? styleOptions?.exclusive?.disconnectValues?.threshold || '1h' - : undefined; - - const connectThreshold = - styleOptions?.exclusive?.connectNullValues?.connectMode === DisableMode.Threshold - ? styleOptions?.exclusive?.connectNullValues?.threshold || '1h' - : undefined; + const { valueMappings, rangeMappings, disconnectThreshold, connectThreshold } = normalizeConfig( + styleOptions + );
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/plugins/explore/public/components/visualizations/state_timeline/to_expression.tssrc/plugins/explore/public/components/visualizations/utils/data_transformation/common.tssrc/plugins/explore/public/components/visualizations/utils/data_transformation/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/plugins/explore/public/components/visualizations/utils/data_transformation/index.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/plugins/explore/public/components/visualizations/state_timeline/to_expression.ts (7)
src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_config.ts (1)
StateTimeLineChartStyle(51-51)src/plugins/explore/public/components/visualizations/utils/utils.ts (2)
getChartRender(449-456)applyAxisStyling(22-89)src/plugins/explore/public/components/visualizations/state_timeline/state_timeline_utils.ts (4)
convertThresholdsToValueMappings(107-116)mergeDataCore(122-210)groupByMergedLabel(212-225)createStateTimeLineSpec(476-534)src/plugins/explore/public/components/visualizations/utils/echarts_spec.ts (5)
pipe(98-102)createBaseConfig(124-151)buildAxisConfigs(156-198)assembleSpec(203-258)applyAxisStyling(267-321)src/plugins/explore/public/components/visualizations/utils/data_transformation/common.ts (2)
map(23-27)pick(30-36)src/plugins/explore/public/components/visualizations/theme/color_utils.ts (2)
resolveColor(77-91)getCategoryNextColor(72-74)src/plugins/explore/public/components/visualizations/constants.ts (1)
DEFAULT_OPACITY(68-68)
src/plugins/explore/public/components/visualizations/utils/data_transformation/common.ts (1)
src/plugins/explore/public/components/visualizations/utils/data_transformation/index.ts (2)
map(20-20)pick(20-20)
⏰ 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). (3)
- GitHub Check: Build min release artifacts on macOS x64
- GitHub Check: lighthouse
- GitHub Check: Build min release artifacts on macOS ARM64
🔇 Additional comments (5)
src/plugins/explore/public/components/visualizations/utils/data_transformation/common.ts (1)
29-36: LGTM!The
pickhelper is well-implemented with appropriate handling for missing columns.src/plugins/explore/public/components/visualizations/state_timeline/to_expression.ts (4)
28-48: LGTM!Clean extraction of configuration values. The helper appropriately centralizes the repeated logic for both echarts and Vega-Lite paths.
58-120: LGTM!The echarts rendering path is well-structured with proper validation and a clean pipe-based transformation flow.
300-349: LGTM!The echarts rendering path correctly handles categorical data without range mappings.
964-986: LGTM!The additional filters in
textLayerappropriately handle edge cases specific to numerical timelines, ensuring text labels only appear when valid.
src/plugins/explore/public/components/visualizations/state_timeline/to_expression.ts
Show resolved
Hide resolved
src/plugins/explore/public/components/visualizations/state_timeline/to_expression.ts
Show resolved
Hide resolved
src/plugins/explore/public/components/visualizations/state_timeline/to_expression.ts
Show resolved
Hide resolved
src/plugins/explore/public/components/visualizations/state_timeline/to_expression.ts
Show resolved
Hide resolved
3 participants
Description
This pr migrate state timeline and optimize value mapping rules:
Screenshot
2026-01-07.14.51.30.mov
2026-01-07.14.42.19.mov
useValueMappingColor
2026-01-07.14.58.17.mov
useThresholdColor
2026-01-07.14.59.24.mov
Testing the changes
Changelog
Check List
yarn test:jestyarn test:jest_integrationSummary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.