fix: External url button #158
Conversation
WalkthroughThe pull request introduces several changes across multiple files, primarily focusing on the removal of the Changes
Possibly related PRs
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: 3
🧹 Outside diff range and nitpick comments (4)
packages/db/src/schema/job.ts (1)
Line range hint
71-93: LGTM! Consider adding JSDoc comments for better documentation.The addition of new types (
JobMetadata,Job,JobStatus) and theupdateJobschema improves type safety and allows for partial updates, which is a good practice. The newjobVariabletable and its associated types and schemas seem to be a well-structured addition for storing job-specific variables.Consider adding JSDoc comments to the exported types and functions for better documentation. For example:
/** Represents the metadata associated with a job */ export type JobMetadata = InferSelectModel<typeof jobMetadata>; /** Represents a job in the system */ export type Job = InferSelectModel<typeof job>; /** Represents the possible statuses of a job */ export type JobStatus = Job["status"]; /** Schema for updating a job, allowing partial updates */ export const updateJob = createInsertSchema(job) .omit({ id: true, jobAgentConfig: true, createdAt: true, updatedAt: true, }) .partial();apps/event-worker/src/job-dispatch/github.ts (1)
108-108: LGTM! Consider adding a comment for clarity.The removal of the
urlproperty from the destructuring assignment is consistent with the overall changes to remove external URL handling. This change aligns well with the database schema update and simplifies the job dispatching logic.Consider adding a brief comment explaining why the
urlis no longer needed:- const { runId: runId_, status: status_ } = await pRetry( + // Note: 'url' is no longer used as external URL handling has been removed + const { runId: runId_, status: status_ } = await pRetry(apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/TargetReleaseTable.tsx (2)
105-125: Handle empty links object to avoid rendering issuesIf
linksis an empty object, the code will render an empty container. Consider adding a check to ensurelinkshas entries before rendering.Apply this diff:
{links != null && Object.keys(links).length > 0 && ( <div className="flex items-center gap-1"> {Object.entries(links).map(([label, url]) => ( // ... rest of the code
75-76: Fix conditional class rendering inTableRowUsing logical AND (
&&) insidecnmay result in unintended class names when the condition is false. It's better to use a ternary operator for clarity.Apply this diff:
className={cn( - idx !== jobs.length - 1 && "border-b-neutral-800/50", + idx !== jobs.length - 1 ? "border-b-neutral-800/50" : "" )}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
- apps/event-worker/src/job-dispatch/github.ts (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/TargetReleaseTable.tsx (2 hunks)
- packages/api/src/router/job.ts (3 hunks)
- packages/db/drizzle/0018_round_franklin_richards.sql (1 hunks)
- packages/db/drizzle/meta/_journal.json (1 hunks)
- packages/db/src/schema/job.ts (1 hunks)
- packages/job-dispatch/src/test/job-variables-deployment.test.ts (0 hunks)
💤 Files with no reviewable changes (1)
- packages/job-dispatch/src/test/job-variables-deployment.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/db/drizzle/0018_round_franklin_richards.sql
🧰 Additional context used
🔇 Additional comments (4)
packages/db/src/schema/job.ts (1)
Line range hint
1-93: Overall changes look good. Confirm the removal ofexternalUrl.The additions of new types, enums, and schemas improve the structure and type safety of the job-related data. These changes align well with the objectives mentioned in the PR summary.
Although not directly visible in the provided code segment, the AI-generated summary mentioned the removal of the
externalUrlfield from thejobtable. Let's verify this change:If the script confirms the removal, no further action is needed. If it doesn't, please double-check the removal of
externalUrlfrom thejobtable.packages/db/drizzle/meta/_journal.json (1)
130-136: LGTM: New migration entry added correctly.The new entry (idx: 18) has been added to the journal with the correct structure and format. This entry corresponds to the migration that removes the
external_urlcolumn from the job table, as mentioned in the PR summary.A few observations:
- The
idxvalue (18) is sequential and correct.- The
versionremains "7", consistent with previous entries.- The
whentimestamp (1729564036579) is later than the previous entry, maintaining chronological order.- The
tag"0018_round_franklin_richards" follows the naming convention and matches the idx.- The
breakpointsvalue is set to true, consistent with other entries.This change aligns with the broader updates mentioned in the AI summary, particularly the removal of the
external_urlcolumn from the job table.packages/api/src/router/job.ts (2)
15-15: Import ofjobMetadatais necessary and correctThe addition of
jobMetadatato the imports reflects its usage in subsequent queries and ensures that the module is available for use.
193-196: Validate the additionalleftJoinstatements for necessity and performanceThe inclusion of
leftJoinwithjobMetadata,environmentPolicy, andenvironmentPolicyReleaseWindowenriches the data retrieved by the query. However, these joins can impact query performance, especially with large datasets. Ensure that these joins are necessary for the functionality and consider indexing the joined columns to optimize performance.If these joins are crucial, confirm that appropriate indexes exist on the following columns to improve query execution speed:
jobMetadata.jobIdenvironment.policyIdenvironmentPolicyReleaseWindow.policyId
| {Object.entries(links).map(([label, url]) => ( | ||
| <Link | ||
| key={label} | ||
| href={url} |
There was a problem hiding this comment.
Validate URLs before rendering
When rendering external links, ensure that the URLs are valid and safe. This helps prevent potential security risks such as XSS attacks.
Consider validating the url before using it in the href attribute. You can use the built-in URL constructor for validation:
<Link
key={label}
- href={url}
+ href={(() => {
+ try {
+ const safeUrl = new URL(url);
+ return safeUrl.toString();
+ } catch {
+ console.error('Invalid URL:', url);
+ return '#';
+ }
+ })()}
target="_blank"
rel="noopener noreferrer"
className={buttonVariants({
variant: "secondary",
size: "sm",
className: "gap-1",
})}
>Committable suggestion was skipped due to low confidence.
| ? (JSON.parse(linksMetadata) as Record<string, string>) | ||
| : null; |
There was a problem hiding this comment.
Add error handling for JSON parsing
If linksMetadata contains invalid JSON, JSON.parse will throw an error, potentially causing the component to crash. Consider adding error handling to manage parsing errors gracefully.
Apply this diff to add error handling:
const links =
linksMetadata != null
- ? (JSON.parse(linksMetadata) as Record<string, string>)
+ ? (() => {
+ try {
+ return JSON.parse(linksMetadata) as Record<string, string>;
+ } catch (e) {
+ console.error('Failed to parse linksMetadata:', e);
+ return null;
+ }
+ })()
: null;📝 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.
| ? (JSON.parse(linksMetadata) as Record<string, string>) | |
| : null; | |
| ? (() => { | |
| try { | |
| return JSON.parse(linksMetadata) as Record<string, string>; | |
| } catch (e) { | |
| console.error('Failed to parse linksMetadata:', e); | |
| return null; | |
| } | |
| })() | |
| : null; |
| job: { | ||
| ...v[0]!.job, | ||
| metadata: v.map((t) => t.job_metadata).filter(isPresent), | ||
| }, |
There was a problem hiding this comment.
Ensure safe access to array elements and handle potential undefined values
The usage of non-null assertion ! in v[0]! assumes that the array v is not empty. If v can be an empty array, accessing v[0]! will result in an undefined value and could lead to runtime errors when properties are accessed. Consider adding a check to ensure v is not empty before accessing its elements.
Apply this diff to safely access the first element of the array:
- ...v[0]!.release_job_trigger,
+ ...(v[0]?.release_job_trigger || {}),Similarly, update the subsequent usages:
- job: {
- ...v[0]!.job,
- metadata: v.map((t) => t.job_metadata).filter(isPresent),
- },
+ job: v[0]?.job
+ ? {
+ ...v[0].job,
+ metadata: v.map((t) => t.job_metadata).filter(isPresent),
+ }
+ : undefined,This ensures that you only access properties if v[0] is defined.
Committable suggestion was skipped due to low confidence.
Summary by CodeRabbit
Release Notes
New Features
TargetReleaseTable, allowing users to view and interact with associated external links.Bug Fixes
Tests
determineVariablesForReleaseJobfunction, ensuring robust handling of various job scenarios.