Repository navigation
feat(ui-core): ECharts-based /charts module to replace recharts - #34273
Conversation
…oss re-renders - Reference lines ride on their own hidden series on the first axes, so hiding a data series or reordering a composed chart no longer moves or removes them. - Series always emit areaStyle and tooltip, so a re-render that drops them clears the old ones under replaceMerge. - dataZoom has fixed ids and EChart re-applies the user's window on rebuild. - Horizontal bars list the first row at the top (inverse category axis). - toNumberOrNull only accepts numbers and non-blank numeric strings. - Component props (ReactNode) move to props.ts so the option builders and their types compile without React for the headless renderer.
karanh37
added
UI
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| valueFormatter: (value) => formatTooltipValue(value), | ||
| ...(props.formatter ? { formatter: props.formatter } : {}), |
There was a problem hiding this comment.
💡 Bug: Removed tooltip formatter/option overrides linger after re-render
EChart updates with notMerge: false, and only series, xAxis, yAxis, grid and dataZoom are in replaceMerge. tooltipConfig adds formatter through a conditional spread (...(props.formatter ? { formatter } : {})), so the key is simply missing when a caller drops tooltip.formatter. ECharts' component merge leaves missing keys alone, so the old top-level formatter keeps rendering. The PR says removed tooltip formatters are cleared, but the integration test only covers the per-series valueFormatter. The same happens to any tooltip/legend/aria keys a caller later removes from the option override. Fix: always write the key (formatter: props.formatter). ECharts merges with overwrite, so an explicit undefined clears the old value, which is the same approach areaStyle/series tooltip already use. Optionally, add tooltip to REPLACE_MERGE_KEYS.
Always set formatter so a removal clears it on merge:
valueFormatter: (value) => formatTooltipValue(value),
// Always set, so a re-render that drops the formatter clears the old one.
formatter: props.formatter,
});
Was this helpful? React with 👍 / 👎
| export const hexToRgba = (hex: string, alpha: number): string => { | ||
| const clean = hex.replace('#', ''); | ||
| const full = | ||
| clean.length === 3 | ||
| ? clean | ||
| .split('') | ||
| .map((c) => c + c) | ||
| .join('') | ||
| : clean; | ||
| const channel = (start: number) => | ||
| Number.parseInt(full.slice(start, start + 2), 16) || 0; | ||
|
|
||
| return `rgba(${channel(0)}, ${channel(2)}, ${channel(4)}, ${alpha})`; | ||
| }; |
There was a problem hiding this comment.
💡 Edge Case: hexToRgba produces wrong area fill for non-hex series colours
ChartSeries.color accepts any string, but areaGradient sends it through hexToRgba, which only understands #RGB/#RRGGBB. Any other value, such as rgb(...), hsl(...) or a named colour, gets parsed as hex. For example, 'red' becomes 'rreedd' and gives rgba(0, 238, 221, 0.2), while rgb(...) falls back to black. So an area chart with such a colour draws a fill that doesn't match its line. Fix: return non-hex input unchanged and use opacity on areaStyle, or validate the input with a /^#([0-9a-f]{3}|[0-9a-f]{6})$/i check and fall back to the raw colour plus areaStyle.opacity.
Only convert real hex values; leave other colours untouched:
export const hexToRgba = (hex: string, alpha: number): string => {
if (!/^#([0-9a-f]{3}|[0-9a-f]{6})$/i.test(hex)) {
return hex;
}
const clean = hex.slice(1);
const full = clean.length === 3 ? clean.replace(/./g, (c) => c + c) : clean;
const channel = (start: number) =>
Number.parseInt(full.slice(start, start + 2), 16);
return `rgba(${channel(0)}, ${channel(2)}, ${channel(4)}, ${alpha})`;
};
Was this helpful? React with 👍 / 👎
Jest test CoverageUI tests summary
|
✅ Playwright Results — workflow succeededValidated commit ✅ 40 passed · ❌ 0 failed · 🟡 0 flaky · ⏭️ 0 skipped · 🧰 0 lifecycle flaky PerformanceBlocking targets: ✅ met · Optimization targets: 🟡 in progress Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting. 🕒 Full workflow signal wall (to summary) 21m 41s ⏱️ Max setup 2m 46s · max shard execution 3m 0s · max shard-job elapsed before upload 6m 5s · reporting 2s 🌐 162.00 requests/attempt · 1.25 app boots/UI scenario · 0.00% common-shard skew Optimization targets still in progress:
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
GeoMapChart draws a choropleth over caller-supplied geometry. The caller owns the map data: loading and trimming the GeoJSON, and resolving raw region values to feature names through resolveRegion. Rows are summed per region, raw values that match no feature are reported through onUnmatchedRegions, and the colour scale spans the data's own range. buildGeoMapOption stays Node-safe for the headless renderer.
- onUnmatchedRegions reports on mount and when the unmatched set changes, never on every parent render, so inline callbacks cannot loop. - visualMap joins the replaceMerge keys, so a removed colour-scale setting, or a switch from a geo option to bars on the same chart, clears it. - Map regions no longer stay selected after a click (selectedMode false).
The Storybook toolbar theme toggle already covers light and dark for every story.
Adds a choropleth over simplified public-domain US state geometry, with Alaska and Hawaii as insets and postal codes resolved to state names.
Code Review 👍 Approved with suggestions 0 closed / 2 findings🟡 Medium risk · Adds a shared charting module and new public chart APIs for later migrations. Introduces a comprehensive ECharts-based chart module with typed components, theme support, and server-side rendering compatibility. Two minor issues to address: tooltip formatter and other top-level option overrides linger after re-render because missing keys are not explicitly set to 💡 Bug: Removed tooltip formatter/option overrides linger after re-render📄 openmetadata-ui-core-components/src/main/resources/ui/src/charts/options/common.ts:92-93 📄 openmetadata-ui-core-components/src/main/resources/ui/src/charts/options/merge.ts:42-48
Always set formatter so a removal clears it on merge💡 Edge Case: hexToRgba produces wrong area fill for non-hex series colours📄 openmetadata-ui-core-components/src/main/resources/ui/src/charts/options/common.ts:51-64 📄 openmetadata-ui-core-components/src/main/resources/ui/src/charts/options/common.ts:67-77
Only convert real hex values; leave other colours untouched🤖 Prompt for agentsOptionsDisplay: compact → Counting what did not apply, without listing it. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
|
Describe your changes:
Fixes
I added an ECharts-based chart module to
@openmetadata/ui-core-components, exposed as a new@openmetadata/ui-core-components/chartsentry. This is phase 0 of removing recharts from both OpenMetadata and Collate. The goal is one shared chart layer, with one palette, one theme and one set of components, instead of about 50 hand-built recharts charts spread acrossopenmetadata-uiandcollate-ui. Nothing uses the module yet. Later PRs migrate the charts area by area, then drop therechartsdependency.charts/options/): plain functions that turn data-shaped props into an ECharts option, for line, area, bar, composed and pie charts. They import no React and touch no DOM, so Collate's server-side PNG renderer can run them in Node. A test guards this.EChart: a wrapper aroundecharts-for-react/lib/corewith a tree-shakenecharts/coreand the SVG renderer..dark-modeclass on any ancestor or on<html>.Skeletonwhile loading, and an empty state that reuseslabel.no-data-found, so there are no new i18n keys.ariaLabeland the series names.LineChart,AreaChart,BarChart,ComposedChartandPieChart. Their props are shaped like the data, so migrating a recharts chart is mostly mapping props. Each also accepts anoptionoverride for unusual cases.GeoMapChart: a generic choropleth. The caller supplies everything map-specific: the GeoJSON (already loaded and trimmed), themapNameit is registered under, and an optionalresolveRegionthat turns raw values such asCAor06into feature names. Core bundles no map data or region tables. Collate's chart renderer keeps its USA and world maps, region aliases and inset trimming, and passes them in.onUnmatchedRegions; when nothing matches, the chart shows its empty state.onRegionClickreports clicks.buildGeoMapOptionstays Node-safe.Type of change:
High-level design:
EChartrenders any ECharts option with the shared theme and the shared loading, empty and accessibility handling. The typed components are built on top of it and cover the chart shapes the apps use today:ReferenceLine) and an optional zoom slider (for rechartsBrush)/chartshas its ownexportsentry and is not re-exported from the root. Apps that render no chart never download echarts.echartsandecharts-for-reactare optional peer dependencies and are external in the Vite build;dist/charts/index.es.jsis about 15 KB.ChartOptionis aComposeOption<…>limited to the chart types and components we register, so an option using anything else fails the type check.notMerge: falsewithreplaceMerge: ['series', 'xAxis', 'yAxis', 'grid', 'dataZoom']:visualMap) setting the caller removes, or a switch from a map to another chart type on the sameEChart, is cleared.echarts/echarts-for-reacttoopenmetadata-ui;collate-uialready has them.Tests:
Use cases covered
<html>or from a scoped.dark-modeancestor, and re-themes when<html>changes.Unit tests
src/charts/**/*.test.ts(x):palette,theme,format,options/merge,options/cartesian,options/pie,options/geo,echart,components,geo-map-chart,node-safety,echarts-integration(136 tests). Full core suite: 553 passing.Backend integration tests
Ingestion integration tests
Playwright (UI) tests
Manual testing performed
yarn test,yarn type-check,yarn lint,yarn buildandyarn build-storybookinopenmetadata-ui-core-components/src/main/resources/ui.dist/charts/index.es.js: echarts is imported, not bundled, and the root entry has no echarts.EChart.UI screen recording / screenshots:
New Charts/Overview stories in Storybook. The module has no consumers yet, so no app screen changes.
Checklist:
I have read the CONTRIBUTING document.
My PR title is
Fixes <issue-number>: <short explanation>My PR is linked to a GitHub issue via
Fixes #<issue-number>above.I have commented on my code, particularly in hard-to-understand areas.
For JSON Schema changes: I updated the migration scripts or explained why it is not needed.
For UI changes: I attached a screen recording and/or screenshots above.
I have added tests (unit / integration / Playwright as applicable) and listed them above.
The issue properly describes why the new feature is needed, what's the goal, and how we are building it. Any discussion or decision-making process is reflected in the issue.
I have updated the documentation.
I have added tests around the new logic.
🤖 Generated with Claude Code