Figma match & more #71
Conversation
| .dark { | ||
| --background: 0 0% 3.9%; | ||
| --foreground: 0 0% 98%; | ||
| --background: var(--neutral-50); |
There was a problem hiding this comment.
love that we are parameterizing!
nikhil-zlai
left a comment
nikhil-zlai
left a comment
There was a problem hiding this comment.
LGTM! but i am the wrong person to approving I think.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (6)
frontend/src/lib/components/EChartTooltip/EChartTooltip.svelte (2)
11-28: Consider extracting series type and adding validation.Two suggestions for improvement:
- Extract the series type to a separate interface for reusability
- Add runtime validation for xValue when null
Consider this refactor:
+interface ChartSeries { + name: string | undefined; + value: number; + color: string; +} let { visible, xValue, series, clickable = false, xAxisCategories = undefined }: { visible: boolean; xValue: number | null; - series: Array<{ - name: string | undefined; - value: number; - color: string; - }>; + series: Array<ChartSeries>; clickable?: boolean; xAxisCategories?: string[]; } = $props(); +$: if (xValue === null && visible) { + console.warn('Tooltip is visible but xValue is null'); +}
32-48: Enhance type safety and defensive programming.The helper functions could benefit from:
- Custom event type for handleSeriesClick
- More defensive programming in getTooltipTitle
Consider these improvements:
+interface SeriesClickEvent { + componentType: 'series'; + data: [number | null, number]; + seriesName: string | undefined; +} -function handleSeriesClick(item: (typeof series)[number]) { +function handleSeriesClick(item: (typeof series)[number]): void { - dispatch('click', { + dispatch<SeriesClickEvent>('click', { componentType: 'series', data: [xValue, item.value], seriesName: item.name }); } function getTooltipTitle(xValue: number | null, xAxisCategories?: string[]): string { if (xValue === null) return ''; - if (xAxisCategories && typeof xValue === 'number' && Number.isInteger(xValue)) { + if (xAxisCategories?.length && typeof xValue === 'number' && Number.isInteger(xValue)) { + if (xValue < 0 || xValue >= xAxisCategories.length) { + console.warn(`xValue ${xValue} is out of bounds for xAxisCategories`); + return ''; + } return xAxisCategories[xValue]; } return formatDate(xValue); }frontend/src/lib/components/EChart/EChart.svelte (3)
14-16: Consider documenting the reason for the height changeThe height property has been changed from '200px' to '230px'. While this change might be necessary for accommodating the new tooltip, it would be helpful to document the reason for this specific value to prevent accidental modifications in the future.
87-102: Consider cross-platform keyboard shortcutsThe keyboard event handlers check for both
metaKey(Mac) andctrlKey(Windows/Linux), which is good. However, consider extracting this check into a reusable function for better maintainability and consistency across the codebase.+function isModifierKeyPressed(event: KeyboardEvent): boolean { + return event.metaKey || event.ctrlKey; +} function handleKeyDown(event: KeyboardEvent) { - if (event.metaKey || event.ctrlKey) { + if (isModifierKeyPressed(event)) { isCommandPressed = true; disableChartInteractions(); } } function handleKeyUp(event: KeyboardEvent) { - if (!event.metaKey && !event.ctrlKey) { + if (!isModifierKeyPressed(event)) { isCommandPressed = false; enableChartInteractions(); if (!isMouseOverTooltip) { isTooltipVisible = false; } } }
324-329: Enhance chart accessibilityWhile adding
role="application"is a good start, consider adding more ARIA attributes to improve accessibility:<div bind:this={chartDiv} ondblclick={handleDoubleClick} role="application" + aria-label="Interactive chart" + aria-description={option.title?.text?.toString() ?? 'Chart visualization'} style="width: 100%; height: 100%;" ></div>frontend/src/routes/models/[slug]/+page.svelte (1)
1-39: Consider improving type safety for event handlersThe event handlers could benefit from stronger typing. Consider creating dedicated types for chart events and zoom events to improve type safety and maintainability.
+interface ChartZoomEvent { + start?: number; + end?: number; + batch?: Array<{ startValue: number; endValue: number }>; +} + +interface ChartClickEvent extends ECElementEvent { + fromTooltip?: boolean; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
frontend/src/lib/components/EChart/EChart.svelte(5 hunks)frontend/src/lib/components/EChartTooltip/EChartTooltip.svelte(1 hunks)frontend/src/routes/models/[slug]/+page.svelte(1 hunks)
🔇 Additional comments (2)
frontend/src/lib/components/EChartTooltip/EChartTooltip.svelte (1)
1-10: LGTM! Well-structured imports and TypeScript setup.
The imports are well-organized and the TypeScript setup is appropriate for the component's needs.
frontend/src/lib/components/EChart/EChart.svelte (1)
123-125: Enhance bar chart type detection
The current bar chart detection might be fragile as it only checks the first series. Consider handling cases where different series types are mixed in the same chart.
-isBarChart = series?.[0]?.type === 'bar';
+isBarChart = series?.every(s => s.type === 'bar') ?? false;
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
frontend/src/lib/components/NavigationBar/NavigationBar.svelte (5)
Line range hint
48-54: Add cleanup for debounced search functionThe debounced search function should be cleaned up when the component is destroyed to prevent potential memory leaks.
Move the debounce initialization into onMount and clean it up in its cleanup function:
-const debouncedSearch = debounce(async () => { +let debouncedSearch: Function; + +onMount(() => { + debouncedSearch = debounce(async () => { if (input.length > 0) { const response = await search(input); searchResults = response.items; } else { searchResults = []; } - }, 300); + }, 300); + + return () => { + debouncedSearch.cancel(); + }; +}); -onDestroy(() => { - debouncedSearch.cancel(); -});
90-91: Add aria-label to logo buttonThe logo button lacks an accessible label for screen readers.
-<Button variant="link" href="/" class="p-0 h-6 w-6"> +<Button variant="link" href="/" class="p-0 h-6 w-6" aria-label="Go to homepage"> <img src="/logo.png" alt="Zipline Logo" /> </Button>
Line range hint
164-165: Replace alert() with a proper notification systemUsing window.alert() for user interactions is not recommended as it blocks the UI and provides a poor user experience.
Consider implementing a proper notification system or toast messages. Would you like me to provide an example implementation?
Line range hint
162-166: Enhance dropdown menu accessibilityThe dropdown menu items lack proper ARIA attributes and keyboard navigation support.
-<DropdownMenuContent> +<DropdownMenuContent aria-label="User menu"> <DropdownMenuItem + role="menuitem" + aria-label="Open settings" onclick={() => alert('Settings clicked')}>Settings</DropdownMenuItem> <DropdownMenuItem + role="menuitem" + aria-label="Sign out" onclick={() => alert('Sign out clicked')}>Sign out</DropdownMenuItem> </DropdownMenuContent>
180-191: Add tooltips to explain disabled actionsThe quick actions are marked as disabled but don't provide any context to users about why they're unavailable or when they'll become available.
Consider adding tooltips to explain why these actions are disabled and what users need to do to enable them. For example:
-<CommandItem disabled> +<CommandItem disabled title="This feature is coming soon"> <Icon src={ExclamationTriangle} micro size="16" /> Show only models with alerts </CommandItem>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
frontend/src/lib/components/EChartTooltip/EChartTooltip.svelte(1 hunks)frontend/src/lib/components/NavigationBar/NavigationBar.svelte(5 hunks)frontend/src/lib/components/ui/badge/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/lib/components/EChartTooltip/EChartTooltip.svelte
- frontend/src/lib/components/ui/badge/index.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
frontend/src/routes/models/[slug]/+page.svelte (2)
69-89: Extract color array to constants fileThe color array should be moved to a separate constants file for better maintainability and reusability.
Create a new file
src/lib/constants/colors.ts:export const CHART_COLORS = [ '#E5174B', '#E54D4A', '#E17545', '#E3994C', '#DFAF4F', '#87BE52', '#53B167', '#4DA67D', '#4EA797', '#4491CE', '#4592CC', '#4172D2', '#5B5AD1', '#785AD4', '#9055D5', '#BF50D3', '#CB5587' ];Then update the code:
+import { CHART_COLORS } from '$lib/constants/colors'; + function createChartOption( customOption: Partial<EChartOption> = {}, customColors = false ): EChartOption { const defaultOption: EChartOption = { - color: customColors - ? [ - '#E5174B', - '#E54D4A', - // ... other colors - '#CB5587' - ] - : undefined, + color: customColors ? CHART_COLORS : undefined,
179-196: Optimize chart option computation with memoizationThe chart options are recomputed on every state change. Consider memoizing the computation to improve performance.
Apply this diff to add memoization:
+import { memoize } from 'lodash'; + +const createMemoizedChartOption = memoize( + (chartData: [string, number][], min: number, max: number, interval: number) => + createChartOption( + { + yAxis: { min, max, interval }, + series: [{ data: chartData, type: 'line' }] + }, + true + ), + (chartData, min, max, interval) => + `${JSON.stringify(chartData)}-${min}-${max}-${interval}` +); + const performanceChartOption = $derived( - createChartOption( - { - yAxis: { - min: 0, - max: 1, - interval: 0.2 - }, - series: [ - { - data: chartData, - type: 'line' - } - ] - }, - true - ) + createMemoizedChartOption(chartData, 0, 1, 0.2) );
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
frontend/src/lib/components/EChart/EChart.svelte (3)
17-17: Consider using a more specific type formarkPoint.The
markPointprop is typed asECElementEvent, but it's only used for itsdataproperty which contains number arrays. Consider creating a more specific type to better represent the expected structure.-markPoint?: ECElementEvent; +type MarkPointData = { + data: [number, number]; +}; +markPoint?: MarkPointData;Also applies to: 27-27
61-70: Add type safety to tooltip configuration.The
customTooltipOptionobject could benefit from explicit typing to ensure type safety and better IDE support.-const customTooltipOption = { +const customTooltipOption: EChartOption = { tooltip: { formatter: () => '', trigger: 'axis', axisPointer: { type: 'line' }, position: 'top' } };
384-389: Enhance tooltip accessibility.The tooltip container could benefit from additional ARIA attributes to improve screen reader support.
<div class="relative z-50 inline-block" role="tooltip" + aria-live="polite" + aria-atomic="true" onmouseenter={handleTooltipMouseEnter} onmouseleave={handleTooltipMouseLeave} >frontend/src/routes/models/[slug]/+page.svelte (1)
700-840: Add error boundary for dialog contentThe dialog content contains complex chart rendering and data processing that could fail. Consider adding error boundaries to gracefully handle failures.
Consider wrapping the dialog content with an error boundary component:
<Dialog bind:open={isSheetOpen}> <DialogContent class="max-w-[85vw] h-[95vh] flex flex-col p-0"> + <ErrorBoundary + fallback={(error) => ( + <div class="p-4 text-error"> + <h3>Error loading chart details</h3> + <p>{error.message}</p> + </div> + )} + > <!-- existing dialog content --> + </ErrorBoundary> </DialogContent> </Dialog>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
frontend/src/lib/components/EChart/EChart.svelte(5 hunks)frontend/src/routes/models/[slug]/+page.svelte(1 hunks)
🔇 Additional comments (4)
frontend/src/lib/components/EChart/EChart.svelte (2)
211-214: Improve null safety in tooltip positioning.
The code uses non-null assertion (!) which could lead to runtime errors. Consider adding proper null checks and early returns.
261-268: Add safety check for empty data array.
The reduce operation on firstSeries.data could throw if the array is empty. Consider adding a safety check.
frontend/src/routes/models/[slug]/+page.svelte (2)
358-371: 🛠️ Refactor suggestion
Improve request handling in selectSeries
The function should handle request cancellation for race conditions and add proper timeout handling.
Apply this diff to improve the implementation:
+let currentRequest: AbortController | null = null;
+
async function selectSeries(seriesName: string | undefined) {
selectedSeries = seriesName;
if (seriesName) {
const { startTimestamp, endTimestamp } = parseDateRangeParams(
new URL($page.url).searchParams
);
try {
+ // Cancel any in-flight request
+ currentRequest?.abort();
+ currentRequest = new AbortController();
+
+ const timeoutId = setTimeout(() => currentRequest?.abort(), 5000);
percentileData = await getFeatureTimeseries(seriesName, startTimestamp, endTimestamp);
+ clearTimeout(timeoutId);
} catch (error) {
+ if (error.name === 'AbortError') {
+ console.log('Request cancelled');
+ return;
+ }
console.error('Error fetching percentile data:', error);
percentileData = null;
}
}
}Likely invalid or redundant comment.
64-67: 🛠️ Refactor suggestion
Add input validation for chart options
The createChartOption function should validate its input parameters to prevent runtime errors.
Apply this diff to add validation:
function createChartOption(
customOption: Partial<EChartOption> = {},
customColors = false
): EChartOption {
+ if (customOption.series && !Array.isArray(customOption.series) && typeof customOption.series !== 'object') {
+ throw new Error('Invalid series configuration');
+ }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
frontend/src/lib/components/EChart/EChart.svelte (2)
64-73: Consider making tooltip configuration more flexible.The hardcoded tooltip configuration could be made more configurable to support different use cases.
Consider extracting the configuration to a separate constant or making it customizable through props:
- const customTooltipOption = { - tooltip: { - formatter: () => '', - trigger: 'axis', - axisPointer: { - type: 'line' - }, - position: 'top' - } - }; + const customTooltipOption = { + tooltip: { + formatter: props.tooltipFormatter ?? (() => ''), + trigger: props.tooltipTrigger ?? 'axis', + axisPointer: { + type: props.axisPointerType ?? 'line' + }, + position: props.tooltipPosition ?? 'top' + } + };
401-408: Simplify tooltip click handling and improve type safety.The click event dispatch and xAxisCategories access could be more robust.
Consider these improvements:
+type TooltipClickEvent = { + detail: unknown; + fromTooltip: boolean; +}; +function getXAxisCategories(chartInstance: echarts.ECharts | null): string[] | undefined { + if (!chartInstance || !isBarChart) return undefined; + const xAxis = chartInstance.getOption()?.xAxis; + return Array.isArray(xAxis) ? (xAxis[0]?.data as string[]) : undefined; +} -xAxisCategories={isBarChart && chartInstance - ? ((chartInstance.getOption()?.xAxis as EChartOption.XAxis[])?.[0]?.data as string[]) - : undefined} -on:click={(event) => - dispatch('click', { - detail: event.detail, - fromTooltip: true - })} +xAxisCategories={getXAxisCategories(chartInstance)} +on:click={(event) => dispatch<TooltipClickEvent>('click', { + detail: event.detail, + fromTooltip: true +})}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
frontend/src/lib/components/EChart/EChart.svelte(6 hunks)frontend/src/lib/components/EChartTooltip/EChartTooltip.svelte(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/lib/components/EChartTooltip/EChartTooltip.svelte
piyush-zlai
left a comment
piyush-zlai
left a comment
There was a problem hiding this comment.
Took a look at a high level - probably will be worth doing a pass with say Sean when he's in.
Summary
Hopefully the last massive PR, since we have a lot of the baseline implemented here. Here is a video walkthrough.
Main changes are noted:
Checklist
Summary by CodeRabbit
Release Notes
New Features
ActionButtons,TrueFalseBadge,CustomEChartLegend,EChartTooltip, andInfoTooltip.loadfunction for redirecting users from the root path to/models.Improvements
Bug Fixes
Chores