feat - Job Tracker #160
feat - Job Tracker #160
Conversation
# Conflicts: # frontend/package-lock.json # frontend/package.json
# Conflicts: # frontend/package-lock.json # frontend/package.json
# Conflicts: # frontend/src/app.css
# Conflicts: # frontend/src/lib/components/Observability/Observability.svelte
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (11)
frontend/src/lib/components/JobOverviewChart/JobOverviewChart.svelte (1)
5-15: Extract props interface.Move interface to a separate type definition.
+interface JobOverviewChartProps { + job: JobTreeNode; + dates: string[]; + getDaysInRange: (start: string, end: string, availableDates: string[]) => number; + formatDate: (dateString: string) => string; +} + let { job, dates, getDaysInRange, formatDate -}: { - job: JobTreeNode; - dates: string[]; - getDaysInRange: (start: string, end: string, availableDates: string[]) => number; - formatDate: (dateString: string) => string; -} = $props(); +}: JobOverviewChartProps = $props();frontend/src/routes/joins/[slug]/services/jobTracker.service.ts (1)
29-30: Extract sample data parameters to constants.Move magic numbers to named constants.
+const SAMPLE_JOB_COUNT = 10; +const SAMPLE_DATE_RANGE = 21; -const jobRunListSampleData = generateJobRunListSampleData(10, 21); +const jobRunListSampleData = generateJobRunListSampleData(SAMPLE_JOB_COUNT, SAMPLE_DATE_RANGE);frontend/src/lib/components/ExpandableCell/ExpandableCell.svelte (2)
29-29: Useon:clickinstead ofonclick.Svelte's event directive is preferred over DOM event attributes.
-<button class="mr-1 ml-1" onclick={() => ($isExpanded = !$isExpanded)}> +<button class="mr-1 ml-1" on:click={() => ($isExpanded = !$isExpanded)}>
29-35: Add aria-label to expand/collapse button.Improve accessibility for screen readers.
<button class="mr-1 ml-1" on:click={() => ($isExpanded = !$isExpanded)} + aria-label={`${$isExpanded ? 'Collapse' : 'Expand'} ${name}`} >frontend/src/lib/components/StatusCell/StatusCell.svelte (1)
37-38: Format dates for better readability.Consider using a date formatting utility.
-<p><span class="font-medium">Start:</span> {startDate}</p> -<p><span class="font-medium">End:</span> {endDate}</p> +<p><span class="font-medium">Start:</span> {formatDate(startDate)}</p> +<p><span class="font-medium">End:</span> {formatDate(endDate)}</p>frontend/src/lib/types/Job/Job.ts (1)
6-10: Add validation for ISO date strings.Consider adding a type guard for date validation.
+export function isValidISODate(date: string): boolean { + const d = new Date(date); + return d instanceof Date && !isNaN(d.getTime()) && d.toISOString() === date; +} export type JobRun = { start: string; // ISO date string end: string; // ISO date string status: JobStatus; };frontend/src/routes/joins/[slug]/services/joins.service.ts (2)
6-7: Move fallback dates to configuration.Hardcoded dates should be configurable.
-const FALLBACK_START_TS = 1672531200000; // 2023-01-01 -const FALLBACK_END_TS = 1677628800000; // 2023-03-01 +import { config } from '$lib/config'; +const { FALLBACK_START_TS, FALLBACK_END_TS } = config.joins;
79-79: Improve error logging.Add error details and context for better debugging.
- console.error('Error fetching data:', error); + console.error(`Error fetching join data for ${joinName}:`, error, { + startTs: requestedDateRange.startTimestamp, + endTs: requestedDateRange.endTimestamp + });frontend/src/lib/util/job-sample-data.ts (2)
9-42: Consider parameterizing the start date and improving duration calculation.The function uses a hardcoded date and simple random duration calculation.
-let currentDate = new Date('2024-01-01'); +let currentDate = new Date(startDate || '2024-01-01'); -const duration = Math.min(Math.floor(Math.random() * 6) + 2, remainingDays); +const minDuration = 2; +const maxDuration = 7; +const duration = Math.min(Math.floor(Math.random() * (maxDuration - minDuration)) + minDuration, remainingDays);
86-656: Consider moving example data to separate files.The example data is comprehensive but makes the file very long. Consider splitting it into separate JSON files.
frontend/src/app.css (1)
104-105: Consider consolidating waiting and queued states.Both states use identical background colors and similar text colors. Consider using a single set of variables for both states to maintain consistency.
Also applies to: 112-113
📜 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 (17)
frontend/package.json(2 hunks)frontend/src/app.css(1 hunks)frontend/src/lib/components/CellDivider/CellDivider.svelte(1 hunks)frontend/src/lib/components/ExpandableCell/ExpandableCell.svelte(1 hunks)frontend/src/lib/components/JobOverviewChart/JobOverviewChart.svelte(1 hunks)frontend/src/lib/components/JobTracker/JobTracker.svelte(1 hunks)frontend/src/lib/components/LogsDownloadButton/LogsDownloadButton.svelte(1 hunks)frontend/src/lib/components/Observability/Observability.svelte(1 hunks)frontend/src/lib/components/StatusCell/StatusCell.svelte(1 hunks)frontend/src/lib/components/ui/table/table.svelte(1 hunks)frontend/src/lib/types/Job/Job.ts(1 hunks)frontend/src/lib/util/job-sample-data.ts(1 hunks)frontend/src/routes/joins/[slug]/+page.server.ts(1 hunks)frontend/src/routes/joins/[slug]/+page.svelte(2 hunks)frontend/src/routes/joins/[slug]/services/jobTracker.service.ts(1 hunks)frontend/src/routes/joins/[slug]/services/joins.service.ts(1 hunks)frontend/tailwind.config.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/src/lib/components/CellDivider/CellDivider.svelte
🔇 Additional comments (14)
frontend/src/lib/components/JobTracker/JobTracker.svelte (2)
149-150: Incorrect use of$statein variable declarationsAs previously mentioned,
$stateis not standard in Svelte.
80-85:⚠️ Potential issueIncorrect use of
$statein variable declarations
$stateis not standard in Svelte. Declare reactive variables directly.Apply this diff:
- let scrollPercentage = $state(0); - let scrollbarWidth = $state(0); - let scrollableDiv = $state<HTMLDivElement | undefined>(undefined); - let isDragging = $state(false); - let startX = 0; - let scrollLeft = 0; + let scrollPercentage = 0; + let scrollbarWidth = 0; + let scrollableDiv: HTMLDivElement | undefined = undefined; + let isDragging = false; + let startX = 0; + let scrollLeft = 0;Likely invalid or redundant comment.
frontend/src/lib/components/ui/table/table.svelte (2)
8-9: Props look good!Clean type definitions and default values.
Also applies to: 14-15
19-23: Binding and class handling implemented well.frontend/src/lib/components/JobOverviewChart/JobOverviewChart.svelte (1)
21-29: 🛠️ Refactor suggestionAdd error handling for dates and overlapping runs.
Validate date ranges and handle overlapping job runs.
frontend/src/routes/joins/[slug]/+page.svelte (1)
5-6: Clean integration of JobTracker!Good use of query parameters for tab state.
Also applies to: 11-11, 16-16, 28-28
frontend/src/routes/joins/[slug]/services/jobTracker.service.ts (1)
7-17: 🛠️ Refactor suggestionAdd type safety for adjacencyList keys.
Ensure keys in adjacencyList exist in jobRunList.
const addRunsToTree = - (jobRunList: JobRunList, adjacencyList: Record<string, string[]>) => + (jobRunList: JobRunList, adjacencyList: Record<keyof JobRunList, Array<keyof JobRunList>>) => (row: string): JobTreeNode => {frontend/src/lib/components/StatusCell/StatusCell.svelte (1)
11-19: Add date validation.Validate ISO date strings to prevent display issues.
frontend/src/lib/types/Job/Job.ts (1)
25-33: Address TODO comment about colors.Update colors to match design or remove TODO comment if colors are final.
✅ Verification successful
Remove outdated TODO comment about colors
The colors are properly defined in the Tailwind config using CSS variables, indicating they're already integrated with the design system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for color definitions in CSS rg "jobs-.*-bg|jobs-.*-text" --type cssLength of output: 41
Script:
#!/bin/bash # Find Tailwind config files fd -e js -e ts -e cjs -e mjs tailwind.config # Search for job status color definitions in all files rg "jobs-(waiting|running|failed|completed|queued|invalid)" # Look for design system files fd -e ts -e js design-system fd -e ts -e js themeLength of output: 1477
Script:
#!/bin/bash # Check Tailwind config content cat frontend/tailwind.config.jsLength of output: 4525
frontend/tailwind.config.js (1)
55-79: LGTM! Well-structured color configuration for job states.The color configuration follows Tailwind's best practices and maintains consistency with existing color definitions.
frontend/src/lib/util/job-sample-data.ts (1)
44-84: LGTM! Well-structured graph generation logic.The function effectively handles root nodes and parent-child relationships.
frontend/src/lib/components/Observability/Observability.svelte (1)
37-37: LGTM! Clean type update.The type change from JoinPageData to JoinData is properly implemented.
Also applies to: 41-41
frontend/package.json (1)
49-49: LGTM! Proper dependency configuration.The svelte-headless-table dependency is properly added with version constraints and Svelte 5 override.
Also applies to: 79-81
frontend/src/app.css (1)
104-115: Verify color contrast ratios for accessibility.Please ensure all background/text color pairs meet WCAG 2.1 Level AA contrast requirements (4.5:1 for normal text).
frontend/src/lib/components/LogsDownloadButton/LogsDownloadButton.svelte
Show resolved
Hide resolved
There was a problem hiding this comment.
Is the plan for this component to initiate the fetch/download request?
There was a problem hiding this comment.
LGTM.
Btw, to add the stripe styling, you could leverage background with a stack of gradients/colors:
background:
repeating-linear-gradient(135deg, hsl(0 0 0 / 30%) 0 10px, hsl(0 0 0 / 0%) 0 20px)
hsl(210 100 50)See this REPL to see it in action, or a more complicated version applied to a <progress> with dynamic color based on value
Summary
I figured we could go ahead and get this merged. It's not an exact match to the figma, but Eugene will be providing some updates tomorrow. Generated sample data is used right now.
I'll create a separate PR which matches this to the figma, add icons, uses correct colors, etc.
Checklist
Summary by CodeRabbit
Release Notes
New Features
UI Improvements
Performance
Types and Styling