[Explore Vis] adapt deprecated styles in embeddable #10984
Conversation
Signed-off-by: Qxisylolo <[email protected]>
WalkthroughAdded two exported utilities ( Changes
Sequence DiagramsequenceDiagram
participant Client
participant ExploreEmbeddable
participant Utils as visualization_builder_utils
participant SpecGen as matchedRule.toSpec
Client->>ExploreEmbeddable: fetch()
ExploreEmbeddable->>ExploreEmbeddable: load saved visualization + styleOptions
alt saved viz has legacy thresholdLines
ExploreEmbeddable->>Utils: adaptLegacyData(type, styles, axesMapping)
Utils-->>ExploreEmbeddable: adaptedStyles
ExploreEmbeddable->>SpecGen: toSpec(..., styles = adaptedStyles)
else no legacy styles
ExploreEmbeddable->>SpecGen: toSpec(..., styles = styleOptions)
end
SpecGen-->>Client: visualization spec
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
github-actions
bot
added
distinguished-contributor
Skip-Changelog
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #10984 +/- ##
==========================================
+ Coverage 60.77% 60.79% +0.02%
==========================================
Files 4533 4531 -2
Lines 122256 122265 +9
Branches 20491 20502 +11
==========================================
+ Hits 74298 74332 +34
+ Misses 42719 42691 -28
- Partials 5239 5242 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Qxisylolo
requested review from
AMoo-Miki,
BionIT,
Flyingliuhub,
LDrago27,
SuZhou-Joe,
ZilongX,
abbyhu2000,
ananzh,
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
Maosaic,
TackAdam,
angle943,
d-buckner,
paulstn,
ps48,
raintygao,
ruchidh,
sumukhswamy and
wanglam
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: 0
🧹 Nitpick comments (3)
src/plugins/explore/public/embeddable/explore_embeddable.test.tsx (2)
70-81: AlignadaptLegacyDatamock shape with the production implementationThe mock currently returns
{ useThresholdColor: true }, whilefetchexpectsadaptLegacyDatato return a config object and then reads itsstylesproperty. In tests this yieldsstyles === undefined, so we always fall back tostyleOptionsand never exercise the adapted-style path, which can hide regressions.Consider making the mock return a config-like object with a
stylesfield so the adapted styles are actually used:- adaptLegacyData: jest.fn().mockReturnValue({ useThresholdColor: true }), + adaptLegacyData: jest.fn().mockImplementation((config) => ({ + ...config, + styles: { + ...(config?.styles ?? {}), + useThresholdColor: true, + }, + })),This keeps the test behavior closer to production and lets you verify that migrated styles are wired through correctly.
517-542: Strengthen the “adapt deprecated styles” test to validate arguments and effectsThis test currently only asserts that
adaptLegacyDatais called. It would still pass iffetchcalled it with incorrect arguments or then ignored its result.To make the test more robust, consider additionally asserting:
- The arguments, e.g. type,
stylesandaxesMapping:expect(adaptLegacyDataSpy).toHaveBeenCalledWith( expect.objectContaining({ type: 'line', styles: expect.anything(), axesMapping: { x: 'field1', y: 'field2' }, }) );
- Optionally, that
matchedRule.toSpecis invoked with the adapted styles (e.g. by checking the corresponding argument).This would give stronger coverage of the legacy-style adaptation flow.
src/plugins/explore/public/embeddable/explore_embeddable.tsx (1)
48-51: Persist effective (adapted) styles intosearchPropsto avoid divergenceThe integration of
adaptLegacyDatahere looks correct, and usingstyles || styleOptionskeeps behavior unchanged when no migration is needed. However,searchProps.styleOptionsremains set to the originalvisualization.params, so consumers ofsearchPropsmay still see the legacy config (e.g., withthresholdLines) while the rendered spec uses the migratedthresholdOptions.If you want the embeddable state and rendered visualization to stay in sync, consider writing back the effective styles:
const styleOptions = visualization.params; const styles = adaptLegacyData({ type: selectedChartType, styles: styleOptions, axesMapping: visualization.axesMapping, })?.styles; const effectiveStyles = styles || styleOptions; this.searchProps.styleOptions = effectiveStyles; const spec = matchedRule.toSpec( visualizationData.transformedData, numericalColumns, categoricalColumns, dateColumns, effectiveStyles, selectedChartType, axesMapping );This keeps the internal state consistent with the migrated configuration while preserving your current fallback behavior.
Also applies to: 408-419
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/plugins/explore/public/embeddable/explore_embeddable.test.tsx(2 hunks)src/plugins/explore/public/embeddable/explore_embeddable.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/plugins/explore/public/embeddable/explore_embeddable.tsx (1)
src/plugins/explore/public/components/visualizations/visualization_builder_utils.ts (1)
adaptLegacyData(85-184)
⏰ 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). (15)
- GitHub Check: Run cypress tests (osd:ciGroup17Explore)
- GitHub Check: Run cypress tests (osd:ciGroup13Explore)
- GitHub Check: Run cypress tests (osd:ciGroup10Explore)
- GitHub Check: Run cypress tests (osd:ciGroup15Explore)
- GitHub Check: Run cypress tests (osd:ciGroup15)
- GitHub Check: Run cypress tests (osd:ciGroup16Explore)
- GitHub Check: Run cypress tests (osd:ciGroup12Explore)
- GitHub Check: Run cypress tests (osd:ciGroup14)
- GitHub Check: Run cypress tests (osd:ciGroup10Slow)
- GitHub Check: Run cypress tests (osd:ciGroup13)
- GitHub Check: Run cypress tests (osd:ciGroup12)
- GitHub Check: Run functional tests on Windows (ciGroup12)
- GitHub Check: Run functional tests on Windows (ciGroup11)
- GitHub Check: Run functional tests on Windows (ciGroup4)
- GitHub Check: Run functional tests on Windows (ciGroup3)
| this.searchProps.searchContext = searchContext; | ||
| const styleOptions = visualization.params; | ||
|
|
||
| const styles = adaptLegacyData({ |
There was a problem hiding this comment.
Perhaps we need to update searchProps.styleOptions as well?
There was a problem hiding this comment.
only table needs searchProps.styleOptions when rendering embeddable currently which doesn't need to adapt legacy data. but I will include this as table will use value mapping in the future and we may need to migrate some data. thanks!
Signed-off-by: Qxisylolo <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/plugins/explore/public/embeddable/explore_embeddable.tsx(2 hunks)
⏰ 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 (ciGroup9)
- GitHub Check: Run functional tests on Windows (ciGroup7)
- GitHub Check: Run cypress tests (osd:ciGroup17Explore)
- GitHub Check: Run functional tests on Windows (ciGroup13)
- GitHub Check: Run functional tests on Linux (ciGroup13)
- GitHub Check: Run functional tests on Windows (ciGroup8)
- GitHub Check: Run functional tests on Windows (ciGroup6)
- GitHub Check: Run functional tests on Windows (ciGroup4)
- GitHub Check: Run cypress tests (osd:ciGroup14Explore)
- GitHub Check: Run functional tests on Windows (ciGroup12)
- GitHub Check: Run cypress tests (osd:ciGroup16Explore)
- GitHub Check: Run cypress tests (osd:ciGroup13Explore)
- GitHub Check: Run functional tests on Windows (ciGroup11)
- GitHub Check: Run functional tests on Windows (ciGroup10)
- GitHub Check: Run functional tests on Windows (ciGroup5)
- GitHub Check: Run functional tests on Windows (ciGroup2)
- GitHub Check: Run cypress tests (osd:ciGroup15Explore)
- GitHub Check: Run cypress tests (osd:ciGroup12Explore)
- GitHub Check: Run cypress tests (osd:ciGroup15)
- GitHub Check: Run cypress tests (osd:ciGroup10Explore)
- GitHub Check: Run functional tests on Linux (ciGroup12)
- GitHub Check: Run functional tests on Linux (ciGroup8)
- GitHub Check: Run functional tests on Linux (ciGroup6)
- GitHub Check: Run functional tests on Windows (ciGroup3)
- GitHub Check: Run functional tests on Linux (ciGroup11)
- GitHub Check: Run functional tests on Linux (ciGroup5)
- GitHub Check: Run functional tests on Linux (ciGroup9)
- GitHub Check: Run functional tests on Linux (ciGroup7)
- GitHub Check: Run functional tests on Windows (ciGroup1)
- GitHub Check: Run functional tests on Linux (ciGroup10)
- GitHub Check: Run cypress tests (osd:ciGroup14)
- GitHub Check: Run cypress tests (osd:ciGroup10Fast)
- GitHub Check: Run cypress tests (osd:ciGroup11)
- GitHub Check: Run cypress tests (osd:ciGroup12)
- GitHub Check: Run functional tests on Linux (ciGroup3)
- GitHub Check: Run functional tests on Linux (ciGroup4)
- GitHub Check: Run cypress tests (osd:ciGroup6)
- GitHub Check: Run cypress tests (osd:ciGroup13)
- GitHub Check: Run cypress tests (osd:ciGroup4)
- GitHub Check: Run cypress tests (osd:ciGroup3)
- GitHub Check: Run cypress tests (osd:ciGroup2)
- GitHub Check: Run cypress tests (osd:ciGroup7)
- GitHub Check: Run cypress tests (osd:ciGroup10Slow)
- GitHub Check: Run cypress tests (osd:ciGroup8)
- GitHub Check: Run functional tests on Linux (ciGroup2)
- GitHub Check: Run cypress tests (osd:ciGroup1)
- GitHub Check: Run cypress tests (osd:ciGroup9)
- GitHub Check: Run cypress tests (osd:ciGroup5)
- GitHub Check: Run functional tests on Linux (ciGroup1)
- GitHub Check: Build min release artifacts on macOS ARM64
- GitHub Check: Build min release artifacts on Windows x64
- GitHub Check: Build min release artifacts on Linux ARM64
- GitHub Check: Build min release artifacts on macOS x64
- GitHub Check: Build and Verify on Windows (ciGroup3)
- GitHub Check: Build min release artifacts on Linux x64
- GitHub Check: Build and Verify on Linux (ciGroup1)
- GitHub Check: Build and Verify on Windows (ciGroup4)
- GitHub Check: Build and Verify on Linux (ciGroup2)
- GitHub Check: Build and Verify on Windows (ciGroup2)
- GitHub Check: Build and Verify on Linux (ciGroup4)
- GitHub Check: Build and Verify on Linux (ciGroup3)
- GitHub Check: Build and Verify on Windows (ciGroup1)
- GitHub Check: Run plugin functional tests on Linux
- GitHub Check: Run plugin functional tests on Windows
- GitHub Check: Lint and validate
- GitHub Check: lighthouse
- GitHub Check: bundle-analyzer
🔇 Additional comments (1)
src/plugins/explore/public/embeddable/explore_embeddable.tsx (1)
48-51: ImportingadaptLegacyDataalongsideconvertStringsToMappingslooks consistentThe new import cleanly reflects the added usage in
fetch()and keeps related visualization utilities co-located; no issues here.
| const styles = adaptLegacyData({ | ||
| type: selectedChartType, | ||
| styles: styleOptions, | ||
| axesMapping: visualization.axesMapping, | ||
| })?.styles; | ||
|
|
||
| this.searchProps.styleOptions = styles; | ||
|
|
There was a problem hiding this comment.
Preserve original style options when legacy adaptation yields no result
Right now you compute:
const styleOptions = visualization.params;const styles = adaptLegacyData(... )?.styles;this.searchProps.styleOptions = styles;toSpec(..., styles || styleOptions, ...)
If adaptLegacyData returns undefined (e.g., non-legacy or unsupported configs), styles is undefined, so:
toSpecstill sees the correct fallback viastyles || styleOptions,- but
this.searchProps.styleOptionsbecomesundefined, losing the original params that were previously always available to the embeddable.
To keep behavior consistent and still prefer adapted styles, you can fold the fallback into a single variable and use it both for searchProps and toSpec:
- const styleOptions = visualization.params;
-
- const styles = adaptLegacyData({
- type: selectedChartType,
- styles: styleOptions,
- axesMapping: visualization.axesMapping,
- })?.styles;
-
- this.searchProps.styleOptions = styles;
+ const styleOptions = visualization.params;
+
+ const adaptedStyles =
+ adaptLegacyData({
+ type: selectedChartType,
+ styles: styleOptions,
+ axesMapping: visualization.axesMapping,
+ })?.styles ?? styleOptions;
+
+ this.searchProps.styleOptions = adaptedStyles;
@@
- dateColumns,
- styles || styleOptions,
+ dateColumns,
+ adaptedStyles,This keeps searchProps.styleOptions always populated (original or adapted) and aligns it with what is actually sent into toSpec.
Also applies to: 421-421
🤖 Prompt for AI Agents
In src/plugins/explore/public/embeddable/explore_embeddable.tsx around lines
408-415 (also apply at 421), adaptLegacyData may return undefined causing styles
to be undefined and this.searchProps.styleOptions to lose the original
visualization.params; create a single fallback variable (e.g., const
resolvedStyles = adaptLegacyData(...)?.styles || styleOptions) and use
resolvedStyles both to set this.searchProps.styleOptions and to pass into toSpec
so searchProps always retains the original params when adaptation yields no
result while still preferring adapted styles when present.
2 participants
Description
Issues Resolved
Screenshot
Testing the changes
Changelog
Check List
yarn test:jestyarn test:jest_integrationSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.