Skip to content

chore: clean up edit policy ui #546

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

Merged
merged 2 commits into from
May 4, 2025
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 @@ -5,6 +5,7 @@ import {
isComparisonCondition,
isCreatedAtCondition,
isMetadataCondition,
isTagCondition,
isVersionCondition,
} from "@ctrlplane/validators/releases";

Expand Down Expand Up @@ -54,7 +55,7 @@ export const DeploymentVersionConditionRender: React.FC<
/>
);

if (isVersionCondition(condition))
if (isVersionCondition(condition) || isTagCondition(condition))
return (
<DeploymentVersionTagConditionRender
condition={condition}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,14 @@ import type {
ColumnOperatorType,
VersionCondition,
} from "@ctrlplane/validators/conditions";
import type { TagCondition } from "@ctrlplane/validators/releases";
import React from "react";

import type { DeploymentVersionConditionRenderProps } from "./deployment-version-condition-props";
import { ColumnConditionRender } from "~/app/[workspaceSlug]/(app)/_components/filter/ColumnConditionRender";

export const DeploymentVersionTagConditionRender: React.FC<
DeploymentVersionConditionRenderProps<VersionCondition>
DeploymentVersionConditionRenderProps<VersionCondition | TagCondition>
> = ({ condition, onChange, className }) => {
const setOperator = (operator: ColumnOperatorType) =>
onChange({ ...condition, operator });
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,20 @@ export const PolicyTabs: React.FC = () => {

const editUrls = policyUrls.edit(policyId);
const baseUrl = policyUrls.byId(policyId);
const editBaseUrl = editUrls.baseUrl();

const configurationUrl = editUrls.configuration();
const timeWindowsUrl = editUrls.timeWindows();
const deploymentFlowUrl = editUrls.deploymentFlow();
const qualitySecurityUrl = editUrls.qualitySecurity();

const pathname = usePathname();

const getInitialTab = () => {
if (pathname === baseUrl) return "overview";
if (pathname.startsWith(editBaseUrl)) return "edit";
if (pathname === configurationUrl) return "configuration";
if (pathname === timeWindowsUrl) return "time-windows";
if (pathname === deploymentFlowUrl) return "deployment-flow";
if (pathname === qualitySecurityUrl) return "quality-security";
return "overview";
};

Expand All @@ -35,30 +42,33 @@ export const PolicyTabs: React.FC = () => {

const onTabChange = (value: string) => {
if (value === "overview") router.push(baseUrl);
if (value === "edit") router.push(editBaseUrl);
if (value === "time-windows") router.push(editUrls.timeWindows());
if (value === "version-conditions") router.push(editUrls.qualitySecurity());
if (value === "approval-gates") router.push(policyUrls.approvalGates());
if (value === "configuration") router.push(configurationUrl);
if (value === "time-windows") router.push(timeWindowsUrl);
if (value === "deployment-flow") router.push(deploymentFlowUrl);
if (value === "quality-security") router.push(qualitySecurityUrl);
setActiveTab(value);
};

return (
<Tabs value={activeTab} onValueChange={onTabChange} className="w-full">
<TabsList className="mb-4">
<TabsTrigger value="overview">Overview</TabsTrigger>
<TabsTrigger value="edit">Edit</TabsTrigger>
<TabsTrigger value="configuration">Edit</TabsTrigger>
<TabsTrigger value="time-windows" className="flex items-center gap-1">
<IconClock className="h-4 w-4" />
Time Windows
</TabsTrigger>
<TabsTrigger
value="version-conditions"
value="deployment-flow"
className="flex items-center gap-1"
>
<IconTag className="h-4 w-4" />
Version Conditions
</TabsTrigger>
<TabsTrigger value="approval-gates" className="flex items-center gap-1">
<TabsTrigger
value="quality-security"
className="flex items-center gap-1"
>
<IconCircleCheck className="h-4 w-4" />
Approval Gates
</TabsTrigger>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
"use client";

import type { UpdatePolicy } from "@ctrlplane/db/schema";
import type React from "react";
import type { UseFormReturn } from "react-hook-form";
import { createContext, useContext } from "react";
import { useRouter } from "next/navigation";

import * as SCHEMA from "@ctrlplane/db/schema";
import { Form, useForm } from "@ctrlplane/ui/form";
import { toast } from "@ctrlplane/ui/toast";

import { api } from "~/trpc/react";
import {
convertEmptySelectorsToNull,
isValidTarget,
} from "../../../_utils/policy-targets";

type Policy = SCHEMA.Policy & {
targets: SCHEMA.PolicyTarget[];
denyWindows: SCHEMA.PolicyRuleDenyWindow[];
deploymentVersionSelector: SCHEMA.PolicyDeploymentVersionSelector | null;
versionAnyApprovals: SCHEMA.PolicyRuleAnyApproval | null;
versionUserApprovals: SCHEMA.PolicyRuleUserApproval[];
versionRoleApprovals: SCHEMA.PolicyRuleRoleApproval[];
};

type PolicyFormContextType = {
form: UseFormReturn<UpdatePolicy>;
policy: Policy;
};

const PolicyFormContext = createContext<PolicyFormContextType | null>(null);

export function usePolicyFormContext() {
const ctx = useContext(PolicyFormContext);
if (ctx == null)
throw new Error(
"usePolicyFormContext must be used within a PolicyFormContext.Provider",
);
return ctx;
}

export const PolicyFormContextProvider: React.FC<{
children: React.ReactNode;
policy: Policy;
}> = ({ children, policy }) => {
console.log(policy);

const form = useForm({
schema: SCHEMA.updatePolicy,
defaultValues: policy,
});

console.log(form.getValues());

const router = useRouter();
const utils = api.useUtils();

const updatePolicy = api.policy.update.useMutation();

const onSubmit = form.handleSubmit((data) => {
const targets =
data.targets === undefined
? undefined
: data.targets.map(convertEmptySelectorsToNull);

const isTargetsValid =
targets === undefined || targets.every(isValidTarget);
if (!isTargetsValid) {
const errorStr = "One or more of your targets are invalid";
form.setError("targets", { message: errorStr });
toast.error("Error creating policy", { description: errorStr });
return;
}

return updatePolicy
.mutateAsync({
id: policy.id,
data: { ...data, targets },
})
.then(() => {
toast.success("Policy updated successfully");
form.reset(data);
router.refresh();
utils.policy.byId.invalidate();
utils.policy.list.invalidate();
})
.catch((error) => {
toast.error("Failed to update policy", {
description: error.message,
});
});
});
Comment on lines +62 to +94
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

Prefer async/await and invalidate queries with parameters

  1. form.handleSubmit already supports async handlers – using await improves readability and ensures thrown errors are caught by the wrapper.
  2. utils.policy.byId.invalidate() without { id } may not hit the correct cache entry.
-const onSubmit = form.handleSubmit((data) => {
+const onSubmit = form.handleSubmit(async (data) => {
   ...
-  return updatePolicy
-    .mutateAsync({ id: policy.id, data: { ...data, targets } })
-    .then(() => {
+  try {
+    await updatePolicy.mutateAsync({ id: policy.id, data: { ...data, targets } });
     toast.success("Policy updated successfully");
     form.reset(data);
     router.refresh();
-    utils.policy.byId.invalidate();
+    utils.policy.byId.invalidate({ id: policy.id });
     utils.policy.list.invalidate();
-  })
-  .catch((error) => {
+  } catch (error: any) {
     toast.error("Failed to update policy", { description: error.message });
-  });
+  }
 });

This yields clearer flow control and guarantees the right cache key is invalidated.

📝 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 onSubmit = form.handleSubmit((data) => {
const targets =
data.targets === undefined
? undefined
: data.targets.map(convertEmptySelectorsToNull);
const isTargetsValid =
targets === undefined || targets.every(isValidTarget);
if (!isTargetsValid) {
const errorStr = "One or more of your targets are invalid";
form.setError("targets", { message: errorStr });
toast.error("Error creating policy", { description: errorStr });
return;
}
return updatePolicy
.mutateAsync({
id: policy.id,
data: { ...data, targets },
})
.then(() => {
toast.success("Policy updated successfully");
form.reset(data);
router.refresh();
utils.policy.byId.invalidate();
utils.policy.list.invalidate();
})
.catch((error) => {
toast.error("Failed to update policy", {
description: error.message,
});
});
});
const onSubmit = form.handleSubmit(async (data) => {
const targets =
data.targets === undefined
? undefined
: data.targets.map(convertEmptySelectorsToNull);
const isTargetsValid =
targets === undefined || targets.every(isValidTarget);
if (!isTargetsValid) {
const errorStr = "One or more of your targets are invalid";
form.setError("targets", { message: errorStr });
toast.error("Error creating policy", { description: errorStr });
return;
}
try {
await updatePolicy.mutateAsync({
id: policy.id,
data: { ...data, targets },
});
toast.success("Policy updated successfully");
form.reset(data);
router.refresh();
utils.policy.byId.invalidate({ id: policy.id });
utils.policy.list.invalidate();
} catch (error: any) {
toast.error("Failed to update policy", {
description: error.message,
});
}
});


return (
<PolicyFormContext.Provider value={{ form, policy }}>
<Form {...form}>
<form onSubmit={onSubmit}>{children}</form>
</Form>
</PolicyFormContext.Provider>
);
};
Loading
Loading