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

Jump to bottom

fix: Variable ui cleanup #203

Merged
merged 1 commit into from
Nov 7, 2024
Merged

fix: Variable ui cleanup #203

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
19 changes: 14 additions & 5 deletions ...rvice/src/app/[workspaceSlug]/systems/[systemSlug]/_components/variables/ConfigFields.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,16 +34,25 @@ import { TargetConditionDialog } from "~/app/[workspaceSlug]/_components/target-
export const ConfigTypeSelector: React.FC<{
value: string | undefined;
onChange: (type: string) => void;
}> = ({ value, onChange }) => (
exclude?: string[];
}> = ({ value, onChange, exclude }) => (
<Select value={value} onValueChange={onChange}>
<SelectTrigger>
<SelectValue placeholder="Select type" />
</SelectTrigger>
<SelectContent>
<SelectItem value="string">String</SelectItem>
<SelectItem value="number">Number</SelectItem>
<SelectItem value="boolean">Boolean</SelectItem>
<SelectItem value="choice">Choice</SelectItem>
{!exclude?.includes("string") && (
<SelectItem value="string">String</SelectItem>
)}
{!exclude?.includes("number") && (
<SelectItem value="number">Number</SelectItem>
)}
{!exclude?.includes("boolean") && (
<SelectItem value="boolean">Boolean</SelectItem>
)}
{!exclude?.includes("choice") && (
<SelectItem value="choice">Choice</SelectItem>
)}
</SelectContent>
</Select>
);
Expand Down
24 changes: 2 additions & 22 deletions ...Slug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/CreateVariableDialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
import { useState } from "react";
import { useRouter } from "next/navigation";
import { IconBulb } from "@tabler/icons-react";
import _ from "lodash";
import { z } from "zod";

import { Alert, AlertTitle } from "@ctrlplane/ui/alert";
Expand Down Expand Up @@ -32,7 +31,6 @@ import { VariableConfig } from "@ctrlplane/validators/variables";

import {
BooleanConfigFields,
ChoiceConfigFields,
ConfigTypeSelector,
NumberConfigFields,
StringConfigFields,
Expand Down Expand Up @@ -60,8 +58,6 @@ export const CreateVariableDialog: React.FC<{
},
});

const { config } = form.watch();

