Frontend milestone #89
Conversation
This reverts commit e26d99d.
WalkthroughThis pull request introduces multiple enhancements across several Svelte components and utility files, primarily focusing on chart functionalities and user interactions. Key changes include the addition of new components for percentile charts and metric toggles, updates to tooltip management in the Changes
Possibly related PRs
Suggested reviewers
Poem
🪧 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: 4
🧹 Outside diff range and nitpick comments (23)
frontend/src/lib/util/colors.ts (2)
4-5: Consider enhancing error handling for non-browser environments.The browser environment check is good defensive programming. However, returning an empty string might lead to silent failures in consuming code.
Consider throwing an error or returning a default color:
-if (typeof window === 'undefined') return ''; +if (typeof window === 'undefined') { + throw new Error('getCssColorAsHex can only be used in browser environments'); + // Or return a default color: + // return '#000000'; +}
7-7: Enhance type safety and error handling for HSL values.The null check is good, but the function could benefit from more robust error handling and type safety.
Consider this enhanced implementation:
-if (!hslValue) return ''; +if (!hslValue) { + console.warn(`No CSS variable found for ${cssVariable}`); + return ''; +} + +// Validate HSL format +const hslRegex = /^\s*(\d+)\s+(\d+)%\s+(\d+)%\s*$/; +if (!hslRegex.test(hslValue)) { + console.warn(`Invalid HSL format for ${cssVariable}: ${hslValue}`); + return ''; +}frontend/src/routes/joins/[slug]/+page.server.ts (3)
3-11: Add a TODO comment for the sample data import.The import of
generatePercentileDatais temporary and should be removed once the backend integration is complete.import { parseDateRangeParams } from '$lib/util/date-ranges'; +// TODO: Remove sample data import once backend integration is complete import { generatePercentileData } from '$lib/util/sample-data';
32-32: Improve sample data configuration and documentation.The hardcoded values (10, 240) in
generatePercentileDatashould be more configurable and better documented.- const distributions = generatePercentileData(10, 240); // todo remove sample data when backend is ready + // TODO: Remove sample data when backend API is ready + // Configuration for sample data generation: + // - numFeatures: 10 (number of features to generate) + // - numDataPoints: 240 (number of data points per feature) + const NUM_SAMPLE_FEATURES = 10; + const NUM_SAMPLE_DATAPOINTS = 240; + const distributions = generatePercentileData(NUM_SAMPLE_FEATURES, NUM_SAMPLE_DATAPOINTS);
Line range hint
1-41: Consider the following architectural recommendations for backend integration:
- Create a separate API endpoint for distributions data in
$lib/api/api.ts- Add error handling for the new API call
- Consider caching strategy for performance optimization
- Add loading states for better UX during API calls
- Plan for data validation and type checking of API responses
Would you like me to help create a GitHub issue to track these implementation tasks?
frontend/src/lib/components/PercentileChart/PercentileChart.svelte (6)
12-12: Add documentation for percentile labelsConsider adding a comment explaining what these percentile values represent and why these specific values (p5, p50, p95) were chosen.
+// Percentile thresholds for distribution visualization: +// p5: 5th percentile (lower bound) +// p50: median +// p95: 95th percentile (upper bound) const targetLabels = ['p5', 'p50', 'p95'];
14-32: Improve type safety and efficiency of series generationThe function can be improved by:
- Using proper type definitions instead of type assertion
- Optimizing the data processing to avoid multiple iterations
+interface ChartSeries extends EChartOption.Series { + name: string; + type: 'line'; + data: [number, number][]; +} -function getPercentileChartSeries(data: FeatureResponse): EChartOption.Series[] { +function getPercentileChartSeries(data: FeatureResponse): ChartSeries[] { if (!data?.points) { return []; } + // Group points by label first to avoid multiple iterations + const pointsByLabel = data.points.reduce((acc, point) => { + if (!acc[point.label]) acc[point.label] = []; + acc[point.label].push([point.ts, point.value]); + return acc; + }, {} as Record<string, [number, number][]>); - return targetLabels - .filter((label) => data.points.some((point) => point.label === label)) - .map((label) => ({ + return targetLabels.reduce<ChartSeries[]>((acc, label) => { + if (pointsByLabel[label]) { + acc.push({ name: label, type: 'line', stack: 'Total', emphasis: { focus: 'series' }, - data: data.points - .filter((point) => point.label === label) - .map((point) => [point.ts, point.value]) - })) as EChartOption.Series[]; + data: pointsByLabel[label] + }); + } + return acc; + }, []); }
34-51: Enhance chart configuration with additional optionsConsider adding tooltip and legend configurations for better user experience and data interpretation.
return createChartOption({ xAxis: { type: 'time', splitLine: { show: true } }, yAxis: { type: 'value', splitLine: { show: true } }, + tooltip: { + trigger: 'axis', + formatter: '{b}: {c}', + axisPointer: { + type: 'cross' + } + }, + legend: { + data: targetLabels, + top: 0 + }, series: processedData });
53-56: Consider memoization and specific default optionsInstead of returning an empty object when data is null, consider returning a base configuration object with default options.
+const defaultChartOption = createChartOption({ + xAxis: { type: 'time', show: false }, + yAxis: { type: 'value', show: false }, + series: [] +}); let percentileChartOption = $derived.by(() => { - if (!data) return {}; + if (!data) return defaultChartOption; return getPercentileChartOption(data); });
59-65: Add error and loading statesConsider wrapping the EChart component with error and loading handlers for better user experience.
+{#if data === undefined} + <div class="loading">Loading chart data...</div> +{:else if data === null} + <div class="error">Failed to load chart data</div> +{:else} <EChart option={percentileChartOption} bind:chartInstance enableMousemove={false} enableCustomZoom={true} enableCustomTooltip={true} /> +{/if} + +<style> + .loading, .error { + text-align: center; + padding: 2rem; + color: var(--text-muted); + } +</style>
1-65: Add unit tests for the componentAs mentioned in the PR objectives, unit tests are still pending. Consider adding tests for:
- Series generation with various data scenarios
- Chart options generation
- Component rendering states
- Interaction with the ECharts instance
Would you like me to help generate a test suite for this component?
frontend/src/lib/util/chart-options.svelte.ts (2)
24-44: Consider color accessibility in the custom paletteThe custom color palette should be evaluated for color blindness and contrast accessibility.
Consider:
- Using a color palette generator that ensures WCAG compliance
- Adding color blind safe alternatives
- Documenting the color choices and their accessibility considerations
Would you like me to help create an accessible color palette?
19-22: Add JSDoc documentation for the exported functionSince this is a new feature for the Distributions tab, proper documentation would help other developers understand the purpose and usage of this utility.
Add comprehensive JSDoc:
+/** + * Creates an ECharts configuration object for the Distributions tab charts. + * @param customOption - Partial ECharts options to override defaults + * @param customColors - Whether to use the custom color palette + * @returns Complete ECharts configuration + * @example + * const options = createChartOption({ series: [...] }, true); + */ export function createChartOption( customOption: Partial<EChartOption> = {}, customColors = false ): EChartOption {frontend/src/lib/components/EChartTooltip/EChartTooltip.svelte (3)
54-58: Consider adding max-width and text overflow handlingWhile the current implementation works well for typical cases, consider these improvements for better handling of edge cases:
- Add max-width to prevent tooltip from extending beyond viewport
- Consider text-overflow handling for very long content
<div - class="absolute text-small bg-neutral-200 border border-neutral-400 rounded-md shadow-lg flex flex-col whitespace-nowrap" + class="absolute text-small bg-neutral-200 border border-neutral-400 rounded-md shadow-lg flex flex-col whitespace-nowrap max-w-[90vw]" style:max-height={tooltipHeight} transition:flyAndScale={{ duration: 200 }} >
62-85: Consider semantic improvements and class handlingTwo suggestions for improvement:
- For non-clickable items, consider using a div instead of a Button component
- The class concatenation could be improved to prevent potential spacing issues
-<Button - variant="ghost" - class="px-3 text-small text-neutral-800 text-left justify-between w-full {!clickable && - 'pointer-events-none'}" - on:click={() => handleSeriesClick(item)} -> +{#if clickable} + <Button + variant="ghost" + class="px-3 text-small text-neutral-800 text-left justify-between w-full" + on:click={() => handleSeriesClick(item)} + > + <!-- content --> + </Button> +{:else} + <div class="px-3 text-small text-neutral-800 flex justify-between w-full"> + <!-- content --> + </div> +{/if}
86-90: Consider adding ARIA label for better accessibilityThe keyboard shortcut implementation is good, but could benefit from improved accessibility.
-<div class="text-foreground text-xs px-3 py-2"> +<div class="text-foreground text-xs px-3 py-2" role="note" aria-label="Keyboard shortcut to lock tooltip"> <Badge variant="key">{isMacOS() ? '⌘' : 'Ctrl'}</Badge> to lock tooltip </div>frontend/src/lib/util/sample-data.ts (5)
Line range hint
1-754: Fix inconsistent labels in percentile sample data.There are non-percentile labels ('A', 'B', 'C') mixed in with the percentile data which could cause issues if the code assumes all labels follow the 'pX' format. Consider removing or replacing these with proper percentile labels.
756-759: Add JSDoc documentation.Consider adding JSDoc documentation to explain the purpose of the function and its parameters.
+/** + * Generates sample time series data for percentiles visualization + * @param numFeatures Number of features to generate data for (default: 1) + * @param numTimePoints Number of time points to generate for each feature (default: 24) + * @returns Array of features with percentile data + */ export function generatePercentileData(
760-776: Extract percentile labels as a constant.Consider extracting the percentile labels array as a named constant at the module level since it could be reused elsewhere.
+export const PERCENTILE_LABELS = [ + 'p0', 'p5', 'p10', 'p20', 'p30', 'p40', 'p50', + 'p60', 'p70', 'p75', 'p80', 'p90', 'p95', 'p99', 'p100' +] as const; + export function generatePercentileData( numFeatures: number = 1, numTimePoints: number = 24 ): FeatureResponse[] { - const percentileLabels = [ - 'p0', 'p5', 'p10', 'p20', 'p30', 'p40', 'p50', - 'p60', 'p70', 'p75', 'p80', 'p90', 'p95', 'p99', 'p100' - ]; + const percentileLabels = PERCENTILE_LABELS;
803-804: Consider removing the sort operation.Since the timestamps are already being generated in ascending order (from oldest to newest), the sort operation is unnecessary.
806-809: Add more meaningful feature names.The current feature naming scheme (
feature_1,feature_2, etc.) could be more descriptive for sample data.Consider using more meaningful names or accepting a name prefix as a parameter:
+export function generatePercentileData( numFeatures: number = 1, numTimePoints: number = 24, + featurePrefix: string = 'feature' ): FeatureResponse[] { ... features.push({ - feature: `feature_${f + 1}`, + feature: `${featurePrefix}_${f + 1}`, points });frontend/src/routes/joins/[slug]/+page.svelte (2)
45-45: Ensuredata.distributionsis defined before usage.To prevent potential runtime errors, consider adding a null or undefined check when deriving
distributionsfromdata.distributions.Apply this diff to add a default value:
-const distributions = $derived(data.distributions); +const distributions = $derived(data.distributions ?? []);
538-547: Add a unique key to the{#each}block for efficient rendering.In Svelte, providing a key in
{#each}blocks improves rendering performance by helping Svelte track array items.Apply this diff to add a key:
-{#each distributions as feature} +{#each distributions as feature (feature.feature)}Ensure that
feature.featureis a unique identifier for each item.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
frontend/src/lib/components/EChartTooltip/EChartTooltip.svelte(1 hunks)frontend/src/lib/components/PercentileChart/PercentileChart.svelte(1 hunks)frontend/src/lib/util/chart-options.svelte.ts(1 hunks)frontend/src/lib/util/colors.ts(1 hunks)frontend/src/lib/util/sample-data.ts(1 hunks)frontend/src/routes/joins/[slug]/+page.server.ts(2 hunks)frontend/src/routes/joins/[slug]/+page.svelte(6 hunks)
🔇 Additional comments (15)
frontend/src/lib/util/colors.ts (1)
Line range hint 3-11: Verify usage patterns across the codebase.
Let's ensure these defensive changes don't impact existing code.
✅ Verification successful
Based on the search results, I can now generate a final response since I have sufficient information about the usage patterns of getCssColorAsHex.
The defensive changes in getCssColorAsHex are appropriate for its usage patterns
The function is used in chart configurations where empty color values are handled gracefully:
- In
chart-options.svelte.ts, the returned empty string is properly checked before use:if (color300 && color700) - In
EChart.svelte, it's used for styling where an empty string is a safe fallback
The changes maintain backward compatibility while adding SSR safety. No issues were found in the existing usage patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of getCssColorAsHex
echo "Searching for getCssColorAsHex usage patterns..."
rg "getCssColorAsHex" -B 2 -A 2
# Search for potential error handling of empty color values
echo "Checking for error handling of color values..."
rg "getCssColorAsHex.*\s*\|\|\s*['\"](#|rgb|hsl)"
Length of output: 2246
frontend/src/routes/joins/[slug]/+page.server.ts (3)
Line range hint 13-20: LGTM! Return type is properly typed.
The addition of distributions: FeatureResponse[] to the return type is well-defined and aligns with the new feature requirements.
34-39: LGTM! Return statement is consistent with type definition.
The addition of distributions to the returned object matches the function signature and maintains good code organization.
30-30: Verify the model lookup change across the codebase.
The change from m.name to m.join.name might affect other parts of the application.
frontend/src/lib/components/PercentileChart/PercentileChart.svelte (2)
1-5: LGTM: Imports and type declarations are well-structured
The component properly imports all necessary dependencies and type definitions.
7-10: Verify Svelte version compatibility
The code uses Svelte 5's runes syntax ($props, $bindable). Ensure that the project's Svelte version supports these features and that all developers are aware of the migration to runes.
✅ Verification successful
Svelte 5 runes syntax is correctly used with compatible version
The codebase is using Svelte 5.0.4 and the runes syntax ($props, $bindable, $derived) is consistently used across multiple components. The usage in PercentileChart.svelte aligns with the project's setup and conventions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check Svelte version in package.json
jq '.dependencies.svelte, .devDependencies.svelte' package.json
# Look for other runes usage in the codebase
rg '\$props\(\)|\$bindable\(\)|\$derived' --type svelte
Length of output: 225
Script:
#!/bin/bash
# Try to find package.json files recursively
fd package.json
# Look for runes syntax with ripgrep without file type filter
rg '\$props\(\)|\$bindable\(\)|\$derived'
# Look for svelte config files that might indicate version
fd 'svelte\.config\.(js|cjs|mjs)'
Length of output: 3718
Script:
#!/bin/bash
# Check frontend package.json for Svelte version
cat frontend/package.json | jq '.dependencies.svelte, .devDependencies.svelte'
# Check Svelte config file for any version-related configuration
cat frontend/svelte.config.js
Length of output: 928
frontend/src/lib/util/chart-options.svelte.ts (2)
19-22: Review type casting in xAxis formatter
The type casting of the formatter object to unknown and then to string seems like a workaround for type incompatibility with ECharts types.
Let's check if there's a better type-safe approach:
Consider using ECharts' built-in time formatter types if available, or document why this type casting is necessary.
Also applies to: 60-64
✅ Verification successful
The previous command failed because of typescript extension, let's try a broader search and analyze the actual file to understand the type casting context better.
Type casting is necessary and correctly implemented
The type casting in the xAxis formatter is actually a valid workaround for ECharts' type definition limitations. Looking at the code context:
- The formatter object
{ month: '{MMM} {d}', day: '{MMM} {d}' }is a valid ECharts time formatter configuration - ECharts' type definitions don't properly capture this object format for time axis formatters
- The type casting is consistent with other ECharts usage patterns in the codebase, as seen in multiple files using
EChartOptiontypes
The type casting here is a pragmatic solution to maintain type safety while working with ECharts' API. No changes are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar ECharts axis formatter implementations
rg -t typescript "formatter.*MMM.*d" --no-filename
Length of output: 87
Script:
#!/bin/bash
# Search for formatter implementations in all files
rg "formatter.*MMM.*d"
# Search for ECharts type imports and usage
rg "EChartOption|echarts\..*Option"
# Look for the specific file to understand the context better
cat frontend/src/lib/util/chart-options.svelte.ts
Length of output: 6251
96-102: Review grid margins for label visibility
The grid margins are very tight (especially right: 1, bottom: 0) which might cause label clipping issues.
Let's check if there are any existing issues with similar grid configurations:
Consider adding some padding to prevent potential label clipping:
grid: {
top: 5,
- right: 1,
- bottom: 0,
+ right: 10,
+ bottom: 5,
left: 0,
containLabel: true
}frontend/src/lib/components/EChartTooltip/EChartTooltip.svelte (2)
51-52: LGTM! Clean implementation of tooltip positioning
The use of relative/absolute positioning with a pointer-events-none height placeholder is a solid approach to prevent layout shifts while maintaining proper tooltip positioning.
59-61: LGTM! Clean header implementation
The header implementation properly handles both categorical and date-based values with consistent styling.
frontend/src/routes/joins/[slug]/+page.svelte (5)
37-38: Necessary imports added for new components and utilities.
The imports for PercentileChart and createChartOption are correctly added to support the new "Distributions" tab functionality.
65-65: Initialize distributionCharts for tracking chart instances.
Proper initialization of distributionCharts ensures that chart instances are stored for synchronization and zooming functionalities.
118-118: Include distributionCharts in chart synchronization.
Adding distributionCharts to allCharts allows the new distribution charts to synchronize zooming and panning with other charts.
492-498: Tabs configured correctly for "Drift" and "Distributions".
The TabsTrigger components for "Drift" and "Distributions" are properly set up with matching value attributes and appropriate icons.
541-544: Proper usage of PercentileChart with data binding.
The PercentileChart component is correctly utilized, binding data and chartInstance for each feature to render distributions.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
frontend/src/routes/joins/[slug]/+page.svelte (2)
Line range hint
255-299: Consider memoizing chart options for performance.The
createComparedFeatureChartOptionfunction recreates options on every call. Consider memoizing the result based on input parameters to avoid unnecessary recalculations.+const memoizedCreateComparedFeatureChartOption = memoize( function createComparedFeatureChartOption( data: FeatureResponse | null, timestamp: number ): EChartOption { // ... existing implementation } +);
557-561: Enhance loading state feedback.The loading and empty states could be more informative and visually consistent with the application's design system.
-{#if isLoadingDistributions} - <div class="mt-6">Loading distributions...</div> -{:else if distributions.length === 0} - <div class="mt-6">No distribution data available</div> +{#if isLoadingDistributions} + <div class="mt-6 flex items-center justify-center"> + <LoadingSpinner size="md" /> + <span class="ml-2">Loading distributions...</span> + </div> +{:else if distributions.length === 0} + <div class="mt-6 text-center text-neutral-600"> + <Icon src={ChartLine} size="24" class="mx-auto mb-2" /> + <p>No distribution data available</p> + </div>frontend/src/routes/joins/[slug]/+page.server.ts (3)
8-9: Consider making fallback timestamps configurableThe fallback timestamps are hardcoded to specific dates in 2023. Consider making these configurable through environment variables or configuration files to make the application more maintainable and flexible.
-const FALLBACK_START_TS = 1672531200000; // 2023-01-01 -const FALLBACK_END_TS = 1677628800000; // 2023-03-01 +const FALLBACK_START_TS = parseInt(process.env.FALLBACK_START_TS ?? '1672531200000'); // Default: 2023-01-01 +const FALLBACK_END_TS = parseInt(process.env.FALLBACK_END_TS ?? '1677628800000'); // Default: 2023-03-01
49-51: Enhance error logging and user feedbackWhile the fallback mechanism is good, the error logging could be more detailed to help with debugging.
} catch (error) { - console.error('Error fetching data:', error); + console.error('Error fetching data with requested range:', { + error, + joinName, + requestedRange: { + start: requestedDateRange.startTimestamp, + end: requestedDateRange.endTimestamp + } + }); // If the requested range fails, fall back to the known working range
95-105: Optimize sorting implementationThe current implementation performs multiple sorting operations. Consider combining the sorts for better performance.
- // Sort main groups - joinTimeseries.items.sort((a, b) => { - const comparison = a.name.localeCompare(b.name); - return sortDirection === 'asc' ? comparison : -comparison; - }); - - // Sort features within each group - joinTimeseries.items.forEach((group) => { - group.items.sort((a, b) => { - return a.feature.localeCompare(b.feature); - }); - }); + // Sort main groups and features in a single pass + joinTimeseries.items.sort((a, b) => { + const comparison = a.name.localeCompare(b.name); + return sortDirection === 'asc' ? comparison : -comparison; + }).forEach(group => { + group.items.sort((a, b) => a.feature.localeCompare(b.feature)); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
frontend/src/routes/joins/[slug]/+page.server.ts(1 hunks)frontend/src/routes/joins/[slug]/+page.svelte(10 hunks)
🔇 Additional comments (5)
frontend/src/routes/joins/[slug]/+page.svelte (3)
26-31: LGTM: Well-organized imports for chart functionality.
The new imports are logically grouped and follow a clear pattern.
453-461:
Remove duplicate ChartControls component.
The ChartControls component is rendered twice with the same props, which could lead to state inconsistencies.
-<div class="sticky top-0 z-20 bg-neutral-200 border-b border-border -mx-8 py-2 px-8 border-l">
+<div class="sticky top-0 z-20 bg-neutral-200 border-b border-border -mx-8 py-2 px-8 border-l {isDateRangeSelectorVisible ? 'hidden' : ''}">
<ChartControls {...props} />
</div>
-<div class="mb-4">
- <ChartControls {...props} />
-</div>Also applies to: 498-506
200-238: 🛠️ Refactor suggestion
Enhance error handling in data fetching.
The current error handling only logs to console and nullifies the data. Consider:
- Adding specific error types and messages
- Implementing user feedback for failed requests
- Adding retry logic for transient failures
try {
+ const loadingState = $state('loading');
const [featureData, nullFeatureData] = await Promise.all([
getFeatureTimeseries({/*...*/}),
getFeatureTimeseries({/*...*/})
]);
+ loadingState = 'success';
} catch (error) {
- console.error('Error fetching data:', error);
+ loadingState = 'error';
+ const errorMessage = error instanceof Error ? error.message : 'Unknown error';
+ console.error('Error fetching feature data:', errorMessage);
+ // Show user-friendly error message
percentileData = null;
comparedFeatureData = null;
nullData = null;
}Likely invalid or redundant comment.
frontend/src/routes/joins/[slug]/+page.server.ts (2)
11-23: LGTM! Well-structured type definitions
The return type is well-defined with clear properties, and the inclusion of isUsingFallback in the dateRange object is a good practice for UI handling.
81-91: Add error handling for API calls
The Promise.all() call could fail if either API request fails.
| async function loadDistributions() { | ||
| if (distributions.length > 0 || isLoadingDistributions) return; | ||
|
|
||
| isLoadingDistributions = true; | ||
| try { | ||
| // Get all unique feature names across all groups | ||
| const allFeatures = Array.from( | ||
| new Set(joinTimeseries.items.flatMap((group) => group.items.map((item) => item.feature))) | ||
| ); | ||
|
|
||
| const seriesIndex = options.series.findIndex((s) => s.name === seriesName); | ||
| if (seriesIndex === -1) return undefined; | ||
| // Fetch percentile data for each feature | ||
| const distributionsPromises = allFeatures.map((featureName) => | ||
| getFeatureTimeseries({ | ||
| joinId: joinTimeseries.name, | ||
| featureName, | ||
| startTs: data.dateRange.startTimestamp, | ||
| endTs: data.dateRange.endTimestamp, | ||
| granularity: 'percentile', | ||
| metricType: 'drift', | ||
| metrics: 'value', | ||
| offset: '1D', | ||
| algorithm: 'psi' | ||
| }) | ||
| ); | ||
|
|
||
| return options.color[seriesIndex]; | ||
| const responses = await Promise.all(distributionsPromises); | ||
| distributions = responses.filter((response) => response.isNumeric); | ||
| } catch (error) { | ||
| console.error('Error loading distributions:', error); | ||
| } finally { | ||
| isLoadingDistributions = false; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add request cancellation handling for distribution loading.
The current implementation doesn't handle component unmounting or cancellation of in-flight requests, which could lead to memory leaks or race conditions.
+let abortController: AbortController | null = null;
async function loadDistributions() {
if (distributions.length > 0 || isLoadingDistributions) return;
+ abortController?.abort();
+ abortController = new AbortController();
isLoadingDistributions = true;
try {
// ... existing implementation
- const responses = await Promise.all(distributionsPromises);
+ const responses = await Promise.all(
+ distributionsPromises.map(p => p.catch(e => {
+ if (e.name === 'AbortError') throw e;
+ return null;
+ }))
+ );
distributions = responses.filter((response) => response?.isNumeric);
} catch (error) {
+ if (error.name !== 'AbortError') {
console.error('Error loading distributions:', error);
+ }
} finally {
isLoadingDistributions = false;
}
}
+onDestroy(() => {
+ abortController?.abort();
+});Committable suggestion skipped: line range outside the PR's diff.
| joinId: joinName, | ||
| startTs, | ||
| endTs, | ||
| metricType: 'drift', |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistency found in metricType and algorithm parameters
The code has a potential issue. The metricType parameter is hardcoded as 'drift', but this value is not one of the valid metric types defined in METRIC_TYPES ('jsd', 'hellinger', 'psi'). Meanwhile, the algorithm parameter is using the correct metricType variable that comes from URL parameters and is properly validated. This appears to be a bug where the wrong value is hardcoded.
- In
frontend/src/routes/joins/[slug]/+page.server.ts: ThemetricType: 'drift'should likely use one of the valid metric types fromMETRIC_TYPESor use the samemetricTypevariable that's being passed toalgorithm.
🔗 Analysis chain
Verify hardcoded metricType value
The metricType parameter is hardcoded as 'drift' in the API call, while a different metricType is passed as algorithm. Is this intentional?
Let me gather more context about the usage of both metricType and algorithm parameters.
From the context, I can now see that algorithm: metricType is using the value of a variable named metricType, while the metricType: 'drift' is a hardcoded string. Let me check where this metricType variable is defined.
Let me check what type MetricType is to understand the expected values.
Let me check the content of the MetricType file to understand the allowed metric types and their usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of metricType in API calls
rg -A 2 "metricType: ['\"](drift|[^'\"])" --type ts
Length of output: 271
Script:
#!/bin/bash
# Search for algorithm parameter usage in the same file
rg -B 5 -A 5 "algorithm" frontend/src/routes/joins/[slug]/+page.server.ts
# Search for any API endpoint definitions that might be accepting these parameters
ast-grep --pattern 'interface $_ {
$$$
metricType?: $_
$$$
}'
ast-grep --pattern 'interface $_ {
$$$
algorithm?: $_
$$$
}'
Length of output: 379
Script:
#!/bin/bash
# Search for metricType variable declaration in the file
rg -B 10 "algorithm: metricType" frontend/src/routes/joins/[slug]/+page.server.ts
# Also search for any parameter named metricType
ast-grep --pattern 'function $_($$$, metricType: $_, $$$) {
$$$
}'
Length of output: 3354
Script:
#!/bin/bash
# Search for MetricType type definition
ast-grep --pattern 'type MetricType = $_'
# Also search for any enum or interface named MetricType
rg "MetricType" --type ts
Length of output: 1300
Script:
#!/bin/bash
# Get the content of the MetricType file
cat frontend/src/lib/types/MetricType/MetricType.ts
# Also search for METRIC_TYPES constant definition
ast-grep --pattern 'const METRIC_TYPES = $_'
Length of output: 907
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (7)
frontend/src/lib/utils/sort.ts (2)
6-22: Consider URL encoding the parameter keyWhile the URL parameter handling is clean, consider encoding the parameter key to handle special characters safely.
export function getSortParamKey(context: SortContext): string { - return `${context}Sort`; + return encodeURIComponent(`${context}Sort`); }
44-52: Consider performance optimization for large datasetsFor large datasets, consider implementing a memoization strategy to cache sorted results, especially if sorting operations are frequent.
+const sortCache = new Map<string, FeatureResponse[]>(); + export function sortDistributions( distributions: FeatureResponse[], direction: SortDirection ): FeatureResponse[] { + const cacheKey = `${distributions.length}-${direction}`; + if (sortCache.has(cacheKey)) { + return [...sortCache.get(cacheKey)!]; + } + - return [...distributions].sort((a, b) => { + const sorted = [...distributions].sort((a, b) => { const comparison = a.feature.localeCompare(b.feature); return direction === 'asc' ? comparison : -comparison; }); + sortCache.set(cacheKey, sorted); + return [...sorted]; }frontend/src/lib/components/ActionButtons/ActionButtons.svelte (1)
58-63: Enhance button accessibilityAdd ARIA attributes to improve accessibility for screen readers and keyboard navigation.
{#if showSort} <Button variant="secondary" size="sm" icon="leading" + aria-label="Toggle sort direction: currently sorting {currentSort === 'asc' ? 'ascending' : 'descending'}" + aria-pressed={currentSort === 'desc'} on:click={handleSort} > <Icon src={ArrowsUpDown} micro size="16" /> Sort {currentSort === 'asc' ? 'A-Z' : 'Z-A'} </Button> {/if}frontend/src/routes/joins/[slug]/+page.server.ts (1)
74-101: Consider implementing retry logicFor better reliability, consider implementing retry logic for transient failures in the
fetchInitialDatafunction.+async function retry<T>( + fn: () => Promise<T>, + retries = 3, + delay = 1000 +): Promise<T> { + try { + return await fn(); + } catch (error) { + if (retries === 0) throw error; + await new Promise(resolve => setTimeout(resolve, delay)); + return retry(fn, retries - 1, delay * 2); + } +} + async function fetchInitialData( joinName: string, startTs: number, endTs: number, metricType: MetricType, sortDirection: SortDirection ) { - const [joinTimeseries, models] = await Promise.all([...]); + const [joinTimeseries, models] = await retry(() => + Promise.all([...]) + );frontend/src/routes/joins/[slug]/+page.svelte (3)
55-55: Add type safety for chart instancesConsider using a more specific type for the chart instances dictionary to prevent potential runtime errors.
-let distributionCharts: { [key: string]: EChartsType } = $state({}); +type ChartInstanceMap = Record<string, EChartsType>; +let distributionCharts: ChartInstanceMap = $state({});
204-213: Extract API configuration to constantsHardcoded values for metricType and algorithm should be extracted to configuration constants for better maintainability.
+const API_CONFIG = { + METRIC_TYPE: 'drift', + ALGORITHM: 'psi', + GRANULARITY: 'percentile', + OFFSET: '1D' +} as const; getFeatureTimeseries({ joinId: joinTimeseries.name, featureName: seriesName, startTs: data.dateRange.startTimestamp, endTs: data.dateRange.endTimestamp, - granularity: 'percentile', - metricType: 'drift', + granularity: API_CONFIG.GRANULARITY, + metricType: API_CONFIG.METRIC_TYPE, metrics: 'value', - offset: '1D', - algorithm: 'psi' + offset: API_CONFIG.OFFSET, + algorithm: API_CONFIG.ALGORITHM })
565-569: Enhance loading state feedbackConsider using a proper loading skeleton or spinner component for better user experience.
{#if isLoadingDistributions} - <div class="mt-6">Loading distributions...</div> + <div class="mt-6 flex items-center gap-2"> + <Spinner size="sm" /> + <span>Loading distributions...</span> + </div> {:else if distributions.length === 0} - <div class="mt-6">No distribution data available</div> + <div class="mt-6 text-neutral-500 flex items-center gap-2"> + <Icon src={InformationCircle} size="16" /> + <span>No distribution data available</span> + </div> {/if}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
frontend/src/lib/components/ActionButtons/ActionButtons.svelte(2 hunks)frontend/src/lib/utils/sort.ts(1 hunks)frontend/src/routes/joins/[slug]/+page.server.ts(1 hunks)frontend/src/routes/joins/[slug]/+page.svelte(10 hunks)
🔇 Additional comments (4)
frontend/src/lib/utils/sort.ts (1)
3-4: LGTM! Well-defined type definitions
The type definitions are clear and align well with the PR objectives for sorting in both drift and distributions contexts.
frontend/src/routes/joins/[slug]/+page.server.ts (1)
86-86:
Inconsistent metricType parameter still present
The metricType: 'drift' hardcoding issue identified in previous reviews still exists. This should be aligned with the valid metric types.
- metricType: 'drift',
+ metricType,frontend/src/routes/joins/[slug]/+page.svelte (2)
237-240: 🛠️ Refactor suggestion
Enhance error handling in selectSeries function
The current error handling only logs to console. Consider adding user feedback and proper error classification.
} catch (error) {
- console.error('Error fetching data:', error);
+ const errorMessage = error instanceof Error ? error.message : 'Unknown error';
+ console.error('Error fetching feature data:', errorMessage);
+ // Show user feedback
+ data.notifications?.show({
+ type: 'error',
+ message: 'Failed to load feature data. Please try again.'
+ });
percentileData = null;
comparedFeatureData = null;
nullData = null;
}Likely invalid or redundant comment.
411-442:
Add request cancellation handling for distributions loading
The current implementation doesn't handle component unmounting or cancellation of in-flight requests.
+let abortController: AbortController | null = null;
async function loadDistributions() {
if (distributions.length > 0 || isLoadingDistributions) return;
+ abortController?.abort();
+ abortController = new AbortController();
isLoadingDistributions = true;
try {
const allFeatures = Array.from(
new Set(joinTimeseries.items.flatMap((group) => group.items.map((item) => item.feature)))
);
const distributionsPromises = allFeatures.map((featureName) =>
getFeatureTimeseries({
joinId: joinTimeseries.name,
featureName,
startTs: data.dateRange.startTimestamp,
endTs: data.dateRange.endTimestamp,
granularity: 'percentile',
metricType: 'drift',
metrics: 'value',
offset: '1D',
- algorithm: 'psi'
+ algorithm: 'psi',
+ signal: abortController.signal
})
);
const responses = await Promise.all(distributionsPromises);
distributions = responses.filter((response) => response.isNumeric);
} catch (error) {
+ if (error.name !== 'AbortError') {
console.error('Error loading distributions:', error);
+ }
} finally {
isLoadingDistributions = false;
}
}
+onDestroy(() => {
+ abortController?.abort();
+});Likely invalid or redundant comment.
frontend/src/lib/utils/sort.ts
Outdated
| export function sortDrift( | ||
| joinTimeseries: JoinTimeSeriesResponse, | ||
| direction: SortDirection | ||
| ): JoinTimeSeriesResponse { | ||
| const sorted = { ...joinTimeseries }; | ||
|
|
||
| // Sort main groups | ||
| sorted.items = [...joinTimeseries.items].sort((a, b) => { | ||
| const comparison = a.name.localeCompare(b.name); | ||
| return direction === 'asc' ? comparison : -comparison; | ||
| }); | ||
|
|
||
| // Sort features within each group | ||
| sorted.items.forEach((group) => { | ||
| group.items.sort((a, b) => a.feature.localeCompare(b.feature)); | ||
| }); | ||
|
|
||
| return sorted; | ||
| } |
There was a problem hiding this comment.
Fix inconsistent sort direction handling in nested sort
The inner sort of features (group.items) ignores the direction parameter and always sorts ascending. This creates an inconsistent user experience when sorting in descending order.
sorted.items.forEach((group) => {
- group.items.sort((a, b) => a.feature.localeCompare(b.feature));
+ group.items.sort((a, b) => {
+ const comparison = a.feature.localeCompare(b.feature);
+ return direction === 'asc' ? comparison : -comparison;
+ });
});📝 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.
| export function sortDrift( | |
| joinTimeseries: JoinTimeSeriesResponse, | |
| direction: SortDirection | |
| ): JoinTimeSeriesResponse { | |
| const sorted = { ...joinTimeseries }; | |
| // Sort main groups | |
| sorted.items = [...joinTimeseries.items].sort((a, b) => { | |
| const comparison = a.name.localeCompare(b.name); | |
| return direction === 'asc' ? comparison : -comparison; | |
| }); | |
| // Sort features within each group | |
| sorted.items.forEach((group) => { | |
| group.items.sort((a, b) => a.feature.localeCompare(b.feature)); | |
| }); | |
| return sorted; | |
| } | |
| export function sortDrift( | |
| joinTimeseries: JoinTimeSeriesResponse, | |
| direction: SortDirection | |
| ): JoinTimeSeriesResponse { | |
| const sorted = { ...joinTimeseries }; | |
| // Sort main groups | |
| sorted.items = [...joinTimeseries.items].sort((a, b) => { | |
| const comparison = a.name.localeCompare(b.name); | |
| return direction === 'asc' ? comparison : -comparison; | |
| }); | |
| // Sort features within each group | |
| sorted.items.forEach((group) => { | |
| group.items.sort((a, b) => { | |
| const comparison = a.feature.localeCompare(b.feature); | |
| return direction === 'asc' ? comparison : -comparison; | |
| }); | |
| }); | |
| return sorted; | |
| } |
| } catch (error) { | ||
| console.error('Error fetching data:', error); | ||
| // If the requested range fails, fall back to the known working range | ||
| const { joinTimeseries, model } = await fetchInitialData( | ||
| joinName, | ||
| FALLBACK_START_TS, | ||
| FALLBACK_END_TS, | ||
| metricType, | ||
| sortDirection | ||
| ); | ||
|
|
||
| const [timeseries, joinTimeseries, models] = await Promise.all([ | ||
| api.getModelTimeseries(params.slug, dateRange.startTimestamp, dateRange.endTimestamp), | ||
| api.getJoinTimeseries(params.slug, dateRange.startTimestamp, dateRange.endTimestamp), | ||
| await api.getModels() // todo eventually we will want to get a single model | ||
| return { | ||
| joinTimeseries, | ||
| model, | ||
| metricType, | ||
| dateRange: { | ||
| startTimestamp: FALLBACK_START_TS, | ||
| endTimestamp: FALLBACK_END_TS, | ||
| dateRangeValue: requestedDateRange.dateRangeValue, | ||
| isUsingFallback: true | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling specificity
The current error handling catches all errors without distinguishing between different types of failures (network, validation, etc.). Consider implementing more specific error handling.
- } catch (error) {
- console.error('Error fetching data:', error);
+ } catch (error: unknown) {
+ if (error instanceof Response) {
+ console.error('API error:', await error.text());
+ } else if (error instanceof Error) {
+ console.error('Error fetching data:', error.message);
+ } else {
+ console.error('Unknown error:', error);
+ }Committable suggestion skipped: line range outside the PR's diff.
| function highlightSeries( | ||
| seriesName: string, | ||
| chart: EChartsType | null, | ||
| type: 'highlight' | 'downplay' | ||
| ) { | ||
| if (chart && seriesName) { | ||
| handleChartHighlight(chart, seriesName, type); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add cleanup for chart event listeners
Consider adding cleanup logic to prevent memory leaks from chart event listeners.
+import { onDestroy } from 'svelte';
function highlightSeries(
seriesName: string,
chart: EChartsType | null,
type: 'highlight' | 'downplay'
) {
if (chart && seriesName) {
handleChartHighlight(chart, seriesName, type);
}
}
+onDestroy(() => {
+ // Cleanup chart instances
+ Object.values(distributionCharts).forEach(chart => chart?.dispose());
+ Object.values(groupByCharts).forEach(chart => chart?.dispose());
+ percentileChart?.dispose();
+ comparedFeatureChart?.dispose();
+ nullRatioChart?.dispose();
+ dialogGroupChart?.dispose();
+});Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
frontend/src/routes/joins/[slug]/+page.svelte (1)
565-568: Enhance loading and empty states.The current loading and empty states use plain text, which could be improved for better user experience.
Consider using proper loading and empty state components:
-{#if isLoadingDistributions} - <div class="mt-6">Loading distributions...</div> -{:else if distributions.length === 0} - <div class="mt-6">No distribution data available</div> +{#if isLoadingDistributions} + <LoadingSpinner class="mt-6" /> +{:else if distributions.length === 0} + <EmptyState + class="mt-6" + icon={ChartLine} + title="No distribution data available" + description="Try adjusting your filters or date range" + />frontend/src/lib/util/sort.ts (1)
10-16: Consider making the default sort direction more explicitWhile the function works correctly, the default fallback could be more explicit for better maintainability.
- return param === 'desc' ? 'desc' : 'asc'; + const DEFAULT_DIRECTION: SortDirection = 'asc'; + return param === 'desc' ? 'desc' : DEFAULT_DIRECTION;frontend/src/lib/components/ActionButtons/ActionButtons.svelte (1)
28-36: Add error handling to the sort handlerWhile the implementation is good, it would benefit from error handling for the URL navigation.
function handleSort() { - const newSort: SortDirection = currentSort === 'asc' ? 'desc' : 'asc'; - const url = updateContextSort($page.url, context, newSort); - goto(url, { replaceState: true }); + try { + const newSort: SortDirection = currentSort === 'asc' ? 'desc' : 'asc'; + const url = updateContextSort($page.url, context, newSort); + goto(url, { replaceState: true }).catch(error => { + console.error('Failed to update sort direction:', error); + // Consider adding user feedback here + }); + } catch (error) { + console.error('Error updating sort:', error); + // Consider adding user feedback here + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
frontend/src/lib/components/ActionButtons/ActionButtons.svelte(2 hunks)frontend/src/lib/util/sort.ts(1 hunks)frontend/src/routes/joins/[slug]/+page.server.ts(1 hunks)frontend/src/routes/joins/[slug]/+page.svelte(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/routes/joins/[slug]/+page.server.ts
🔇 Additional comments (9)
frontend/src/routes/joins/[slug]/+page.svelte (3)
55-55: 🛠️ Refactor suggestion
Add cleanup for chart instances to prevent memory leaks.
Chart instances should be properly disposed when the component is destroyed to prevent memory leaks.
Add cleanup logic in an onDestroy lifecycle hook:
+import { onDestroy } from 'svelte';
+
let distributionCharts: { [key: string]: EChartsType } = $state({});
+
+onDestroy(() => {
+ Object.values(distributionCharts).forEach(chart => chart?.dispose());
+});Likely invalid or redundant comment.
237-240: 🛠️ Refactor suggestion
Enhance error handling with user feedback.
The current error handling only logs to console, which doesn't help users understand what went wrong.
Add user-facing error handling:
} catch (error) {
- console.error('Error fetching data:', error);
+ const errorMessage = error instanceof Error ? error.message : 'Unknown error';
+ console.error('Error fetching feature data:', errorMessage);
+ // Show error to user
+ data.notifications?.show({
+ type: 'error',
+ message: `Failed to load feature data: ${errorMessage}`
+ });
percentileData = null;
comparedFeatureData = null;
nullData = null;
}Likely invalid or redundant comment.
411-442: 🛠️ Refactor suggestion
Add request cancellation for distribution loading.
The current implementation doesn't handle component unmounting or cancellation of in-flight requests, which could lead to race conditions.
Add request cancellation handling:
+let abortController: AbortController | null = null;
async function loadDistributions() {
if (distributions.length > 0 || isLoadingDistributions) return;
+ abortController?.abort();
+ abortController = new AbortController();
isLoadingDistributions = true;
try {
const distributionsPromises = allFeatures.map((featureName) =>
getFeatureTimeseries({
...params,
+ signal: abortController.signal
})
);
const responses = await Promise.all(distributionsPromises);
distributions = responses.filter((response) => response.isNumeric);
} catch (error) {
+ if (error.name !== 'AbortError') {
console.error('Error loading distributions:', error);
+ }
} finally {
isLoadingDistributions = false;
}
}
+onDestroy(() => {
+ abortController?.abort();
+});Likely invalid or redundant comment.
frontend/src/lib/util/sort.ts (4)
1-4: LGTM! Well-defined type system
The type definitions are clear, properly constrained, and align well with the PR's sorting functionality requirements.
6-8: LGTM! Clean utility function
The function is pure, focused, and follows good practices.
18-22: LGTM! Good immutable implementation
The function follows good practices by avoiding URL mutation and maintaining pure function principles.
44-52: LGTM! Clean sorting implementation
The function is well-implemented with proper immutability and consistent sort direction handling.
frontend/src/lib/components/ActionButtons/ActionButtons.svelte (2)
Line range hint 1-24: LGTM! Well-structured component setup
The component setup is clean with proper typing and appropriate defaults.
58-63: LGTM! Clean and accessible button implementation
The sort button implementation is well-structured with clear labeling and proper event handling.
| <ChartControls | ||
| {isZoomed} | ||
| onResetZoom={resetZoom} | ||
| {isUsingFallbackDates} | ||
| dateRange={{ | ||
| startTimestamp: data.dateRange.startTimestamp, | ||
| endTimestamp: data.dateRange.endTimestamp | ||
| }} | ||
| /> |
There was a problem hiding this comment.
Remove duplicate ChartControls component.
The ChartControls component is rendered twice with the same props, which could lead to state inconsistencies.
Refactor to use a single instance with CSS positioning:
-<div class="sticky top-0 z-20 bg-neutral-200 border-b border-border -mx-8 py-2 px-8 border-l">
+<div class="sticky top-0 z-20 bg-neutral-200 border-b border-border -mx-8 py-2 px-8 border-l {isDateRangeSelectorVisible ? 'hidden' : ''}">
<ChartControls {...props} />
</div>
-<div class="mb-4">
- <ChartControls {...props} />
-</div>Also applies to: 505-513
| export function sortDrift( | ||
| joinTimeseries: JoinTimeSeriesResponse, | ||
| direction: SortDirection | ||
| ): JoinTimeSeriesResponse { | ||
| const sorted = { ...joinTimeseries }; | ||
|
|
||
| // Sort main groups | ||
| sorted.items = [...joinTimeseries.items].sort((a, b) => { | ||
| const comparison = a.name.localeCompare(b.name); | ||
| return direction === 'asc' ? comparison : -comparison; | ||
| }); | ||
|
|
||
| // Sort features within each group | ||
| sorted.items.forEach((group) => { | ||
| group.items.sort((a, b) => a.feature.localeCompare(b.feature)); | ||
| }); | ||
|
|
||
| return sorted; | ||
| } |
There was a problem hiding this comment.
Fix inconsistent sorting behavior and mutation issues
There are several concerns with the current implementation:
- Shallow copy might not protect against mutations of nested objects
- Features are always sorted ascending, ignoring the direction parameter
- Direct mutation of the items array in the forEach loop
export function sortDrift(
joinTimeseries: JoinTimeSeriesResponse,
direction: SortDirection
): JoinTimeSeriesResponse {
- const sorted = { ...joinTimeseries };
+ const sorted = {
+ ...joinTimeseries,
+ items: joinTimeseries.items.map(group => ({
+ ...group,
+ items: [...group.items]
+ }))
+ };
// Sort main groups
sorted.items = [...joinTimeseries.items].sort((a, b) => {
const comparison = a.name.localeCompare(b.name);
return direction === 'asc' ? comparison : -comparison;
});
// Sort features within each group
sorted.items.forEach((group) => {
- group.items.sort((a, b) => a.feature.localeCompare(b.feature));
+ group.items.sort((a, b) => {
+ const comparison = a.feature.localeCompare(b.feature);
+ return direction === 'asc' ? comparison : -comparison;
+ });
});
return sorted;
}📝 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.
| export function sortDrift( | |
| joinTimeseries: JoinTimeSeriesResponse, | |
| direction: SortDirection | |
| ): JoinTimeSeriesResponse { | |
| const sorted = { ...joinTimeseries }; | |
| // Sort main groups | |
| sorted.items = [...joinTimeseries.items].sort((a, b) => { | |
| const comparison = a.name.localeCompare(b.name); | |
| return direction === 'asc' ? comparison : -comparison; | |
| }); | |
| // Sort features within each group | |
| sorted.items.forEach((group) => { | |
| group.items.sort((a, b) => a.feature.localeCompare(b.feature)); | |
| }); | |
| return sorted; | |
| } | |
| export function sortDrift( | |
| joinTimeseries: JoinTimeSeriesResponse, | |
| direction: SortDirection | |
| ): JoinTimeSeriesResponse { | |
| const sorted = { | |
| ...joinTimeseries, | |
| items: joinTimeseries.items.map(group => ({ | |
| ...group, | |
| items: [...group.items] | |
| })) | |
| }; | |
| // Sort main groups | |
| sorted.items = [...joinTimeseries.items].sort((a, b) => { | |
| const comparison = a.name.localeCompare(b.name); | |
| return direction === 'asc' ? comparison : -comparison; | |
| }); | |
| // Sort features within each group | |
| sorted.items.forEach((group) => { | |
| group.items.sort((a, b) => { | |
| const comparison = a.feature.localeCompare(b.feature); | |
| return direction === 'asc' ? comparison : -comparison; | |
| }); | |
| }); | |
| return sorted; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
frontend/src/routes/joins/[slug]/+page.server.ts (1)
8-9: Enhance timestamp constant documentationConsider adding more context to the fallback timestamp constants, such as why these specific dates were chosen and what they represent in the application's context.
-const FALLBACK_START_TS = 1698796800000; // 2023-11-01 -const FALLBACK_END_TS = 1703980800000; // 2023-12-31 +// Default 2-month range for data fetching when user's date range fails +const FALLBACK_START_TS = 1698796800000; // 2023-11-01 00:00:00 UTC +const FALLBACK_END_TS = 1703980800000; // 2023-12-31 00:00:00 UTC
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
frontend/src/routes/joins/[slug]/+page.server.ts(1 hunks)
🔇 Additional comments (4)
frontend/src/routes/joins/[slug]/+page.server.ts (4)
11-24: LGTM! Well-structured return type with clear properties
The return type is well-defined with proper typing for all properties. The inclusion of isUsingFallback in the dateRange object is a good practice for handling fallback scenarios transparently.
49-51: Enhance error handling specificity
The current error handling is too generic. Consider implementing more specific error handling to distinguish between different types of failures.
86-86: Inconsistency found in metricType and algorithm parameters
The code has a potential issue where metricType parameter is hardcoded as 'drift', but this value is not one of the valid metric types.
81-91: Add error handling for API calls
The Promise.all() call could fail if either API request fails. Consider adding try-catch block and proper error handling.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
frontend/src/lib/components/EChart/EChart.svelte (1)
Line range hint
238-306: Add error boundaries around tooltip positioning logicThe tooltip positioning logic should handle edge cases more gracefully.
Consider wrapping the complex calculations in try-catch blocks:
function showTooltip(params: { offsetX: number; offsetY: number }) { + try { if (isCommandPressed) return; // ... existing code ... const pointInGrid = chartInstance!.convertFromPixel({ seriesIndex: 0 }, pointInPixel); + } catch (error) { + console.error('Error showing tooltip:', error); + isTooltipVisible = false; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
frontend/src/lib/components/EChart/EChart.svelte(10 hunks)
🔇 Additional comments (4)
frontend/src/lib/components/EChart/EChart.svelte (4)
7-7: LGTM: Well-structured prop additions for custom legend support
The new props showCustomLegend and legendGroup are properly typed and integrated with appropriate default values.
Also applies to: 18-19, 30-31
105-130: Array bounds check needed before accessing series[0]
The code accesses series[0] without verifying the array is non-empty.
Additionally, consider simplifying the command key tooltip logic:
-if (exactX !== null && chartInstance) {
+if (!exactX || !chartInstance) {
+ return;
+}
const option = chartInstance.getOption();
const series = option.series as EChartOption.Series[];
+if (!series?.length) {
+ return;
+}
const firstSeries = series[0];146-146: triggerOn property should be nested within tooltip
The triggerOn property needs to be placed inside the tooltip configuration.
Also applies to: 152-152
447-453: LGTM: Clean custom legend integration
The CustomEChartLegend component is properly integrated with appropriate conditional rendering and prop passing.
| const isBarChart = $derived.by(() => { | ||
| const series = mergedOption.series as EChartOption.Series[]; | ||
| return series?.some((s) => s.type === 'bar'); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null check for series array
The bar chart detection logic should handle the case where mergedOption.series is undefined.
Apply this diff to add defensive programming:
const isBarChart = $derived.by(() => {
const series = mergedOption.series as EChartOption.Series[];
+ if (!Array.isArray(series)) {
+ return false;
+ }
return series?.some((s) => s.type === 'bar');
});📝 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 isBarChart = $derived.by(() => { | |
| const series = mergedOption.series as EChartOption.Series[]; | |
| return series?.some((s) => s.type === 'bar'); | |
| }); | |
| const isBarChart = $derived.by(() => { | |
| const series = mergedOption.series as EChartOption.Series[]; | |
| if (!Array.isArray(series)) { | |
| return false; | |
| } | |
| return series?.some((s) => s.type === 'bar'); | |
| }); |
## Summary Stacked on #89. Quick change to make sure `MetricTypeToggle.svelte` stays synced with the url query params [Demo](https://drive.google.com/file/d/1YNBxIi0fP-Qi-LVIw5FexwZQVEfeCWaS/view?usp=drive_link) ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update
## Summary Stacking this PR on #89. This makes sure the mark point line updates when the chart data changes. [Demo](https://drive.google.com/file/d/1utbMc0nxHFsnr7fHzP_QcDPzWt8MJcB8/view?usp=drive_link) ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
frontend/src/routes/joins/[slug]/+page.svelte (1)
54-56: Enhance type safety for chart instancesConsider using a more type-safe approach for managing chart instances:
-let distributionCharts: { [key: string]: EChartsType } = $state({}); -let groupByCharts: { [key: string]: EChartsType } = $state({}); +type ChartInstances = Record<string, EChartsType | null>; +let distributionCharts: ChartInstances = $state({}); +let groupByCharts: ChartInstances = $state({});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
frontend/src/routes/joins/[slug]/+page.svelte(10 hunks)
🔇 Additional comments (3)
frontend/src/routes/joins/[slug]/+page.svelte (3)
444-446:
Add cleanup for chart instances
Chart instances should be properly disposed when the component is destroyed:
+import { onDestroy } from 'svelte';
+
onMount(() => {
loadDistributions();
});
+
+onDestroy(() => {
+ // Cleanup chart instances
+ Object.values(distributionCharts).forEach(chart => chart?.dispose());
+ Object.values(groupByCharts).forEach(chart => chart?.dispose());
+ percentileChart?.dispose();
+ comparedFeatureChart?.dispose();
+ nullRatioChart?.dispose();
+ dialogGroupChart?.dispose();
+});Likely invalid or redundant comment.
435-441: 🛠️ Refactor suggestion
Improve error handling in distribution loading
The error handling could be more specific and provide better feedback to users:
try {
const responses = await Promise.all(distributionsPromises);
distributions = responses.filter((response) => response.isNumeric);
} catch (error) {
- console.error('Error loading distributions:', error);
+ const errorMessage = error instanceof Error ? error.message : 'Unknown error';
+ console.error('Error loading distributions:', errorMessage);
+ distributions = [];
+ // Consider showing a user-friendly error message
+ // throw new Error(`Failed to load distributions: ${errorMessage}`);
} finally {
isLoadingDistributions = false;
}Likely invalid or redundant comment.
Line range hint 1-773: Verify the impact of chart-related changes
Let's verify the usage of chart-related utilities and components across the codebase:
✅ Verification successful
Let me gather more information about chart-related cleanup patterns and component usage to ensure there are no memory leaks or inconsistencies.
Let me check one final aspect regarding the chart cleanup implementation in the EChart component since we didn't see its full implementation in the previous results.
Based on the gathered information, I can now provide the final response:
Chart implementation follows best practices, no cleanup issues found
The implementation properly handles chart lifecycle and cleanup:
- The base
EChartcomponent correctly implements disposal of chart instances and cleanup of observers - Chart connections are properly managed through the
connect()utility - All chart instances are properly typed and managed through Svelte's state management
- Resource cleanup is handled in the
onDestroylifecycle hook
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent usage of chart utilities
echo "Checking chart utility usage..."
rg -l "createChartOption|handleChartHighlight|getSeriesColor" frontend/src/
# Check for proper cleanup in other chart components
echo -e "\nChecking chart cleanup patterns..."
rg "onDestroy.*dispose\(\)" frontend/src/
# Verify ChartControls usage
echo -e "\nChecking ChartControls usage patterns..."
rg -A 5 "ChartControls" frontend/src/
Length of output: 2577
Script:
#!/bin/bash
# Check EChart component implementation for cleanup patterns
echo "Checking EChart component implementation..."
rg -A 10 "class=\"EChart\"" frontend/src/lib/components/EChart/EChart.svelte
# Check for chart instance cleanup in components using ECharts
echo -e "\nChecking chart instance cleanup in components..."
rg -A 5 "let.*:.*EChartsType" frontend/src/
# Check for chart connection cleanup
echo -e "\nChecking chart connection cleanup..."
rg -A 10 "connect\(.*\)" frontend/src/
Length of output: 4597
Script:
#!/bin/bash
# Check EChart component for proper cleanup implementation
echo "Checking EChart component cleanup..."
rg -B 5 -A 15 "onDestroy|onMount" frontend/src/lib/components/EChart/EChart.svelte
# Check if there are any other components using ECharts that need cleanup
echo -e "\nChecking other components using ECharts..."
rg -l "import.*type.*EChartsType" frontend/src/
# Verify chart disposal
echo -e "\nChecking chart disposal patterns..."
rg "chartInstance.*dispose" frontend/src/
Length of output: 2100
| <ChartControls | ||
| {isZoomed} | ||
| onResetZoom={resetZoom} | ||
| {isUsingFallbackDates} | ||
| dateRange={{ | ||
| startTimestamp: data.dateRange.startTimestamp, | ||
| endTimestamp: data.dateRange.endTimestamp | ||
| }} | ||
| showActionButtons={true} | ||
| showCluster={selectedTab === 'drift'} | ||
| showSort={true} | ||
| context={selectedTab} | ||
| /> |
There was a problem hiding this comment.
Remove duplicate ChartControls component
The ChartControls component is rendered twice with the same props. This could lead to state inconsistencies:
-<div class="sticky top-0 z-20 bg-neutral-200 border-b border-border -mx-8 py-2 px-8 border-l">
+<div class="sticky top-0 z-20 bg-neutral-200 border-b border-border -mx-8 py-2 px-8 border-l {isDateRangeSelectorVisible ? 'hidden' : ''}">
<ChartControls {...props} />
</div>
-<div class="mb-4">
- <ChartControls {...props} />
-</div>Also applies to: 544-556
| <span | ||
| class="mb-4 ml-2 text-xl-medium flex items-center gap-2 w-fit" | ||
| role="presentation" | ||
| onmouseenter={() => highlightSeries(selectedSeries ?? '', dialogGroupChart, 'highlight')} | ||
| onmouseleave={() => highlightSeries(selectedSeries ?? '', dialogGroupChart, 'downplay')} | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility for interactive elements
Add proper ARIA attributes and keyboard interaction support:
<span
class="mb-4 ml-2 text-xl-medium flex items-center gap-2 w-fit"
- role="presentation"
+ role="button"
+ tabindex="0"
+ aria-label={`Highlight series ${selectedSeries}`}
+ onkeydown={(e) => {
+ if (e.key === 'Enter' || e.key === ' ') {
+ highlightSeries(selectedSeries ?? '', dialogGroupChart, 'highlight');
+ }
+ }}
onmouseenter={() => highlightSeries(selectedSeries ?? '', dialogGroupChart, 'highlight')}
onmouseleave={() => highlightSeries(selectedSeries ?? '', dialogGroupChart, 'downplay')}
>📝 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.
| <span | |
| class="mb-4 ml-2 text-xl-medium flex items-center gap-2 w-fit" | |
| role="presentation" | |
| onmouseenter={() => highlightSeries(selectedSeries ?? '', dialogGroupChart, 'highlight')} | |
| onmouseleave={() => highlightSeries(selectedSeries ?? '', dialogGroupChart, 'downplay')} | |
| > | |
| <span | |
| class="mb-4 ml-2 text-xl-medium flex items-center gap-2 w-fit" | |
| role="button" | |
| tabindex="0" | |
| aria-label={`Highlight series ${selectedSeries}`} | |
| onkeydown={(e) => { | |
| if (e.key === 'Enter' || e.key === ' ') { | |
| highlightSeries(selectedSeries ?? '', dialogGroupChart, 'highlight'); | |
| } | |
| }} | |
| onmouseenter={() => highlightSeries(selectedSeries ?? '', dialogGroupChart, 'highlight')} | |
| onmouseleave={() => highlightSeries(selectedSeries ?? '', dialogGroupChart, 'downplay')} | |
| > |
| // Get all points at the selected timestamp | ||
| const currentPoints = data.current.filter((point) => point.ts === timestamp); | ||
| const baselinePoints = data.baseline.filter((point) => point.ts === timestamp); | ||
|
|
||
| $effect(() => { | ||
| selectSeries(selectedSeries); | ||
| }); | ||
| // Create pairs of current and baseline points by label | ||
| const labels = [ | ||
| ...new Set([...currentPoints.map((p) => p.label), ...baselinePoints.map((p) => p.label)]) | ||
| ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize data processing performance
The current implementation processes data points on every render. Consider memoizing the processed data:
+const memoizedPoints = $derived(() => {
+ if (!data?.current || !data?.baseline) return { currentPoints: [], baselinePoints: [], labels: [] };
+
+ const currentPoints = data.current.filter((point) => point.ts === timestamp);
+ const baselinePoints = data.baseline.filter((point) => point.ts === timestamp);
+ const labels = [...new Set([
+ ...currentPoints.map((p) => p.label),
+ ...baselinePoints.map((p) => p.label)
+ ])];
+
+ return { currentPoints, baselinePoints, labels };
+});Committable suggestion skipped: line range outside the PR's diff.
This reverts commit c5e3270.
## Summary Wanted to scope back the amount of data generated, as it was leading to some performance issues. There is some optimization that can be done, but this fixes immediate performance issues. ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
frontend/src/routes/joins/[slug]/+page.server.ts (1)
49-71: Enhance error logging for better debuggingWhile the fallback mechanism is good, the error logging could be more detailed to help with debugging.
Consider this enhancement:
- } catch (error) { - console.error('Error fetching data:', error); + } catch (error) { + console.error('Error fetching data with requested range:', { + error, + joinName, + requestedRange: { + start: requestedDateRange.startTimestamp, + end: requestedDateRange.endTimestamp + } + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
frontend/src/routes/joins/[slug]/+page.server.ts(1 hunks)spark/src/main/scala/ai/chronon/spark/scripts/ObservabilityDemoDataLoader.scala(1 hunks)
🔇 Additional comments (4)
frontend/src/routes/joins/[slug]/+page.server.ts (3)
3-23: LGTM! Well-structured type definitions
The expanded return type with metricType and dateRange properties is properly typed and provides good type safety.
81-91:
Add error handling for Promise.all
The Promise.all call needs proper error handling to gracefully handle API failures.
86-90:
Fix inconsistent metricType usage
There's an inconsistency in the metricType parameter usage. The API call uses a hardcoded 'drift' value while passing the metricType parameter as algorithm.
Apply this fix:
- metricType: 'drift',
+ metricType,Likely invalid or redundant comment.
spark/src/main/scala/ai/chronon/spark/scripts/ObservabilityDemoDataLoader.scala (1)
40-46: Verify date range alignment with frontend
The default end date and row count have been reduced. The end date (2023-03-01) aligns with the frontend's FALLBACK_END_TS constant, which is good for consistency.
Let's verify this alignment is consistent across the codebase:
✅ Verification successful
Alignment verified between frontend and data loader defaults
The search results confirm that the end date 2023-03-01 in ObservabilityDemoDataLoader.scala perfectly aligns with the frontend's FALLBACK_END_TS constant (1677628800000) in frontend/src/routes/joins/[slug]/+page.server.ts. Similarly, the start date 2023-01-01 aligns with FALLBACK_START_TS (1672531200000).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any other hardcoded timestamps or date strings that might need alignment
rg -g '!*.{json,lock}' --type-add 'ts:*.{ts,js,tsx,jsx}' -t ts -t scala "202[34]-(0[1-9]|1[0-2])-(0[1-9]|[12][0-9]|3[01])"
Length of output: 4550
## Summary
This PR adds the following features:
- Wire up all charts to real data
- Distributions tab - view percentile charts for numerical data over
time
- UX Improvements
- Tooltip behavior near end of page
- Chart series highlight on hover in legend, tooltip, title, etc
- Let user sort A-Z and Z-A
- Add drift metric toggle
- Consolidate chart controls
[Video demo and
walkthrough](https://drive.google.com/file/d/1ZHaHd1yeEz0KPvQ-4FyN7xa4C3QEZuJj/view?usp=drive_link)
## Checklist
- [ ] Added Unit Tests
- [x] 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 a new `PercentileChart` component for visualizing
percentile data.
- Added a custom legend feature to the `EChart` component.
- New `MetricTypeToggle` component allows users to switch between metric
types.
- Enhanced `ActionButtons` with sorting functionality.
- New alert components for improved UI notifications.
- **Improvements**
- Enhanced tooltip functionality in the `EChartTooltip` component.
- Improved date range selection logic in the `DateRangeSelector`
component.
- Streamlined chart interaction logic in the `EChart` component.
- **Bug Fixes**
- Fixed issues with mouse event handling in various chart components.
- **Chores**
- Updated internal APIs to support new data structures and improve data
fetching.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
---------
Co-authored-by: nikhil-zlai <[email protected]>
Co-authored-by: Piyush Narang <[email protected]>
## Summary
This PR adds the following features:
- Wire up all charts to real data
- Distributions tab - view percentile charts for numerical data over
time
- UX Improvements
- Tooltip behavior near end of page
- Chart series highlight on hover in legend, tooltip, title, etc
- Let user sort A-Z and Z-A
- Add drift metric toggle
- Consolidate chart controls
[Video demo and
walkthrough](https://drive.google.com/file/d/1ZHaHd1yeEz0KPvQ-4FyN7xa4C3QEZuJj/view?usp=drive_link)
## Checklist
- [ ] Added Unit Tests
- [x] 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 a new `PercentileChart` component for visualizing
percentile data.
- Added a custom legend feature to the `EChart` component.
- New `MetricTypeToggle` component allows users to switch between metric
types.
- Enhanced `ActionButtons` with sorting functionality.
- New alert components for improved UI notifications.
- **Improvements**
- Enhanced tooltip functionality in the `EChartTooltip` component.
- Improved date range selection logic in the `DateRangeSelector`
component.
- Streamlined chart interaction logic in the `EChart` component.
- **Bug Fixes**
- Fixed issues with mouse event handling in various chart components.
- **Chores**
- Updated internal APIs to support new data structures and improve data
fetching.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
---------
Co-authored-by: nikhil-zlai <[email protected]>
Co-authored-by: Piyush Narang <[email protected]>
## Summary
This PR adds the following features:
- Wire up all charts to real data
- Distributions tab - view percentile charts for numerical data over
time
- UX Improvements
- Tooltip behavior near end of page
- Chart series highlight on hover in legend, tooltip, title, etc
- Let user sort A-Z and Z-A
- Add drift metric toggle
- Consolidate chart controls
[Video demo and
walkthrough](https://drive.google.com/file/d/1ZHaHd1yeEz0KPvQ-4FyN7xa4C3QEZuJj/view?usp=drive_link)
## Checklist
- [ ] Added Unit Tests
- [x] 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 a new `PercentileChart` component for visualizing
percentile data.
- Added a custom legend feature to the `EChart` component.
- New `MetricTypeToggle` component allows users to switch between metric
types.
- Enhanced `ActionButtons` with sorting functionality.
- New alert components for improved UI notifications.
- **Improvements**
- Enhanced tooltip functionality in the `EChartTooltip` component.
- Improved date range selection logic in the `DateRangeSelector`
component.
- Streamlined chart interaction logic in the `EChart` component.
- **Bug Fixes**
- Fixed issues with mouse event handling in various chart components.
- **Chores**
- Updated internal APIs to support new data structures and improve data
fetching.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
---------
Co-authored-by: nikhil-zlai <[email protected]>
Co-authored-by: Piyush Narang <[email protected]>
## Summary
This PR adds the following features:
- Wire up all charts to real data
- Distributions tab - view percentile charts for numerical data over
time
- UX Improvements
- Tooltip behavior near end of page
- Chart series highlight on hover in legend, tooltip, title, etc
- Let user sort A-Z and Z-A
- Add drift metric toggle
- Consolidate chart controls
[Video demo and
walkthrough](https://drive.google.com/file/d/1ZHaHd1yeEz0KPvQ-4FyN7xa4C3QEZuJj/view?usp=drive_link)
## Cheour clientslist
- [ ] Added Unit Tests
- [x] 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 a new `PercentileChart` component for visualizing
percentile data.
- Added a custom legend feature to the `EChart` component.
- New `MetricTypeToggle` component allows users to switch between metric
types.
- Enhanced `ActionButtons` with sorting functionality.
- New alert components for improved UI notifications.
- **Improvements**
- Enhanced tooltip functionality in the `EChartTooltip` component.
- Improved date range selection logic in the `DateRangeSelector`
component.
- Streamlined chart interaction logic in the `EChart` component.
- **Bug Fixes**
- Fixed issues with mouse event handling in various chart components.
- **Chores**
- Updated internal APIs to support new data structures and improve data
fetching.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
---------
Co-authored-by: nikhil-zlai <[email protected]>
Co-authored-by: Piyush Narang <[email protected]>
Summary
This PR adds the following features:
Video demo and walkthrough
Checklist
Summary by CodeRabbit
New Features
PercentileChartcomponent for visualizing percentile data.EChartcomponent.MetricTypeTogglecomponent allows users to switch between metric types.ActionButtonswith sorting functionality.Improvements
EChartTooltipcomponent.DateRangeSelectorcomponent.EChartcomponent.Bug Fixes
Chores