Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { Handle, Position } from "reactflow";
import { cn } from "@ctrlplane/ui";
import { JobStatus, JobStatusReadable } from "@ctrlplane/validators/jobs";

import { useJobDrawer } from "~/app/[workspaceSlug]/(app)/_components/job-drawer/useJobDrawer";
import { useDeploymentEnvResourceDrawer } from "~/app/[workspaceSlug]/(app)/_components/deployment-resource-drawer/useDeploymentResourceDrawer";
import { api } from "~/trpc/react";
import { ReleaseIcon } from "../../ReleaseCell";

Expand All @@ -19,7 +19,7 @@ type DeploymentNodeProps = NodeProps<{

export const DeploymentNode: React.FC<DeploymentNodeProps> = ({ data }) => {
const { deployment, environment, resource } = data;
const { setJobId } = useJobDrawer();
const { setDeploymentEnvResourceId } = useDeploymentEnvResourceDrawer();

const resourceId = resource.id;
const environmentId = environment.id;
Expand Down Expand Up @@ -49,16 +49,15 @@ export const DeploymentNode: React.FC<DeploymentNodeProps> = ({ data }) => {
<>
<div
className={cn(
"relative flex h-[70px] w-[250px] items-center gap-2 rounded-md border border-neutral-800 bg-neutral-900/30 px-4",
"relative flex h-[70px] w-[250px] cursor-pointer items-center gap-2 rounded-md border border-neutral-800 bg-neutral-900/30 px-4",
isInProgress && "border-blue-500",
isPending && "border-neutral-500",
isCompleted && "border-green-500",
releaseJobTrigger == null && "border-neutral-800",
releaseJobTrigger != null && "cursor-pointer",
)}
onClick={() => {
if (releaseJobTrigger != null) setJobId(releaseJobTrigger.job.id);
}}
onClick={() =>
setDeploymentEnvResourceId(deployment.id, environment.id, resource.id)
}
>
<ReleaseIcon job={releaseJobTrigger?.job} />
<div className="flex min-w-0 flex-1 flex-col items-start">
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
"use client";

import { IconLoader2, IconShip } from "@tabler/icons-react";

import {
Drawer,
DrawerContent,
DrawerDescription,
DrawerTitle,
} from "@ctrlplane/ui/drawer";

import { api } from "~/trpc/react";
import { ReleaseTable } from "./ReleaseTable";
import { useDeploymentEnvResourceDrawer } from "./useDeploymentResourceDrawer";

export const DeploymentResourceDrawer: React.FC = () => {
const {
deploymentId,
environmentId,
resourceId,
setDeploymentEnvResourceId,
} = useDeploymentEnvResourceDrawer();
const isOpen =
deploymentId != null && environmentId != null && resourceId != null;
const setIsOpen = () => setDeploymentEnvResourceId(null, null, null);

const { data: deployment, ...deploymentQ } = api.deployment.byId.useQuery(
deploymentId ?? "",
{ enabled: isOpen },
);

const { data: resource, ...resourceQ } = api.resource.byId.useQuery(
resourceId ?? "",
{ enabled: isOpen },
);

const { data: environment, ...environmentQ } = api.environment.byId.useQuery(
environmentId ?? "",
{ enabled: isOpen },
);

const { data: releaseWithTriggersData, ...releaseWithTriggersQ } =
api.job.config.byDeploymentEnvAndResource.useQuery(
{
deploymentId: deploymentId ?? "",
environmentId: environmentId ?? "",
resourceId: resourceId ?? "",
},
{ enabled: isOpen, refetchInterval: 5_000 },
);
Comment on lines +43 to +50
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 27, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Optimize query conditions to prevent unnecessary API calls

The current implementation uses empty strings when IDs are null, which could trigger unnecessary API calls. Consider enhancing the enabled condition to check for valid IDs:

   api.job.config.byDeploymentEnvAndResource.useQuery(
     {
-      deploymentId: deploymentId ?? "",
-      environmentId: environmentId ?? "",
-      resourceId: resourceId ?? "",
+      deploymentId,
+      environmentId,
+      resourceId,
     },
-    { enabled: isOpen, refetchInterval: 5_000 },
+    { 
+      enabled: isOpen && deploymentId && environmentId && resourceId,
+      refetchInterval: 5_000 
+    },
   );
📝 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
api.job.config.byDeploymentEnvAndResource.useQuery(
{
deploymentId: deploymentId ?? "",
environmentId: environmentId ?? "",
resourceId: resourceId ?? "",
},
{ enabled: isOpen, refetchInterval: 5_000 },
);
api.job.config.byDeploymentEnvAndResource.useQuery(
{
deploymentId,
environmentId,
resourceId,
},
{
enabled: isOpen && deploymentId && environmentId && resourceId,
refetchInterval: 5_000
},
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

isOpen already checks if the vars are null

Copy link
Contributor

Choose a reason for hiding this comment

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

@adityachoudhari26, understood. Since isOpen already checks if the variables are null, the current implementation is appropriate.


✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#237
File: apps/webservice/src/app/[workspaceSlug]/(app)/_components/deployment-resource-drawer/DeploymentResourceDrawer.tsx:43-50
Timestamp: 2024-11-27T23:16:35.378Z
Learning: In `DeploymentResourceDrawer.tsx`, the `isOpen` variable already checks whether `deploymentId`, `environmentId`, and `resourceId` are non-null, so additional null checks in query `enabled` conditions are not necessary.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

const releaseWithTriggers = releaseWithTriggersData ?? [];

const loading =
deploymentQ.isLoading ||
resourceQ.isLoading ||
environmentQ.isLoading ||
releaseWithTriggersQ.isLoading;

return (
<Drawer open={isOpen} onOpenChange={setIsOpen}>
<DrawerContent
showBar={false}
className="scrollbar-thin scrollbar-thumb-neutral-800 scrollbar-track-neutral-900 left-auto right-0 top-0 mt-0 h-screen w-2/3 max-w-7xl overflow-auto rounded-none focus-visible:outline-none"
>
{loading && (
<div className="flex h-full w-full items-center justify-center">
<IconLoader2 className="h-8 w-8 animate-spin" />
{/*
Drawer component throws an error if the title and description are not present, so just render empty elements.
Technically shadcn recommends using the VisuallyHidden radix component, but this fixes without any additional dependencies.
*/}
<DrawerTitle />
<DrawerDescription />
</div>
)}
{!loading &&
deployment != null &&
resource != null &&
environment != null && (
<>
<DrawerTitle className="flex flex-col gap-2 border-b p-6">
<div className="flex items-center gap-2">
<div className="flex-shrink-0 rounded bg-amber-500/20 p-1 text-amber-400">
<IconShip className="h-4 w-4" />
</div>
{deployment.name}
</div>
<div className="flex flex-col gap-1 text-xs text-muted-foreground">
<span>Resource: {resource.name}</span>
<span>Environment: {environment.name}</span>
</div>
</DrawerTitle>

<div className="flex flex-col gap-4 p-6">
<ReleaseTable
releasesWithTriggers={releaseWithTriggers}
environment={environment}
deployment={deployment}
resource={resource}
/>
</div>
</>
)}
</DrawerContent>
</Drawer>
);
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
import type { RouterOutputs } from "@ctrlplane/api";
import type * as SCHEMA from "@ctrlplane/db/schema";

import {
Table,
TableBody,
TableCell,
TableHead,
TableHeader,
TableRow,
} from "@ctrlplane/ui/table";

import { ReleaseRows } from "./TableRow";

type ReleaseWithTriggers =
RouterOutputs["job"]["config"]["byDeploymentEnvAndResource"][number];

type ReleaseTableProps = {
releasesWithTriggers: ReleaseWithTriggers[];
environment: SCHEMA.Environment & { system: SCHEMA.System };
deployment: SCHEMA.Deployment;
resource: SCHEMA.Resource;
};

export const ReleaseTable: React.FC<ReleaseTableProps> = ({
releasesWithTriggers,
environment,
deployment,
resource,
}) => (
<div className="rounded-md border border-neutral-800">
<Table className="table-fixed">
<TableHeader>
<TableRow>
<TableHead>Release</TableHead>
<TableHead>Status</TableHead>
<TableHead>Created</TableHead>
<TableHead>Links</TableHead>
<TableHead />
</TableRow>
</TableHeader>
<TableBody>
{releasesWithTriggers.map((release) => (
<ReleaseRows
release={release}
environment={environment}
deployment={deployment}
resource={resource}
key={release.id}
/>
))}
{releasesWithTriggers.length === 0 && (
<TableRow>
<TableCell
colSpan={5}
className="text-center text-muted-foreground"
>
No releases found
</TableCell>
</TableRow>
)}
</TableBody>
</Table>
</div>
);
Loading
Loading