const onSubmit = form.handleSubmit(async (values) => {
await create.mutateAsync({
...values,
Expand Down Expand Up @@ -118,13 +114,8 @@ export const CreateVariableDialog: React.FC<{
<FormControl>
<ConfigTypeSelector
value={value.type}
onChange={(type: string) => {
if (type === "choice") {
onChange({ type, options: [] });
return;
}
onChange({ type });
}}
onChange={(type: string) => onChange({ type })}
exclude={["choice"]}
/>
</FormControl>
</FormItem>
Expand Down Expand Up @@ -155,21 +146,10 @@ export const CreateVariableDialog: React.FC<{
}
/>
)}

{value.type === "choice" && (
<ChoiceConfigFields
config={value}
updateConfig={(updates) =>
onChange({ ...value, ...updates })
}
/>
)}
</>
)}
/>

<pre className="text-xs">{JSON.stringify(config, null, 2)}</pre>

<FormField
control={form.control}
name="description"
Expand Down
198 changes: 198 additions & 0 deletions apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/runbooks/EditRunbookDialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,198 @@
"use client";

import type {
JobAgent,
Runbook,
RunbookVariable,
Workspace,
} from "@ctrlplane/db/schema";
import { useState } from "react";
import { useRouter } from "next/navigation";
import { z } from "zod";

import { createRunbookVariable } from "@ctrlplane/db/schema";
import { Button } from "@ctrlplane/ui/button";
import {
Dialog,
DialogContent,
DialogHeader,
DialogTitle,
DialogTrigger,
} from "@ctrlplane/ui/dialog";
import {
Form,
FormControl,
FormField,
FormItem,
FormLabel,
FormMessage,
FormRootError,
useForm,
} from "@ctrlplane/ui/form";
import { Input } from "@ctrlplane/ui/input";
import { Textarea } from "@ctrlplane/ui/textarea";

import { JobAgentConfig } from "~/components/form/job-agent/JobAgentConfig";
import { JobAgentSelector } from "~/components/form/job-agent/JobAgentSelector";
import { api } from "~/trpc/react";
import { RunbookVariablesEditor } from "./create/RunbookVariableEditor";

const updateRunbookSchema = z.object({
name: z.string().min(1),
description: z.string(),
variables: z.array(createRunbookVariable),
jobAgentId: z.string().uuid({ message: "Must be a valid job agent ID" }),
jobAgentConfig: z.record(z.any()),
});

export const EditRunbookDialog: React.FC<{
workspace: Workspace;
jobAgents: JobAgent[];
runbook: Runbook & { variables: RunbookVariable[] };
children: React.ReactNode;
}> = ({ workspace, jobAgents, runbook, children }) => {
const [open, setOpen] = useState(false);
const update = api.runbook.update.useMutation();
const form = useForm({
schema: updateRunbookSchema,
disabled: update.isPending,
defaultValues: {
...runbook,
description: runbook.description ?? "",
jobAgentId: runbook.jobAgentId ?? "",
},
Comment on lines +59 to +63
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

Avoid spreading runbook into defaultValues

Spreading ...runbook into defaultValues may include unexpected fields that are not defined in the form schema, potentially causing validation issues or unintended behavior. It's better to explicitly set only the fields defined in the schema.

Apply this diff to only include necessary fields:

 defaultValues: {
-  ...runbook,
-  description: runbook.description ?? "",
-  jobAgentId: runbook.jobAgentId ?? "",
+  name: runbook.name,
+  description: runbook.description ?? "",
+  variables: runbook.variables,
+  jobAgentId: runbook.jobAgentId ?? "",
+  jobAgentConfig: runbook.jobAgentConfig ?? {},
 },
📝 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
defaultValues: {
...runbook,
description: runbook.description ?? "",
jobAgentId: runbook.jobAgentId ?? "",
},
defaultValues: {
name: runbook.name,
description: runbook.description ?? "",
variables: runbook.variables,
jobAgentId: runbook.jobAgentId ?? "",
jobAgentConfig: runbook.jobAgentConfig ?? {},
},

});

const router = useRouter();
const onSubmit = form.handleSubmit(async (data) =>
update
.mutateAsync({ id: runbook.id, data })
.then(() => router.refresh())
.then(() => setOpen(false)),
);
Comment on lines +68 to +72
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

Handle errors in form submission

The onSubmit handler does not handle errors from the mutation or router refresh. This could lead to unhandled promise rejections and a poor user experience if the update fails.

Apply this diff to handle errors using try...catch:

-const onSubmit = form.handleSubmit(async (data) =>
-  update
-    .mutateAsync({ id: runbook.id, data })
-    .then(() => router.refresh())
-    .then(() => setOpen(false)),
-);
+const onSubmit = form.handleSubmit(async (data) => {
+  try {
+    await update.mutateAsync({ id: runbook.id, data });
+    router.refresh();
+    setOpen(false);
+  } catch (error) {
+    // Handle error, e.g., display error message to the user
+    form.setError("root", { message: "Failed to update runbook." });
+  }
+});
📝 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
update
.mutateAsync({ id: runbook.id, data })
.then(() => router.refresh())
.then(() => setOpen(false)),
);
const onSubmit = form.handleSubmit(async (data) => {
try {
await update.mutateAsync({ id: runbook.id, data });
router.refresh();
setOpen(false);
} catch (error) {
// Handle error, e.g., display error message to the user
form.setError("root", { message: "Failed to update runbook." });
}
});


const jobAgentId = form.watch("jobAgentId");
const jobAgent = jobAgents.find((j) => j.id === jobAgentId);
return (
<Dialog open={open} onOpenChange={setOpen}>
<DialogTrigger asChild>{children}</DialogTrigger>
<DialogContent className="scrollbar-thin scrollbar-track-neutral-900 scrollbar-thumb-neutral-800 max-h-[95vh] max-w-3xl overflow-y-auto">
<DialogHeader>
<DialogTitle>Edit Runbook</DialogTitle>
</DialogHeader>
<Form {...form}>
<form onSubmit={onSubmit} className="space-y-8">
<div className="space-y-3">
<div>General</div>
<FormField
control={form.control}
name="name"
render={({ field }) => (
<FormItem>
<FormLabel>Name</FormLabel>
<FormControl>
<Input
placeholder="Deploy Hotfix, Rollback Release, Scale Service..."
{...field}
/>
</FormControl>
<FormMessage />
</FormItem>
)}
/>
<FormField
control={form.control}
name="description"
render={({ field }) => (
<FormItem>
<FormLabel>Description</FormLabel>
<FormControl>
<Textarea
placeholder="Describe the purpose of this runbook..."
{...field}
/>
</FormControl>
<FormMessage />
</FormItem>
)}
/>
</div>

<div className="space-y-3">
<div>Variables</div>

<div className="text-sm text-muted-foreground">
Variables in runbooks make automation flexible and reusable.
They let you customize runbooks with user inputs and use
environment-specific values without hardcoding. This allows
runbooks to adapt to different scenarios without changing their
core logic.
</div>

<FormField
control={form.control}
name="variables"
render={({ field }) => (
<FormItem>
<FormControl>
<RunbookVariablesEditor
value={field.value}
onChange={field.onChange}
/>
</FormControl>
<FormMessage />
</FormItem>
)}
/>
</div>

<div className="space-y-3">
<div>Agent</div>

<FormField
control={form.control}
name="jobAgentId"
render={({ field: { value, onChange } }) => (
<FormItem>
<FormLabel>Job Agent</FormLabel>
<FormControl>
<JobAgentSelector
jobAgents={jobAgents}
workspace={workspace}
value={value}
onChange={onChange}
/>
</FormControl>
<FormMessage />
</FormItem>
)}
/>

<FormField
control={form.control}
name="jobAgentConfig"
render={({ field: { value, onChange } }) => (
<FormItem>
<FormLabel>Config</FormLabel>
<FormControl>
<JobAgentConfig
jobAgent={jobAgent}
workspace={workspace}
value={value}
onChange={onChange}
/>
</FormControl>
<FormMessage />
</FormItem>
)}
/>
</div>

<Button type="submit">Save</Button>
<FormRootError />
</form>
</Form>
</DialogContent>
</Dialog>
);
};
46 changes: 19 additions & 27 deletions apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/runbooks/RunbookRow.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

