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.

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: Set release channels for policy and env #183

Merged
merged 7 commits into from
Oct 30, 2024
Merged
Show file tree
Hide file tree
Changes from 3 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
@@ -0,0 +1,145 @@
"use client";

import React, { useState } from "react";
import { useParams, useRouter, useSearchParams } from "next/navigation";
import {
IconDotsVertical,
IconFilter,
IconInfoCircle,
IconLoader2,
IconPlant,
IconTarget,
} from "@tabler/icons-react";

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

import { api } from "~/trpc/react";
import { TabButton } from "../TabButton";
import { EnvironmentDropdownMenu } from "./EnvironmentDropdownMenu";
import { EditFilterForm } from "./Filter";
import { Overview } from "./Overview";
import { ReleaseChannels } from "./ReleaseChannels";

const param = "environment_id";
export const useEnvironmentDrawer = () => {
const router = useRouter();
const params = useSearchParams();
const environmentId = params.get(param);

const setEnvironmentId = (id: string | null) => {
const url = new URL(window.location.href);
if (id === null) {
url.searchParams.delete(param);
router.replace(`${url.pathname}?${url.searchParams.toString()}`);
return;
}
url.searchParams.set(param, id);
router.replace(`${url.pathname}?${url.searchParams.toString()}`);
};

const removeEnvironmentId = () => setEnvironmentId(null);

return { environmentId, setEnvironmentId, removeEnvironmentId };
};

