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: Init target variable ui #171

Merged
merged 3 commits into from
Oct 25, 2024
Merged

fix: Init target variable ui #171

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c7bc93e
fix: Init target variable ui
adityachoudhari26 Oct 25, 2024
388e746
cleanup
adityachoudhari26 Oct 25, 2024
47f1093
cleanup
adityachoudhari26 Oct 25, 2024
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
219 changes: 219 additions & 0 deletions ...bservice/src/app/[workspaceSlug]/_components/target-drawer/CreateTargetVariableDialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,219 @@
import React, { useState } from "react";
import { z } from "zod";

import { Button } from "@ctrlplane/ui/button";
import { Checkbox } from "@ctrlplane/ui/checkbox";
import {
Dialog,
DialogContent,
DialogFooter,
DialogHeader,
DialogTrigger,
} from "@ctrlplane/ui/dialog";
import {
Form,
FormControl,
FormField,
FormItem,
FormLabel,
FormMessage,
useForm,
} from "@ctrlplane/ui/form";
import { Input } from "@ctrlplane/ui/input";
import {
Select,
SelectContent,
SelectItem,
SelectTrigger,
SelectValue,
} from "@ctrlplane/ui/select";

import { api } from "~/trpc/react";

type CreateTargetVariableDialogProps = {
targetId: string;
existingKeys: string[];
children: React.ReactNode;
};

