fix: Add new jobs ui in jobsv2 page#468
Conversation
WalkthroughThis pull request introduces new React components and a page for handling job deployment releases. The updates include a collapsible row component for displaying job details and a table component with search functionality. Additionally, a new page fetches deployment metadata and renders the job table. On the backend, a new API router procedure is added to query job details associated with deployment versions, and a new type alias improves type safety for release targets. Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/jobsv2/_components/DeploymentVersionJobsTable.tsx (1)
69-79: Re-evaluate the skeleton loader's length.Generating 30 rows in the skeleton might be excessive for smaller datasets and can negatively impact performance. Consider reducing the number of placeholder rows or making it dynamic based on expected page size.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/jobsv2/_components/CollapsibleRow.tsx (3)
67-83: Ensure expandedResources state updates remain performant.You are cloning and mutating the object in each toggle. This is acceptable, but for large sets of resources, consider optimizing or using a memoized callback to reinforce performance.
140-145: Guard against JSON parsing errors.The code implicitly assumes valid JSON for
linksMetadata. If invalid,JSON.parsemay throw. Consider adding a try/catch or a fallback if parsing fails.- const links = linksMetadata != null ? (JSON.parse(linksMetadata) as Record<string, string>) : null; + let links: Record<string, string> | null = null; + if (linksMetadata) { + try { + links = JSON.parse(linksMetadata); + } catch { + // fallback or log an error + } + }
135-377: Remove redundant fragments.According to static analysis, several
<></>wrappers contain only a single child component or element. Removing them can simplify your JSX structure.- <> <TableRow ... /> - </> + <TableRow ... />🧰 Tools
🪛 Biome (1.9.4)
[error] 135-377: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
[error] 263-371: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/jobsv2/page.tsx (3)
7-14: Consider using non-Promise params typeThe
paramsproperty is typed as a Promise, which is unusual for Next.js page components. Typically,paramsis a direct object containing route parameters, not a Promise. This makes the code more complex by requiringawait props.paramsin both functions.type PageProps = { - params: Promise<{ + params: { workspaceSlug: string; systemSlug: string; deploymentSlug: string; releaseId: string; - }>; + }; };
29-41: Consider implementing error handling for API requestsThe code checks for null values but doesn't handle potential exceptions from the API calls. Consider adding try/catch blocks to handle potential API failures gracefully.
export default async function ReleasePage(props: PageProps) { const params = await props.params; + try { const deploymentVersion = await api.deployment.version.byId(params.releaseId); const deployment = await api.deployment.bySlug(params); if (deploymentVersion == null || deployment == null) return notFound(); return ( <DeploymentVersionJobsTable deploymentVersion={deploymentVersion} deployment={deployment} /> ); + } catch (error) { + console.error("Error fetching deployment data:", error); + // Handle the error appropriately or return an error UI + throw error; // Let Next.js error boundary handle it + } }
16-41: Consider optimizing data fetchingBoth
generateMetadataandReleasePagefetch the same data independently. In Next.js App Router, you can take advantage of React's cache to avoid duplicate data fetching.Consider extracting the data fetching logic to a shared function:
// Create a shared data fetching function async function getPageData(params: { workspaceSlug: string; systemSlug: string; deploymentSlug: string; releaseId: string; }) { const deploymentVersion = await api.deployment.version.byId(params.releaseId); const deployment = await api.deployment.bySlug(params); if (deploymentVersion == null || deployment == null) { return { notFound: true }; } return { deploymentVersion, deployment, notFound: false }; } // Then use it in both functions export async function generateMetadata(props: PageProps): Promise<Metadata> { const params = await props.params; const { deploymentVersion, deployment, notFound: isNotFound } = await getPageData(params); if (isNotFound) return notFound(); return { title: `${deploymentVersion.tag} | ${deployment.name} | ${deployment.system.name} | ${deployment.system.workspace.name}`, }; } export default async function ReleasePage(props: PageProps) { const params = await props.params; const { deploymentVersion, deployment, notFound: isNotFound } = await getPageData(params); if (isNotFound) return notFound(); return ( <DeploymentVersionJobsTable deploymentVersion={deploymentVersion} deployment={deployment} /> ); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/jobsv2/_components/CollapsibleRow.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/jobsv2/_components/DeploymentVersionJobsTable.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/jobsv2/page.tsx(1 hunks)packages/api/src/router/deployment-version.ts(2 hunks)packages/db/src/schema/release.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{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...
**/*.{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/db/src/schema/release.tsapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/jobsv2/page.tsxapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/jobsv2/_components/DeploymentVersionJobsTable.tsxpackages/api/src/router/deployment-version.tsapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/jobsv2/_components/CollapsibleRow.tsx
🧬 Code Graph Analysis (5)
packages/db/src/schema/release.ts (1)
packages/rule-engine/src/manager/types.ts (1)
ReleaseTarget(1-7)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/jobsv2/page.tsx (2)
packages/db/src/schema/deployment-version.ts (1)
deploymentVersion(109-136)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/jobsv2/_components/DeploymentVersionJobsTable.tsx (1)
DeploymentVersionJobsTable(45-105)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/jobsv2/_components/DeploymentVersionJobsTable.tsx (4)
packages/db/src/schema/deployment-version.ts (1)
deploymentVersion(109-136)packages/ui/src/table.tsx (2)
Table(112-112)TableBody(114-114)packages/db/src/schema/environment.ts (1)
environment(57-82)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/jobsv2/_components/CollapsibleRow.tsx (1)
CollapsibleRow(58-381)
packages/api/src/router/deployment-version.ts (6)
packages/api/src/router/job.ts (1)
jobRouter(803-866)packages/api/src/trpc.ts (2)
createTRPCRouter(55-55)protectedProcedure(150-150)packages/db/src/schema/job.ts (1)
job(74-106)packages/db/src/schema/resource.ts (1)
resource(59-87)packages/db/src/schema/release.ts (1)
releaseTarget(19-41)packages/db/src/schema/environment.ts (1)
environment(57-82)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/jobsv2/_components/CollapsibleRow.tsx (5)
packages/db/src/schema/job.ts (2)
JobStatus(136-136)job(74-106)apps/webservice/src/app/[workspaceSlug]/(app)/_hooks/channel/useDeploymentVersionChannel.ts (1)
useDeploymentVersionChannel(13-61)packages/ui/src/table.tsx (2)
TableRow(117-117)TableCell(118-118)packages/ui/src/button.tsx (2)
Button(80-80)buttonVariants(80-80)apps/webservice/src/app/[workspaceSlug]/(app)/_components/job/JobDropdownMenu.tsx (1)
JobDropdownMenu(284-394)
🪛 Biome (1.9.4)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/jobsv2/_components/CollapsibleRow.tsx
[error] 135-377: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
[error] 263-371: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (5)
packages/db/src/schema/release.ts (1)
43-43: Ensure consistency with existingReleaseTargetdefinitions.This new type alias aligns nicely with the existing database table schema. However, if there are other
ReleaseTargetinterfaces (e.g., inpackages/rule-engine), consider consolidating or referencing this type to keep the codebase consistent.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/jobsv2/_components/DeploymentVersionJobsTable.tsx (1)
89-102:Details
❓ Verification inconclusive
Confirm polling interval and resource usage.
The
refetchIntervalis set to 5 seconds, which may be frequent depending on system load. Ensure this polling rate strikes a good balance between real-time responsiveness and potential server overhead.
🏁 Script executed:
#!/bin/bash # This script checks if the average CPU usage spikes due to polling every 5 seconds. # You can adjust or remove the script if you prefer a different approach. echo "Please consider validating CPU usage in the production or staging environment." echo "If CPU usage is high, consider increasing the interval to 10 or 15 seconds."Length of output: 327
Action: Validate Polling Interval and Resource Usage
The polling interval is currently set to 5 seconds. Please verify that this rate doesn’t lead to excessive CPU or resource load in your production or staging environments. If you observe any significant resource spikes, consider increasing the interval to 10 or 15 seconds to better balance responsiveness with server overhead.
- File:
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/jobsv2/_components/DeploymentVersionJobsTable.tsx- Affected Lines: 89-102
packages/api/src/router/deployment-version.ts (3)
295-307: Validate creation of placeholder jobs.When no jobs exist, a placeholder job is inserted with a pending status. Check that this doesn't cause unintended side effects in downstream logic that expects real job entities.
347-353: Verify correctness of statusCounts grouping.While the logic looks correct, ensure that no additional statuses are overlooked. Maintaining an explicit list of possible statuses can avert runtime surprises.
12-12:Details
✅ Verification successful
Confirm necessity of new import
ilike.Ensure the
ilikeoperator is indeed needed and used consistently throughout the query logic. Unused or experimental imports can clutter code.
🏁 Script executed:
#!/bin/bash # Check if `ilike` is used anywhere besides this line rg -A 2 "ilike"Length of output: 9898
Ilike Operator Import Confirmed
The
ilikeoperator is employed extensively across our codebase in various query logic (e.g., in resource, job, environment, and deployment-version modules). Its usage in the file under review is consistent with our broader implementation, so the import is both necessary and appropriate.
| const jobsQuery = api.deployment.version.job.list.useQuery( | ||
| { versionId: deploymentVersion.id, query: debouncedSearch }, | ||
| { refetchInterval: 5_000 }, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for the query.
Currently, the component displays a loading or empty state, but there's no fallback UI in case of an error (e.g., network error). Consider handling jobsQuery.isError to provide user feedback when something goes wrong.
+ if (jobsQuery.isError) {
+ return (
+ <div className="flex w-full items-center justify-center py-8">
+ <span className="text-sm text-red-500">
+ Error loading jobs: {jobsQuery.error.message}
+ </span>
+ </div>
+ );
+ }📝 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 jobsQuery = api.deployment.version.job.list.useQuery( | |
| { versionId: deploymentVersion.id, query: debouncedSearch }, | |
| { refetchInterval: 5_000 }, | |
| ); | |
| const jobsQuery = api.deployment.version.job.list.useQuery( | |
| { versionId: deploymentVersion.id, query: debouncedSearch }, | |
| { refetchInterval: 5_000 }, | |
| ); | |
| if (jobsQuery.isError) { | |
| return ( | |
| <div className="flex w-full items-center justify-center py-8"> | |
| <span className="text-sm text-red-500"> | |
| Error loading jobs: {jobsQuery.error.message} | |
| </span> | |
| </div> | |
| ); | |
| } |
...s/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/jobsv2/page.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/api/src/router/deployment-version.ts (2)
199-369: Well-structured implementation of the job listing procedure.The implementation provides a comprehensive way to fetch and organize job data associated with deployment versions. The query is properly constructed with filters, joins, and error handling. The data processing logic effectively transforms the raw database results into a structured response grouped by environment.
A few optimization opportunities:
- Consider extracting some of the complex data transformation logic (especially lines 272-321 and 323-367) into separate helper functions for better readability and maintainability.
- For larger datasets, consider implementing pagination to improve performance.
300-312: Consider improving the default job object.When no jobs are found for a target, a default job object is created with some undefined properties (createdAt and externalId). This could cause issues with sorting or UI rendering.
Consider providing default values for all properties to ensure consistent behavior:
const jobs = targetJobs.length > 0 ? targetJobs : [ { id: resource.id, metadata: {} as Record<string, string>, type: "", status: JobStatus.Pending, - externalId: undefined as string | undefined, - createdAt: undefined as Date | undefined, + externalId: null as string | null, + createdAt: new Date(), }, ];
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/jobsv2/page.tsx(1 hunks)packages/api/src/router/deployment-version.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/jobsv2/page.tsx
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{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...
**/*.{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/deployment-version.ts
🧬 Code Graph Analysis (1)
packages/api/src/router/deployment-version.ts (4)
packages/db/src/schema/job.ts (1)
job(74-106)packages/db/src/schema/resource.ts (1)
resource(59-87)packages/db/src/schema/release.ts (1)
releaseTarget(19-41)packages/db/src/schema/environment.ts (1)
environment(57-82)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (5)
packages/api/src/router/deployment-version.ts (5)
227-230: TRPC error handling added correctly.The appropriate TRPC error handling has been implemented, using a specific error code and message when a version is not found. This addresses the previous review comment.
215-221: Good implementation of search functionality.The search query implementation is well-designed, using case-insensitive pattern matching on both resource names and environment names. The conditional logic to handle empty queries is also appropriate.
329-346: Thoughtful sorting of job statuses.The implementation prioritizes showing failed jobs first, which is a good user experience choice for quickly identifying issues.
352-358: Clear status count aggregation.Grouping and counting jobs by status provides valuable summary information for the UI. This will help users quickly understand the overall state of jobs in each environment.
373-373: Well-integrated into the existing router structure.The new jobRouter is properly exported as part of the versionRouter, making it accessible through the established API pattern.
Summary by CodeRabbit