fix: Deployment job graph popover #266
Conversation
WalkthroughThis pull request introduces a new Changes
Sequence DiagramsequenceDiagram
participant User
participant DeploymentPage
participant JobHistoryPopover
participant API
participant DailyJobsChart
User->>DeploymentPage: View Deployment
DeploymentPage->>JobHistoryPopover: Render Popover Trigger
User->>JobHistoryPopover: Click Popover
JobHistoryPopover->>API: Fetch Daily Job Counts
API-->>JobHistoryPopover: Return Job Counts
JobHistoryPopover->>DailyJobsChart: Render Chart
DailyJobsChart->>User: Display Interactive Job Statistics
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
Documentation and Community
|
![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 (5)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/DailyJobsChart.tsx (3)
110-115: Use user's locale instead of hardcoded 'en-US' for X-axis labelsConsider using the user's locale when formatting dates on the X-axis to enhance internationalization support.
Apply this diff:
- return date.toLocaleDateString("en-US", { + return date.toLocaleDateString(undefined, {
147-151: Use user's locale instead of hardcoded 'en-US' in tooltip date formattingTo support users from various locales, use the default locale when formatting dates in the tooltip.
Apply this diff:
- {new Date(label).toLocaleDateString("en-US", { + {new Date(label).toLocaleDateString(undefined, {
203-264: RefactoronClickhandler for better readabilityThe
onClickhandler in theBarcomponent is lengthy and could be extracted into a separate function to improve readability and maintainability.Apply this refactor:
+ const handleBarClick = (e: any) => { + const start = new Date(e.date); + const end = addDays(start, 1); + + // ...existing code... + + router.push(filterLink); + }; ... <Bar key={status} dataKey={status.toLowerCase()} stackId="jobs" className="cursor-pointer" yAxisId="right" fill={color} - onClick={(e) => { - // existing onClick code - }} + onClick={handleBarClick} />apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/JobHistoryPopover.tsx (1)
37-37: Consider makingPopoverContentwidth responsiveRather than hardcoding the width, consider making the
PopoverContentwidth responsive to enhance usability on various screen sizes.Apply this diff:
- <PopoverContent className="w-[700px]"> + <PopoverContent className="w-full max-w-[700px]">packages/api/src/router/job.ts (1)
290-346: Consider extracting shared SQL query logic.Both
dailyCountprocedures inbyWorkspaceIdandbyDeploymentIdshare similar SQL query structure. Consider extracting the common logic into a reusable function to improve maintainability.Example refactor:
+ const createDailyCountQuery = ( + db: Tx, + dateTruncExpr: SQL, + whereClause: SQL + ) => { + const subquery = db + .select({ + date: dateTruncExpr.as("date"), + status: schema.job.status, + countPerStatus: sql<number>`COUNT(*)`.as("countPerStatus"), + }) + .from(schema.releaseJobTrigger) + .innerJoin( + schema.job, + eq(schema.releaseJobTrigger.jobId, schema.job.id), + ) + .where( + and( + whereClause, + notInArray(schema.job.status, [ + JobStatus.Pending, + JobStatus.Cancelled, + JobStatus.Skipped, + ]), + ), + ) + .groupBy(dateTruncExpr, schema.job.status) + .as("sub"); + + return db + .select({ + date: subquery.date, + totalCount: sql<number>`SUM(${subquery.countPerStatus})`.as( + "totalCount", + ), + statusCounts: sql<Record<JobStatus, number>>` + jsonb_object_agg(${subquery.status}, ${subquery.countPerStatus}) + `.as("statusCounts"), + }) + .from(subquery) + .groupBy(subquery.date) + .orderBy(subquery.date); + }; byWorkspaceId: createTRPCRouter({ dailyCount: protectedProcedure .input(z.object({ workspaceId: z.string().uuid(), timezone: z.string() })) .meta({ authorizationCheck: ({ canUser, input }) => canUser .perform(Permission.SystemList) .on({ type: "workspace", id: input.workspaceId }), }) .query(async ({ ctx, input }) => { const dateTruncExpr = sql<Date>`date_trunc('day', ${schema.releaseJobTrigger.createdAt} AT TIME ZONE 'UTC' AT TIME ZONE '${sql.raw(input.timezone)}')`; - // ... existing query code ... + return createDailyCountQuery( + ctx.db, + dateTruncExpr, + eq(schema.system.workspaceId, input.workspaceId) + ); }), }), byDeploymentId: createTRPCRouter({ dailyCount: protectedProcedure .input(z.object({ deploymentId: z.string().uuid(), timezone: z.string() })) .meta({ authorizationCheck: ({ canUser, input }) => canUser .perform(Permission.JobList) .on({ type: "deployment", id: input.deploymentId }), }) .query(async ({ ctx, input }) => { const dateTruncExpr = sql<Date>`date_trunc('day', ${schema.releaseJobTrigger.createdAt} AT TIME ZONE 'UTC' AT TIME ZONE '${sql.raw(input.timezone)}')`; - // ... existing query code ... + return createDailyCountQuery( + ctx.db, + dateTruncExpr, + eq(schema.release.deploymentId, input.deploymentId) + ); }), }),Also applies to: 232-289
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/DailyJobsChart.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/JobHistoryChart.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/DeploymentPageContent.tsx(4 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/JobHistoryPopover.tsx(1 hunks)packages/api/src/router/job.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/JobHistoryPopover.tsx (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/systems/JobHistoryChart.tsx (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/_components/DailyJobsChart.tsx (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/api/src/router/job.ts (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/DeploymentPageContent.tsx (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
🔇 Additional comments (8)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/JobHistoryChart.tsx (2)
13-13: Importing DailyJobsChart component
The DailyJobsChart component is correctly imported, promoting reusability and simplifying the codebase.
69-69: Rendering DailyJobsChart with dailyCounts data
DailyJobsChart is rendered with the appropriate dailyCounts prop, ensuring accurate visualization of job executions.
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/JobHistoryPopover.tsx (1)
1-51: Implementation of JobHistoryPopover component is correct
The JobHistoryPopover component is well-structured and integrates the DailyJobsChart appropriately to display job execution data.
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/DeploymentPageContent.tsx (3)
6-11: Importing additional icons from @tabler/icons-react
The import statements for the additional icons, including IconHistory, are correct.
39-39: Importing JobHistoryPopover component
The JobHistoryPopover component is correctly imported, enabling its use within this file.
144-165: Adding JobHistoryPopover for job execution data
The JobHistoryPopover is properly integrated, providing users with access to job execution data related to the deployment.
packages/api/src/router/job.ts (2)
232-232: LGTM! Clean input declaration.
The consolidated input declaration maintains type safety while improving code readability.
290-346: LGTM! Well-scoped implementation with proper authorization.
The new router correctly implements deployment-level job counting with:
- Proper authorization checks
- Optimized SQL query using only necessary table joins
- Consistent status filtering
Summary by CodeRabbit
Release Notes
New Features
DailyJobsChartcomponent for visualizing job execution data.JobHistoryPopovercomponent to display job execution data in a popover format.DeploymentPageContentwith a newJobHistoryPopoverbutton for accessing job history.Bug Fixes
dailyCountprocedure underbyWorkspaceIdfor improved functionality.Documentation