export const CreateTargetVariableDialog: React.FC<
CreateTargetVariableDialogProps
> = ({ targetId, existingKeys, children }) => {
const [open, setOpen] = useState(false);
const createTargetVariable = api.target.variable.create.useMutation();
const schema = z.object({
key: z
.string()
.refine((k) => k.length > 0, { message: "Key is required" })
.refine((k) => !existingKeys.includes(k), {
message: "Variable key must be unique",
}),
type: z.enum(["string", "number", "boolean"]),
value: z.union([z.string(), z.number(), z.boolean()]).refine((v) => {
if (typeof v === "string") return v.length > 0;
return true;
}),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
value: z.union([z.string(), z.number(), z.boolean()]).refine((v) => {
if (typeof v === "string") return v.length > 0;
return true;
}),
value: z.union([z.string(), z.number(), z.boolean()]).refine((v) =>
typeof v === "string" ? v.length > 0 : true
),

sensitive: z.boolean().default(false),
});
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

Enhance validation constraints.

Consider adding additional validation rules for better data integrity:

  1. Maximum length constraint for string values
  2. Number range validation
  3. Key format validation (e.g., no spaces, valid characters)
 const schema = z.object({
   key: z
     .string()
     .refine((k) => k.length > 0, { message: "Key is required" })
+    .regex(/^[a-zA-Z0-9_-]+$/, { message: "Key can only contain letters, numbers, underscores, and hyphens" })
+    .max(50, { message: "Key must not exceed 50 characters" })
     .refine((k) => !existingKeys.includes(k), {
       message: "Variable key must be unique",
     }),
   type: z.enum(["string", "number", "boolean"]),
-  value: z.union([z.string(), z.number(), z.boolean()]).refine((v) => {
-    if (typeof v === "string") return v.length > 0;
-    return true;
-  }),
+  value: z.union([
+    z.string().min(1).max(1000),
+    z.number().min(-Number.MAX_SAFE_INTEGER).max(Number.MAX_SAFE_INTEGER),
+    z.boolean()
+  ]),
   sensitive: z.boolean().default(false),
 });
📝 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 schema = z.object({
key: z
.string()
.refine((k) => k.length > 0, { message: "Key is required" })
.refine((k) => !existingKeys.includes(k), {
message: "Variable key must be unique",
}),
type: z.enum(["string", "number", "boolean"]),
value: z.union([z.string(), z.number(), z.boolean()]).refine((v) => {
if (typeof v === "string") return v.length > 0;
return true;
}),
sensitive: z.boolean().default(false),
});
const schema = z.object({
key: z
.string()
.refine((k) => k.length > 0, { message: "Key is required" })
.regex(/^[a-zA-Z0-9_-]+$/, { message: "Key can only contain letters, numbers, underscores, and hyphens" })
.max(50, { message: "Key must not exceed 50 characters" })
.refine((k) => !existingKeys.includes(k), {
message: "Variable key must be unique",
}),
type: z.enum(["string", "number", "boolean"]),
value: z.union([
z.string().min(1).max(1000),
z.number().min(-Number.MAX_SAFE_INTEGER).max(Number.MAX_SAFE_INTEGER),
z.boolean()
]),
sensitive: z.boolean().default(false),
});

const form = useForm({
schema,
defaultValues: { key: "", value: "", type: "string" },
});
const { sensitive, type } = form.watch();
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

Add error handling for the mutation.

While the form setup is good, consider adding error handling for the mutation to improve user experience.

 const createTargetVariable = api.target.variable.create.useMutation();
+const createTargetVariable = api.target.variable.create.useMutation({
+  onError: (error) => {
+    // Handle error appropriately, e.g., show toast notification
+    console.error('Failed to create target variable:', error);
+  }
+});

Committable suggestion was skipped due to low confidence.


const utils = api.useUtils();
const onSubmit = form.handleSubmit((data) =>
createTargetVariable
.mutateAsync({ targetId, ...data })
.then(() => utils.target.byId.invalidate(targetId))
.then(() => form.reset()),
);
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

Improve form submission handling.

Consider these enhancements for better user experience:

  1. Reset form on dialog close
  2. Disable form inputs during submission
  3. Close dialog on successful submission
 const onSubmit = form.handleSubmit((data) =>
   createTargetVariable
     .mutateAsync({ targetId, ...data })
     .then(() => utils.target.byId.invalidate(targetId))
-    .then(() => form.reset()),
+    .then(() => {
+      form.reset();
+      setOpen(false);
+    }),
 );

+// Add useEffect for cleanup
+React.useEffect(() => {
+  if (!open) {
+    form.reset();
+  }
+}, [open, form]);

Committable suggestion was skipped due to low confidence.


return (
<Dialog open={open} onOpenChange={setOpen}>
<DialogTrigger asChild>{children}</DialogTrigger>
<DialogContent>
<DialogHeader>Create Target Variable</DialogHeader>
<Form {...form}>
<form onSubmit={onSubmit} className="space-y-4">
<FormField
control={form.control}
name="key"
render={({ field }) => (
<FormItem>
<FormLabel>Key</FormLabel>
<FormControl>
<Input {...field} />
</FormControl>
<FormMessage />
</FormItem>
)}
/>
<FormField
control={form.control}
name="type"
render={({ field: { value, onChange } }) => {
const onTypeChange = (type: string) => {
if (type === "string") form.setValue("value", "");
if (type === "number") form.setValue("value", 0);
if (type === "boolean") form.setValue("value", false);
if (type !== "string") form.setValue("sensitive", false);
onChange(type);
};

return (
<FormItem>
<FormLabel>Type</FormLabel>
<FormControl>
<Select value={value} onValueChange={onTypeChange}>
<SelectTrigger>
<SelectValue placeholder="Variable type..." />
</SelectTrigger>
<SelectContent>
<SelectItem value="string">String</SelectItem>
<SelectItem value="number">Number</SelectItem>
<SelectItem value="boolean">Boolean</SelectItem>
</SelectContent>
</Select>
</FormControl>
<FormMessage />
</FormItem>
);
}}
/>

{type === "string" && (
<FormField
control={form.control}
name="value"
render={({ field: { value, onChange } }) => (
<FormItem>
<FormLabel>Value</FormLabel>
<FormControl>
<Input
value={value as string}
onChange={onChange}
type={sensitive ? "password" : "text"}
/>
</FormControl>
<FormMessage />
</FormItem>
)}
/>
)}

{type === "number" && (
<FormField
control={form.control}
name="value"
render={({ field: { value, onChange } }) => (
<FormItem>
<FormLabel>Value</FormLabel>
<FormControl>
<Input
value={value as number}
onChange={onChange}
type="number"
/>
</FormControl>
</FormItem>
)}
/>
)}

{type === "boolean" && (
<FormField
control={form.control}
name="value"
render={({ field: { value, onChange } }) => (
<FormItem>
<FormLabel>Value</FormLabel>
<FormControl>
<Select
value={value ? "true" : "false"}
onValueChange={(v) => onChange(v === "true")}
>
<SelectTrigger>
<SelectValue placeholder="Value..." />
</SelectTrigger>
<SelectContent>
<SelectItem value="true">True</SelectItem>
<SelectItem value="false">False</SelectItem>
</SelectContent>
</Select>
</FormControl>
</FormItem>
)}
/>
)}
Comment on lines +166 to +190
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

Simplify boolean value selection.

Consider using a Checkbox component instead of a Select for boolean values, as it's more intuitive and accessible.

-<Select
-  value={value ? "true" : "false"}
-  onValueChange={(v) => onChange(v === "true")}
->
-  <SelectTrigger>
-    <SelectValue placeholder="Value..." />
-  </SelectTrigger>
-  <SelectContent>
-    <SelectItem value="true">True</SelectItem>
-    <SelectItem value="false">False</SelectItem>
-  </SelectContent>
-</Select>
+<div className="flex items-center gap-2">
+  <Checkbox
+    checked={value as boolean}
+    onCheckedChange={onChange}
+  />
+  <label className="text-sm">
+    Enable
+  </label>
+</div>
📝 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
{type === "boolean" && (
<FormField
control={form.control}
name="value"
render={({ field: { value, onChange } }) => (
<FormItem>
<FormLabel>Value</FormLabel>
<FormControl>
<Select
value={value ? "true" : "false"}
onValueChange={(v) => onChange(v === "true")}
>
<SelectTrigger>
<SelectValue placeholder="Value..." />
</SelectTrigger>
<SelectContent>
<SelectItem value="true">True</SelectItem>
<SelectItem value="false">False</SelectItem>
</SelectContent>
</Select>
</FormControl>
</FormItem>
)}
/>
)}
{type === "boolean" && (
<FormField
control={form.control}
name="value"
render={({ field: { value, onChange } }) => (
<FormItem>
<FormLabel>Value</FormLabel>
<FormControl>
<div className="flex items-center gap-2">
<Checkbox
checked={value as boolean}
onCheckedChange={onChange}
/>
<label className="text-sm">
Enable
</label>
</div>
</FormControl>
</FormItem>
)}
/>
)}


{type === "string" && (
<FormField
control={form.control}
name="sensitive"
render={({ field: { value, onChange } }) => (
<FormItem>
<FormControl>
<div className="flex items-center gap-2">
<Checkbox checked={value} onCheckedChange={onChange} />
<label htmlFor="sensitive" className="text-sm">
Sensitive
</label>
</div>
</FormControl>
</FormItem>
)}
/>
)}

<DialogFooter>
<Button type="submit" disabled={createTargetVariable.isPending}>
Create
</Button>
</DialogFooter>
</form>
</Form>
</DialogContent>
</Dialog>
);
Comment on lines +74 to +220
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

Enhance accessibility and user experience.

Consider these accessibility improvements:

  1. Add aria-label to the dialog
  2. Add help text for sensitive fields
  3. Add loading states to inputs during submission
-<Dialog open={open} onOpenChange={setOpen}>
+<Dialog 
+  open={open} 
+  onOpenChange={setOpen}
+  aria-label="Create Target Variable Dialog"
+>

Also, consider adding a description for the sensitive checkbox:

 <div className="flex items-center gap-2">
   <Checkbox checked={value} onCheckedChange={onChange} />
   <label htmlFor="sensitive" className="text-sm">
     Sensitive
   </label>
+  <p className="text-sm text-muted-foreground">
+    Mark this variable as sensitive to mask its value in the UI
+  </p>
 </div>

Committable suggestion was skipped due to low confidence.

};
66 changes: 66 additions & 0 deletions ...bservice/src/app/[workspaceSlug]/_components/target-drawer/DeleteTargetVariableDialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
import React, { useState } from "react";

