Lineage DAG #252
Lineage DAG #252
Conversation
WalkthroughThis pull request updates multiple frontend modules. The changes include dependency additions and updates in Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Utility
Client->>API: Request lineage data (ILineageRequestArgs)
API->>API: Call getJoin() to fetch join data
API->>Utility: Invoke joinToLineage(join)
Utility-->>API: Return ILineageResponse
API-->>Client: Respond with lineage data
sequenceDiagram
participant User
participant TransformControls
participant transformContext
User->>TransformControls: Click zoom/reset button
TransformControls->>transformContext: Execute transformation action
transformContext-->>TransformControls: Return updated view
TransformControls-->>User: Display updated chart
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. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🪧 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 (
|
|
Creating this PR to replace #234 as |
sean-zlai
force-pushed
the
sean/echarts-to-layerchart2
branch
from
6daa599 to
b70d4c2
Compare
sean-zlai
force-pushed
the
sean/lineage-dag3
branch
2 times, most recently
from
b1fa04b to
b08db56
Compare
sean-zlai
force-pushed
the
sean/echarts-to-layerchart2
branch
from
7a6b200 to
6130fdb
Compare
sean-zlai
force-pushed
the
sean/lineage-dag3
branch
from
b08db56 to
d1cdd81
Compare
sean-zlai
force-pushed
the
sean/echarts-to-layerchart2
branch
from
6130fdb to
f4dafc8
Compare
sean-zlai
force-pushed
the
sean/lineage-dag3
branch
3 times, most recently
from
8fead02 to
8da6530
Compare
sean-zlai
force-pushed
the
sean/lineage-dag3
branch
from
8da6530 to
1e2e2bc
Compare
sean-zlai
force-pushed
the
sean/echarts-to-layerchart2
branch
2 times, most recently
from
ebe38bc to
6dc0099
Compare
sean-zlai
force-pushed
the
sean/lineage-dag3
branch
from
1e2e2bc to
3f0fb64
Compare
sean-zlai
force-pushed
the
sean/echarts-to-layerchart2
branch
from
91f1421 to
e76e315
Compare
sean-zlai
force-pushed
the
sean/lineage-dag3
branch
2 times, most recently
from
012526d to
f41d73c
Compare
sean-zlai
force-pushed
the
sean/echarts-to-layerchart2
branch
from
6928aab to
1708448
Compare
sean-zlai
force-pushed
the
sean/lineage-dag3
branch
from
f41d73c to
9092ac2
Compare
sean-zlai
changed the base branch from
main
to
sean/thrift-observability-apis
sean-zlai
force-pushed
the
sean/thrift-observability-apis
branch
from
1dfdd63 to
1927b04
Compare
sean-zlai
force-pushed
the
sean/lineage-dag3
branch
from
9092ac2 to
78b9719
Compare
sean-zlai
force-pushed
the
sean/lineage-dag3
branch
3 times, most recently
from
098e8d8 to
08f6854
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: 0
🧹 Nitpick comments (8)
frontend/src/lib/api/utils.ts (1)
13-13: Spelling nitpick."Insteaad" is misspelled. Suggest "instead".
frontend/src/routes/joins/[slug]/overview/+page.svelte (1)
114-121: Remove commented code if unneeded.Dead code can confuse future maintainers.
frontend/src/routes/joins/+page.svelte (1)
7-7: Address the TODO comment.The comment suggests this is a temporary solution.
Would you like me to help implement a more permanent solution?
frontend/src/routes/joins/[slug]/observability/distributions/+page.ts (1)
11-11: Consider implementing error state display.The TODO suggests showing errored columns in an error state.
Would you like me to help implement the error state display?
frontend/src/lib/components/charts/TransformControls.svelte (2)
44-66: Add keyboard event handling for accessibility.Consider adding keyboard shortcuts for zoom and reset actions.
67-108: Consider adding aria-labels to buttons.Enhance accessibility by adding descriptive aria-labels to transform control buttons.
<Button on:click={() => transform.zoomIn()} size="icon" variant="ghost" class="rounded-full" + aria-label="Zoom in" >frontend/src/lib/components/charts/FeaturesLineChart.svelte (1)
58-61: Clean up commented code.Remove or implement the commented item style:
- // item: () => 'flex items-center gap-2'frontend/src/routes/joins/[slug]/overview/ConfProperties.svelte (1)
185-186: Remove@ts-expect-errorby improving type safety.The type assertion can be improved to avoid using
@ts-expect-error.Consider using type predicates or type guards:
- // @ts-expect-error: find way to do this without TS complaining - return SOURCE_PROPERTIES.includes(key); + return SOURCE_PROPERTIES.includes(key as SourceProperties);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (20)
frontend/package.json(2 hunks)frontend/src/lib/api/api.ts(3 hunks)frontend/src/lib/api/utils.ts(1 hunks)frontend/src/lib/components/LogicalNodeTable.svelte(1 hunks)frontend/src/lib/components/charts/FeaturesLineChart.svelte(1 hunks)frontend/src/lib/components/charts/PercentileLineChart.svelte(1 hunks)frontend/src/lib/components/charts/TransformControls.svelte(1 hunks)frontend/src/lib/types/LogicalNode.ts(1 hunks)frontend/src/routes/joins/+page.svelte(1 hunks)frontend/src/routes/joins/[slug]/+layout.svelte(1 hunks)frontend/src/routes/joins/[slug]/+page.server.ts(1 hunks)frontend/src/routes/joins/[slug]/lineage/+page.svelte(0 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(4 hunks)frontend/src/routes/joins/[slug]/overview/+page.svelte(1 hunks)frontend/src/routes/joins/[slug]/overview/ConfProperties.svelte(1 hunks)frontend/src/routes/joins/[slug]/services/joins.service.ts(2 hunks)frontend/tailwind.config.js(1 hunks)hub/src/main/scala/ai/chronon/hub/handlers/VertxExtensions.scala(0 hunks)
💤 Files with no reviewable changes (2)
- frontend/src/routes/joins/[slug]/lineage/+page.svelte
- hub/src/main/scala/ai/chronon/hub/handlers/VertxExtensions.scala
✅ Files skipped from review due to trivial changes (2)
- frontend/src/lib/components/charts/PercentileLineChart.svelte
- frontend/src/routes/joins/[slug]/observability/distributions/+page.svelte
🧰 Additional context used
🧠 Learnings (2)
frontend/src/routes/joins/[slug]/+layout.svelte (1)
Learnt from: sean-zlai
PR: zipline-ai/chronon#244
File: frontend/src/routes/joins/[slug]/+layout.svelte:9-9
Timestamp: 2025-01-17T21:50:45.432Z
Learning: In Svelte 5, `$derived` is a new reactivity primitive that replaces the older reactive declarations. Don't suggest changing `$derived` expressions to `$store` syntax.
frontend/src/routes/joins/[slug]/services/joins.service.ts (1)
Learnt from: ken-zlai
PR: zipline-ai/chronon#160
File: frontend/src/routes/joins/[slug]/services/joins.service.ts:34-34
Timestamp: 2025-01-17T00:33:14.792Z
Learning: The join timeseries API in `frontend/src/routes/joins/[slug]/services/joins.service.ts` specifically requires 'drift' as the metric type, regardless of the metricType parameter passed to the function.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: scala_compile_fmt_fix
- GitHub Check: non_spark_tests
🔇 Additional comments (29)
frontend/src/lib/api/utils.ts (3)
15-17: Runtime typing check.Using
@ts-expect-errormay hide real issues. Verify correct typings or explicitly cast.
79-89: Potential key collisions.Verify merging nodeGraph doesn't overwrite existing keys with different data.
94-101: Looks good!Return structure is clear and well-formed.
frontend/src/routes/joins/[slug]/overview/+page.svelte (3)
39-45: Check node uniqueness.Ensure
key.namecollisions won't cause hidden nodes.
46-55: Validate recursion.
getEdgescalls itself recursively. Confirm no endless loops.
392-403: Cross-platform logic approved.The key detection for Mac vs. non-Mac is concise.
frontend/src/routes/joins/[slug]/+page.server.ts (1)
4-4: Verify redirect path.Check if appending "/overview" to
url.hrefforms correct absolute path.frontend/src/routes/joins/+page.svelte (1)
8-10: LGTM! Clean refactoring.Sorting logic is now more concise.
frontend/src/lib/types/LogicalNode.ts (1)
6-32: LGTM! Well-structured configuration.Clean implementation with consistent color formatting and appropriate icon sourcing.
frontend/src/routes/joins/[slug]/observability/distributions/+page.ts (1)
7-8: LGTM! Improved null safety.Good use of optional chaining and nullish coalescing.
frontend/src/routes/joins/[slug]/+layout.svelte (1)
16-31: LGTM! Clean UI reorganization.Header and tab structure changes improve the navigation flow.
frontend/src/lib/components/LogicalNodeTable.svelte (1)
48-53: Verify if enabling pointer events for all items is intentional.The removal of
pointer-events-noneallows interaction with visually disabled items. This might conflict with the TODO comment about enabling items "once we have data".frontend/src/routes/joins/[slug]/services/joins.service.ts (2)
32-40: LGTM! Efficient concurrent API calls.Using Promise.all for parallel requests improves performance.
47-76: Good error handling for drift data.The try-catch block gracefully handles missing drift series data.
frontend/src/lib/api/api.ts (7)
3-13: LGTM!Consistent interface naming convention.
48-48: LGTM!Return type aligns with interface renaming.
51-52: LGTM!Clean implementation using
.then()to extract property.
55-56: LGTM!Consistent with
getJoinimplementation.
59-60: LGTM!Maintains consistency with other methods.
63-66: LGTM!Clean multi-line formatting.
144-163: Address TODO and commented code.The method contains:
- Commented-out URL parameter construction
- TODO comment about API endpoint
Please confirm the timeline for implementing the API endpoint.
frontend/tailwind.config.js (1)
190-201: LGTM!Well-structured animation configurations with varying speeds.
frontend/src/routes/joins/[slug]/observability/drift/+page.svelte (3)
54-54: LGTM!Robust null check for drift series.
93-93: LGTM!Safe join name retrieval with optional chaining.
340-354: LGTM!Clean removal of mock values.
frontend/package.json (4)
65-65: New dependency added.
"@layerstack/svelte-actions": "^0.0.11"is now included.
70-70: Dependency update.
"layerchart"upgraded to^0.99.1. Verify for any breaking changes.
72-72: New dependency added.
svelte-inspect-valuewith version^0.1.0-next.0is introduced; ensure compatibility with your Svelte setup.
85-87: Override addition.
The override for"svelte-inspect-value"enforcing"svelte": "^5.0.0"ensures version compatibility.
…etween `streaming` and `non-streaming" via link color/animation
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
frontend/src/lib/types/LogicalNode.ts (2)
86-87: Clarify TODO and implement different logic for joins.The comment indicates incomplete implementation for different join vs groupBy logic.
Would you like me to help implement the different logic for joins vs groupBys?
80-104: Simplify isStreaming function using early returns.The nested conditionals make the code harder to read.
export function isStreaming(node: CombinedLogicalNode): boolean { if ('topic' in node || 'mutationTopic' in node) { return true; - } else { - if ('sources' in node) { - return ( - node.sources?.some((source) => { - let result = false; - if ('entities' in source && source.entities) { - result = isStreaming(source.entities); - } else if ('events' in source && source.events) { - result = isStreaming(source.events); - } else if ('joinSource' in source && source.joinSource) { - result = isStreaming(source.joinSource as CombinedLogicalNode); - } - return result; - }) ?? false - ); - } else { - return false; - } } + + if (!('sources' in node)) { + return false; + } + + return node.sources?.some(source => { + if ('entities' in source && source.entities) { + return isStreaming(source.entities); + } + if ('events' in source && source.events) { + return isStreaming(source.events); + } + if ('joinSource' in source && source.joinSource) { + return isStreaming(source.joinSource as CombinedLogicalNode); + } + return false; + }) ?? false; }frontend/src/routes/joins/[slug]/overview/+page.svelte (2)
50-59: Memoize getEdges function for performance.The function is called repeatedly and could benefit from memoization.
+const edgesCache = new Map<string, DagreData['edges']>(); + function getEdges(node: INodeKey): DagreData['edges'] { + const cacheKey = JSON.stringify(node); + if (edgesCache.has(cacheKey)) { + return edgesCache.get(cacheKey)!; + } + const parents = connections?.get(node)?.parents; - return ( + const edges = ( parents?.flatMap((p) => { return [{ source: p.name!, target: node.name! }, ...getEdges(p)]; }) ?? [] ); + edgesCache.set(cacheKey, edges); + return edges; }
373-384: Simplify tooltip keyboard shortcut logic.The keyboard event handling could be more concise.
-<svelte:window - onkeydown={(e) => { - if (isMacOS() ? e.metaKey : e.ctrlKey) { - hideTooltip = true; - } - }} - onkeyup={(e) => { - if (isMacOS() ? !e.metaKey : !e.ctrlKey) { - hideTooltip = false; - } - }} -/> +<svelte:window + onkeydown={(e) => hideTooltip = isMacOS() ? e.metaKey : e.ctrlKey} + onkeyup={(e) => hideTooltip = isMacOS() ? e.metaKey : e.ctrlKey} +/>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (4)
frontend/src/lib/api/utils.ts(1 hunks)frontend/src/lib/types/LogicalNode.ts(1 hunks)frontend/src/routes/joins/[slug]/overview/+page.svelte(1 hunks)frontend/src/routes/joins/[slug]/overview/ConfProperties.svelte(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: non_spark_tests
- GitHub Check: scala_compile_fmt_fix
- GitHub Check: enforce_triggered_workflows
- GitHub Check: enforce_triggered_workflows
🔇 Additional comments (1)
frontend/src/routes/joins/[slug]/overview/ConfProperties.svelte (1)
71-72:❓ Verification inconclusive
Verify $state compatibility.
Using experimental $state syntax. Ensure Svelte version supports it.
🌐 Web query:
Does Svelte support $state syntax? What version introduced it?💡 Result:
Svelte introduced the
$statesyntax in Svelte 5, marking a major shift in reactivity and state management[3][4][9]. This new syntax replaces older patterns like implicitletreactivity and stores, offering a more explicit and unified approach to reactive variables[4][6].Key details:
- Explicit Reactivity: Variables wrapped in
$state()become reactive without needing.valueaccessors or store subscriptions[3][4].- Proxied Objects: When used with objects/arrays,
$statecreates granular reactivity through JavaScript Proxies (e.g.,$state({ count: 0 })becomes reactive at the property level)[1][3].- Cross-Component Sharing: Unlike Svelte 4's stores,
$stateobjects can be directly exported/imported between components while maintaining reactivity[1][10].The migration guide confirms this is a breaking change requiring updates to existing Svelte 4 codebases[4]. Current documentation shows
$stateas core to Svelte 5's reactivity system[3][6][9].Citations:
- 1: https://dev.to/mandrasch/svelte-5-share-state-between-components-for-dummies-4gd2
- 2: https://developer.mozilla.org/en-US/docs/Learn_web_development/Core/Frameworks_libraries/Svelte_getting_started
- 3: https://labs.thisdot.co/blog/svelte-5-is-here
- 4: https://svelte.dev/docs/svelte/v5-migration-guide
- 5: https://svelte.dev/tutorial
- 6: https://frontendmasters.com/blog/introducing-svelte-5/
- 7: https://svelte.dev/docs/kit/state-management
- 8: https://svelte.dev/docs/kit
- 9: https://vercel.com/blog/whats-new-in-svelte-5
- 10: https://joyofcode.xyz/how-to-share-state-in-svelte-5
Ensure Svelte 5+ is used for
$statesyntax compatibility.The code using
$stateis correct in Svelte 5. Please verify that your project’s dependencies (e.g., inpackage.json) specify Svelte version 5 or later. If you’re still on Svelte 4, an upgrade is required.
- Confirm Svelte version in your project setup.
| // @ts-expect-error: Bad typing | ||
| const connections: NodeGraph['connections'] = new InternMap([], JSON.stringify); | ||
| // @ts-expect-error: Bad typing | ||
| const infoMap: NodeGraph['infoMap'] = new InternMap([], JSON.stringify); |
There was a problem hiding this comment.
Fix type errors instead of suppressing them.
Remove @ts-expect-error and properly type the InternMap instantiations.
-// @ts-expect-error: Bad typing
-const connections: NodeGraph['connections'] = new InternMap([], JSON.stringify);
-// @ts-expect-error: Bad typing
-const infoMap: NodeGraph['infoMap'] = new InternMap([], JSON.stringify);
+const connections = new InternMap<INodeKey, { parents: INodeKey[] }>([], JSON.stringify);
+const infoMap = new InternMap<INodeKey, { conf: ILogicalNode }>([], JSON.stringify);📝 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.
| // @ts-expect-error: Bad typing | |
| const connections: NodeGraph['connections'] = new InternMap([], JSON.stringify); | |
| // @ts-expect-error: Bad typing | |
| const infoMap: NodeGraph['infoMap'] = new InternMap([], JSON.stringify); | |
| // Updated type instantiations without suppressing type errors | |
| const connections = new InternMap<INodeKey, { parents: INodeKey[] }>([], JSON.stringify); | |
| const infoMap = new InternMap<INodeKey, { conf: ILogicalNode }>([], JSON.stringify); |
frontend/src/lib/api/utils.ts
Outdated
| // Transfer connections and infoMap from joinSource join to root join graph | ||
| const joinSourceLineage = joinToLineage(source.joinSource.join as IJoin); | ||
|
|
There was a problem hiding this comment.
Add stack depth limit for recursive join processing.
The recursive call to joinToLineage could cause stack overflow with deeply nested joins.
+const MAX_JOIN_DEPTH = 100;
+
+function joinToLineage(join: IJoin, depth: number = 0): ILineageResponse {
+ if (depth > MAX_JOIN_DEPTH) {
+ throw new Error('Maximum join depth exceeded');
+ }
+
// Transfer connections and infoMap from joinSource join to root join graph
- const joinSourceLineage = joinToLineage(source.joinSource.join as IJoin);
+ const joinSourceLineage = joinToLineage(source.joinSource.join as IJoin, depth + 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.
| // Transfer connections and infoMap from joinSource join to root join graph | |
| const joinSourceLineage = joinToLineage(source.joinSource.join as IJoin); | |
| const MAX_JOIN_DEPTH = 100; | |
| function joinToLineage(join: IJoin, depth: number = 0): ILineageResponse { | |
| if (depth > MAX_JOIN_DEPTH) { | |
| throw new Error('Maximum join depth exceeded'); | |
| } | |
| // Transfer connections and infoMap from joinSource join to root join graph | |
| const joinSourceLineage = joinToLineage(source.joinSource.join as IJoin, depth + 1); | |
| // ...rest of the function implementation... | |
| } |
| Operation | ||
| } from '$src/lib/types/codegen'; | ||
| import { keys } from '@layerstack/utils'; | ||
| import Self from './ConfProperties.svelte'; |
There was a problem hiding this comment.
Avoid self-import.
Self-import could cause circular dependency issues.
-import Self from './ConfProperties.svelte';
+import type { SvelteComponent } from 'svelte';
+const Self = 'ConfProperties.svelte' as unknown as typeof SvelteComponent;📝 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.
| import Self from './ConfProperties.svelte'; | |
| import type { SvelteComponent } from 'svelte'; | |
| const Self = 'ConfProperties.svelte' as unknown as typeof SvelteComponent; |
| {#if keys(conf).some((key) => { | ||
| // @ts-expect-error: find way to do this without TS complaining | ||
| return SOURCE_PROPERTIES.includes(key); | ||
| })} |
There was a problem hiding this comment.
Fix TypeScript error.
Replace type assertion with proper type checking.
- {#if keys(conf).some((key) => {
- // @ts-expect-error: find way to do this without TS complaining
- return SOURCE_PROPERTIES.includes(key);
- })}
+ {#if keys(conf).some((key): key is SourceProperties =>
+ SOURCE_PROPERTIES.includes(key as SourceProperties)
+ )}📝 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.
| {#if keys(conf).some((key) => { | |
| // @ts-expect-error: find way to do this without TS complaining | |
| return SOURCE_PROPERTIES.includes(key); | |
| })} | |
| {#if keys(conf).some((key): key is SourceProperties => | |
| SOURCE_PROPERTIES.includes(key as SourceProperties) | |
| )} |
| <!-- TODO: Fix "Type `IQuery` is not assignable to type `string`" and remove `as any` --> | ||
| {#if source?.entities} | ||
| <div class="border rounded-md px-4 py-2 bg-neutral-500/10"> | ||
| <Self conf={source.entities as typeof conf} {includeUpstream} /> | ||
| </div> | ||
| {/if} | ||
|
|
||
| {#if source?.events} | ||
| <div class="border rounded-md px-4 py-2 bg-neutral-500/10"> | ||
| <Self conf={source.events as typeof conf} {includeUpstream} /> | ||
| </div> | ||
| {/if} | ||
|
|
||
| {#if source?.joinSource} | ||
| <div class="border rounded-md px-4 py-2 bg-neutral-500/10"> | ||
| <Self conf={source.joinSource as typeof conf} {includeUpstream} /> | ||
| </div> | ||
| {/if} | ||
| {/each} |
There was a problem hiding this comment.
Fix type assertions.
Multiple as type assertions indicate type safety issues.
-<Self conf={source.entities as typeof conf} {includeUpstream} />
+<Self conf={source.entities satisfies typeof conf} {includeUpstream} />
-<Self conf={source.events as typeof conf} {includeUpstream} />
+<Self conf={source.events satisfies typeof conf} {includeUpstream} />
-<Self conf={source.joinSource as typeof conf} {includeUpstream} />
+<Self conf={source.joinSource satisfies typeof conf} {includeUpstream} />📝 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: Fix "Type `IQuery` is not assignable to type `string`" and remove `as any` --> | |
| {#if source?.entities} | |
| <div class="border rounded-md px-4 py-2 bg-neutral-500/10"> | |
| <Self conf={source.entities as typeof conf} {includeUpstream} /> | |
| </div> | |
| {/if} | |
| {#if source?.events} | |
| <div class="border rounded-md px-4 py-2 bg-neutral-500/10"> | |
| <Self conf={source.events as typeof conf} {includeUpstream} /> | |
| </div> | |
| {/if} | |
| {#if source?.joinSource} | |
| <div class="border rounded-md px-4 py-2 bg-neutral-500/10"> | |
| <Self conf={source.joinSource as typeof conf} {includeUpstream} /> | |
| </div> | |
| {/if} | |
| {/each} | |
| <!-- TODO: Fix "Type `IQuery` is not assignable to type `string`" and remove `as any` --> | |
| {#if source?.entities} | |
| <div class="border rounded-md px-4 py-2 bg-neutral-500/10"> | |
| <Self conf={source.entities satisfies typeof conf} {includeUpstream} /> | |
| </div> | |
| {/if} | |
| {#if source?.events} | |
| <div class="border rounded-md px-4 py-2 bg-neutral-500/10"> | |
| <Self conf={source.events satisfies typeof conf} {includeUpstream} /> | |
| </div> | |
| {/if} | |
| {#if source?.joinSource} | |
| <div class="border rounded-md px-4 py-2 bg-neutral-500/10"> | |
| <Self conf={source.joinSource satisfies typeof conf} {includeUpstream} /> | |
| </div> | |
| {/if} | |
| {/each} |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
frontend/src/lib/types/LogicalNode.ts (3)
12-38: Add type safety to configuration object.Add a type definition for the configuration object structure.
+type LogicalNodeConfig = { + label: string; + icon: any; // Consider using a more specific type + color: string; +}; + +export const logicalNodeConfig: Record<LogicalType, LogicalNodeConfig> = {
70-75: Improve readability of subtype checks.Replace chained ternary with if-else.
-return 'snapshotTable' in node ? 'entity' : 'table' in node ? 'event' : null; +if ('snapshotTable' in node) return 'entity'; +if ('table' in node) return 'event'; +return null;
80-112: Simplify streaming check logic.Extract repeated patterns into helper functions.
+function checkSourceStreaming(source: any): boolean { + if ('entities' in source && source.entities) { + return isStreaming(source.entities); + } + if ('events' in source && source.events) { + return isStreaming(source.events); + } + if ('joinSource' in source && source.joinSource) { + return isStreaming(source.joinSource as CombinedLogicalNode); + } + return false; +} + export function isStreaming(node: CombinedLogicalNode): boolean { if ('topic' in node || 'mutationTopic' in node) { return true; } else if ('table' in node || 'snapshotTable' in node) { return false; } else if ('sources' in node) { - return ( - node.sources?.some((source) => { - let result = false; - if ('entities' in source && source.entities) { - result = isStreaming(source.entities); - } else if ('events' in source && source.events) { - result = isStreaming(source.events); - } else if ('joinSource' in source && source.joinSource) { - result = isStreaming(source.joinSource as CombinedLogicalNode); - } - return result; - }) ?? false - ); + return node.sources?.some(checkSourceStreaming) ?? false; } // ... rest of the function
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
frontend/src/lib/types/LogicalNode.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: non_spark_tests
- GitHub Check: scala_compile_fmt_fix
- GitHub Check: frontend_tests
- GitHub Check: enforce_triggered_workflows
🔇 Additional comments (1)
frontend/src/lib/types/LogicalNode.ts (1)
1-10: LGTM!Clean imports and robust type definition.
| // @ts-expect-error - TODO: fix types | ||
| return node.join.joinParts?.some((joinPart) => isStreaming(joinPart.groupBy)) ?? true; |
There was a problem hiding this comment.
❓ Verification inconclusive
Fix type issues marked with @ts-expect-error.
Address the TODO comments to fix type issues.
Also applies to: 107-108
🏁 Script executed:
#!/bin/bash
# Search for the type definitions of joinParts and groupBy
ast-grep --pattern 'type $name = {
$$$
joinParts?: $type
$$$
}'Length of output: 71
Fix type issues and resolve TODOs in LogicalNode.ts
- Lines 103-104 & 107-108: The use of
@ts-expect-errorindicates unresolved type mismatches, possibly with propertiesjoinPartsandgroupBy. - Since automated searches for their type definitions returned no results, please manually verify these types to ensure they are properly defined and used.
| /** Determine node type from it's properties */ | ||
| export function getLogicalNodeType(node: { key: INodeKey; value: INodeInfo }) { | ||
| if ('joinParts' in node.value.conf!) { | ||
| return LogicalType.JOIN; | ||
| } else if ('sources' in node.value.conf!) { | ||
| return LogicalType.GROUP_BY; | ||
| } else if ('query' in node.value.conf! && typeof node.value.conf.query === 'string') { | ||
| return LogicalType.STAGING_QUERY; | ||
| } else if ('modelType' in node.value.conf!) { | ||
| return LogicalType.MODEL; | ||
| } else { | ||
| return LogicalType.TABULAR_DATA; | ||
| } | ||
| } | ||
|
|
||
| export function getLogicalNodeConfig(node: { key: INodeKey; value: INodeInfo }) { | ||
| const nodeType = getLogicalNodeType(node); | ||
| if (nodeType === LogicalType.TABULAR_DATA) { | ||
| return { | ||
| ...logicalNodeConfig[LogicalType.TABULAR_DATA], | ||
| icon: | ||
| getLogicalNodeSubtype(node.value.conf as CombinedLogicalNode) === 'entity' | ||
| ? IconTableCells | ||
| : IconSignal | ||
| }; | ||
| } else { | ||
| return logicalNodeConfig[node.key.logicalType!]; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace non-null assertions with type guards.
Use type guards to avoid ! operators.
-if ('joinParts' in node.value.conf!) {
+if (node.value.conf && 'joinParts' in node.value.conf) {📝 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.
| /** Determine node type from it's properties */ | |
| export function getLogicalNodeType(node: { key: INodeKey; value: INodeInfo }) { | |
| if ('joinParts' in node.value.conf!) { | |
| return LogicalType.JOIN; | |
| } else if ('sources' in node.value.conf!) { | |
| return LogicalType.GROUP_BY; | |
| } else if ('query' in node.value.conf! && typeof node.value.conf.query === 'string') { | |
| return LogicalType.STAGING_QUERY; | |
| } else if ('modelType' in node.value.conf!) { | |
| return LogicalType.MODEL; | |
| } else { | |
| return LogicalType.TABULAR_DATA; | |
| } | |
| } | |
| export function getLogicalNodeConfig(node: { key: INodeKey; value: INodeInfo }) { | |
| const nodeType = getLogicalNodeType(node); | |
| if (nodeType === LogicalType.TABULAR_DATA) { | |
| return { | |
| ...logicalNodeConfig[LogicalType.TABULAR_DATA], | |
| icon: | |
| getLogicalNodeSubtype(node.value.conf as CombinedLogicalNode) === 'entity' | |
| ? IconTableCells | |
| : IconSignal | |
| }; | |
| } else { | |
| return logicalNodeConfig[node.key.logicalType!]; | |
| } | |
| } | |
| /** Determine node type from it's properties */ | |
| export function getLogicalNodeType(node: { key: INodeKey; value: INodeInfo }) { | |
| if (node.value.conf && 'joinParts' in node.value.conf) { | |
| return LogicalType.JOIN; | |
| } else if ('sources' in node.value.conf!) { | |
| return LogicalType.GROUP_BY; | |
| } else if ('query' in node.value.conf! && typeof node.value.conf.query === 'string') { | |
| return LogicalType.STAGING_QUERY; | |
| } else if ('modelType' in node.value.conf!) { | |
| return LogicalType.MODEL; | |
| } else { | |
| return LogicalType.TABULAR_DATA; | |
| } | |
| } | |
| export function getLogicalNodeConfig(node: { key: INodeKey; value: INodeInfo }) { | |
| const nodeType = getLogicalNodeType(node); | |
| if (nodeType === LogicalType.TABULAR_DATA) { | |
| return { | |
| ...logicalNodeConfig[LogicalType.TABULAR_DATA], | |
| icon: | |
| getLogicalNodeSubtype(node.value.conf as CombinedLogicalNode) === 'entity' | |
| ? IconTableCells | |
| : IconSignal | |
| }; | |
| } else { | |
| return logicalNodeConfig[node.key.logicalType!]; | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/src/lib/types/LogicalNode.ts (1)
73-73: Replace non-null assertion with type guard.Use type guards to avoid
!operators.-return logicalNodeConfig[node.key.logicalType!]; +return node.key.logicalType ? logicalNodeConfig[node.key.logicalType] : logicalNodeConfig[LogicalType.TABULAR_DATA];frontend/src/routes/joins/[slug]/overview/+page.svelte (2)
150-183: Extract edge rendering logic to a separate function.The edge calculation logic is complex and would be more maintainable as a separate function.
+function calculateEdgePoints(source: CustomNode, target: CustomNode, e: any) { + const sourceType = getLogicalNodeType(source.value.conf as CombinedLogicalNode); + const targetType = getLogicalNodeType(target.value.conf as CombinedLogicalNode); + + const sourcePoint = { + x: source.x + source.width / 2, + y: source.y + }; + const targetPoint = { + x: target.x - target.width / 2, + y: target.y + }; + + let points: Array<{ x: number; y: number }> = []; + if (sourceType === LogicalType.TABULAR_DATA && targetType === LogicalType.JOIN) { + points = e.points; + points[0] = sourcePoint; + points[points.length - 1] = targetPoint; + } else { + points = [sourcePoint, targetPoint]; + } + + return points; +}
274-326: Move tooltip content to a separate component.Extract the tooltip content into a reusable component to improve maintainability.
Create a new file
TooltipContent.svelte:<script lang="ts"> import { Tooltip } from 'layerchart'; import { tooltipProps } from '$src/lib/components/charts/common.js'; export let data: any; </script> <Tooltip.List {...tooltipProps.list}> <!-- MetaData --> {#each ['name'] as prop} {#if data.value.conf.metaData?.[prop] !== undefined} <Tooltip.Item {...tooltipProps.item} label={prop} value={data.value.conf.metaData?.[prop]} valueAlign="left" /> {/if} {/each} <!-- Rest of the tooltip content --> </Tooltip.List>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (4)
frontend/src/lib/api/utils.ts(1 hunks)frontend/src/lib/types/LogicalNode.ts(1 hunks)frontend/src/routes/joins/[slug]/overview/+page.svelte(1 hunks)frontend/src/routes/joins/[slug]/overview/ConfProperties.svelte(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/routes/joins/[slug]/overview/ConfProperties.svelte
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: scala_compile_fmt_fix
- GitHub Check: non_spark_tests
- GitHub Check: enforce_triggered_workflows
- GitHub Check: enforce_triggered_workflows
🔇 Additional comments (3)
frontend/src/lib/api/utils.ts (2)
15-18: Fix type errors instead of suppressing them.Remove @ts-expect-error and properly type the InternMap instantiations.
-// @ts-expect-error: Bad typing -const connections: NodeGraph['connections'] = new InternMap([], JSON.stringify); -// @ts-expect-error: Bad typing -const infoMap: NodeGraph['infoMap'] = new InternMap([], JSON.stringify); +const connections = new InternMap<INodeKey, { parents: INodeKey[] }>([], JSON.stringify); +const infoMap = new InternMap<INodeKey, { conf: ILogicalNode }>([], JSON.stringify);
102-102: Add stack depth limit for recursive join processing.The recursive call to
joinToLineagecould cause stack overflow with deeply nested joins.+const MAX_JOIN_DEPTH = 100; + +function joinToLineage(join: IJoin, depth: number = 0): ILineageResponse { + if (depth > MAX_JOIN_DEPTH) { + throw new Error('Maximum join depth exceeded'); + } + // Transfer connections and infoMap from joinSource join to root join graph - const joinSourceLineage = joinToLineage(source.joinSource.join as IJoin); + const joinSourceLineage = joinToLineage(source.joinSource.join as IJoin, depth + 1);frontend/src/lib/types/LogicalNode.ts (1)
101-102: Fix type issues and resolve TODOs.The use of
@ts-expect-errorindicates unresolved type mismatches withjoinPartsandgroupBy.#!/bin/bash # Search for the type definitions of joinParts and groupBy ast-grep --pattern 'type $name = { $$$ joinParts?: $type $$$ }'Also applies to: 105-106
…h tall heights (can still pan/zoom)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/src/routes/joins/[slug]/overview/+page.svelte (3)
148-180: Consider extracting edge rendering logic into a separate function.The edge rendering logic is complex and would be more maintainable as a standalone function.
+function createNodeEdge(e: any, nodesById: Map<string, CustomNode>) { + const source = nodesById.get(e.v) as CustomNode; + const target = nodesById.get(e.w) as CustomNode; + + const sourceType = getLogicalNodeType(source.value.conf as CombinedLogicalNode); + const targetType = getLogicalNodeType(target.value.conf as CombinedLogicalNode); + + const sourcePoint = { + x: source.x + source.width / 2, + y: source.y + }; + const targetPoint = { + x: target.x - target.width / 2, + y: target.y + }; + + let points: Array<{ x: number; y: number }> = []; + if (sourceType === LogicalType.TABULAR_DATA && targetType === LogicalType.JOIN) { + points = e.points; + points[0] = sourcePoint; + points[points.length - 1] = targetPoint; + } else { + points = [sourcePoint, targetPoint]; + } + + return { + ...e, + source, + target, + points + }; +} + -{@const nodeEdges = edges.map((e) => { - const source = nodesById.get(e.v) as CustomNode; - const target = nodesById.get(e.w) as CustomNode; - - const sourceType = getLogicalNodeType(source.value.conf as CombinedLogicalNode); - const targetType = getLogicalNodeType(target.value.conf as CombinedLogicalNode); - - const sourcePoint = { - x: source.x + source.width / 2, - y: source.y - }; - const targetPoint = { - x: target.x - target.width / 2, - y: target.y - }; - - let points: Array<{ x: number; y: number }> = []; - if (sourceType === LogicalType.TABULAR_DATA && targetType === LogicalType.JOIN) { - points = e.points; - points[0] = sourcePoint; - points[points.length - 1] = targetPoint; - } else { - points = [sourcePoint, targetPoint]; - } - - return { - ...e, - source, - target, - points - }; -})} +{@const nodeEdges = edges.map((e) => createNodeEdge(e, nodesById))}
184-227: Consider extracting node rendering logic into a separate component.The node rendering logic would be more maintainable as a standalone Svelte component.
+<!-- NodeComponent.svelte --> +<script lang="ts"> + export let node: CustomNode; + export let fadeNode: (node: CustomNode) => boolean; + export let onSelect: (node: CustomNode) => void; + export let onHover: (e: Event, node: CustomNode) => void; + export let onLeave: () => void; + + $: [namespace, ...nameParts] = node.label?.split('.') ?? []; + $: config = getLogicalNodeConfig(node); + $: Icon = config?.icon; +</script> + +<Group + class={cn( + 'bg-neutral-300 border border-neutral-400 rounded-md', + 'cursor-pointer hover:outline outline-surface-content/20 outline-offset-0 hover:outline-offset-1 transition-all', + fadeNode(node) && 'opacity-20' + )} + x={node.x - node.width / 2} + y={node.y - node.height / 2} + style="width:{node.width}px; height:{node.height}px" + onclick={() => onSelect(node)} + onpointermove={(e) => onHover(e, node)} + onpointerleave={onLeave} +> + <div class="h-full px-2 grid grid-cols-[auto_1fr] gap-3 items-center"> + <div + style:--color={config.color} + class="bg-[hsl(var(--color)/5%)] border border-[hsl(var(--color))] text-[hsl(var(--color))] w-8 h-8 rounded flex items-center justify-center" + > + <Icon /> + </div> + <div class="align-middle truncate"> + <div class="text-xs text-surface-content/50"> + {namespace} + </div> + <div class="text-sm text-surface-content truncate"> + {nameParts.join('.')} + </div> + </div> + </div> +</Group>Then use it in the main component:
-{#each nodes as _node (_node.label)} - {@const node = _node as CustomNode} - {@const [namespace, ...nameParts] = node.label?.split('.') ?? []} - {@const config = getLogicalNodeConfig(node)} - {@const Icon = config?.icon} - <Group - class={cn( - 'bg-neutral-300 border border-neutral-400 rounded-md', - 'cursor-pointer hover:outline outline-surface-content/20 outline-offset-0 hover:outline-offset-1 transition-all', - fadeNode(node) && 'opacity-20' - )} - x={node.x - node.width / 2} - y={node.y - node.height / 2} - style="width:{node.width}px; height:{node.height}px" - onclick={() => { - selectedNode = node; - }} - onpointermove={(e) => { - hoveredNode = node; - tooltip.show(e, node); - }} - onpointerleave={() => { - hoveredNode = null; - tooltip.hide(); - }} - > - <div class="h-full px-2 grid grid-cols-[auto_1fr] gap-3 items-center"> - <div - style:--color={config.color} - class="bg-[hsl(var(--color)/5%)] border border-[hsl(var(--color))] text-[hsl(var(--color))] w-8 h-8 rounded flex items-center justify-center" - > - <Icon /> - </div> - <div class="align-middle truncate"> - <div class="text-xs text-surface-content/50"> - {namespace} - </div> - <div class="text-sm text-surface-content truncate"> - {nameParts.join('.')} - </div> - </div> - </div> - </Group> -{/each} +{#each nodes as _node (_node.label)} + {@const node = _node as CustomNode} + <NodeComponent + {node} + {fadeNode} + onSelect={(n) => (selectedNode = n)} + onHover={(e, n) => { + hoveredNode = n; + tooltip.show(e, n); + }} + onLeave={() => { + hoveredNode = null; + tooltip.hide(); + }} + /> +{/each}
343-358: Extract duplicated node styling into a shared component.The node styling is duplicated between the chart and dialog. Consider creating a shared component.
+<!-- NodeDisplay.svelte --> +<script lang="ts"> + export let namespace: string; + export let nameParts: string[]; + export let config: any; + export let Icon: any; +</script> + +<div class="h-full px-2 grid grid-cols-[auto_1fr] gap-3 items-center"> + <div + style:--color={config.color} + class="bg-[hsl(var(--color)/5%)] border border-[hsl(var(--color))] text-[hsl(var(--color))] w-8 h-8 rounded flex items-center justify-center" + > + <Icon /> + </div> + <div class="align-middle truncate"> + <div class="text-xs text-surface-content/50"> + {namespace} + </div> + <div class="text-sm text-surface-content truncate"> + {nameParts.join('.')} + </div> + </div> +</div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
frontend/src/routes/joins/[slug]/overview/+page.svelte(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: scala_compile_fmt_fix
- GitHub Check: non_spark_tests
- GitHub Check: frontend_tests
- GitHub Check: enforce_triggered_workflows
🔇 Additional comments (3)
frontend/src/routes/joins/[slug]/overview/+page.svelte (3)
1-110: LGTM! Well-organized code structure.The imports are logically grouped, types are well-defined, and graph-related functions are properly documented.
112-116: LGTM! Clean implementation of collapsible section.
386-397: LGTM! Clean keyboard shortcut implementation.Platform-specific key detection is properly handled.
## Summary
- **Data Generation & API**
- Job tracker data is generated using Thrift data structures and the
lineage for a specific join.
- `getJobTrackerData` generates random job tracker data with overlapping
`dateRanges`.
- **Frontend Enhancements**
- **Styling & Status Updates**
- Made status colors more consistent (`frontend/src/app.css`).
- Created `StatusBar.svelte` to centralize job tracker status styling.
- Standardized status styling, display text, etc., in
`frontend/src/lib/job/status.ts`.
- **Job Tree Structure**
- Added `tree-builder.ts` to streamline converting `ILineageResponse`
and `IJobTrackerResponseArgs` into `JobTreeNode`.
- Handles recursive structures, overlapping dates (choosing the most
recent `submittedTs`), and leaf nodes.
- Unit tests added, with a focus on `organizeTasksByDate` due to its
complex logic.
- **UX Improvements**
- Root nodes in the job tracker table are expanded by default for better
visibility.
---
## Checklist
- [x] Added Unit Tests
- [ ] Covered by existing CI
- [x] Integration tested
- [ ] Documentation update
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
frontend/src/lib/job/tree-builder/tree-builder.ts (2)
18-26: Consider a map for constant-time lookups.
64-79: Double-check handling of empty or null node names.frontend/src/lib/job/tree-builder/tree-builder.test.ts (1)
5-133: LGTM! Comprehensive test coverage for buildJobTrackerTree.Tests cover empty lineage, overlapping tasks, and hierarchical structures.
Consider adding test cases for:
- Circular dependencies
- Deep hierarchies (>3 levels)
- Large number of overlapping tasks
frontend/src/lib/api/api.ts (2)
146-165: Track temporary implementation of getJoinLineage.The current implementation uses a temporary solution pending API endpoint.
Would you like me to create an issue to track the implementation of the actual API endpoint?
167-228: Improve mock data generation.While the mock data is functional, consider:
- Making date ranges configurable
- Adding more realistic status distributions
- Extracting mock data generation to a separate utility
+import { generateMockJobTrackerData } from './mockUtils'; + async getJobTrackerData( node: INodeKeyArgs, daysToGenerate: number = 60 ): Promise<IJobTrackerResponseArgs> { - const startDate = new Date('2024-01-01'); - // ... rest of mock implementation + return generateMockJobTrackerData(node, daysToGenerate); }frontend/src/routes/joins/[slug]/job-tracking/+page.svelte (1)
104-110: Improve readability of scrollbar width calculation.-function calculateScrollbarWidth(containerWidth: number, scrollWidth: number) { - if (!jobStructureHead) return 0; - const fixedColumnWidth = jobStructureHead.getBoundingClientRect().width; - const scrollableWidth = scrollWidth - fixedColumnWidth; - const visibleWidth = containerWidth - fixedColumnWidth; - return (visibleWidth / scrollableWidth) * 100; +function calculateScrollbarWidth(containerWidth: number, scrollWidth: number) { + if (!jobStructureHead) return 0; + + const fixedColumnWidth = jobStructureHead.getBoundingClientRect().width; + const scrollableArea = { + total: scrollWidth - fixedColumnWidth, + visible: containerWidth - fixedColumnWidth + }; + + return (scrollableArea.visible / scrollableArea.total) * 100; +}frontend/src/lib/job/status.ts (2)
14-20: Consider using a type alias for better type safety.+type StatusColorType = 'bg' | 'border' | 'active-border'; export function getStatusColorVariable( status: Status, - type: 'bg' | 'border' | 'active-border' + type: StatusColorType ): string {
39-58: Consider memoizing the result for performance.+const statusClassesCache = new Map<string, string>(); + export function getStatusClasses( status: Status | undefined, isActive: boolean, includeHover: boolean ): string { if (status === undefined) return ''; + + const cacheKey = `${status}-${isActive}-${includeHover}`; + const cached = statusClassesCache.get(cacheKey); + if (cached) return cached; const statusString = Status[status]; const classes = [ `bg-jobs-${statusString}-bg`, isActive ? `border-jobs-${statusString}-active-border` : `border-jobs-${statusString}-border` ]; if (includeHover) { classes.push(`hover:border-jobs-${statusString}-active-border`); } - return classes.join(' '); + const result = classes.join(' '); + statusClassesCache.set(cacheKey, result); + return result;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (20)
frontend/src/app.css(1 hunks)frontend/src/lib/api/api.ts(3 hunks)frontend/src/lib/components/ExpandableCell.svelte(1 hunks)frontend/src/lib/components/JobOverviewChart.svelte(2 hunks)frontend/src/lib/components/StatusBar.svelte(1 hunks)frontend/src/lib/components/StatusCell.svelte(2 hunks)frontend/src/lib/components/ui/table/index.ts(3 hunks)frontend/src/lib/components/ui/table/table-head.svelte(1 hunks)frontend/src/lib/job/status.ts(1 hunks)frontend/src/lib/job/tree-builder/tree-builder.test.ts(1 hunks)frontend/src/lib/job/tree-builder/tree-builder.ts(1 hunks)frontend/src/lib/types/Job/Job.ts(0 hunks)frontend/src/lib/util/job-sample-data.ts(0 hunks)frontend/src/lib/util/lineage-job-sample-data.ts(1 hunks)frontend/src/routes/joins/[slug]/+layout.ts(1 hunks)frontend/src/routes/joins/[slug]/job-tracking/+page.svelte(6 hunks)frontend/src/routes/joins/[slug]/job-tracking/+page.ts(1 hunks)frontend/src/routes/joins/[slug]/services/jobTracker.service.ts(0 hunks)frontend/src/routes/joins/[slug]/services/joins.service.ts(1 hunks)frontend/tailwind.config.js(3 hunks)
💤 Files with no reviewable changes (3)
- frontend/src/routes/joins/[slug]/services/jobTracker.service.ts
- frontend/src/lib/util/job-sample-data.ts
- frontend/src/lib/types/Job/Job.ts
🧰 Additional context used
🧠 Learnings (1)
frontend/src/routes/joins/[slug]/services/joins.service.ts (1)
Learnt from: ken-zlai
PR: zipline-ai/chronon#160
File: frontend/src/routes/joins/[slug]/services/joins.service.ts:34-34
Timestamp: 2025-01-17T00:33:14.792Z
Learning: The join timeseries API in `frontend/src/routes/joins/[slug]/services/joins.service.ts` specifically requires 'drift' as the metric type, regardless of the metricType parameter passed to the function.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: non_spark_tests
- GitHub Check: frontend_tests
- GitHub Check: scala_compile_fmt_fix
- GitHub Check: enforce_triggered_workflows
🔇 Additional comments (34)
frontend/src/lib/components/ui/table/table-head.svelte (1)
5-7: Clean implementation of element reference.Type-safe DOM reference implementation with proper undefined initialization.
Also applies to: 11-11, 15-15
frontend/src/lib/components/ui/table/index.ts (1)
10-10: Well-structured type definitions.Clean type exports with proper HTML attribute inheritance.
Also applies to: 31-33, 54-55
frontend/src/lib/job/tree-builder/tree-builder.ts (5)
7-13: Good structure for the interface.
31-59: Be mindful of potential timezone discrepancies.
84-141: Overlap logic is complex; add more tests if possible.
146-188: Recursive approach is clear and cohesive.
192-207: High-level tree construction is concise.frontend/src/routes/joins/[slug]/job-tracking/+page.ts (1)
4-13: Load function is straightforward; no issues noted.frontend/src/routes/joins/[slug]/+layout.ts (1)
13-14: Simplified concurrency logic; looks good.frontend/src/lib/components/ExpandableCell.svelte (1)
24-29: LGTM! Improved accessibility with button element.The change from div to button enhances keyboard navigation and screen reader support.
frontend/src/routes/joins/[slug]/services/joins.service.ts (2)
36-44: LGTM! Improved performance with concurrent API calls.Using Promise.all for parallel requests optimizes data fetching.
75-104:✅ Verification successful
Verify error handling for drift data.
While the try-catch block handles missing drift data, ensure the UI gracefully handles this case.
🏁 Script executed:
#!/bin/bash # Search for UI error handling of missing drift data rg -A 5 "joinDrift\?" frontend/src/routes/joins/Length of output: 1871
UI Handling for Missing Drift Data Verified
- UI pages use optional chaining (e.g.,
joinDrift?.driftSeries ?? []) to gracefully handle missing data.- The try-catch in the service pairs well with the UI's defensive defaults.
- Note: The distributions page has a TODO regarding error states for errored columns—confirm if this is acceptable.
frontend/src/lib/job/tree-builder/tree-builder.test.ts (1)
135-261: LGTM! Thorough test coverage for organizeTasksByDate.Tests handle empty arrays, single tasks, and various overlap scenarios.
frontend/src/routes/joins/[slug]/job-tracking/+page.svelte (2)
32-34: LGTM!Efficient implementation for generating initial expanded IDs.
260-268: LGTM!Safe iteration over tasks with proper null checks.
frontend/src/lib/util/lineage-job-sample-data.ts (1)
1-561: LGTM!Comprehensive sample data covering various job states and relationships.
Consider adding a comment to indicate this is test data:
+// Sample data for testing purposes only. Do not use in production. import { LogicalType, Status,frontend/src/lib/components/StatusCell.svelte (6)
2-2: No issues
7-7: Type import OK
8-8: New component reference
14-14: Allowing undefined
24-24: Good usage
Ensure test coverage.
30-31: Simple conditionalfrontend/src/lib/components/StatusBar.svelte (3)
1-4: Module import
5-14: Props look good
16-19: Dynamic classesfrontend/src/lib/components/JobOverviewChart.svelte (5)
2-3: Imports OK
16-16: Div container
19-19: Iteration logic
20-24: Date range usage
26-28: Rendering statusfrontend/src/lib/job/status.ts (2)
3-12: LGTM!Complete mapping of all job statuses to human-readable strings.
22-37: LGTM!Robust implementation with proper null checks and selective stripe application.
frontend/tailwind.config.js (1)
18-24: LGTM!Well-structured configuration with comprehensive job status styling and animations.
Also applies to: 72-111, 206-217
frontend/src/app.css (1)
104-144: LGTM!Well-organized CSS with proper variable reuse and clear documentation.
Summary
Checklist
Summary by CodeRabbit
StatusBarcomponent for job status representation.