Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: External url button #158

Merged
merged 1 commit into from
Oct 22, 2024
+3,751 −73
Merged

fix: External url button #158

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 1 addition & 9 deletions apps/event-worker/src/job-dispatch/github.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,14 +103,9 @@ export const dispatchGithubJob = async (je: Job) => {

let runId: number | null = null;
let status: string | null = null;
let externalUrl: string | null = null;

try {
const {
runId: runId_,
status: status_,
url: externalUrl_,
} = await pRetry(
const { runId: runId_, status: status_ } = await pRetry(
async () => {
const runs = await octokit.actions.listWorkflowRuns({
owner: parsed.data.owner,
Expand All @@ -134,7 +129,6 @@ export const dispatchGithubJob = async (je: Job) => {

runId = runId_;
status = status_;
externalUrl = externalUrl_;
} catch (error) {
logger.error(`Job ${je.id} dispatch to GitHub failed`, { error });
await db.update(job).set({
Expand All @@ -147,15 +141,13 @@ export const dispatchGithubJob = async (je: Job) => {
logger.info(`Job ${je.id} dispatched to GitHub`, {
runId,
status,
externalUrl,
});

await db
.update(job)
.set({
externalId: runId.toString(),
status: convertStatus(status ?? JobStatus.InProgress),
externalUrl,
})
.where(eq(job.id, je.id));
};
142 changes: 81 additions & 61 deletions ...ems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/TargetReleaseTable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,14 @@ import type { JobStatus } from "@ctrlplane/validators/jobs";
import React, { Fragment } from "react";
import Link from "next/link";
import { usePathname } from "next/navigation";
import { IconLoader2 } from "@tabler/icons-react";
import { IconExternalLink, IconLoader2 } from "@tabler/icons-react";
import { capitalCase } from "change-case";
import _ from "lodash";

import { cn } from "@ctrlplane/ui";
import { buttonVariants } from "@ctrlplane/ui/button";
import { Table, TableBody, TableCell, TableRow } from "@ctrlplane/ui/table";
import { ReservedMetadataKey } from "@ctrlplane/validators/targets";

import { JobTableStatusIcon } from "~/app/[workspaceSlug]/_components/JobTableStatusIcon";
import { api } from "~/trpc/react";
Expand Down Expand Up @@ -56,70 +58,88 @@ export const TargetReleaseTable: React.FC<TargetReleaseTableProps> = ({
)}
</TableCell>
</TableRow>
{jobs.map((job, idx) => (
<TableRow
key={job.id}
className={cn(
idx !== jobs.length - 1 && "border-b-neutral-800/50",
)}
>
<TableCell className="hover:bg-neutral-800/55">
<Link
href={`${pathname}?target_id=${job.target?.id}`}
className="block w-full hover:text-blue-300"
>
{job.target?.name}
</Link>
</TableCell>
<TableCell>
<div className="flex items-center gap-1">
<JobTableStatusIcon status={job.job.status} />
{capitalCase(job.job.status)}
</div>
</TableCell>
<TableCell>{job.type}</TableCell>
<TableCell>
{job.job.externalId != null ? (
<code className="font-mono text-xs">
{job.job.externalId}
</code>
) : (
<span className="text-sm text-muted-foreground">
No external ID
</span>
{jobs.map((job, idx) => {
const linksMetadata = job.job.metadata.find(
(m) => m.key === String(ReservedMetadataKey.Links),
)?.value;

const links =
linksMetadata != null
? (JSON.parse(linksMetadata) as Record<string, string>)
: null;
Comment on lines +68 to +69
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
? (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;


return (
<TableRow
key={job.id}
className={cn(
idx !== jobs.length - 1 && "border-b-neutral-800/50",
)}
</TableCell>
<TableCell>
{job.job.externalUrl != null ? (
>
<TableCell className="hover:bg-neutral-800/55">
<Link
href={job.job.externalUrl}
rel="nofollow noreferrer"
target="_blank"
href={`${pathname}?target_id=${job.target?.id}`}
className="block w-full hover:text-blue-300"
>
{job.job.externalUrl}
{job.target?.name}
</Link>
) : (
<span className="text-sm text-muted-foreground">
No external URL
</span>
)}
</TableCell>
<TableCell>
<div className="flex justify-end">
<TargetDropdownMenu
release={release}
deploymentName={deploymentName}
target={job.target}
environmentId={job.environmentId}
job={{
id: job.job.id,
status: job.job.status as JobStatus,
}}
/>
</div>
</TableCell>
</TableRow>
))}
</TableCell>
<TableCell>
<div className="flex items-center gap-1">
<JobTableStatusIcon status={job.job.status} />
{capitalCase(job.job.status)}
</div>
</TableCell>
<TableCell>{job.type}</TableCell>
<TableCell>
{job.job.externalId != null ? (
<code className="font-mono text-xs">
{job.job.externalId}
</code>
) : (
<span className="text-sm text-muted-foreground">
No external ID
</span>
)}
</TableCell>
<TableCell>
{links != null && (
<div className="flex items-center gap-1">
{Object.entries(links).map(([label, url]) => (
<Link
key={label}
href={url}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

target="_blank"
rel="noopener noreferrer"
className={buttonVariants({
variant: "secondary",
size: "sm",
className: "gap-1",
})}
>
<IconExternalLink className="h-4 w-4" />
{label}
</Link>
))}
</div>
)}
</TableCell>
<TableCell>
<div className="flex justify-end">
<TargetDropdownMenu
release={release}
deploymentName={deploymentName}
target={job.target}
environmentId={job.environmentId}
job={{
id: job.job.id,
status: job.job.status as JobStatus,
}}
/>
</div>
</TableCell>
</TableRow>
);
})}
</Fragment>
);
})
Expand Down
7 changes: 6 additions & 1 deletion packages/api/src/router/job.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
environmentPolicyReleaseWindow,
job,
jobAgent,
jobMetadata,
release,
releaseJobTrigger,
system,
Expand Down Expand Up @@ -189,6 +190,7 @@ const releaseJobTriggerRouter = createTRPCRouter({
.input(z.string().uuid())
.query(({ ctx, input }) =>
releaseJobTriggerQuery(ctx.db)
.leftJoin(jobMetadata, eq(jobMetadata.jobId, job.id))
.leftJoin(
environmentPolicy,
eq(environment.policyId, environmentPolicy.id),
Expand All @@ -204,7 +206,10 @@ const releaseJobTriggerRouter = createTRPCRouter({
.groupBy((row) => row.release_job_trigger.id)
.map((v) => ({
...v[0]!.release_job_trigger,
job: v[0]!.job,
job: {
...v[0]!.job,
metadata: v.map((t) => t.job_metadata).filter(isPresent),
},
Comment on lines +209 to +212
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

jobAgent: v[0]!.job_agent,
target: v[0]!.target,
release: { ...v[0]!.release, deployment: v[0]!.deployment },
Expand Down
1 change: 1 addition & 0 deletions packages/db/drizzle/0018_round_franklin_richards.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ALTER TABLE "job" DROP COLUMN IF EXISTS "external_url";
Loading
Loading