import type * as schema from "@ctrlplane/db/schema";
import { useState } from "react";
import { IconDotsVertical } from "@tabler/icons-react";
import { IconBolt, IconDotsVertical, IconEdit } from "@tabler/icons-react";

import { Button } from "@ctrlplane/ui/button";
import {
Expand All @@ -12,8 +12,7 @@ import {
DropdownMenuTrigger,
} from "@ctrlplane/ui/dropdown-menu";

import { api } from "~/trpc/react";
import { EditAgentConfigDialog } from "../_components/EditAgentConfigDialog";
import { EditRunbookDialog } from "./EditRunbookDialog";
import { TriggerRunbookDialog } from "./TriggerRunbook";

export const RunbookRow: React.FC<{
Expand All @@ -24,7 +23,6 @@ export const RunbookRow: React.FC<{
workspace: schema.Workspace;
jobAgents: schema.JobAgent[];
}> = ({ runbook, workspace, jobAgents }) => {
const updateRunbook = api.runbook.update.useMutation();
const [open, setOpen] = useState(false);

return (
Expand All @@ -45,33 +43,27 @@ export const RunbookRow: React.FC<{
runbook={runbook}
onSuccess={() => setOpen(false)}
>
<DropdownMenuItem onSelect={(e) => e.preventDefault()}>
<DropdownMenuItem
onSelect={(e) => e.preventDefault()}
className="flex cursor-pointer items-center gap-2"
>
<IconBolt className="h-4 w-4" />
Trigger Runbook
</DropdownMenuItem>
</TriggerRunbookDialog>
{runbook.jobAgent != null && (
<EditAgentConfigDialog
jobAgent={runbook.jobAgent}
workspace={workspace}
jobAgents={jobAgents}
value={runbook.jobAgentConfig}
onSubmit={(data) =>
updateRunbook
.mutateAsync({
id: runbook.id,
data: {
jobAgentId: data.jobAgentId,
jobAgentConfig: data.config,
},
})
.then(() => setOpen(false))
}
<EditRunbookDialog
runbook={runbook}
workspace={workspace}
jobAgents={jobAgents}
>
<DropdownMenuItem
onSelect={(e) => e.preventDefault()}
className="flex cursor-pointer items-center gap-2"
>
<DropdownMenuItem onSelect={(e) => e.preventDefault()}>
Edit Job Agent
</DropdownMenuItem>
</EditAgentConfigDialog>
)}
<IconEdit className="h-4 w-4" />
Edit Runbook
</DropdownMenuItem>
</EditRunbookDialog>
</DropdownMenuContent>
</DropdownMenu>
</div>
Expand Down
Loading
Loading