import {
AlertDialog,
AlertDialogAction,
AlertDialogCancel,
AlertDialogContent,
AlertDialogDescription,
AlertDialogFooter,
AlertDialogHeader,
AlertDialogTrigger,
} from "@ctrlplane/ui/alert-dialog";
import { buttonVariants } from "@ctrlplane/ui/button";

import { api } from "~/trpc/react";

type DeleteTargetVariableDialogProps = {
variableId: string;
targetId: string;
onClose: () => void;
children: React.ReactNode;
};

export const DeleteTargetVariableDialog: React.FC<
DeleteTargetVariableDialogProps
> = ({ variableId, targetId, onClose, children }) => {
const [open, setOpen] = useState(false);
const deleteTargetVariable = api.target.variable.delete.useMutation();
const utils = api.useUtils();

Comment on lines +24 to +30
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 adding loading state management.

The component could benefit from handling loading states during the deletion process to prevent multiple clicks and provide better user feedback.

 export const DeleteTargetVariableDialog: React.FC<
   DeleteTargetVariableDialogProps
 > = ({ variableId, targetId, onClose, children }) => {
   const [open, setOpen] = useState(false);
+  const [isDeleting, setIsDeleting] = useState(false);
   const deleteTargetVariable = api.target.variable.delete.useMutation();
   const utils = api.useUtils();
📝 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
export const DeleteTargetVariableDialog: React.FC<
DeleteTargetVariableDialogProps
> = ({ variableId, targetId, onClose, children }) => {
const [open, setOpen] = useState(false);
const deleteTargetVariable = api.target.variable.delete.useMutation();
const utils = api.useUtils();
export const DeleteTargetVariableDialog: React.FC<
DeleteTargetVariableDialogProps
> = ({ variableId, targetId, onClose, children }) => {
const [open, setOpen] = useState(false);
const [isDeleting, setIsDeleting] = useState(false);
const deleteTargetVariable = api.target.variable.delete.useMutation();
const utils = api.useUtils();

const onDelete = () =>
deleteTargetVariable
.mutateAsync(variableId)
.then(() => utils.target.byId.invalidate(targetId))
.then(() => setOpen(false));
Comment on lines +31 to +35
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

Enhance error handling and user feedback in delete operation.

The current implementation lacks error handling and user feedback. Consider implementing a more robust deletion flow.

-  const onDelete = () =>
-    deleteTargetVariable
-      .mutateAsync(variableId)
-      .then(() => utils.target.byId.invalidate(targetId))
-      .then(() => setOpen(false));
+  const onDelete = async () => {
+    try {
+      setIsDeleting(true);
+      await deleteTargetVariable.mutateAsync(variableId);
+      await utils.target.byId.invalidate(targetId);
+      setOpen(false);
+      // Consider adding a toast notification for success
+    } catch (error) {
+      // Consider adding error handling UI (e.g., toast notification)
+      console.error('Failed to delete target variable:', error);
+    } finally {
+      setIsDeleting(false);
+    }
+  };

Committable suggestion was skipped due to low confidence.


return (
<AlertDialog
open={open}
onOpenChange={(o) => {
setOpen(o);
if (!o) onClose();
}}
>
<AlertDialogTrigger asChild>{children}</AlertDialogTrigger>
<AlertDialogContent>
<AlertDialogHeader>Are you sure?</AlertDialogHeader>
<AlertDialogDescription>
Deleting a target variable can change what values are passed to
pipelines running for this target.
</AlertDialogDescription>

<AlertDialogFooter>
<AlertDialogCancel>Cancel</AlertDialogCancel>
<div className="flex-grow" />
<AlertDialogAction
className={buttonVariants({ variant: "destructive" })}
onClick={onDelete}
>
Delete
</AlertDialogAction>
</AlertDialogFooter>
</AlertDialogContent>
</AlertDialog>
);
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

Enhance dialog UI to handle loading states.

The dialog UI should reflect the loading state during deletion and prevent user interactions while processing.

       <AlertDialogFooter>
-        <AlertDialogCancel>Cancel</AlertDialogCancel>
+        <AlertDialogCancel disabled={isDeleting}>Cancel</AlertDialogCancel>
         <div className="flex-grow" />
         <AlertDialogAction
           className={buttonVariants({ variant: "destructive" })}
           onClick={onDelete}
+          disabled={isDeleting}
         >
-          Delete
+          {isDeleting ? "Deleting..." : "Delete"}
         </AlertDialogAction>
       </AlertDialogFooter>

Committable suggestion was skipped due to low confidence.

};
Loading
Loading