Skip to content

fix: Release channel ui cleanup #188

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

Merged
merged 1 commit into from
Nov 1, 2024
Merged

fix: Release channel ui cleanup #188

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
70 changes: 50 additions & 20 deletions apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/EnvironmentDrawer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
"use client";

import React, { useState } from "react";
import React from "react";
import { useParams, useRouter, useSearchParams } from "next/navigation";
import {
IconDotsVertical,
Expand All @@ -21,11 +21,38 @@ import { EditFilterForm } from "./Filter";
import { Overview } from "./Overview";
import { ReleaseChannels } from "./ReleaseChannels";

export enum EnvironmentDrawerTab {
Overview = "overview",
Targets = "targets",
ReleaseChannels = "release-channels",
}

const tabParam = "tab";
const useEnvironmentDrawerTab = () => {
const router = useRouter();
const params = useSearchParams();
const tab = params.get(tabParam);

const setTab = (tab: EnvironmentDrawerTab | null) => {
const url = new URL(window.location.href);
if (tab === null) {
url.searchParams.delete(tabParam);
router.replace(`${url.pathname}?${url.searchParams.toString()}`);
return;
}
url.searchParams.set(tabParam, tab);
router.replace(`${url.pathname}?${url.searchParams.toString()}`);
};

return { tab, setTab };
};
Comment on lines +31 to +48
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 1, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider adding validation for tab parameter values.

The hook handles tab state well, but it might benefit from validating that the tab value from URL params matches the EnvironmentDrawerTab enum values.

Consider this improvement:

 const useEnvironmentDrawerTab = () => {
   const router = useRouter();
   const params = useSearchParams();
   const tab = params.get(tabParam);
+  const isValidTab = (value: string | null): value is EnvironmentDrawerTab => {
+    return value != null && Object.values(EnvironmentDrawerTab).includes(value as EnvironmentDrawerTab);
+  };
+  const validTab = isValidTab(tab) ? tab : null;

   const setTab = (tab: EnvironmentDrawerTab | null) => {
     // ... existing code ...
   };

-  return { tab, setTab };
+  return { tab: validTab, setTab };
 };
📝 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 useEnvironmentDrawerTab = () => {
const router = useRouter();
const params = useSearchParams();
const tab = params.get(tabParam);
const setTab = (tab: EnvironmentDrawerTab | null) => {
const url = new URL(window.location.href);
if (tab === null) {
url.searchParams.delete(tabParam);
router.replace(`${url.pathname}?${url.searchParams.toString()}`);
return;
}
url.searchParams.set(tabParam, tab);
router.replace(`${url.pathname}?${url.searchParams.toString()}`);
};
return { tab, setTab };
};
const useEnvironmentDrawerTab = () => {
const router = useRouter();
const params = useSearchParams();
const tab = params.get(tabParam);
const isValidTab = (value: string | null): value is EnvironmentDrawerTab => {
return value != null && Object.values(EnvironmentDrawerTab).includes(value as EnvironmentDrawerTab);
};
const validTab = isValidTab(tab) ? tab : null;
const setTab = (tab: EnvironmentDrawerTab | null) => {
const url = new URL(window.location.href);
if (tab === null) {
url.searchParams.delete(tabParam);
router.replace(`${url.pathname}?${url.searchParams.toString()}`);
return;
}
url.searchParams.set(tabParam, tab);
router.replace(`${url.pathname}?${url.searchParams.toString()}`);
};
return { tab: validTab, setTab };
};


const param = "environment_id";
export const useEnvironmentDrawer = () => {
const router = useRouter();
const params = useSearchParams();
const environmentId = params.get(param);
const { tab, setTab } = useEnvironmentDrawerTab();

const setEnvironmentId = (id: string | null) => {
const url = new URL(window.location.href);
Expand All @@ -38,13 +65,17 @@ export const useEnvironmentDrawer = () => {
router.replace(`${url.pathname}?${url.searchParams.toString()}`);
};

const removeEnvironmentId = () => setEnvironmentId(null);
const removeEnvironmentId = () => {
setTab(null);
setEnvironmentId(null);
};

return { environmentId, setEnvironmentId, removeEnvironmentId };
return { environmentId, setEnvironmentId, removeEnvironmentId, tab, setTab };
};

export const EnvironmentDrawer: React.FC = () => {
const { environmentId, removeEnvironmentId } = useEnvironmentDrawer();
const { environmentId, removeEnvironmentId, tab, setTab } =
useEnvironmentDrawer();
const isOpen = Boolean(environmentId);
const setIsOpen = removeEnvironmentId;
const environmentQ = api.environment.byId.useQuery(environmentId ?? "", {
Expand All @@ -65,8 +96,6 @@ export const EnvironmentDrawer: React.FC = () => {
);
const deployments = deploymentsQ.data;

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

const loading =
environmentQ.isLoading || workspaceQ.isLoading || deploymentsQ.isLoading;

Expand Down Expand Up @@ -107,42 +136,43 @@ export const EnvironmentDrawer: React.FC = () => {
<div className="flex w-full gap-6 p-6">
<div className="space-y-1">
<TabButton
active={activeTab === "overview"}
onClick={() => setActiveTab("overview")}
active={tab === EnvironmentDrawerTab.Overview || tab == null}
onClick={() => setTab(EnvironmentDrawerTab.Overview)}
icon={<IconInfoCircle className="h-4 w-4" />}
label="Overview"
/>
<TabButton
active={activeTab === "targets"}
onClick={() => setActiveTab("targets")}
active={tab === EnvironmentDrawerTab.Targets}
onClick={() => setTab(EnvironmentDrawerTab.Targets)}
icon={<IconTarget className="h-4 w-4" />}
label="Targets"
/>
<TabButton
active={activeTab === "release-channels"}
onClick={() => setActiveTab("release-channels")}
active={tab === EnvironmentDrawerTab.ReleaseChannels}
onClick={() => setTab(EnvironmentDrawerTab.ReleaseChannels)}
icon={<IconFilter className="h-4 w-4" />}
label="Release Channels"
Comment on lines +139 to 154
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider extracting tab configuration to reduce repetition.

The tab button definitions follow a repetitive pattern that could be simplified using a configuration object.

Consider this improvement:

const TAB_CONFIG = [
  {
    tab: EnvironmentDrawerTab.Overview,
    icon: <IconInfoCircle className="h-4 w-4" />,
    label: "Overview",
    isDefault: true
  },
  {
    tab: EnvironmentDrawerTab.Targets,
    icon: <IconTarget className="h-4 w-4" />,
    label: "Targets"
  },
  {
    tab: EnvironmentDrawerTab.ReleaseChannels,
    icon: <IconFilter className="h-4 w-4" />,
    label: "Release Channels"
  }
] as const;

// In JSX:
{TAB_CONFIG.map(({ tab, icon, label, isDefault }) => (
  <TabButton
    key={tab}
    active={currentTab === tab || (isDefault && currentTab == null)}
    onClick={() => setTab(tab)}
    icon={icon}
    label={label}
  />
))}

Copy link
Contributor Author

@adityachoudhari26 adityachoudhari26 Nov 1, 2024

Choose a reason for hiding this comment

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

eh imo this isn't really needed, its only 3

Copy link
Contributor

@coderabbitai coderabbitai bot Nov 1, 2024

Choose a reason for hiding this comment

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

@adityachoudhari26, understood. We'll proceed without the suggested refactor.

✏️ Learnings added 
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#188
File: apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/EnvironmentDrawer.tsx:139-154
Timestamp: 2024-11-01T02:36:22.750Z
Learning: In `EnvironmentDrawer.tsx`, for the `EnvironmentDrawer` component, when there are only a few `TabButton` instances (e.g., 3), it's acceptable to define them individually without extracting them into a configuration to reduce repetition.

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.

/>
</div>

{environment != null && (
<div className="w-full overflow-auto">
{activeTab === "overview" && (
{(tab === EnvironmentDrawerTab.Overview || tab == null) && (
<Overview environment={environment} />
)}
{activeTab === "targets" && workspace != null && (
{tab === EnvironmentDrawerTab.Targets && workspace != null && (
<EditFilterForm
environment={environment}
workspaceId={workspace.id}
/>
)}
{activeTab === "release-channels" && deployments != null && (
<ReleaseChannels
environment={environment}
deployments={deployments}
/>
)}
{tab === EnvironmentDrawerTab.ReleaseChannels &&
deployments != null && (
<ReleaseChannels
environment={environment}
deployments={deployments}
/>
)}
</div>
)}
</div>
Expand Down
89 changes: 65 additions & 24 deletions ...src/app/[workspaceSlug]/_components/environment-policy-drawer/EnvironmentPolicyDrawer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

import type * as SCHEMA from "@ctrlplane/db/schema";
import type React from "react";
import { useState } from "react";
import { useRouter, useSearchParams } from "next/navigation";
import {
IconCalendar,
Expand Down Expand Up @@ -34,11 +33,41 @@ import { ReleaseChannels } from "./ReleaseChannels";
import { ReleaseManagement } from "./ReleaseManagement";
import { RolloutAndTiming } from "./RolloutAndTiming";

export enum EnvironmentPolicyDrawerTab {
Overview = "overview",
Approval = "approval",
Concurrency = "concurrency",
Management = "management",
ReleaseChannels = "release-channels",
Rollout = "rollout",
}

const tabParam = "tab";
const useEnvironmentPolicyDrawerTab = () => {
const router = useRouter();
const params = useSearchParams();
const tab = params.get(tabParam) as EnvironmentPolicyDrawerTab | null;

const setTab = (tab: EnvironmentPolicyDrawerTab | null) => {
const url = new URL(window.location.href);
if (tab === null) {
url.searchParams.delete(tabParam);
router.replace(`${url.pathname}?${url.searchParams.toString()}`);
return;
}
url.searchParams.set(tabParam, tab);
router.replace(`${url.pathname}?${url.searchParams.toString()}`);
};

return { tab, setTab };
};

const param = "environment_policy_id";
export const useEnvironmentPolicyDrawer = () => {
const router = useRouter();
const params = useSearchParams();
const environmentPolicyId = params.get(param);
const { tab, setTab } = useEnvironmentPolicyDrawerTab();

const setEnvironmentPolicyId = (id: string | null) => {
const url = new URL(window.location.href);
Expand All @@ -57,6 +86,8 @@ export const useEnvironmentPolicyDrawer = () => {
environmentPolicyId,
setEnvironmentPolicyId,
removeEnvironmentPolicyId,
tab,
setTab,
};
};

Expand All @@ -65,20 +96,30 @@ type Deployment = SCHEMA.Deployment & {
};

const View: React.FC<{
activeTab: string;
activeTab: EnvironmentPolicyDrawerTab;
environmentPolicy: SCHEMA.EnvironmentPolicy & {
releaseWindows: SCHEMA.EnvironmentPolicyReleaseWindow[];
releaseChannels: SCHEMA.ReleaseChannel[];
};
deployments?: Deployment[];
}> = ({ activeTab, environmentPolicy, deployments }) => {
return {
overview: <Overview environmentPolicy={environmentPolicy} />,
approval: <ApprovalAndGovernance environmentPolicy={environmentPolicy} />,
concurrency: <DeploymentControl environmentPolicy={environmentPolicy} />,
management: <ReleaseManagement environmentPolicy={environmentPolicy} />,
rollout: <RolloutAndTiming environmentPolicy={environmentPolicy} />,
"release-channels": deployments != null && (
[EnvironmentPolicyDrawerTab.Overview]: (
<Overview environmentPolicy={environmentPolicy} />
),
[EnvironmentPolicyDrawerTab.Approval]: (
<ApprovalAndGovernance environmentPolicy={environmentPolicy} />
),
[EnvironmentPolicyDrawerTab.Concurrency]: (
<DeploymentControl environmentPolicy={environmentPolicy} />
),
[EnvironmentPolicyDrawerTab.Management]: (
<ReleaseManagement environmentPolicy={environmentPolicy} />
),
[EnvironmentPolicyDrawerTab.Rollout]: (
<RolloutAndTiming environmentPolicy={environmentPolicy} />
),
[EnvironmentPolicyDrawerTab.ReleaseChannels]: deployments != null && (
<ReleaseChannels policy={environmentPolicy} deployments={deployments} />
),
}[activeTab];
Expand All @@ -105,7 +146,7 @@ const PolicyDropdownMenu: React.FC<{
);

export const EnvironmentPolicyDrawer: React.FC = () => {
const { environmentPolicyId, removeEnvironmentPolicyId } =
const { environmentPolicyId, removeEnvironmentPolicyId, tab, setTab } =
useEnvironmentPolicyDrawer();
const isOpen = Boolean(environmentPolicyId);
const setIsOpen = removeEnvironmentPolicyId;
Expand All @@ -121,8 +162,6 @@ export const EnvironmentPolicyDrawer: React.FC = () => {
);
const deployments = deploymentsQ.data;

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

return (
<Drawer open={isOpen} onOpenChange={setIsOpen}>
<DrawerContent
Expand Down Expand Up @@ -150,38 +189,40 @@ export const EnvironmentPolicyDrawer: React.FC = () => {
<div className="flex w-full gap-6 p-6">
<div className="space-y-1">
<TabButton
active={activeTab === "overview"}
onClick={() => setActiveTab("overview")}
active={
tab === EnvironmentPolicyDrawerTab.Overview || tab == null
}
onClick={() => setTab(EnvironmentPolicyDrawerTab.Overview)}
icon={<IconInfoCircle className="h-4 w-4" />}
label="Overview"
/>
<TabButton
active={activeTab === "approval"}
onClick={() => setActiveTab("approval")}
active={tab === EnvironmentPolicyDrawerTab.Approval}
onClick={() => setTab(EnvironmentPolicyDrawerTab.Approval)}
icon={<IconEye className="h-4 w-4" />}
label="Approval & Governance"
/>
<TabButton
active={activeTab === "concurrency"}
onClick={() => setActiveTab("concurrency")}
active={tab === EnvironmentPolicyDrawerTab.Concurrency}
onClick={() => setTab(EnvironmentPolicyDrawerTab.Concurrency)}
icon={<IconCircuitDiode className="h-4 w-4" />}
label="Deployment Control"
/>
<TabButton
active={activeTab === "management"}
onClick={() => setActiveTab("management")}
active={tab === EnvironmentPolicyDrawerTab.Management}
onClick={() => setTab(EnvironmentPolicyDrawerTab.Management)}
icon={<IconRocket className="h-4 w-4" />}
label="Release Management"
/>
<TabButton
active={activeTab === "release-channels"}
onClick={() => setActiveTab("release-channels")}
active={tab === EnvironmentPolicyDrawerTab.ReleaseChannels}
onClick={() => setTab(EnvironmentPolicyDrawerTab.ReleaseChannels)}
icon={<IconFilter className="h-4 w-4" />}
label="Release Channels"
/>
<TabButton
active={activeTab === "rollout"}
onClick={() => setActiveTab("rollout")}
active={tab === EnvironmentPolicyDrawerTab.Rollout}
onClick={() => setTab(EnvironmentPolicyDrawerTab.Rollout)}
icon={<IconCalendar className="h-4 w-4" />}
label="Rollout and Timing"
/>
Expand All @@ -190,7 +231,7 @@ export const EnvironmentPolicyDrawer: React.FC = () => {
{environmentPolicy != null && (
<div className="w-full overflow-auto">
<View
activeTab={activeTab}
activeTab={tab ?? EnvironmentPolicyDrawerTab.Overview}
environmentPolicy={environmentPolicy}
deployments={deployments}
/>
Expand Down
Loading
Loading