fix: Job drawer #162
fix: Job drawer #162
Conversation
WalkthroughThis pull request introduces several enhancements across various components in the web service application. Key changes include the addition of new components such as Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
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: 16
🧹 Outside diff range and nitpick comments (11)
apps/webservice/src/app/[workspaceSlug]/_components/job-drawer/Job.ts (1)
4-15: Add JSDoc documentation for better maintainability.Consider adding JSDoc comments to document the purpose of the
Jobtype and its relationship withReleaseJobTrigger.+/** + * Represents a job with extended properties for UI display. + * Extends ReleaseJobTrigger with additional metadata, status, and relationship fields. + */ export type Job = SCHEMA.ReleaseJobTrigger & {apps/webservice/src/app/[workspaceSlug]/_components/job-drawer/useJobDrawer.tsx (2)
1-3: Consider using a more descriptive constant name.The constant name
paramis too generic. Consider renaming it to something more specific likeJOB_ID_PARAMorJOB_DRAWER_PARAMto better indicate its purpose.-const param = "job_id"; +const JOB_ID_PARAM = "job_id";
21-24: Add TypeScript interface for hook return value.Consider adding a TypeScript interface to better document the hook's return value and improve type safety.
+interface JobDrawerHook { + jobId: string | null; + setJobId: (id: JobId | null) => void; + removeJobId: () => void; +} + -export const useJobDrawer = () => { +export const useJobDrawer = (): JobDrawerHook => {apps/webservice/src/app/[workspaceSlug]/_components/job-drawer/JobAgent.tsx (2)
13-29: Enhance table accessibility.The table lacks proper semantic markup and accessibility attributes.
Apply these improvements:
- <table width="100%" className="table-fixed text-xs"> + <table + width="100%" + className="table-fixed text-xs" + role="grid" + aria-label="Job Agent Details" + > <tbody> <tr> - <td className="w-[110px] p-1 pr-2 text-muted-foreground">Name</td> + <td className="w-[110px] p-1 pr-2 text-muted-foreground" scope="row">Name</td> <td>{job.jobAgent?.name ?? 'N/A'}</td> </tr> <tr> - <td className="w-[110px] p-1 pr-2 text-muted-foreground">Type</td> + <td className="w-[110px] p-1 pr-2 text-muted-foreground" scope="row">Type</td> <td>{job.jobAgent?.type ?? 'N/A'}</td> </tr>
6-8: Consider making the job prop more specific.The
Jobtype might be too broad for this component's needs. Consider creating a more specific type that only includes the required properties.Example improvement:
type JobAgentProps = { job: Pick<Job, 'jobAgent' | 'job'>; };apps/webservice/src/app/[workspaceSlug]/_components/job-drawer/JobMetadata.tsx (1)
25-28: Improve search implementation readability.The keys ["0", "1"] in the search configuration are not descriptive. Consider using named constants or a type-safe approach.
const { search, setSearch, result } = useMatchSorterWithSearch( sortedMetadata, - { keys: ["0", "1"] }, + { keys: ["key", "value"] }, );apps/webservice/src/app/[workspaceSlug]/_components/job-drawer/JobDrawer.tsx (3)
24-24: Use strict equality comparison.Replace loose equality (
!=) with strict equality (!==) for type-safe comparisons.- const isOpen = jobId != null && jobId != ""; + const isOpen = jobId !== null && jobId !== "";
61-63: Add aria-label to improve accessibility.The button with the vertical dots icon needs an aria-label for better accessibility.
-<Button variant="ghost" size="icon" className="h-6 w-6"> +<Button variant="ghost" size="icon" className="h-6 w-6" aria-label="More options"> <IconDotsVertical className="h-3 w-3" /> </Button>
69-86: Remove unnecessary Fragment wrapper.The Fragment (
<>...</>) wrapper around the map is not needed since it's the only child element.-<> {Object.entries(links).map(([label, url]) => ( <Link key={label} href={url} target="_blank" rel="noopener noreferrer" className={buttonVariants({ variant: "secondary", size: "sm", className: "gap-1", })} > <IconExternalLink className="h-4 w-4" /> {label} </Link> ))} -</>apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/JobDropdownMenu.tsx (1)
220-223: Consider using Promise chaining alternatives.The chain of .then() calls could be simplified using async/await for better readability.
-onClick={() => - forceRelease - .mutateAsync({ - releaseId: release.id, - targetId: target.id, - environmentId: environmentId, - isForcedRelease: true, - }) - .then((rjt) => setJobId(rjt.jobId)) - .then(() => router.refresh()) - .then(() => setOpen(false)) - .then(() => onClose()) +onClick={async () => { + const rjt = await forceRelease.mutateAsync({ + releaseId: release.id, + targetId: target.id, + environmentId: environmentId, + isForcedRelease: true, + }); + setJobId(rjt.jobId); + router.refresh(); + setOpen(false); + onClose(); +}}packages/api/src/router/release.ts (1)
Line range hint
332-359: Add validation for release dependencies.While the transaction ensures atomicity, there's no validation that the referenced dependencies exist before insertion. This could lead to foreign key constraint violations or orphaned references.
Consider adding validation before the dependency insertion:
if (input.releaseDependencies.length > 0) { + const dependencyIds = input.releaseDependencies.map(rd => rd.dependencyId); + const existingDependencies = await db + .select({ id: release.id }) + .from(release) + .where(inArray(release.id, dependencyIds)); + + if (existingDependencies.length !== dependencyIds.length) { + throw new Error("Some release dependencies do not exist"); + } + await db.insert(releaseDependency).values( input.releaseDependencies.map((rd) => ({ ...rd, releaseId: rel.id, })), ); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (18)
- apps/webservice/src/app/[workspaceSlug]/_components/ConfigEditor.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/JobTableStatusIcon.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/job-drawer/Job.ts (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/job-drawer/JobAgent.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/job-drawer/JobDrawer.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/job-drawer/JobMetadata.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/job-drawer/JobProperties.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/job-drawer/useJobDrawer.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/layout.tsx (2 hunks)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/JobDropdownMenu.tsx (10 hunks)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/TargetReleaseTable.tsx (4 hunks)
- packages/api/src/router/job.ts (4 hunks)
- packages/api/src/router/release.ts (1 hunks)
- packages/db/drizzle/0020_overjoyed_zarek.sql (1 hunks)
- packages/db/drizzle/meta/_journal.json (1 hunks)
- packages/db/src/schema/job.ts (1 hunks)
- packages/validators/src/auth/index.ts (1 hunks)
- packages/validators/src/jobs/index.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (35)
packages/db/drizzle/0020_overjoyed_zarek.sql (2)
1-1: LGTM! Good choice using timezone-aware timestamps.Using
timestamp with time zoneis appropriate for applications that need to handle dates across different time zones. This ensures consistent timestamp handling regardless of the server's timezone.Also applies to: 3-3
2-2: Verify existing data before applying NOT NULL constraints.Adding NOT NULL constraints could fail if there are existing rows with NULL values in these columns.
Let's verify the existing data:
Additionally, consider:
- Having a rollback plan in case the migration fails
- Updating application code to always set these timestamps
- Running this migration during low-traffic periods
Also applies to: 4-4
✅ Verification successful
Let me try to gather more context about the database schema and migrations to better understand the situation.
Let me try a different approach to examine the migration files.
Looking at the migration files, I can see that the
jobtable was created with a default value forcreated_atusingDEFAULT now(), which means there shouldn't be any NULL values in this column. The current migration is just making this constraint explicit by addingNOT NULL.Let me verify this by checking the table creation SQL:
The NOT NULL constraint can be safely added
Looking at the table creation SQL, the
created_atcolumn was defined withDEFAULT now(), which means every row created would automatically get a timestamp value. Since this has been true since the table's creation (migration 0000), there cannot be any NULL values in thecreated_atcolumn. The current migration is simply making this implicit guarantee explicit by adding the NOT NULL constraint.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for NULL values in created_at and updated_at columns # Note: This script assumes psql is configured. Adjust connection details as needed. echo "Checking for NULL values in job table..." cat << 'EOF' > check_nulls.sql SELECT COUNT(*) as null_count FROM job WHERE created_at IS NULL OR updated_at IS NULL; EOF # Using fd to locate the SQL file and execute it fd -t f 'check_nulls.sql' -x echo "Found potential NULL values in job table. Please ensure all rows have valid timestamps before applying the migration."Length of output: 386
Script:
#!/bin/bash # Let's examine the migration files to understand the table creation and any previous modifications fd -e sql . drizzle/ -x cat {}Length of output: 130
Script:
#!/bin/bash # Search for SQL files in the repository fd -e sql . packages/db -x cat {}Length of output: 49073
Script:
#!/bin/bash # Search for the job table creation statement rg -A 10 'CREATE TABLE.*"job"' packages/dbLength of output: 921
apps/webservice/src/app/[workspaceSlug]/_components/job-drawer/Job.ts (2)
1-2: LGTM!The imports are correctly typed and necessary for the type definition.
5-8: Verify the status field type change implications.The original status field from
SCHEMA.Jobis being replaced with a different type (JobStatus). Let's verify that this change is compatible with the validator.✅ Verification successful
Let me try to locate these files using a different approach.
Let me check the specific schema and type definitions we found.
Status field type change is safe and consistent
The verification shows that the type change is fully compatible:
- The schema defines
jobStatusas a Postgres enum with all possible status values- The
JobStatusenum in@ctrlplane/validators/jobsexactly matches these values- The type change in the file under review correctly imports and uses the
JobStatustype from the validators packageThis is a well-structured type change that maintains type safety and consistency across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the compatibility between SCHEMA.Job status and JobStatus # Check the original status field type in schema echo "Checking schema status definition:" rg -A 5 "status.*Job.*=" "@ctrlplane/db/schema" # Check the JobStatus type definition echo -e "\nChecking JobStatus validator definition:" rg -A 5 "JobStatus.*=" "@ctrlplane/validators/jobs"Length of output: 403
Script:
#!/bin/bash # First, let's find the schema and validator files echo "Looking for schema files:" fd schema echo -e "\nLooking for job-related files:" fd -t f job -i echo -e "\nLooking for status type definitions:" rg -i "type.*status.*=" rg -i "jobstatus.*="Length of output: 5685
Script:
#!/bin/bash # Check the job status enum definition in schema echo "Checking job status enum in schema:" cat packages/db/src/schema/job.ts echo -e "\nChecking JobStatus usage in the file under review:" cat "apps/webservice/src/app/[workspaceSlug]/_components/job-drawer/Job.ts" echo -e "\nChecking JobStatusReadable definition:" cat "packages/validators/src/jobs/index.ts"Length of output: 4776
apps/webservice/src/app/[workspaceSlug]/_components/job-drawer/useJobDrawer.tsx (1)
5-9: LGTM! Clean hook implementation.The hook follows React best practices and properly manages URL parameter state using Next.js navigation hooks.
apps/webservice/src/app/[workspaceSlug]/_components/ConfigEditor.tsx (1)
35-35: LGTM! Good UI improvement.Disabling the minimap is a sensible choice for this configuration editor, given its compact size (200px height) and focused purpose. This change reduces visual clutter without compromising functionality.
apps/webservice/src/app/[workspaceSlug]/_components/JobTableStatusIcon.tsx (2)
9-9: LGTM! Clean import addition.The
cnutility import is well-placed and properly utilized throughout the component for class name management.
12-15: LGTM! Well-structured component interface.The addition of the optional
classNameprop follows React best practices for component customization.apps/webservice/src/app/[workspaceSlug]/layout.tsx (2)
8-8: LGTM!The import statement follows the established pattern for drawer components.
43-43: LGTM!The JobDrawer component is correctly integrated alongside other drawer components in the workspace layout.
apps/webservice/src/app/[workspaceSlug]/_components/job-drawer/JobMetadata.tsx (2)
1-11: LGTM! Clean imports and type definitions.The imports are well-organized and the type definition is properly typed with React.FC.
6-6: Verify Job type implementation.Let's ensure the Job type is properly defined and includes all necessary metadata fields.
✅ Verification successful
Let me try a different approach to locate and verify the Job type definition.
Job type is properly defined and correctly imported
The Job type is well-defined in
Job.tsin the same directory and includes all necessary metadata fields. The type definition shows:
- It extends
SCHEMA.ReleaseJobTrigger- Contains a
jobfield with metadata array (metadata: SCHEMA.JobMetadata[])- Includes all required fields like jobAgent, target, release, environment, etc.
The type is being correctly used in JobMetadata.tsx to access and display the metadata field (
const { metadata } = job.job).🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Job type definition and its usage ast-grep --pattern 'type Job = { $$$ job: { $$$ metadata: $$$ $$$ } $$$ }'Length of output: 96
Script:
#!/bin/bash # First, let's find the Job.ts/tsx file in the same directory fd "Job.ts" "apps/webservice/src" # Then let's try to find any type definitions named Job ast-grep --pattern 'type Job = $_' # As a backup, let's also search for interface definitions ast-grep --pattern 'interface Job $_' # Let's also check the actual file content where JobMetadata is using this type cat "apps/webservice/src/app/[workspaceSlug]/_components/job-drawer/JobMetadata.tsx"Length of output: 3848
packages/db/src/schema/job.ts (1)
52-56: Timestamp improvements look good, verify timezone handling in queries.The addition of timezone support and explicit non-null constraints improves data integrity and timezone handling. These changes align well with best practices for timestamp management in PostgreSQL.
Let's verify the impact on existing queries and related components:
Consider reviewing the results to ensure all timestamp operations properly handle the timezone information.
packages/db/drizzle/meta/_journal.json (2)
144-150: LGTM! Journal entry follows consistent structure.The new migration entry maintains proper sequential ordering and follows the established format.
144-150: Verify the corresponding migration file.Let's verify that the migration file exists and contains the expected timestamp column updates.
✅ Verification successful
Based on the shell script results, I can generate the final response as we have clear evidence that:
- The migration file exists at the expected location
- The migration file contains timestamp column updates for
created_atandupdated_atcolumns in thejobtableMigration file verified successfully
The migration file
0020_overjoyed_zarek.sqlexists and contains the expected timestamp column updates, changing bothcreated_atandupdated_atcolumns in thejobtable to usetimestamp with time zonedata type.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify migration file exists and contains timestamp updates # Test 1: Check if migration file exists fd "0020_overjoyed_zarek.sql" --type f # Test 2: Verify migration content includes timestamp updates rg -i "timestamp with time zone" "packages/db/drizzle/0020_overjoyed_zarek.sql"Length of output: 379
packages/validators/src/auth/index.ts (2)
27-27: LGTM! Permission addition follows established patterns.The new
JobGetpermission is correctly placed and follows the consistent naming convention.
27-27: Verify if additional job-related permissions are needed.While
JobGetandJobUpdatepermissions exist, consider ifJobListorJobDeletepermissions might be needed for completeness of the job management functionality.apps/webservice/src/app/[workspaceSlug]/_components/job-drawer/JobDrawer.tsx (1)
1-20: LGTM! Well-organized imports.The imports are logically grouped and all dependencies are properly utilized in the component.
apps/webservice/src/app/[workspaceSlug]/_components/job-drawer/JobProperties.tsx (1)
1-13: LGTM! Clean imports and type definitions.The imports are well-organized and the props interface is properly typed.
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/TargetReleaseTable.tsx (4)
6-6: LGTM! Import organization is clean and well-structured.The new imports are properly organized and all are being utilized within the component.
Also applies to: 11-11, 15-15, 18-18
30-30: Consider handling event propagation for nested clickable elements.While making the entire row clickable improves UX, it could lead to unintended behavior when clicking on the link or dropdown menu within the row.
Consider preventing event propagation in nested clickable elements:
-<Link +<Link + onClick={(e) => e.stopPropagation()} href={`${pathname}?target_id=${job.target.id}`} className="block w-full hover:text-blue-300" >Also applies to: 76-79
131-144: LGTM! Clean implementation of the job dropdown menu.The dropdown menu implementation is well-structured and consistent with the job-centric approach. The button UI is clean and follows the design system.
83-86: Verify that job.target is guaranteed to exist.The code assumes
job.targetalways exists. Please ensure this assumption is safe, particularly for edge cases.apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/JobDropdownMenu.tsx (6)
63-63: LGTM: useJobDrawer integration enhances job tracking.The integration of useJobDrawer hook across components provides consistent job tracking functionality.
Also applies to: 186-186, 243-243
81-81: LGTM: Form initialization improved.Using the current job status as the default form value improves UX by showing the current state.
90-91: LGTM: Comprehensive cache invalidation.The cache invalidation strategy ensures both release-level and job-level data consistency after updates.
128-132: LGTM: Dynamic status options rendering.The implementation correctly maps all possible job statuses to select options.
156-156: LGTM: Proper loading state handling.The button is correctly disabled during the mutation to prevent duplicate submissions.
283-290: LGTM: Clean component interface update.The component props have been updated to use children prop for better composition, following React best practices.
Also applies to: 295-295
packages/api/src/router/release.ts (1)
307-308:⚠️ Potential issueAdd null check and consider consistency with
toEnvironment.The non-null assertion on
releaseJobTriggers[0]!could throw if no job triggers are created due to policy filters. Additionally, this return value is inconsistent with thetoEnvironmentprocedure which returns the entire array.Consider this safer implementation:
- return releaseJobTriggers[0]!; + if (releaseJobTriggers.length === 0) { + throw new Error("No job triggers were created due to policy restrictions"); + } + return releaseJobTriggers[0];Let's verify if
toEnvironmentis being used differently thantoTarget:packages/api/src/router/job.ts (5)
2-14: LGTM!The added type imports enhance type safety and clarity in the code.
43-43: LGTM!Including
userin the imports aligns with its usage in the code.
62-65: Ensure that changing toinnerJoindoes not exclude necessary dataSwitching from
leftJointoinnerJoinwill exclude records where the joined tables have no matching entries. Please verify that this change is intentional and that it won't lead to missing or incomplete data in the results.
68-112: LGTM!The
processReleaseJobTriggerWithAdditionalDataRowsfunction effectively processes and structures the data rows, improving code readability and maintainability.
281-304: LGTM!The new
byIdendpoint correctly retrieves job details by ID with the necessary joins and authorization checks. This addition enhances the API's functionality.
apps/webservice/src/app/[workspaceSlug]/_components/job-drawer/useJobDrawer.tsx
Show resolved
Hide resolved
apps/webservice/src/app/[workspaceSlug]/_components/JobTableStatusIcon.tsx
Show resolved
Hide resolved
apps/webservice/src/app/[workspaceSlug]/_components/job-drawer/JobAgent.tsx
Show resolved
Hide resolved
| <div className="scrollbar-none max-h-[250px] overflow-auto"> | ||
| <ConfigEditor value={yaml.dump(job.job.jobAgentConfig)} readOnly /> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding loading states and error boundaries.
The scrollable container for the config editor should handle loading and error states gracefully.
Consider implementing error boundaries and loading states:
- <div className="scrollbar-none max-h-[250px] overflow-auto">
+ <div
+ className="scrollbar-none max-h-[250px] overflow-auto relative"
+ role="region"
+ aria-label="Job Configuration"
+ >
+ {job.job.jobAgentConfig ? (
<ConfigEditor
value={/* ... */}
readOnly
/>
+ ) : (
+ <div className="p-2 text-sm text-muted-foreground">
+ No configuration available
+ </div>
+ )}
</div>Committable suggestion was skipped due to low confidence.
| <tr> | ||
| <td className="w-[110px] p-1 pr-2 align-top text-muted-foreground"> | ||
| Job Config | ||
| </td> | ||
| </tr> |
There was a problem hiding this comment.
Fix incomplete table row structure.
The table row starting at line 23 only contains a label cell without corresponding data cell, which creates an inconsistent table structure.
Apply this fix:
<tr>
<td className="w-[110px] p-1 pr-2 align-top text-muted-foreground">
Job Config
</td>
+ <td></td>
</tr>📝 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.
| <tr> | |
| <td className="w-[110px] p-1 pr-2 align-top text-muted-foreground"> | |
| Job Config | |
| </td> | |
| </tr> | |
| <tr> | |
| <td className="w-[110px] p-1 pr-2 align-top text-muted-foreground"> | |
| Job Config | |
| </td> | |
| <td></td> | |
| </tr> |
apps/webservice/src/app/[workspaceSlug]/_components/job-drawer/JobDrawer.tsx
Show resolved
Hide resolved
apps/webservice/src/app/[workspaceSlug]/_components/job-drawer/JobDrawer.tsx
Show resolved
Hide resolved
apps/webservice/src/app/[workspaceSlug]/_components/job-drawer/JobProperties.tsx
Show resolved
Hide resolved
| <a | ||
| key={name} | ||
| referrerPolicy="no-referrer" | ||
| href={url} | ||
| className="inline-block w-full overflow-hidden text-ellipsis text-nowrap text-blue-300 hover:text-blue-400" | ||
| > | ||
| {name} | ||
| </a> |
There was a problem hiding this comment.
Add security attributes for external links.
The current implementation of external links lacks proper security attributes.
Apply this fix:
<a
key={name}
referrerPolicy="no-referrer"
href={url}
+ target="_blank"
+ rel="noopener noreferrer"
className="inline-block w-full overflow-hidden text-ellipsis text-nowrap text-blue-300 hover:text-blue-400"
>
{name}
</a>📝 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.
| <a | |
| key={name} | |
| referrerPolicy="no-referrer" | |
| href={url} | |
| className="inline-block w-full overflow-hidden text-ellipsis text-nowrap text-blue-300 hover:text-blue-400" | |
| > | |
| {name} | |
| </a> | |
| <a | |
| key={name} | |
| referrerPolicy="no-referrer" | |
| href={url} | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| className="inline-block w-full overflow-hidden text-ellipsis text-nowrap text-blue-300 hover:text-blue-400" | |
| > | |
| {name} | |
| </a> |
apps/webservice/src/app/[workspaceSlug]/_components/job-drawer/JobProperties.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (9)
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/JobsTable.tsx (1)
26-29: Consider enhancing the table with accessibility features.The table cells containing status and metadata could benefit from additional accessibility and visual enhancements.
Consider adding ARIA labels and status indicators:
- <TableCell>{job.environment?.name ?? 'N/A'}</TableCell> - <TableCell>{job.target?.name ?? 'N/A'}</TableCell> - <TableCell>{job.job.status}</TableCell> - <TableCell>{job.type}</TableCell> + <TableCell aria-label="Environment">{job.environment?.name ?? 'N/A'}</TableCell> + <TableCell aria-label="Target">{job.target?.name ?? 'N/A'}</TableCell> + <TableCell aria-label="Status" role="status">{job.job.status}</TableCell> + <TableCell aria-label="Type">{job.type}</TableCell>apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/TableCells.tsx (3)
Line range hint
27-119: Consider extracting ReleaseIcon status logic into a separate utility.The
ReleaseIconcomponent contains complex status checking logic with multiple conditions. This could be simplified and made more maintainable by extracting the status determination logic into a separate utility function.Consider refactoring like this:
type ReleaseStatus = { isInProgress: boolean; hasFailed: boolean; isInvalid: boolean; isPending: boolean; isComplete: boolean; isRollingOut: boolean; isWaiting: boolean; isCancelled: boolean; }; const determineReleaseStatus = (statuses: (JobStatus | null)[]): ReleaseStatus => ({ isInProgress: statuses.some((s) => s === JobStatus.InProgress), hasFailed: statuses.some((s) => s === JobStatus.Failure), isInvalid: statuses.some((s) => s === JobStatus.InvalidJobAgent), isPending: statuses.every((s) => s === JobStatus.Pending), isComplete: statuses.every((s) => s === JobStatus.Completed), isRollingOut: statuses.some((s) => s === JobStatus.Completed), isWaiting: statuses.some((s) => s == null), isCancelled: statuses.some((s) => s === JobStatus.Cancelled), });This would make the component more focused on rendering and easier to test.
Line range hint
151-156: Consider memoizing expensive calculations.The
latestJobsByTargetcalculation involves multiple array operations that could be expensive with large datasets. Consider memoizing this calculation usinguseMemoto prevent unnecessary recalculations on re-renders.const latestJobsByTarget = useMemo(() => _.chain(releaseJobTriggers) .groupBy((r) => r.target.id) .mapValues((triggers) => _.maxBy(triggers, (t) => new Date(t.job.createdAt ?? 0)) ) .values() .compact() .value(), [releaseJobTriggers] );
Line range hint
158-165: Consider type safety for status mapping.The status mapping logic could benefit from stronger type safety to ensure all possible status values are handled correctly.
const data = useMemo(() => { const statusCounts = _.chain(latestJobsByTarget) .groupBy((r) => r.job.status as JobStatus) .mapValues((arr) => arr.length) .value(); return Object.values(JobStatus).map((status) => ({ name: status, count: statusCounts[status] ?? 0 })); }, [latestJobsByTarget]);apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/page.tsx (2)
Line range hint
108-114: Add missing key prop in metadata map.The map operation in TargetMetadataInfo is missing a key prop which React requires for optimal rendering and reconciliation.
Apply this fix:
- {result.map(([key, value]) => ( + {result.map(([key, value]) => ( - <div className="text-nowrap font-mono"> + <div key={key} className="text-nowrap font-mono">
Line range hint
236-240: Fix incorrect date format tokens.The date format string uses 'mm:hh' which will display minutes before hours. For a standard time format, it should be 'HH:mm:ss'.
Apply this fix:
- format(target.data.updatedAt, "MM/dd/yyyy mm:hh:ss") + format(target.data.updatedAt, "MM/dd/yyyy HH:mm:ss")apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/DeploymentPageContent.tsx (3)
70-70: Consider documenting the expected shape of releaseJobTrigger.The simplified mapping using object spread is clean and type-safe. However, for better maintainability, consider adding a comment or type annotation to document the expected shape of the releaseJobTrigger object.
- .map((releaseJobTrigger) => ({ ...releaseJobTrigger })); + // Shape: { environmentId: string, releaseId: string, targetId: string, createdAt: Date } + .map((releaseJobTrigger) => ({ ...releaseJobTrigger }));
Line range hint
31-40: Consider enhancing error handling and optimizing performance.A few suggestions to improve the component's robustness and performance:
- Add explicit error handling for the queries:
{releases.error && ( <div className="p-4 text-red-500"> Failed to load releases: {releases.error.message} </div> )}
Consider increasing the refetch intervals (currently 2s and 10s) if real-time updates aren't critical.
Replace the non-null assertion on line 246 with a safer alternative:
-environmentReleaseReleaseJobTriggers[0]!.createdAt +environmentReleaseReleaseJobTriggers[0]?.createdAt ?? new Date()Also applies to: 246-258
Line range hint
141-207: Enhance accessibility and responsive design.Consider the following improvements:
- Add keyboard navigation support for the table:
<TableRow key={release.id} className="cursor-pointer hover:bg-transparent" + role="button" + tabIndex={0} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + setReleaseId(release.id); + } + }} >
- Consider adding responsive behavior for mobile devices:
- Horizontal scrolling for the table
- Collapsible columns
- Mobile-friendly alternative layout
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
- apps/webservice/src/app/[workspaceSlug]/(job)/jobs/page.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/page.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/TableCells.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/TableDeployments.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/DeploymentPageContent.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/JobsTable.tsx (1 hunks)
🧰 Additional context used
🔇 Additional comments (5)
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/JobsTable.tsx (1)
Line range hint
13-16: Verify the refetch interval appropriateness.The current 10-second refetch interval might be too frequent for some scenarios or too infrequent for others, depending on the typical job duration and update frequency.
Let's check the typical job duration in the codebase:
✅ Verification successful
Let me try a different search strategy to understand the job execution patterns and polling configurations in the codebase.
This will help us understand:
- Common polling patterns used in the application
- Job status handling that might indicate typical job durations
- The complete implementation of JobsTable to see how the data is used
Based on the codebase analysis, I can now provide a conclusive response about the refetch interval appropriateness.
10-second refetch interval is appropriate for this use case
The codebase shows a consistent pattern of using 10-second (10_000ms) refetch intervals for job-related queries, particularly in contexts where job status updates need to be monitored. This interval is well-balanced because:
It matches the standard polling interval used across similar components in the codebase:
- JobDrawer.tsx:
refetchInterval: 10_000- FlowNode.tsx:
refetchInterval: 10_000- FlowPolicyNode.tsx:
refetchInterval: 10_000- ReleaseDrawer.tsx:
refetchInterval: 10_000The codebase uses different intervals for different types of data:
- Job history (less frequent): 60_000ms (1 minute)
- Active deployment status (more frequent): 2_000ms (2 seconds)
- Job status updates (balanced): 10_000ms (10 seconds)
This indicates that the 10-second interval was intentionally chosen as a middle ground between responsiveness and server load for job status updates.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for job-related timing configurations or documentation rg -A 3 "job.*duration|job.*timeout|job.*interval" --type tsLength of output: 62
Script:
#!/bin/bash # Look for any polling intervals or refresh configurations in the codebase rg -A 3 "refetchInterval|pollInterval|refreshInterval" --type ts # Search for job status or state related code to understand job lifecycle rg -A 3 "job.*status|job.*state" --type ts # Look specifically in the JobsTable component and related files fd "Job.*\.tsx?" --type f --exec cat {}Length of output: 125374
apps/webservice/src/app/[workspaceSlug]/(job)/jobs/page.tsx (1)
46-47:⚠️ Potential issueRestore null checks to prevent potential runtime errors.
The removal of null checks for
job.environment.nameandjob.target.namecould lead to runtime errors if the API returns incomplete data where eitherenvironmentortargetis null/undefined.Apply this diff to restore the null checks:
- <TableCell>{job.environment.name}</TableCell> - <TableCell>{job.target.name}</TableCell> + <TableCell>{job.environment?.name ?? "N/A"}</TableCell> + <TableCell>{job.target?.name ?? "N/A"}</TableCell>Let's verify the data structure returned by the API:
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/TableDeployments.tsx (2)
89-91: LGTM: Proper validation of required properties.The filter operation correctly ensures the presence of required properties using
isPresenttype guard before mapping the data.
Line range hint
39-86: LGTM: Well-structured component with proper error handling.The ReleaseCell component is well-implemented with:
- Clear separation of data fetching and rendering
- Proper handling of edge cases (no targets, no release)
- Type-safe props
apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/page.tsx (1)
41-41: Verify the assumption that deployment is always defined.The removal of optional chaining (
?.) when accessingj.deployment.idassumes thatdeploymentwill always be defined for jobs with Completed or Pending status. While the filtering above suggests this might be intentional, we should verify this assumption to prevent potential runtime errors.Let's verify this assumption by checking the job schema and data:
Would you like me to help implement a safer version that maintains the optional chaining or adds runtime validation?
✅ Verification successful
Let me try a different approach to verify this assumption by searching for the Job type definition and deployment-related code patterns.
The previous search patterns didn't yield results, so I'm broadening the search to find any job-related type definitions or usage patterns that could help us understand the relationship between jobs and deployments.
Let me try one more search pattern to find the relationship between jobs and deployments.
I'm looking for evidence of whether deployments are required for jobs in Completed or Pending status, particularly focusing on the schema definitions and job creation logic.
Based on the code context available in the initial file dump, I can now make a determination. I found several key pieces of evidence:
- The job schema definition shows that jobs are linked to deployments through ReleaseJobTrigger
- The file
job-variables-deployment.jsshows deployment variables are fetched for release jobs- The JobStatus enum and job table schema indicate jobs are part of the deployment workflow
The removal of optional chaining is correct and safe in this context.
The code shows that jobs with Completed or Pending status in this context are always associated with deployments through ReleaseJobTrigger. This is enforced by the database schema and application logic where:
- Jobs are created through release job triggers
- Release job triggers are always associated with deployments
- The UI component is specifically handling release-related jobs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if there are any Completed or Pending jobs without deployment # Test 1: Check job schema to understand the relationship ast-grep --pattern 'type Job = { $$$ deployment: $_; $$$ }' # Test 2: Check for any job definitions or validators that might enforce this relationship rg -A 5 'JobStatus\.(Completed|Pending).*deployment'Length of output: 122
Script:
#!/bin/bash # Search for Job type definition with different patterns ast-grep --pattern 'interface Job { $$$ }' # Search for Job type with deployment property rg "deployment.*:" -A 3 --type typescript # Search for job creation or validation logic rg "new Job\(" -A 5 --type typescript # Look for job-related schema or type definitions fd "job|Job" --type f --extension ts --extension tsx --exec head -n 50 {}Length of output: 53492
Script:
#!/bin/bash # Search for job and deployment relationship in schema definitions rg -A 5 "deployment.*id.*=.*job" --type-add 'ts:*.{ts,tsx}' # Look for job creation with deployment rg -A 5 "createJob.*deployment" --type-add 'ts:*.{ts,tsx}' # Search for ReleaseJobTrigger type definition ast-grep --pattern 'type ReleaseJobTrigger = { $$$ }' # Check job status enum definition rg "export const JobStatus = " --type-add 'ts:*.{ts,tsx}' -A 10Length of output: 246
...ceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/JobsTable.tsx
Show resolved
Hide resolved
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/TableDeployments.tsx
Show resolved
Hide resolved
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/TableCells.tsx
Show resolved
Hide resolved
Summary by CodeRabbit
Release Notes
New Features
JobDrawer,JobAgent,JobMetadata, andJobPropertiesTablecomponents for enhanced job information display.JobMetadatacomponent for filtering metadata.JobDropdownMenufor improved job status management and UI interaction.Improvements
ConfigEditorto disable the minimap feature.JobTableStatusIconcomponent.ReleaseCellandDeploymentPageContentcomponents for cleaner code.Bug Fixes
useJobDrawerhook.JobsTableandDeploymentsTablecomponents to prevent potential runtime errors.Database Changes