Migrate ECharts usage to LayerChart #251
Conversation
WalkthroughThis PR migrates chart functionality from ECharts to the new LayerChart library. Dependencies in the package file were updated accordingly, and new Svelte components—FeaturesLineChart and PercentileLineChart—were introduced with interactive tooltips and color scales. Legacy ECharts components, utilities, and related functions have been removed. Page components were updated to simplify chart logic and state management. Additional modifications include updates to common chart types, constants, and the Tailwind configuration to support LayerChart styling. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Page
participant ChartComponent
participant LayerChartLib
User->>Page: Interact (click/hover)
Page->>ChartComponent: Send data & events
ChartComponent->>LayerChartLib: Render chart with config
LayerChartLib-->>ChartComponent: Render update (tooltips, colors)
ChartComponent-->>Page: Dispatch interaction events
Page-->>User: Update display
Possibly related PRs
Poem
Tip 🌐 Web search-backed reviews and chat
Warning Review ran into problems🔥 ProblemsGitHub Actions and Pipeline Checks: Resource not accessible by integration - https://docs.github.com/rest/actions/workflow-runs#list-workflow-runs-for-a-repository. Please grant the required permissions to the CodeRabbit GitHub App under the organization or repository settings. ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
frontend/src/routes/joins/[slug]/observability/distributions/+page.svelte (1)
Line range hint
2-87: Remove commented out ECharts code.Clean up the file by removing the commented sections instead of keeping them. This improves maintainability.
-// import { untrack } from 'svelte'; -// import { connect, type EChartsType, type EChartOption } from 'echarts'; [... remove all commented sections ...]
🧹 Nitpick comments (6)
frontend/src/routes/joins/[slug]/observability/drift/+page.svelte (4)
47-54: UpdateresetZoomfunction for LayerChart.
Line range hint
68-161: Remove large block of commented-out code.
203-203: Avoid usinganyin_selectedEvent.
292-293: ReplaceResetZoomButtonwith LayerChart equivalent.frontend/src/routes/joins/[slug]/observability/distributions/+page.svelte (1)
122-124: Consider responsive height.Fixed height might not work well on different screen sizes.
-<div class="h-[230px]"> +<div class="h-[230px] md:h-[300px] lg:h-[400px]">frontend/src/lib/components/charts/PercentileLineChart.svelte (1)
16-49: Make percentile colors configurable.Consider moving the hardcoded colors to a configuration object or props.
+type Props = { + colors?: { + p95?: string; + p50?: string; + p5?: string; + }; data: TimeSeriesItem[]; } & ComponentProps<typeof LineChart>; -let { data, ...restProps } = $props<Props>(); +let { data, colors = { + p95: '#4B92FF', + p50: '#7DFFB3', + p5: '#FDDD61' +}, ...restProps } = $props<Props>();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
frontend/package.json(1 hunks)frontend/src/lib/components/charts/FeaturesLineChart.svelte(1 hunks)frontend/src/lib/components/charts/PercentileLineChart.svelte(1 hunks)frontend/src/lib/components/charts/common.ts(1 hunks)frontend/src/lib/util/format.ts(2 hunks)frontend/src/routes/joins/[slug]/observability/distributions/+page.svelte(3 hunks)frontend/src/routes/joins/[slug]/observability/drift/+page.svelte(7 hunks)frontend/tailwind.config.js(2 hunks)
🔇 Additional comments (9)
frontend/src/lib/util/format.ts (1)
Line range hint
4-28:formatDatefunction enhancements look good.frontend/src/lib/components/charts/common.ts (1)
1-64: Chart common properties are well-defined.frontend/src/lib/components/charts/FeaturesLineChart.svelte (3)
1-18: Clean TypeScript setup with proper type definitions!
40-62: Well-implemented tooltip with proper formatting and error handling!
20-39: Consider legend overflow handling for many features.The legend placement at bottom-left with scrollbar-none might hide features when there are many series.
✅ Verification successful
Legend overflow is properly handled ✓
The current implementation already includes proper overflow handling with
overflow-auto, and the hidden scrollbar is an intentional design choice.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential feature count issues rg -l "feature:" | xargs rg "feature:" -A 1 | grep -c "feature:"Length of output: 63
frontend/tailwind.config.js (2)
8-8: Correct Tailwind configuration for LayerChart!
81-87: Clarify surface.300 color mapping.The comment indicates uncertainty about the correct color. Consider adding a new CSS variable for this specific use case.
frontend/src/lib/components/charts/PercentileLineChart.svelte (1)
1-14: Clean component setup with proper TypeScript definitions!frontend/package.json (1)
69-71: Verify the migration status.Both
echartsandlayerchartare present. If migration is complete, removeecharts.
There was a problem hiding this comment.
@ken-zlai and I talked about changing the endpoints to no-longer produce time-points and directly consume double arrays and timestamp arrays - as defined within TileSummarySeries and TileDriftSeries thrift objects (using the thrift router work that is merged recently)
It might save you effort to do this migration on top of what Ken was planning to do. Or you could even pick that task up. Happy to walk you through what needs to be done on that task as well.:-)
|
Thanks for the heads up @nikhil-zlai. Might be good for us to sync up to make sure I understand everything. I see these thrift generated interfaces in my local codegen'd files.
Is the driver for this change to reduce the amount of over-the-wire transfer size (and less transforming on the backend)? |
Lets do it! Please throw a meeting on my cal any day this week after 11 PST.
That's right. Sending separate arrays of floats and timestamps is optimal, specially when there are many series in a single chart - we don't need to duplicate timestamps for each series / line, and we don't need to duplicate the label string. Basically this positions the front-end to deal with as few strings as possible. So, one additional benefit is also to reduce the amount of transformation needed in the front-end library. For example, echarts/uplot takes separate float arrays per series and timestamp array. |
|
At the moment LayerChart does not support separating out per-dimension data (but does support data per series either on a common data structure (property per series) or data split per series. Supporting separating dimension data will take some thought, but we can transform the data client side for now (and still have the over the network benefits). As I type and think about this, this might be possible to handle via the dimension accessors as it's just needs look up based on index position from 2 different arrays (although I'm not sure if the index position is passed in all contexts). Oh well, something to think about, and transforming client side is still possible (just more intensive on the client). |
|
how do you feel about doing some simple benchmarking prior to our meeting? we can try to use arrays per label like in echart / uplot, vs map per tick like in layer chart in a simple benchmark. for baseline: https://leeoniya.github.io/uPlot/bench/uPlot.html I am very curious to see the delta in perf. |
I'll try to setup some benchmarks before our meeting, but TBH I wouldn't be surprised if LayerChart isn't a few orders slower than uplot based on their published benchmarks since that is their primary focus.
I'll start by setting up a LayerChart example using data.json from their benchmark and see what kind of limits I encounter. Some of the benefits using LayerChart over echarts/uplot
Here's a quick recording of this PR using LayerChart (using SVG render context ATM but might switch to canvas in the future with a simple CleanShot.2025-01-20.at.22.28.06-converted.mp4Performance wise "feels" very similar to the current echarts solution, but I've also also improved the tooltip handling (tracking data with smart container positioning) and replaced the null data ratios with pie charts (which wouldn't be possible with uplot). There is a lot of performance tuning that can be improved on, especially on pages with multiple charts (only rendering based on viewport visibility, etc). |
|
Chiming in here - I was going to start this thrift migration today (ticket). However, given this migration and our chat tomorrow, I can pick up a few other tasks in the meantime to keep me busy. We can decide tomorrow on who it makes the most sense to convert existing api's -> thrift. |
sean-zlai
force-pushed
the
sean/echarts-to-layerchart2
branch
3 times, most recently
from
6130fdb to
f4dafc8
Compare
sean-zlai
force-pushed
the
sean/echarts-to-layerchart2
branch
2 times, most recently
from
ebe38bc to
6dc0099
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
frontend/src/routes/joins/[slug]/observability/distributions/+page.svelte (1)
20-27: Enhance error handling.Add error state UI and logging service integration.
} catch (err) { - console.error('Error loading distributions:', err); + const error = err instanceof Error ? err.message : String(err); + console.error('Error loading distributions:', error); + isLoadingDistributions = false; + distributions = []; }frontend/src/routes/joins/[slug]/observability/drift/+page.svelte (1)
61-110: Improve error handling and loading state.Add loading state and user feedback for API errors.
+let isLoading = $state(false); +let error = $state<string | null>(null); async function selectSeriesPoint(seriesPoint: typeof selectedSeriesPoint) { + isLoading = true; + error = null; try { // ... existing code ... } catch (error) { console.error('Error fetching data:', error); + error = 'Failed to load feature data. Please try again.'; percentileData = null; comparedFeatureData = null; nullData = null; + } finally { + isLoading = false; } }
🧹 Nitpick comments (5)
frontend/src/lib/constants/common.ts (1)
1-2: Consider using Number.MIN_SAFE_INTEGER for NULL_VALUE.A more recognizable constant might be clearer than the magic number -1234567890.
-export const NULL_VALUE = -1234567890; +export const NULL_VALUE = Number.MIN_SAFE_INTEGER;frontend/src/lib/components/charts/common.ts (1)
6-24: Consider semantic naming for colors.Add semantic names to colors for better maintainability.
export const colors = [ - '#E5174B', - '#E54D4A', + '#E5174B', // primary + '#E54D4A', // secondary // ... rest of colors ];frontend/src/lib/components/charts/FeaturesLineChart.svelte (1)
82-85: Add debouncing to tooltip click handler.Prevent rapid-fire clicks from causing performance issues.
+import { debounce } from '$lib/util/debounce'; + <button class="col-span-full grid grid-cols-[1fr,auto] gap-2 hover:bg-neutral-400 py-1 px-3 rounded" - onclick={() => onitemclick?.({ series: s, data: seriesTooltipData, value })} + onclick={debounce(() => onitemclick?.({ series: s, data: seriesTooltipData, value }), 300)} >frontend/src/routes/joins/[slug]/observability/drift/+page.svelte (2)
35-48: Add error handling for xDomain updates.Consider validating xDomain values to prevent invalid zoom states.
let xDomain = $state<DomainType | undefined>(null); +function setXDomain(domain: DomainType | null) { + if (domain && (isNaN(domain[0]) || isNaN(domain[1]))) { + console.warn('Invalid xDomain values detected'); + return; + } + xDomain = domain; +}
259-282: Extract hardcoded colors to constants.Move hardcoded chart colors to a common configuration file.
+// In common.ts +export const CHART_COLORS = { + baseline: '#4B92FF', + current: '#7DFFB3' +} as const; -color: '#4B92FF' // TODO: copied from ECharts defaults +color: CHART_COLORS.baseline -color: '#7DFFB3' // TODO: copied from ECharts defaults +color: CHART_COLORS.current
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
frontend/package.json(1 hunks)frontend/src/lib/components/CustomEChartLegend.svelte(0 hunks)frontend/src/lib/components/EChart.svelte(0 hunks)frontend/src/lib/components/EChartTooltip.svelte(0 hunks)frontend/src/lib/components/PercentileChart.svelte(0 hunks)frontend/src/lib/components/charts/FeaturesLineChart.svelte(1 hunks)frontend/src/lib/components/charts/PercentileLineChart.svelte(1 hunks)frontend/src/lib/components/charts/common.ts(1 hunks)frontend/src/lib/constants/common.ts(1 hunks)frontend/src/lib/util/chart-options.svelte.ts(0 hunks)frontend/src/lib/util/chart.ts(0 hunks)frontend/src/routes/joins/[slug]/observability/distributions/+page.svelte(3 hunks)frontend/src/routes/joins/[slug]/observability/drift/+page.svelte(4 hunks)frontend/src/routes/observability/+page.svelte(0 hunks)frontend/tailwind.config.js(2 hunks)
💤 Files with no reviewable changes (7)
- frontend/src/routes/observability/+page.svelte
- frontend/src/lib/util/chart-options.svelte.ts
- frontend/src/lib/components/PercentileChart.svelte
- frontend/src/lib/components/CustomEChartLegend.svelte
- frontend/src/lib/components/EChart.svelte
- frontend/src/lib/components/EChartTooltip.svelte
- frontend/src/lib/util/chart.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/package.json
- frontend/src/lib/components/charts/PercentileLineChart.svelte
🔇 Additional comments (4)
frontend/tailwind.config.js (2)
8-8: LGTM!Content configuration correctly includes LayerChart files for style processing.
98-104: Verify color mapping for surface.300.The comment indicates uncertainty about the color mapping. Consider defining a new CSS variable for this specific use case.
frontend/src/routes/joins/[slug]/observability/drift/+page.svelte (2)
2-30: LGTM!Imports and type definitions are properly structured for LayerChart integration.
335-346: LGTM!Keyboard event handling properly manages tooltip locking across platforms.
sean-zlai
force-pushed
the
sean/echarts-to-layerchart2
branch
from
91f1421 to
e76e315
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
frontend/src/lib/components/charts/PercentileLineChart.svelte (1)
20-54: Extract color constants for better maintainability.Move color values to the common.ts file for consistency and reuse.
- { label: 'p95', color: '#4B92FF' }, - { label: 'p50', color: '#7DFFB3' }, - { label: 'p5', color: '#FDDD61' } + { label: 'p95', color: colors.p95 }, + { label: 'p50', color: colors.p50 }, + { label: 'p5', color: colors.p5 }frontend/src/lib/components/charts/common.ts (1)
26-44: Use Tailwind classes for consistent styling.Replace hardcoded colors with Tailwind classes in tooltipProps.
- class: 'text-small bg-neutral-200 border border-neutral-400 rounded-md shadow-lg', + class: 'text-small bg-neutral-200 border-neutral-400 rounded-md shadow-lg',frontend/src/routes/joins/[slug]/observability/distributions/+page.svelte (1)
77-84: Add tests for the chart component.The chart component lacks test coverage.
Would you like me to help create tests for the chart interactions and rendering?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
frontend/package.json(1 hunks)frontend/src/lib/components/CustomEChartLegend.svelte(0 hunks)frontend/src/lib/components/EChart.svelte(0 hunks)frontend/src/lib/components/EChartTooltip.svelte(0 hunks)frontend/src/lib/components/PercentileChart.svelte(0 hunks)frontend/src/lib/components/charts/FeaturesLineChart.svelte(1 hunks)frontend/src/lib/components/charts/PercentileLineChart.svelte(1 hunks)frontend/src/lib/components/charts/common.ts(1 hunks)frontend/src/lib/constants/common.ts(1 hunks)frontend/src/lib/util/chart-options.svelte.ts(0 hunks)frontend/src/lib/util/chart.ts(0 hunks)frontend/src/routes/joins/[slug]/observability/distributions/+page.svelte(3 hunks)frontend/src/routes/joins/[slug]/observability/drift/+page.svelte(4 hunks)frontend/src/routes/observability/+page.svelte(0 hunks)frontend/tailwind.config.js(2 hunks)
💤 Files with no reviewable changes (7)
- frontend/src/routes/observability/+page.svelte
- frontend/src/lib/util/chart-options.svelte.ts
- frontend/src/lib/components/PercentileChart.svelte
- frontend/src/lib/components/CustomEChartLegend.svelte
- frontend/src/lib/components/EChart.svelte
- frontend/src/lib/util/chart.ts
- frontend/src/lib/components/EChartTooltip.svelte
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/src/lib/constants/common.ts
- frontend/tailwind.config.js
- frontend/package.json
- frontend/src/lib/components/charts/FeaturesLineChart.svelte
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: enforce_triggered_workflows
🔇 Additional comments (2)
frontend/src/lib/components/charts/PercentileLineChart.svelte (2)
1-16: LGTM! Well-structured type definitions.
17-18: LGTM! Props handling follows Svelte conventions.
| onmouseenter={() => { | ||
| /*highlightSeries(selectedSeries ?? '', dialogGroupChart, 'highlight')*/ | ||
| }} | ||
| onmouseleave={() => { | ||
| /*highlightSeries(selectedSeries ?? '', dialogGroupChart, 'downplay')*/ | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement or remove commented highlight functionality.
Either implement the highlight functionality or remove the commented code.
- onmouseenter={() => {
- /*highlightSeries(selectedSeries ?? '', dialogGroupChart, 'highlight')*/
- }}
- onmouseleave={() => {
- /*highlightSeries(selectedSeries ?? '', dialogGroupChart, 'downplay')*/
- }}
+ onmouseenter={() => {
+ // TODO: Implement highlight functionality using LayerChart API
+ }}
+ onmouseleave={() => {
+ // TODO: Implement downplay functionality using LayerChart API
+ }}📝 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.
| onmouseenter={() => { | |
| /*highlightSeries(selectedSeries ?? '', dialogGroupChart, 'highlight')*/ | |
| }} | |
| onmouseleave={() => { | |
| /*highlightSeries(selectedSeries ?? '', dialogGroupChart, 'downplay')*/ | |
| }} | |
| onmouseenter={() => { | |
| // TODO: Implement highlight functionality using LayerChart API | |
| }} | |
| onmouseleave={() => { | |
| // TODO: Implement downplay functionality using LayerChart API | |
| }} |
| <!-- TODO: Remove mockNullValue once data is populated --> | ||
| {@const mockNullValue = Math.random() * 100} | ||
| <div class="grid gap-2"> | ||
| <div class="h-[230px]"> | ||
| <!-- TODO: colors copied from ECharts defaults --> | ||
| <PieChart | ||
| data={[ | ||
| { | ||
| label: 'Null Value Percentage', | ||
| // value: c.data.nullValue | ||
| value: mockNullValue | ||
| }, | ||
| { | ||
| label: 'Non-null Value Percentage', | ||
| // value: 100 - c.data.nullValue | ||
| value: 100 - mockNullValue | ||
| } |
There was a problem hiding this comment.
Remove mock data and implement actual null value handling.
Replace mockNullValue with actual data from the API.
- {@const mockNullValue = Math.random() * 100}
+ {@const nullValue = c.data.nullValue}
- value: mockNullValue
+ value: nullValue
- value: 100 - mockNullValue
+ value: 100 - nullValue📝 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.
| <!-- TODO: Remove mockNullValue once data is populated --> | |
| {@const mockNullValue = Math.random() * 100} | |
| <div class="grid gap-2"> | |
| <div class="h-[230px]"> | |
| <!-- TODO: colors copied from ECharts defaults --> | |
| <PieChart | |
| data={[ | |
| { | |
| label: 'Null Value Percentage', | |
| // value: c.data.nullValue | |
| value: mockNullValue | |
| }, | |
| { | |
| label: 'Non-null Value Percentage', | |
| // value: 100 - c.data.nullValue | |
| value: 100 - mockNullValue | |
| } | |
| <!-- TODO: Remove mockNullValue once data is populated --> | |
| {@const nullValue = c.data.nullValue} | |
| <div class="grid gap-2"> | |
| <div class="h-[230px]"> | |
| <!-- TODO: colors copied from ECharts defaults --> | |
| <PieChart | |
| data={[ | |
| { | |
| label: 'Null Value Percentage', | |
| // value: c.data.nullValue | |
| value: nullValue | |
| }, | |
| { | |
| label: 'Non-null Value Percentage', | |
| // value: 100 - c.data.nullValue | |
| value: 100 - nullValue | |
| }, |
ken-zlai
left a comment
ken-zlai
left a comment
There was a problem hiding this comment.
@sean-zlai This is really excellent work here, code largely looks good. Charts are snappy and the net-negative code is awesome!
A couple of things, mainly UI related:
- I noticed that things like the tooltip and legend are slightly different than the figma/previous version (mainly padding and sizing). Curious if we plan on matching these in the future.
- One improvement would be to show a cursor: pointer when they are hovering over a clickable point
- The legend previously had an expand/hide functionality if there was overflow. Meaning, if there were too many series you could click a button to show/hide them all. Now I notice that it scrolls left and right. I'm not picky on the treatment, but think that the expand functionality is a bit more intuitive. However, I remember it being a bit tricky to implement so if reusing the code I had is not easy then lets not do it.
Let me know if we plan on fixing here/in the future. I'm happy to create a few tickets if we want to.
Thanks, and great feedback! See responses below
I had tighten it up a little, but we can definitely align to the design. It's mostly tweaking padding/margin classes here. I'll take a look and adjust.
Very good feedback. If it was using <BarChart props={{ highlight: { points: { class: 'cursor-pointer' } } }}>but since we're using
Yeah, I looked at this for a bit and the biggest challenge is the I also considered implementing a button that scrolls the overflow for you (kind of similar to a carousel), similar to the Skeleton Multi Column example or MUI's scrollable tabs example. This would allow the legend to remain a single line, but with a nice way to paginate through all the options without requiring horizontal swiping, which isn't convenient on a traditional mouse (trackpad and magic mouse aren't bad though). I could also look into exposing more chart state externally (already do for tooltip and brush) but it would take some thought. I'll knock out #1 and #2 and let me know if #3 is worth waiting before we merge. Thanks for the review! |
|
@sean-zlai Sounds like a plan to get #1 and #2. As for #3, I'm open to a simple treatment - the MUI's scrollable tabs is generally nice since you can see when there is overflow/click thru if needed. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/src/lib/components/charts/FeaturesLineChart.svelte (2)
27-31: Consider memoizing the color scale.The color scale is recomputed on every change to features.
-const colorScale = $derived(scaleOrdinal<string>().domain(features).range(colors)); +const colorScale = $derived(() => { + const scale = scaleOrdinal<string>(); + return scale.domain(features).range(colors); +})();
70-102: Consider accessibility improvements.Add ARIA labels for interactive elements.
<button class="col-span-full grid grid-cols-[1fr,auto] gap-4 hover:bg-neutral-400 py-2 px-3 rounded" + aria-label={`View details for ${s.label ?? s.key}`} onclick={() => onitemclick?.({ series: s, data: seriesTooltipData, value })} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
frontend/src/lib/components/charts/FeaturesLineChart.svelte(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: enforce_triggered_workflows
- GitHub Check: enforce_triggered_workflows
🔇 Additional comments (4)
frontend/src/lib/components/charts/FeaturesLineChart.svelte (4)
1-12: LGTM! Clean imports and dependencies.All necessary imports are present and well-organized.
13-26: LGTM! Well-defined TypeScript types.Props interface is clear and type-safe.
55-68: LGTM! Clean mark point implementation.The mark point visualization is well-implemented.
33-54: Consider adding error boundaries.The chart might fail if data is malformed.
Add error handling for invalid data:
+{#if data.length === 0} + <div class="text-center text-gray-500">No data available</div> +{:else} <LineChart ... > +{/if}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/src/lib/components/charts/FeaturesLineChart.svelte (3)
3-11: Consider organizing imports.Group related imports:
- External dependencies (layerchart, d3)
- Internal components/utils
- Types
- Constants
import type { ComponentProps } from 'svelte'; import { accessor, Circle, findRelatedData, Line, LineChart, Tooltip } from 'layerchart'; import { scaleOrdinal } from 'd3'; + +import type { TimeSeriesItem } from '$lib/types/Model/Model'; import { colors, lineChartProps, tooltipProps, type DateValue } from './common'; -import type { TimeSeriesItem } from '$lib/types/Model/Model'; import { formatDate, formatValue } from '$lib/util/format'; import { Badge } from '../ui/badge'; import { isMacOS } from '$src/lib/util/browser'; import { NULL_VALUE } from '$src/lib/constants/common';
33-54: Consider performance optimization for large datasets.The chart uses canvas rendering which is good for performance. However, data transformation in the series prop could be optimized.
- series={data.map((d) => { - return { - key: d.feature, - data: d.points.map((p) => { - return { - date: new Date(p.ts), - value: p.value === NULL_VALUE ? null : p.value - }; - }), - color: colorScale(d.feature) - }; - })} + series={$derived(data.map((d) => ({ + key: d.feature, + data: d.points.map((p) => ({ + date: new Date(p.ts), + value: p.value === NULL_VALUE ? null : p.value + })), + color: colorScale(d.feature) + })))}
82-93: Consider accessibility improvements.Add ARIA attributes for better screen reader support.
<button class="col-span-full grid grid-cols-[1fr,auto] gap-6 hover:bg-neutral-400 py-2 px-3 rounded" onclick={() => onitemclick?.({ series: s, data: seriesTooltipData, value })} + role="button" + aria-label={`View details for ${s.label ?? (s.key !== 'default' ? s.key : 'value')}`} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
frontend/src/lib/components/charts/FeaturesLineChart.svelte(1 hunks)
🔇 Additional comments (3)
frontend/src/lib/components/charts/FeaturesLineChart.svelte (3)
13-25: LGTM! Well-defined types.Clean type definitions with good use of utility types.
27-30: LGTM! Clean state management.Efficient use of derived state for features and color scale.
78-80: Verify type safety of value accessor.The type cast
(y as unknown)seems suspicious.
|
@ken-zlai #1 and #2 are done. The tooltip items are not 100% matching, but much closer (removed vertical gap between items, gave each item more height via padding, and increased gap between label and value). #3 will take some time. Would you be OK if we merge as is and circle back after landing #318? That PR is really close once I figure out the backend |
|
@sean-zlai Sounds good to me! Here is a ticket for #3 |
…ne charts interactions
…raction improvements)
…interdown` to BrushContext if defined)
…other improvements)
…tting brush range)
sean-zlai
force-pushed
the
sean/echarts-to-layerchart2
branch
from
6928aab to
1708448
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/src/lib/components/charts/PercentileLineChart.svelte (2)
12-15: Add JSDoc documentation for Props type.Document the expected shape and purpose of each prop.
+/** + * @typedef Props + * @property {TimeSeriesItem[]} data - Array of time series data points + * @property {BrushProps['onbrushend']} [onbrushend] - Optional callback when brush selection ends + */ type Props = { data: TimeSeriesItem[]; onbrushend?: BrushProps['onbrushend']; } & Omit<LineChartProps, 'data'>;
24-26: Extract color values to constants.Move color values to a constants file for better maintainability.
+const PERCENTILE_COLORS = { + P95: '#4B92FF', + P50: '#7DFFB3', + P5: '#FDDD61' +} as const; + { label: 'p95', color: '#4B92FF' }, { label: 'p50', color: '#7DFFB3' }, { label: 'p5', color: '#FDDD61' }frontend/src/routes/joins/[slug]/observability/distributions/+page.svelte (1)
77-84: Use CSS variable for chart height.Extract the fixed height to a CSS variable for consistency.
+<style> + :root { + --chart-height: 230px; + } +</style> -<div class="h-[230px]"> +<div class="h-[var(--chart-height)]">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
frontend/package.json(1 hunks)frontend/src/lib/components/CustomEChartLegend.svelte(0 hunks)frontend/src/lib/components/EChart.svelte(0 hunks)frontend/src/lib/components/EChartTooltip.svelte(0 hunks)frontend/src/lib/components/PercentileChart.svelte(0 hunks)frontend/src/lib/components/charts/FeaturesLineChart.svelte(1 hunks)frontend/src/lib/components/charts/PercentileLineChart.svelte(1 hunks)frontend/src/lib/components/charts/common.ts(1 hunks)frontend/src/lib/constants/common.ts(1 hunks)frontend/src/lib/util/chart-options.svelte.ts(0 hunks)frontend/src/lib/util/chart.ts(0 hunks)frontend/src/routes/joins/[slug]/observability/distributions/+page.svelte(3 hunks)frontend/src/routes/joins/[slug]/observability/drift/+page.svelte(4 hunks)frontend/src/routes/observability/+page.svelte(0 hunks)frontend/tailwind.config.js(2 hunks)
💤 Files with no reviewable changes (7)
- frontend/src/routes/observability/+page.svelte
- frontend/src/lib/components/CustomEChartLegend.svelte
- frontend/src/lib/components/EChart.svelte
- frontend/src/lib/util/chart.ts
- frontend/src/lib/components/EChartTooltip.svelte
- frontend/src/lib/util/chart-options.svelte.ts
- frontend/src/lib/components/PercentileChart.svelte
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/lib/constants/common.ts
- frontend/package.json
- frontend/tailwind.config.js
🔇 Additional comments (9)
frontend/src/lib/components/charts/common.ts (5)
1-4: Clean imports and type definition.
26-44: Well-structured tooltip config.Good choice disabling motion for performance.
46-48: Clean highlight config.
50-71: Test grid line visibility.Verify if 30% opacity provides sufficient contrast for grid lines.
6-24: Verify color accessibility.Check color contrast ratios for accessibility compliance.
frontend/src/routes/joins/[slug]/observability/distributions/+page.svelte (1)
37-38: LGTM! Clean state management.Good use of derived state for zoom tracking.
frontend/src/lib/components/charts/FeaturesLineChart.svelte (2)
179-184: Implement or remove commented highlight functionality.Either implement the highlight functionality or remove the commented code.
296-312: Remove mock data and implement actual null value handling.Replace mockNullValue with actual data from the API.
frontend/src/routes/joins/[slug]/observability/drift/+page.svelte (1)
335-346: 🛠️ Refactor suggestionAdd cleanup for keyboard event listeners.
Remove event listeners when component is destroyed.
+<script> + import { onDestroy } from 'svelte'; + + function handleKeyDown(e: KeyboardEvent) { + if (isMacOS() ? e.metaKey : e.ctrlKey) { + lockedTooltip = true; + } + } + + function handleKeyUp(e: KeyboardEvent) { + if (isMacOS() ? !e.metaKey : !e.ctrlKey) { + lockedTooltip = false; + } + } + + onDestroy(() => { + window.removeEventListener('keydown', handleKeyDown); + window.removeEventListener('keyup', handleKeyUp); + }); +</script> + -<svelte:window - onkeydown={(e) => { - if (isMacOS() ? e.metaKey : e.ctrlKey) { - lockedTooltip = true; - } - }} - onkeyup={(e) => { - if (isMacOS() ? !e.metaKey : !e.ctrlKey) { - lockedTooltip = false; - } - }} -/> +<svelte:window on:keydown={handleKeyDown} on:keyup={handleKeyUp} />Likely invalid or redundant comment.
## Summary Some of the benefits using LayerChart over echarts/uplot - Broad support of chart types (cartesian, polar, hierarchy, graph, force, geo) - Simplicity in setup and customization (composable chart components) - Responsive charts, both for viewport/container, and also theming (light/dark, etc) - Flexibility in design/stying (CSS variables, classes, color scales, etc) including transitions - Ability to opt into canvas or svg rendering context as the use case requires. LayerChart's canvas support also has CSS variable/styling support as well (which is unique as far as I'm aware). Html layers are also available, which are great for multiline text (with truncation). ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced new interactive charts, including `FeaturesLineChart` and `PercentileLineChart`, enhancing data visualization with detailed tooltips. - **Refactor** - Replaced legacy ECharts components with LayerChart components, streamlining chart interactions and state management. - **Chores** - Updated dependency configurations and Tailwind CSS settings for improved styling and performance. - Removed unused ECharts-related components and functions to simplify the codebase. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --- - To see the specific tasks where the Asana app for GitHub is being used, see below: - https://app.asana.com/0/0/1209163154826936 <!-- av pr metadata This information is embedded by the av CLI when creating PRs to track the status of stacks when using Aviator. Please do not delete or edit this section of the PR. ``` {"parent":"main","parentHead":"","trunk":"main"} ``` --> --------- Co-authored-by: Sean Lynch <[email protected]>
Summary
Some of the benefits using LayerChart over echarts/uplot
Checklist
Summary by CodeRabbit
FeaturesLineChartandPercentileLineChart, enhancing data visualization with detailed tooltips.