export const EnvironmentDrawer: React.FC = () => {
const { environmentId, removeEnvironmentId } = useEnvironmentDrawer();
const isOpen = Boolean(environmentId);
const setIsOpen = removeEnvironmentId;
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

Fix 'onOpenChange' handler to properly handle the open state

The setIsOpen function is assigned to removeEnvironmentId, which doesn't accept any arguments. However, the onOpenChange handler expects a function that accepts a boolean parameter indicating the open state. This mismatch can lead to unexpected behavior when opening or closing the drawer.

To fix this, modify setIsOpen to handle the open parameter correctly:

-const setIsOpen = removeEnvironmentId;
+const setIsOpen = (open: boolean) => {
+  if (!open) {
+    removeEnvironmentId();
+  }
+};
📝 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
const setIsOpen = removeEnvironmentId;
const setIsOpen = (open: boolean) => {
if (!open) {
removeEnvironmentId();
}
};

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

const { workspaceSlug } = useParams<{ workspaceSlug: string }>();
const workspaceQ = api.workspace.bySlug.useQuery(workspaceSlug, {
enabled: isOpen,
});
const workspace = workspaceQ.data;

const deploymentsQ = api.deployment.bySystemId.useQuery(
environment?.systemId ?? "",
{ enabled: isOpen && environment != null },
);
const deployments = deploymentsQ.data;

const [activeTab, setActiveTab] = useState("overview");

const loading =
environmentQ.isLoading || workspaceQ.isLoading || deploymentsQ.isLoading;
Comment on lines +69 to +70
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

Adjust loading state calculation to account for conditionally enabled queries

With workspaceQ and deploymentsQ now conditionally enabled, their isLoading states may not reflect the actual loading status. This could lead to incorrect determination of the overall loading state.

Update the loading state calculation to consider the enabled status of each query:

-const loading =
-  environmentQ.isLoading || workspaceQ.isLoading || deploymentsQ.isLoading;
+const loading =
+  environmentQ.isLoading ||
+  (workspaceQ.isLoading && workspaceQ.options.enabled) ||
+  (deploymentsQ.isLoading && deploymentsQ.options.enabled);
📝 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
const loading =
environmentQ.isLoading || workspaceQ.isLoading || deploymentsQ.isLoading;
const loading =
environmentQ.isLoading ||
(workspaceQ.isLoading && workspaceQ.options.enabled) ||
(deploymentsQ.isLoading && deploymentsQ.options.enabled);


return (
<Drawer open={isOpen} onOpenChange={setIsOpen}>
<DrawerContent
showBar={false}
className="left-auto right-0 top-0 mt-0 h-screen w-2/3 max-w-6xl overflow-auto rounded-none focus-visible:outline-none"
>
Comment on lines +74 to +77
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider making drawer width more responsive.

The current fixed width of 2/3 viewport with a max-width might be too wide on certain screen sizes. Consider using responsive breakpoints for better adaptability.

-className="left-auto right-0 top-0 mt-0 h-screen w-2/3 max-w-6xl overflow-auto rounded-none focus-visible:outline-none"
+className="left-auto right-0 top-0 mt-0 h-screen w-full overflow-auto rounded-none focus-visible:outline-none md:w-3/4 lg:w-2/3 max-w-6xl focus-visible:outline-none"
📝 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
<DrawerContent
showBar={false}
className="left-auto right-0 top-0 mt-0 h-screen w-2/3 max-w-6xl overflow-auto rounded-none focus-visible:outline-none"
>
<DrawerContent
showBar={false}
className="left-auto right-0 top-0 mt-0 h-screen w-full overflow-auto rounded-none focus-visible:outline-none md:w-3/4 lg:w-2/3 max-w-6xl focus-visible:outline-none"
>

<DrawerTitle className="flex items-center gap-2 border-b p-6">
<div className="flex-shrink-0 rounded bg-green-500/20 p-1 text-green-400">
<IconPlant className="h-4 w-4" />
</div>
{environment?.name}
{environment != null && (
<EnvironmentDropdownMenu environment={environment}>
<Button variant="ghost" size="icon" className="h-6 w-6">
<IconDotsVertical className="h-4 w-4" />
</Button>
</EnvironmentDropdownMenu>
)}
</DrawerTitle>

{loading && (
<div className="flex h-full items-center justify-center">
<IconLoader2 className="h-8 w-8 animate-spin" />
</div>
)}

{!loading && (
<div className="flex w-full gap-6 p-6">
<div className="space-y-1">
<TabButton
active={activeTab === "overview"}
onClick={() => setActiveTab("overview")}
icon={<IconInfoCircle className="h-4 w-4" />}
label="Overview"
/>
<TabButton
active={activeTab === "targets"}
onClick={() => setActiveTab("targets")}
icon={<IconTarget className="h-4 w-4" />}
label="Targets"
/>
<TabButton
active={activeTab === "release-channels"}
onClick={() => setActiveTab("release-channels")}
icon={<IconFilter className="h-4 w-4" />}
label="Release Channels"
/>
</div>

{environment != null && (
<div className="w-full overflow-auto">
{activeTab === "overview" && (
<Overview environment={environment} />
)}
{activeTab === "targets" && workspace != null && (
<EditFilterForm
environment={environment}
workspaceId={workspace.id}
/>
)}
{activeTab === "release-channels" && deployments != null && (
<ReleaseChannels
environment={environment}
deployments={deployments}
/>
)}
</div>
)}
</div>
)}
</DrawerContent>
</Drawer>
);
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
import type * as SCHEMA from "@ctrlplane/db/schema";
import { useState } from "react";
import { IconTrash } from "@tabler/icons-react";

import {
AlertDialog,
AlertDialogAction,
AlertDialogCancel,
AlertDialogContent,
AlertDialogDescription,
AlertDialogFooter,
AlertDialogHeader,
AlertDialogTitle,
AlertDialogTrigger,
} from "@ctrlplane/ui/alert-dialog";
import { buttonVariants } from "@ctrlplane/ui/button";
import {
DropdownMenu,
DropdownMenuContent,
DropdownMenuItem,
DropdownMenuTrigger,
} from "@ctrlplane/ui/dropdown-menu";

import { api } from "~/trpc/react";
import { useEnvironmentDrawer } from "./EnvironmentDrawer";

const DeleteEnvironmentDialog: React.FC<{
environment: SCHEMA.Environment;
children: React.ReactNode;
onClose: () => void;
}> = ({ environment, children, onClose }) => {
Comment on lines +27 to +31
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 29, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add type safety for required environment fields.

The component assumes environment.systemId exists but doesn't validate it through types.

Consider adding a type guard or making the required fields explicit:

-  environment: SCHEMA.Environment;
+  environment: SCHEMA.Environment & {
+    systemId: string;  // Make required fields explicit
+  };
📝 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
const DeleteEnvironmentDialog: React.FC<{
environment: SCHEMA.Environment;
children: React.ReactNode;
onClose: () => void;
}> = ({ environment, children, onClose }) => {
const DeleteEnvironmentDialog: React.FC<{
environment: SCHEMA.Environment & {
systemId: string; // Make required fields explicit
};
children: React.ReactNode;
onClose: () => void;
}> = ({ environment, children, onClose }) => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the type already has an explicit systemId

Copy link
Contributor

Choose a reason for hiding this comment

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

@adityachoudhari26, thank you for the clarification. You're correct that SCHEMA.Environment already includes systemId. No changes are necessary.

(🐰💭)


✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#183
File: apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/EnvironmentDropdownMenu.tsx:27-31
Timestamp: 2024-10-30T00:04:33.373Z
Learning: The `SCHEMA.Environment` type includes the `systemId` field.

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 [open, setOpen] = useState(false);
const deleteEnvironment = api.environment.delete.useMutation();
const utils = api.useUtils();
const { removeEnvironmentId } = useEnvironmentDrawer();

const onDelete = () =>
deleteEnvironment
.mutateAsync(environment.id)
.then(() => utils.environment.bySystemId.invalidate(environment.systemId))
.then(() => removeEnvironmentId())
.then(() => setOpen(false));

Comment on lines +37 to +43
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 and loading state for deletion.

The deletion operation lacks error handling and loading state management, which could lead to a poor user experience if the operation fails.

Consider implementing this improved version:

-  const onDelete = () =>
-    deleteEnvironment
-      .mutateAsync(environment.id)
-      .then(() => utils.environment.bySystemId.invalidate(environment.systemId))
-      .then(() => removeEnvironmentId())
-      .then(() => setOpen(false));
+  const onDelete = async () => {
+    try {
+      await deleteEnvironment.mutateAsync(environment.id);
+      await utils.environment.bySystemId.invalidate(environment.systemId);
+      removeEnvironmentId();
+      setOpen(false);
+    } catch (error) {
+      console.error('Failed to delete environment:', error);
+      // Add error notification here
+    }
+  };
📝 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
const onDelete = () =>
deleteEnvironment
.mutateAsync(environment.id)
.then(() => utils.environment.bySystemId.invalidate(environment.systemId))
.then(() => removeEnvironmentId())
.then(() => setOpen(false));
const onDelete = async () => {
try {
await deleteEnvironment.mutateAsync(environment.id);
await utils.environment.bySystemId.invalidate(environment.systemId);
removeEnvironmentId();
setOpen(false);
} catch (error) {
console.error('Failed to delete environment:', error);
// Add error notification here
}
};

return (
<AlertDialog
open={open}
onOpenChange={(o) => {
setOpen(o);
if (!o) onClose();
}}
>
<AlertDialogTrigger asChild>{children}</AlertDialogTrigger>
<AlertDialogContent>
<AlertDialogHeader>
<AlertDialogTitle>Delete Environment</AlertDialogTitle>
</AlertDialogHeader>
<AlertDialogDescription>
Are you sure you want to delete this environment? You will have to
recreate it from scratch.
</AlertDialogDescription>
<AlertDialogFooter>
<AlertDialogCancel>Cancel</AlertDialogCancel>
<AlertDialogAction
onClick={onDelete}
className={buttonVariants({ variant: "destructive" })}
>
Delete
</AlertDialogAction>
</AlertDialogFooter>
</AlertDialogContent>
</AlertDialog>
);
};

type EnvironmentDropdownMenuProps = {
environment: SCHEMA.Environment;
children: React.ReactNode;
};

export const EnvironmentDropdownMenu: React.FC<
EnvironmentDropdownMenuProps
> = ({ environment, children }) => {
const [open, setOpen] = useState(false);

return (
<DropdownMenu open={open} onOpenChange={setOpen}>
<DropdownMenuTrigger asChild>{children}</DropdownMenuTrigger>
<DropdownMenuContent>
<DeleteEnvironmentDialog
environment={environment}
onClose={() => setOpen(false)}
>
<DropdownMenuItem
onSelect={(e) => e.preventDefault()}
className="flex cursor-pointer items-center gap-2"
>
<IconTrash className="h-4 w-4 text-red-500" />
<span>Delete</span>
</DropdownMenuItem>
</DeleteEnvironmentDialog>
</DropdownMenuContent>
</DropdownMenu>
);
};
Loading
Loading