Common conf routes #400
Common conf routes #400
Conversation
WalkthroughThis update refactors the lineage retrieval functionality across the frontend. Key changes include renaming the lineage conversion function from Changes
Possibly related PRs
Suggested reviewers
Poem
Warning Review ran into problems🔥 ProblemsGitHub Actions and Pipeline Checks: Resource not accessible by integration - https://docs.github.com/rest/actions/workflow-runs#list-workflow-runs-for-a-repository. Please grant the required permissions to the CodeRabbit GitHub App under the organization or repository settings. 🪧 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 (
|
sean-zlai
force-pushed
the
sean/conf-routes
branch
2 times, most recently
from
b860fcc to
57fd32f
Compare
…rtical real estate
sean-zlai
force-pushed
the
sean/conf-routes
branch
from
57fd32f to
39290a0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
frontend/src/lib/types/LogicalNode.ts (1)
106-107:⚠️ Potential issueFix type issues in isStreaming function.
Multiple @ts-expect-error comments indicate underlying type problems that should be addressed.
Consider updating the types:
interface JoinPart { groupBy: CombinedLogicalNode; } interface JoinNode { joinParts?: JoinPart[]; join?: { joinParts?: JoinPart[]; }; }Also applies to: 110-111
🧹 Nitpick comments (7)
frontend/src/lib/api/utils.ts (1)
24-31: Handle missing table or metaData carefully.Consider a clearer fallback name or warning if neither
metaDatanortableis defined.frontend/src/routes/[conf]/[name]/job-tracking/+page.ts (1)
25-25: Consider implementing lazy loading for lower levels.The TODO comment suggests implementing lazy loading to improve initial load performance.
frontend/src/routes/[conf]/[name]/observability/+layout.ts (1)
6-7: Consider making fallback dates configurable.Hardcoded timestamps from 2023 might become outdated.
-const FALLBACK_START_TS = 1672531200000; // 2023-01-01 -const FALLBACK_END_TS = 1677628800000; // 2023-03-01 +const FALLBACK_START_TS = process.env.FALLBACK_START_TS ?? 1672531200000; +const FALLBACK_END_TS = process.env.FALLBACK_END_TS ?? 1677628800000;frontend/src/routes/[conf]/[name]/observability/distributions/+page.svelte (1)
20-27: Enhance error handling with specific messages.Current error logging could be more informative for debugging.
} catch (err) { - console.error('Error loading distributions:', err); + console.error(`Failed to load distributions for ${data.confName}:`, err); + isLoadingDistributions = false; }frontend/src/lib/api/api.ts (1)
173-174: Track API endpoint implementation status.Multiple TODO comments about pending API endpoints should be tracked.
Would you like me to create issues to track the implementation of these API endpoints?
Also applies to: 185-186, 192-193
frontend/src/routes/[conf]/[name]/observability/drift/+page.svelte (1)
36-36: Consider adding type annotation for props.Removing explicit type annotation might reduce type safety.
-const { data } = $props(); +const { data }: { data: JoinData } = $props();frontend/src/routes/[conf]/[name]/overview/ConfProperties.svelte (1)
104-156: Consider adding null checks for nested properties.The model display could be more robust with null coalescing for nested properties.
-{#each Object.entries(conf.modelParams ?? {}) as [key, value]} +{#each Object.entries(conf?.modelParams ?? {}) as [key, value]} -{conf.outputSchema?.params?.map((p) => `${p.name}: ${p.dataType}`).join(', ')} +{conf?.outputSchema?.params?.map((p) => `${p?.name}: ${p?.dataType}`).join(', ') ?? ''}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (27)
frontend/src/lib/api/api.ts(2 hunks)frontend/src/lib/api/utils.ts(3 hunks)frontend/src/lib/types/LogicalNode.ts(1 hunks)frontend/src/routes/+layout.ts(1 hunks)frontend/src/routes/[conf]/+page.server.ts(1 hunks)frontend/src/routes/[conf]/+page.svelte(1 hunks)frontend/src/routes/[conf]/[name]/+layout.svelte(1 hunks)frontend/src/routes/[conf]/[name]/+layout.ts(1 hunks)frontend/src/routes/[conf]/[name]/+page.svelte(1 hunks)frontend/src/routes/[conf]/[name]/job-tracking/+page.ts(1 hunks)frontend/src/routes/[conf]/[name]/observability/+layout.ts(1 hunks)frontend/src/routes/[conf]/[name]/observability/distributions/+page.svelte(1 hunks)frontend/src/routes/[conf]/[name]/observability/distributions/+page.ts(1 hunks)frontend/src/routes/[conf]/[name]/observability/drift/+page.svelte(2 hunks)frontend/src/routes/[conf]/[name]/overview/+page.svelte(7 hunks)frontend/src/routes/[conf]/[name]/overview/ConfProperties.svelte(3 hunks)frontend/src/routes/groupbys/+page.server.ts(0 hunks)frontend/src/routes/groupbys/+page.svelte(0 hunks)frontend/src/routes/joins/+page.server.ts(0 hunks)frontend/src/routes/joins/[slug]/+layout.ts(0 hunks)frontend/src/routes/joins/[slug]/job-tracking/+page.ts(0 hunks)frontend/src/routes/joins/[slug]/observability/ModelTable.svelte(0 hunks)frontend/src/routes/joins/[slug]/services/joins.service.ts(0 hunks)frontend/src/routes/models/+page.server.ts(0 hunks)frontend/src/routes/models/+page.svelte(0 hunks)frontend/src/routes/stagingqueries/+page.server.ts(0 hunks)frontend/src/routes/stagingqueries/+page.svelte(0 hunks)
💤 Files with no reviewable changes (11)
- frontend/src/routes/groupbys/+page.svelte
- frontend/src/routes/stagingqueries/+page.svelte
- frontend/src/routes/models/+page.svelte
- frontend/src/routes/groupbys/+page.server.ts
- frontend/src/routes/joins/+page.server.ts
- frontend/src/routes/models/+page.server.ts
- frontend/src/routes/joins/[slug]/job-tracking/+page.ts
- frontend/src/routes/joins/[slug]/observability/ModelTable.svelte
- frontend/src/routes/stagingqueries/+page.server.ts
- frontend/src/routes/joins/[slug]/+layout.ts
- frontend/src/routes/joins/[slug]/services/joins.service.ts
✅ Files skipped from review due to trivial changes (2)
- frontend/src/routes/[conf]/[name]/+page.svelte
- frontend/src/routes/+layout.ts
🔇 Additional comments (39)
frontend/src/lib/api/utils.ts (16)
5-5: Type usage is clear.
12-12: Import looks correct.
15-15: Confirm parameter type integrity.Double-check if all callers pass a valid
CombinedLogicalNodeinstead of anIJoin.
22-23: Use ofgetLogicalNodeTypeis consistent.
33-34: Casting might mask type errors.Ensure
confindeed satisfiesILogicalNode.
37-37: Connections initialization looks fine.
39-41: Comment references previous join logic.
42-44: Conditional check is valid.
46-48: Sensible approach tojoinParts.
50-52: Comment about groupBy flows well.
53-57: Source processing logic is correct.
59-61: Comment about model is fine.
62-63: Source check is straightforward.
66-72: Return structure is consistent.
75-81:processJoinPartsis neatly extracted.
140-142: Beware of type casting.Ensure
source.joinSource.joinis actually a validCombinedLogicalNode.frontend/src/routes/[conf]/+page.svelte (1)
7-9: Reactive sorting looks fine.frontend/src/routes/[conf]/[name]/observability/distributions/+page.ts (1)
4-8: Renaming from join to conf is coherent.Verify references to
confandconfNamedon’t break existing logic.Also applies to: 15-15
frontend/src/routes/[conf]/+page.server.ts (1)
13-44: Well-structured error handling and response mapping!The implementation includes proper error handling and uses a clean mapping approach.
frontend/src/routes/[conf]/[name]/overview/ConfProperties.svelte (10)
80-82: LGTM! Improved metadata label rendering.Added null check for metaDataLabel to prevent empty label display.
104-150: LGTM! Enhanced model properties display.Added comprehensive model information display with type, params and schema details.
158-184: LGTM! Added source display logic.Added recursive source display with proper type differentiation.
22-24: LGTM!New type imports enhance type safety for model-related functionality.
80-82: LGTM!Added null check prevents rendering empty label.
104-150: LGTM!Enhanced model information display with proper type handling and detailed schema information.
158-185: LGTM!Added source display with proper type handling and recursive rendering.
22-24: LGTM: New type imports enhance type safety.
80-82: LGTM: Improved null safety in metadata display.The additional checks prevent rendering empty labels and accessing undefined properties.
Also applies to: 87-96
158-185: LGTM: Clean implementation of source handling.The code elegantly handles both
leftandsourceproperties with appropriate recursion.frontend/src/routes/[conf]/[name]/observability/drift/+page.svelte (2)
91-91: LGTM! Updated join name retrieval.Updated to use conf instead of join for name retrieval.
91-91: LGTM!Updated data access pattern from join to conf.
frontend/src/routes/[conf]/[name]/overview/+page.svelte (8)
45-45: LGTM! Enhanced data reactivity.Added derived store for nodeGraph to improve reactivity.
246-246: LGTM! Standardized animation timing.Added consistent animation duration for edge transitions.
Also applies to: 263-263
321-321: LGTM! Added null safety.Added null check for query.selects to prevent runtime errors.
364-375: LGTM! Added navigation feature.Added button to navigate to selected node's configuration.
36-38: LGTM!Added necessary imports for navigation button.
45-45: LGTM!Improved reactivity using derived stores.
Also applies to: 63-63
246-247: LGTM!Standardized animation duration for consistent UX.
Also applies to: 263-264
364-376: LGTM!Added navigation button with proper URL handling and conditional rendering.
| function getConfApi(api: Api, confType: ConfType, confName: string) { | ||
| switch (confType) { | ||
| case ConfType.JOIN: | ||
| return { | ||
| conf: api.getJoin(confName), | ||
| lineage: api.getJoinLineage({ name: confName }) | ||
| }; | ||
| case ConfType.GROUP_BY: | ||
| return { | ||
| conf: api.getGroupBy(confName), | ||
| lineage: api.getGroupByLineage({ name: confName }) | ||
| }; | ||
| case ConfType.MODEL: | ||
| return { | ||
| conf: api.getModel(confName), | ||
| lineage: api.getModelLineage({ name: confName }) | ||
| }; | ||
| case ConfType.STAGING_QUERY: | ||
| return { | ||
| conf: api.getStagingQuery(confName), | ||
| lineage: api.getStagingQueryLineage({ name: confName }) | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
Add error handling for invalid confType.
The switch statement should handle invalid confType to prevent undefined return.
function getConfApi(api: Api, confType: ConfType, confName: string) {
switch (confType) {
case ConfType.JOIN:
return {
conf: api.getJoin(confName),
lineage: api.getJoinLineage({ name: confName })
};
case ConfType.GROUP_BY:
return {
conf: api.getGroupBy(confName),
lineage: api.getGroupByLineage({ name: confName })
};
case ConfType.MODEL:
return {
conf: api.getModel(confName),
lineage: api.getModelLineage({ name: confName })
};
case ConfType.STAGING_QUERY:
return {
conf: api.getStagingQuery(confName),
lineage: api.getStagingQueryLineage({ name: confName })
};
+ default:
+ throw new Error(`Invalid configuration type: ${confType}`);
}
}📝 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.
| function getConfApi(api: Api, confType: ConfType, confName: string) { | |
| switch (confType) { | |
| case ConfType.JOIN: | |
| return { | |
| conf: api.getJoin(confName), | |
| lineage: api.getJoinLineage({ name: confName }) | |
| }; | |
| case ConfType.GROUP_BY: | |
| return { | |
| conf: api.getGroupBy(confName), | |
| lineage: api.getGroupByLineage({ name: confName }) | |
| }; | |
| case ConfType.MODEL: | |
| return { | |
| conf: api.getModel(confName), | |
| lineage: api.getModelLineage({ name: confName }) | |
| }; | |
| case ConfType.STAGING_QUERY: | |
| return { | |
| conf: api.getStagingQuery(confName), | |
| lineage: api.getStagingQueryLineage({ name: confName }) | |
| }; | |
| } | |
| } | |
| function getConfApi(api: Api, confType: ConfType, confName: string) { | |
| switch (confType) { | |
| case ConfType.JOIN: | |
| return { | |
| conf: api.getJoin(confName), | |
| lineage: api.getJoinLineage({ name: confName }) | |
| }; | |
| case ConfType.GROUP_BY: | |
| return { | |
| conf: api.getGroupBy(confName), | |
| lineage: api.getGroupByLineage({ name: confName }) | |
| }; | |
| case ConfType.MODEL: | |
| return { | |
| conf: api.getModel(confName), | |
| lineage: api.getModelLineage({ name: confName }) | |
| }; | |
| case ConfType.STAGING_QUERY: | |
| return { | |
| conf: api.getStagingQuery(confName), | |
| lineage: api.getStagingQueryLineage({ name: confName }) | |
| }; | |
| default: | |
| throw new Error(`Invalid configuration type: ${confType}`); | |
| } | |
| } |
| export const load: PageServerLoad = async ({ params, fetch }) => { | ||
| const api = new Api({ fetch }); | ||
|
|
||
| const confType = getEntity(params.conf as EntityId).type; | ||
| const confApi = getConfApi(api, confType, params.name); | ||
|
|
||
| const [conf, lineage] = await Promise.all([confApi?.conf, confApi?.lineage]); | ||
|
|
||
| return { | ||
| conf, | ||
| lineage | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Add error handling for failed API calls.
The load function should handle API failures gracefully.
export const load: PageServerLoad = async ({ params, fetch }) => {
const api = new Api({ fetch });
const confType = getEntity(params.conf as EntityId).type;
- const confApi = getConfApi(api, confType, params.name);
+ try {
+ const confApi = getConfApi(api, confType, params.name);
+ const [conf, lineage] = await Promise.all([confApi?.conf, confApi?.lineage]);
- const [conf, lineage] = await Promise.all([confApi?.conf, confApi?.lineage]);
-
- return {
- conf,
- lineage
- };
+ return { conf, lineage };
+ } catch (error) {
+ console.error('Failed to load configuration:', error);
+ throw error;
+ }
};📝 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 const load: PageServerLoad = async ({ params, fetch }) => { | |
| const api = new Api({ fetch }); | |
| const confType = getEntity(params.conf as EntityId).type; | |
| const confApi = getConfApi(api, confType, params.name); | |
| const [conf, lineage] = await Promise.all([confApi?.conf, confApi?.lineage]); | |
| return { | |
| conf, | |
| lineage | |
| }; | |
| }; | |
| export const load: PageServerLoad = async ({ params, fetch }) => { | |
| const api = new Api({ fetch }); | |
| const confType = getEntity(params.conf as EntityId).type; | |
| try { | |
| const confApi = getConfApi(api, confType, params.name); | |
| const [conf, lineage] = await Promise.all([confApi?.conf, confApi?.lineage]); | |
| return { conf, lineage }; | |
| } catch (error) { | |
| console.error('Failed to load configuration:', error); | |
| throw error; | |
| } | |
| }; |
| // todo think about where this is best called - might be worth lazily lower levels | ||
| const jobTrackerDataMap = new Map<string, IJobTrackerResponseArgs>(); | ||
| await Promise.all( | ||
| Array.from(nodes).map(async (node) => { | ||
| const data = await api.getJobTrackerData(node); | ||
| jobTrackerDataMap.set(node.name ?? '', data); | ||
| }) | ||
| ); |
There was a problem hiding this comment.
Add error handling for job tracker data fetching.
Wrap API calls in try-catch to handle failures gracefully.
const jobTrackerDataMap = new Map<string, IJobTrackerResponseArgs>();
await Promise.all(
Array.from(nodes).map(async (node) => {
+ try {
const data = await api.getJobTrackerData(node);
jobTrackerDataMap.set(node.name ?? '', data);
+ } catch (error) {
+ console.error(`Failed to fetch job tracker data for ${node.name}:`, error);
+ // Set empty data to allow partial rendering
+ jobTrackerDataMap.set(node.name ?? '', {});
+ }
})
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // todo think about where this is best called - might be worth lazily lower levels | |
| const jobTrackerDataMap = new Map<string, IJobTrackerResponseArgs>(); | |
| await Promise.all( | |
| Array.from(nodes).map(async (node) => { | |
| const data = await api.getJobTrackerData(node); | |
| jobTrackerDataMap.set(node.name ?? '', data); | |
| }) | |
| ); | |
| // todo think about where this is best called - might be worth lazily lower levels | |
| const jobTrackerDataMap = new Map<string, IJobTrackerResponseArgs>(); | |
| await Promise.all( | |
| Array.from(nodes).map(async (node) => { | |
| try { | |
| const data = await api.getJobTrackerData(node); | |
| jobTrackerDataMap.set(node.name ?? '', data); | |
| } catch (error) { | |
| console.error(`Failed to fetch job tracker data for ${node.name}:`, error); | |
| // Set empty data to allow partial rendering | |
| jobTrackerDataMap.set(node.name ?? '', {}); | |
| } | |
| }) | |
| ); |
| <!-- TODO: Hide if user only has access to control plane (and not data plane) --> | ||
| {#if page.params.conf === 'joins'} | ||
| <TabsTrigger href="/{page.params.conf}/{page.params.name}/observability"> | ||
| <IconClipboardDocumentCheck16Solid class="mr-2 h-4 w-4" /> | ||
| Observability | ||
| </TabsTrigger> | ||
| {/if} |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Implement proper access control.
The TODO comment suggests implementing proper access control based on user permissions.
🏁 Script executed:
#!/bin/bash
# Search for user permission checks in the codebase
rg -A 5 "hasPermission|isAuthorized|canAccess"Length of output: 48
Ensure proper access control is implemented.
- No permission-check logic (e.g., using
hasPermission/isAuthorized/canAccess) is present. - The TODO must be replaced with actual checks to hide the tab for users lacking data plane access.
- File:
frontend/src/routes/[conf]/[name]/+layout.svelte(Lines 29–35)
| }); | ||
|
|
||
| // If empty data is results, use fallback date range | ||
| const useFallback = joinDrift.driftSeries.every((ds) => Object.keys(ds).length <= 1); // Only `key` returned on results |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen empty data check.
Current check might miss edge cases where other properties exist but data is still effectively empty.
-const useFallback = joinDrift.driftSeries.every((ds) => Object.keys(ds).length <= 1); // Only `key` returned on results
+const useFallback = joinDrift.driftSeries.every((ds) => !ds.driftValue && Object.keys(ds).length <= 1);📝 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 useFallback = joinDrift.driftSeries.every((ds) => Object.keys(ds).length <= 1); // Only `key` returned on results | |
| const useFallback = joinDrift.driftSeries.every((ds) => !ds.driftValue && Object.keys(ds).length <= 1); // Only `key` returned on results |
| async getGroupByLineage({ | ||
| name | ||
| // type, | ||
| // branch, | ||
| // direction | ||
| }: ILineageRequestArgs): Promise<ILineageResponse> { | ||
| // TODO: Remove this once we have the API endpoint | ||
| return this.getGroupBy(name!).then((groupBy) => { | ||
| return confToLineage(groupBy); | ||
| }); | ||
| } | ||
|
|
||
| async getModelLineage({ | ||
| name | ||
| // type, | ||
| // branch, | ||
| // direction | ||
| }: ILineageRequestArgs): Promise<ILineageResponse> { | ||
| // TODO: Remove this once we have the API endpoint | ||
| return this.getModel(name!).then((model) => { | ||
| return confToLineage(model); | ||
| }); | ||
| } | ||
|
|
||
| async getStagingQueryLineage({ name }: ILineageRequestArgs): Promise<ILineageResponse> { | ||
| // TODO: Remove this once we have the API endpoint | ||
| return this.getStagingQuery(name!).then((stagingQuery) => { | ||
| return confToLineage(stagingQuery); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce duplication in lineage methods.
Similar pattern repeated across getGroupByLineage, getModelLineage, and getStagingQueryLineage.
+private async getConfLineage<T>(name: string, getConfFn: (name: string) => Promise<T>): Promise<ILineageResponse> {
+ return getConfFn(name).then(confToLineage);
+}
async getGroupByLineage({ name }: ILineageRequestArgs): Promise<ILineageResponse> {
- return this.getGroupBy(name!).then((groupBy) => {
- return confToLineage(groupBy);
- });
+ return this.getConfLineage(name!, this.getGroupBy.bind(this));
}📝 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.
| async getGroupByLineage({ | |
| name | |
| // type, | |
| // branch, | |
| // direction | |
| }: ILineageRequestArgs): Promise<ILineageResponse> { | |
| // TODO: Remove this once we have the API endpoint | |
| return this.getGroupBy(name!).then((groupBy) => { | |
| return confToLineage(groupBy); | |
| }); | |
| } | |
| async getModelLineage({ | |
| name | |
| // type, | |
| // branch, | |
| // direction | |
| }: ILineageRequestArgs): Promise<ILineageResponse> { | |
| // TODO: Remove this once we have the API endpoint | |
| return this.getModel(name!).then((model) => { | |
| return confToLineage(model); | |
| }); | |
| } | |
| async getStagingQueryLineage({ name }: ILineageRequestArgs): Promise<ILineageResponse> { | |
| // TODO: Remove this once we have the API endpoint | |
| return this.getStagingQuery(name!).then((stagingQuery) => { | |
| return confToLineage(stagingQuery); | |
| }); | |
| private async getConfLineage<T>(name: string, getConfFn: (name: string) => Promise<T>): Promise<ILineageResponse> { | |
| return getConfFn(name).then(confToLineage); | |
| } | |
| async getGroupByLineage({ name }: ILineageRequestArgs): Promise<ILineageResponse> { | |
| // TODO: Remove this once we have the API endpoint | |
| return this.getConfLineage(name!, this.getGroupBy.bind(this)); | |
| } | |
| async getModelLineage({ | |
| name | |
| // type, | |
| // branch, | |
| // direction | |
| }: ILineageRequestArgs): Promise<ILineageResponse> { | |
| // TODO: Remove this once we have the API endpoint | |
| return this.getModel(name!).then((model) => { | |
| return confToLineage(model); | |
| }); | |
| } | |
| async getStagingQueryLineage({ name }: ILineageRequestArgs): Promise<ILineageResponse> { | |
| // TODO: Remove this once we have the API endpoint | |
| return this.getStagingQuery(name!).then((stagingQuery) => { | |
| return confToLineage(stagingQuery); | |
| }); | |
| } |
| const api = new Api(); | ||
| const { data }: { data: JoinData } = $props(); | ||
| const { data } = $props(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type annotation for data prop.
Missing type annotation could lead to type safety issues.
-const { data } = $props();
+const { data }: { data: JoinData } = $props();📝 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 { data } = $props(); | |
| const { data }: { data: JoinData } = $props(); |
ken-zlai
left a comment
ken-zlai
left a comment
There was a problem hiding this comment.
@sean-zlai Thanks for doing this! I had some stashed code to get started, but it’s great to have this now - really clean implementation.
I also noticed the issue where clicking "Open" might take you to a non-existent node. I’m fine with it as it is since it's just a limitation of the sample data. However, I’m wondering if we could:
- Disable the "Open" button if the node doesn’t exist (e.g., by making a lightweight API call to check for its existence).
Would love to hear your thoughts!
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/src/routes/[conf]/[name]/overview/+page.svelte (2)
366-373: Add aria-label to improve button accessibility.<Button variant="outline" href="{config.url}/{selectedNode.id}" + aria-label="Open configuration details" on:click={() => (selectedNode = null)} > Open <IconArrowRight class="ml-2" /> </Button>
321-321: Consider additional null safety checks.While
?? {}handles undefined query, accessing nested properties could still fail.-{#each Object.entries(data.value.conf.query?.selects ?? {}) as [key, _]} +{#each Object.entries(data.value.conf?.query?.selects ?? {}) as [key, _]}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
frontend/src/routes/+layout.svelte(1 hunks)frontend/src/routes/[conf]/[name]/+layout.svelte(1 hunks)frontend/src/routes/[conf]/[name]/overview/+page.svelte(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/routes/[conf]/[name]/+layout.svelte
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: enforce_triggered_workflows
🔇 Additional comments (2)
frontend/src/routes/+layout.svelte (1)
35-37: Clean layout structure.Good consolidation of layout properties into a single container.
frontend/src/routes/[conf]/[name]/overview/+page.svelte (1)
246-246: LGTM! Animation duration optimized.The reduced duration of 150ms will make the transitions feel snappier.
Also applies to: 263-263
| const { data } = $props(); | ||
| const { connections, infoMap } = data.lineage.nodeGraph ?? {}; | ||
| const { connections, infoMap } = $derived(data.lineage.nodeGraph ?? {}); |
There was a problem hiding this comment.
Add null safety checks for nodeGraph destructuring.
The destructuring could fail if data.lineage is undefined.
-const { connections, infoMap } = $derived(data.lineage.nodeGraph ?? {});
+const { connections = new Map(), infoMap = new Map() } = $derived(data.lineage?.nodeGraph ?? {});📝 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 { connections, infoMap } = $derived(data.lineage.nodeGraph ?? {}); | |
| const { connections = new Map(), infoMap = new Map() } = $derived(data.lineage?.nodeGraph ?? {}); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/src/lib/components/ChartControls.svelte (1)
35-42: LGTM: Clean date fallback implementation.Consider extracting the date conversion to a helper function for reusability.
+const toLocalDate = (timestamp: number) => fromAbsolute(timestamp, getLocalTimeZone()); + <DateRangeSelector fallbackDateRange={isUsingFallbackDates ? { - start: fromAbsolute(dateRange.startTimestamp, getLocalTimeZone()), - end: fromAbsolute(dateRange.endTimestamp, getLocalTimeZone()) + start: toLocalDate(dateRange.startTimestamp), + end: toLocalDate(dateRange.endTimestamp) } : undefined} />frontend/src/lib/components/DateRangeSelector.svelte (2)
94-96: Consider accessibility improvements.Add
aria-labelto the tooltip trigger for screen readers.-<TooltipTrigger> +<TooltipTrigger aria-label="Date range selection information">Also applies to: 174-176
112-134: Simplify complex conditional rendering.Extract date formatting logic into a helper function to reduce template complexity.
+<script> +function formatDateRange(range: DateRange) { + if (!range?.start) return 'Pick a date'; + const start = df.format(range.start.toDate(getLocalTimeZone())); + if (!range.end) return start; + return `${start} - ${df.format(range.end.toDate(getLocalTimeZone()))}`; +} +</script> -{#if calendarDateRangePopoverOpen || (calendarDateRange?.start == null && fallbackDateRange?.start == null)} - Pick a date -{:else if fallbackDateRange?.start} - {#if fallbackDateRange.end} - {df.format(fallbackDateRange.start.toDate(getLocalTimeZone()))} - {df.format( - fallbackDateRange.end.toDate(getLocalTimeZone()) - )} - {:else} - {df.format(fallbackDateRange.start.toDate(getLocalTimeZone()))} - {/if} -{:else if calendarDateRange?.start} - {#if calendarDateRange.end} - {df.format(calendarDateRange.start.toDate(getLocalTimeZone()))} - {df.format( - calendarDateRange.end.toDate(getLocalTimeZone()) - )} - {:else} - {df.format(calendarDateRange.start.toDate(getLocalTimeZone()))} - {/if} -{/if} +{calendarDateRangePopoverOpen ? 'Pick a date' : + formatDateRange(fallbackDateRange) || formatDateRange(calendarDateRange)}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
frontend/src/lib/components/ChartControls.svelte(2 hunks)frontend/src/lib/components/DateRangeSelector.svelte(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: frontend_tests
- GitHub Check: enforce_triggered_workflows
🔇 Additional comments (3)
frontend/src/lib/components/ChartControls.svelte (1)
7-7: LGTM: Import changes align with new date handling.frontend/src/lib/components/DateRangeSelector.svelte (2)
22-25: LGTM: Clean imports and prop definition.The new imports and prop definition are well-structured.
177-189: LGTM: Clear fallback data indication.The tooltip content effectively communicates when fallback data is being shown.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
frontend/src/lib/components/ActionButtons.svelte (1)
16-16: 🛠️ Refactor suggestionRemove unused prop
showClusterprop is defined but never used.- showCluster?: boolean;
🧹 Nitpick comments (1)
frontend/src/lib/components/ActionButtons.svelte (1)
34-37: Add ARIA labelAdd aria-label for better accessibility.
- <Button variant="secondary" size="sm" icon="leading" on:click={toggleSort}> + <Button variant="secondary" size="sm" icon="leading" on:click={toggleSort} aria-label="Toggle sort order">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
frontend/src/lib/components/ActionButtons.svelte(1 hunks)frontend/src/lib/components/ChartControls.svelte(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/lib/components/ChartControls.svelte
🔇 Additional comments (1)
frontend/src/lib/components/ActionButtons.svelte (1)
22-29: Clean sort implementationGood use of derived stores and utility functions.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/src/routes/[conf]/[name]/observability/drift/+page.svelte (1)
92-92: Improve path string manipulation.Replace string manipulation with a dedicated path utility.
-const joinName = data.conf?.metaData?.name?.replace('/', '.') ?? 'Unknown'; +import { normalizePath } from '$lib/utils/path'; +const joinName = data.conf?.metaData?.name ? normalizePath(data.conf.metaData.name) : 'Unknown';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
frontend/src/routes/[conf]/[name]/observability/drift/+page.svelte(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: enforce_triggered_workflows
🔇 Additional comments (1)
frontend/src/routes/[conf]/[name]/observability/drift/+page.svelte (1)
36-36: Add type annotation for data prop.
| showDefaults: false | ||
| }); | ||
| const sortDirection = $derived(params[sortKey]); | ||
| // TODO: Determine why sortDirection is not updating if client-side routed to page (page refresh or HMR fixes it though) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Investigate and fix the sortDirection update issue.
The TODO comment indicates a client-side routing bug that affects state updates.
Run this script to check for similar routing issues:
🏁 Script executed:
#!/bin/bash
# Search for similar routing-related issues
rg -A 3 "client-side rout" Length of output: 467
Action Required: Fix sortDirection Update on Client-Side Navigation
- File:
frontend/src/routes/[conf]/[name]/observability/drift/+page.svelte(line 45) - Issue: The
sortDirectionstate does not update on client-side routing (only fixes on refresh/HMR). - Note: Search for similar issues returned only this instance; please investigate the state update handling and ensure consistent behavior across routing methods.
Summary
Checklist
Summary by CodeRabbit