Route and component restructuring (includes performance improvements) #244
Conversation
…ecessary loading and rendering offscreen content and providing better organization
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request focuses on simplifying import paths across multiple frontend Svelte components by removing redundant subdirectories. The changes span various files in the frontend, primarily updating import statements to directly reference component files. Additionally, new layout and page components have been introduced for the joins and observability sections, along with modifications to data fetching logic and component structures. Changes
Suggested reviewers
Poem
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🪧 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: 13
🔭 Outside diff range comments (2)
frontend/src/routes/joins/[slug]/observability/drift/+page.svelte (2)
Line range hint
45-46: Add cleanup for chart instances.Charts should be properly disposed when components are unmounted to prevent memory leaks.
Add an onDestroy handler:
+import { onDestroy } from 'svelte'; + +onDestroy(() => { + Object.values(groupByCharts).forEach(chart => chart.dispose()); + if (percentileChart) percentileChart.dispose(); + if (comparedFeatureChart) comparedFeatureChart.dispose(); + if (nullRatioChart) nullRatioChart.dispose(); + if (dialogGroupChart) dialogGroupChart.dispose(); +});Also applies to: 51-52
Line range hint
453-600: Extract dialog content into separate components.The dialog content is complex and would be more maintainable if split into smaller components.
Consider creating these components:
ChartDialog.sveltePercentileSection.svelteDistributionSection.svelteNullRatioSection.svelte
🧹 Nitpick comments (6)
frontend/src/routes/joins/[slug]/job-tracker/+page.svelte (1)
23-23: Add type annotation for data prop.-let { data } = $props(); +let { data }: { data: { jobTree: JobTreeNode[], dates: string[] } } = $props();Also applies to: 25-25
frontend/src/lib/components/ui/tabs/tabs-trigger.svelte (1)
22-23: Improve URL matching logic.The regex pattern could be more robust.
- $: isActive = - href === '/' ? $page.url.pathname === href : $page.url.pathname.match(href + '($|\\/)') != null; + $: isActive = href === '/' + ? $page.url.pathname === href + : href + ? $page.url.pathname.startsWith(href) && + ($page.url.pathname === href || $page.url.pathname[href.length] === '/') + : false;frontend/src/routes/joins/[slug]/observability/+layout.svelte (3)
43-43: Track the TODO for component relocation.The comment indicates this component should be moved to drift/distribution pages.
Would you like me to create a GitHub issue to track this refactoring task?
59-80: Add accessibility attributes to the table.Add
aria-labelto the table for better screen reader support.- <Table density="compact"> + <Table density="compact" aria-label="Model Information">
87-94: Use absolute paths for navigation.Replace relative paths with absolute paths to prevent navigation issues.
- <TabsTrigger href="./drift" class="flex items-center"> + <TabsTrigger href="/joins/{data.model.id}/observability/drift" class="flex items-center"> - <TabsTrigger href="./distributions" class="flex items-center"> + <TabsTrigger href="/joins/{data.model.id}/observability/distributions" class="flex items-center">frontend/src/routes/joins/[slug]/observability/drift/+page.svelte (1)
434-435: Memoize chart options for better performance.Cache chart options to prevent unnecessary recalculations on re-renders.
+const memoizedOptions = $derived.by(() => + Object.fromEntries( + joinTimeseries.items.map(group => [ + group.name, + createGroupByChartOption(group.items) + ]) + ) +); - option={createGroupByChartOption(group.items)} + option={memoizedOptions[group.name]}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (20)
frontend/src/lib/components/ChartControls.svelte(1 hunks)frontend/src/lib/components/EChart.svelte(1 hunks)frontend/src/lib/components/ExpandableCell.svelte(1 hunks)frontend/src/lib/components/PercentileChart.svelte(1 hunks)frontend/src/lib/components/StatusCell.svelte(1 hunks)frontend/src/lib/components/ui/tabs/tabs-trigger.svelte(1 hunks)frontend/src/routes/+layout.svelte(1 hunks)frontend/src/routes/joins/+page.svelte(1 hunks)frontend/src/routes/joins/[slug]/+layout.svelte(1 hunks)frontend/src/routes/joins/[slug]/+layout.ts(1 hunks)frontend/src/routes/joins/[slug]/+page.server.ts(1 hunks)frontend/src/routes/joins/[slug]/+page.svelte(0 hunks)frontend/src/routes/joins/[slug]/job-tracker/+page.svelte(5 hunks)frontend/src/routes/joins/[slug]/lineage/+page.svelte(1 hunks)frontend/src/routes/joins/[slug]/observability/+layout.svelte(1 hunks)frontend/src/routes/joins/[slug]/observability/+page.server.ts(1 hunks)frontend/src/routes/joins/[slug]/observability/distributions/+page.svelte(1 hunks)frontend/src/routes/joins/[slug]/observability/distributions/+page.ts(1 hunks)frontend/src/routes/joins/[slug]/observability/drift/+page.svelte(2 hunks)frontend/src/routes/observability/+page.svelte(1 hunks)
💤 Files with no reviewable changes (1)
- frontend/src/routes/joins/[slug]/+page.svelte
✅ Files skipped from review due to trivial changes (9)
- frontend/src/routes/joins/[slug]/lineage/+page.svelte
- frontend/src/lib/components/StatusCell.svelte
- frontend/src/lib/components/PercentileChart.svelte
- frontend/src/lib/components/ExpandableCell.svelte
- frontend/src/lib/components/ChartControls.svelte
- frontend/src/lib/components/EChart.svelte
- frontend/src/routes/observability/+page.svelte
- frontend/src/routes/joins/+page.svelte
- frontend/src/routes/+layout.svelte
🔇 Additional comments (4)
frontend/src/routes/joins/[slug]/job-tracker/+page.svelte (4)
5-7: Clean import paths using $lib alias.Also applies to: 20-21
216-216: Consistent usage of data.dates prop.
Line range hint
241-245: Consistent usage of data.dates in iteration.
Line range hint
255-259: Consistent usage of data.dates in range calculation.
frontend/src/routes/joins/[slug]/observability/distributions/+page.svelte
Outdated
Show resolved
Hide resolved
frontend/src/routes/joins/[slug]/observability/distributions/+page.svelte
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/src/lib/components/ui/tabs/tabs-trigger.svelte (2)
17-20: Consider documenting the z-index value.The z-index: 10 seems arbitrary. Document why this specific value is needed.
22-23: Consider extracting the path matching logic.The regex could be more readable if extracted to a named function.
-$: isActive = - href === '/' ? $page.url.pathname === href : $page.url.pathname.match(href + '($|\\/)') != null; +const isPathMatch = (currentPath: string, targetPath: string) => + targetPath === '/' ? currentPath === targetPath : currentPath.match(targetPath + '($|\\/)') != null; +$: isActive = isPathMatch($page.url.pathname, href);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
frontend/src/lib/components/ui/tabs/tabs-trigger.svelte(1 hunks)
🔇 Additional comments (3)
frontend/src/lib/components/ui/tabs/tabs-trigger.svelte (3)
4-9: LGTM! Clean type definitions.
13-14: LGTM! Props are properly initialized.
26-44: LGTM! Clean conditional rendering with proper accessibility.The component correctly implements both tab trigger and router link modes while maintaining accessibility.
…nents to maintain chart controls location. Readd controls and wire up (zoom, cursor position, etc)
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
frontend/src/routes/joins/[slug]/observability/drift/+page.svelte (1)
Line range hint
1-430: Consider splitting into smaller components.The file is quite large. Consider extracting these into separate components:
- Chart creation logic
- Event handling logic
- Dialog content
🧹 Nitpick comments (3)
frontend/src/routes/joins/[slug]/observability/ModelTable.svelte (1)
9-9: Consider using responsive width classes.Replace fixed width with responsive classes for better adaptability.
-<div class="border rounded-md w-1/2 my-6"> +<div class="border rounded-md w-full md:w-2/3 lg:w-1/2 my-6">frontend/src/routes/joins/[slug]/observability/ObservabilityNavTabs.svelte (1)
8-20: Add accessibility improvements and use absolute paths.Consider these enhancements:
- Add aria-label to TabsList
- Use absolute paths for navigation
-<Tabs class="w-full"> +<Tabs class="w-full" aria-label="Observability navigation"> - <TabsList> + <TabsList aria-label="Observability sections"> - <TabsTrigger href="./drift" class="flex items-center"> + <TabsTrigger href="/joins/[slug]/observability/drift" class="flex items-center">frontend/src/routes/joins/[slug]/observability/distributions/+page.svelte (1)
78-81: Simplify CSS utilities.Consider using Tailwind's built-in utilities for better maintainability.
-class="sticky top-0 z-20 bg-neutral-100 border-b border-border -mx-8 py-2 px-8 border-l" +class="sticky top-0 z-20 bg-neutral-100 border-b border-l border-border -mx-8 p-2 px-8"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (5)
frontend/src/lib/components/PercentileChart.svelte(2 hunks)frontend/src/routes/joins/[slug]/observability/ModelTable.svelte(1 hunks)frontend/src/routes/joins/[slug]/observability/ObservabilityNavTabs.svelte(1 hunks)frontend/src/routes/joins/[slug]/observability/distributions/+page.svelte(1 hunks)frontend/src/routes/joins/[slug]/observability/drift/+page.svelte(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/lib/components/PercentileChart.svelte
🔇 Additional comments (5)
frontend/src/routes/joins/[slug]/observability/ModelTable.svelte (1)
1-7: Clean and well-structured imports!frontend/src/routes/joins/[slug]/observability/ObservabilityNavTabs.svelte (1)
1-6: Well-organized component imports!frontend/src/routes/joins/[slug]/observability/distributions/+page.svelte (2)
21-30: Add error handling for promise chain.Previous review already highlighted this issue.
96-119: Well-structured conditional rendering with proper loading states!frontend/src/routes/joins/[slug]/observability/drift/+page.svelte (1)
451-487: Well-organized chart rendering with proper instance management!
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/src/routes/joins/[slug]/observability/distributions/+page.svelte (1)
23-30:⚠️ Potential issueAdd error handling to the promise
Include a
.catchblock to handle promise rejections.Apply this diff:
try { data.distributionsPromise.then((d) => { distributions = d; isLoadingDistributions = false; + }).catch((err) => { + console.error('Error loading distributions:', err); + isLoadingDistributions = false; + }); } catch (err) { console.error('Error loading distributions:', err); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
frontend/src/routes/joins/[slug]/observability/distributions/+page.svelte(1 hunks)frontend/src/routes/joins/[slug]/observability/distributions/+page.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/routes/joins/[slug]/observability/distributions/+page.ts
🧰 Additional context used
🪛 GitHub Check: frontend_tests
frontend/src/routes/joins/[slug]/observability/distributions/+page.svelte
[failure] 50-50:
'currentZoomState' is assigned a value but never used. Allowed unused vars must match /^$$(Props|Events|Slots|Generic)$/u
frontend/src/routes/joins/[slug]/observability/distributions/+page.svelte
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
frontend/src/routes/joins/[slug]/observability/drift/+page.svelte (1)
Line range hint
164-196: Improve error handling in selectSeries.Currently, errors are only logged to console. Consider showing a user-friendly error message using a toast or alert component.
async function selectSeries(seriesName: string | undefined) { try { // ... existing code ... } catch (error) { console.error('Error fetching data:', error); + const errorMessage = error instanceof Error ? error.message : 'Failed to fetch data'; + toast.error(errorMessage); percentileData = null; comparedFeatureData = null; nullData = null; } }
♻️ Duplicate comments (1)
frontend/src/routes/joins/[slug]/observability/distributions/+page.svelte (1)
24-31:⚠️ Potential issueHandle promise errors with
.catch()Errors in promises aren't caught by
try-catch; add.catch()to handle them.+data.distributionsPromise + .then((d) => { + distributions = d; + isLoadingDistributions = false; + }) + .catch((err) => { + console.error('Error loading distributions:', err); + isLoadingDistributions = false; + });
🧹 Nitpick comments (2)
frontend/src/routes/joins/[slug]/observability/drift/+page.svelte (2)
23-23: Consider lazy loading the fade transition.Since the fade transition is used only once, consider lazy importing it to reduce the initial bundle size.
-import { fade } from 'svelte/transition'; +const fade = async () => (await import('svelte/transition')).fade;
Line range hint
143-152: Consolidate zoom handling logic.The zoom handling logic is duplicated between
handleZoomandhandleComparedFeatureZoom. Consider extracting it into a shared utility function.+function handleChartZoom(event: CustomEvent<EChartOption.DataZoom>, setZoomed: (value: boolean) => void) { + const detail = event.detail as { + start?: number; + end?: number; + batch?: Array<{ startValue: number; endValue: number }>; + }; + const start = detail.start ?? detail.batch?.[0]?.startValue ?? 0; + const end = detail.end ?? detail.batch?.[0]?.endValue ?? 100; + setZoomed(start !== 0 || end !== 100); + return { start, end }; +} -function handleZoom(event: CustomEvent<EChartOption.DataZoom>) { - const detail = event.detail as { - start?: number; - end?: number; - batch?: Array<{ startValue: number; endValue: number }>; - }; - const start = detail.start ?? detail.batch?.[0]?.startValue ?? 0; - const end = detail.end ?? detail.batch?.[0]?.endValue ?? 100; - isZoomed = start !== 0 || end !== 100; - currentZoomState = { start, end }; +function handleZoom(event: CustomEvent<EChartOption.DataZoom>) { + const { start, end } = handleChartZoom(event, (value) => isZoomed = value); + currentZoomState = { start, end }; } -function handleComparedFeatureZoom(event: CustomEvent<EChartOption.DataZoom>) { - const detail = event.detail as { - start?: number; - end?: number; - batch?: Array<{ startValue: number; endValue: number }>; - }; - const start = detail.start ?? detail.batch?.[0]?.startValue ?? 0; - const end = detail.end ?? detail.batch?.[0]?.endValue ?? 100; - isComparedFeatureZoomed = start !== 0 || end !== 100; +function handleComparedFeatureZoom(event: CustomEvent<EChartOption.DataZoom>) { + handleChartZoom(event, (value) => isComparedFeatureZoomed = value); }Also applies to: 378-387
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
frontend/src/routes/joins/[slug]/observability/distributions/+page.svelte(1 hunks)frontend/src/routes/joins/[slug]/observability/drift/+page.svelte(3 hunks)
🔇 Additional comments (1)
frontend/src/routes/joins/[slug]/observability/drift/+page.svelte (1)
438-449: Extract chart controls section to a shared component.This section is duplicated in the distributions page.
Also applies to: 510-521
ken-zlai
left a comment
ken-zlai
left a comment
There was a problem hiding this comment.
Nice. This largely looks good to me. Only one nit - when you switch from Drift -> Distributions or the oppisite, the sticky section fades away and back in quickly.
Screen.Recording.2025-01-17.at.4.59.44.PM.mov
I understand there are two versions of it, but the behavior seems a bit off UX wise.
I'm happy to merge as is so we can start resolving merge conflicts, but lets touch base on this next week to discuss :)
|
@sean-zlai Ahh, interesting - that sounds good to me! |
Summary
ui)load()function. This also enables browser caching viasetHeaders({ 'cache-control': ... })(example)Checklist
Summary by CodeRabbit
Summary by CodeRabbit
Refactor
New Features
Changes