Skip to content

fix: Extract reusable filter stuff #173

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 2 commits into from
Oct 25, 2024
Merged

fix: Extract reusable filter stuff #173

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b58f7a5
fix: Extract reusable filter stuff
adityachoudhari26 Oct 25, 2024
1404e0e
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
2 changes: 1 addition & 1 deletion apps/event-worker/src/target-scan/gke.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { CoreV1Api } from "@kubernetes/client-node";
import _ from "lodash";

import { logger } from "@ctrlplane/logger";
import { ReservedMetadataKey } from "@ctrlplane/validators/targets";
import { ReservedMetadataKey } from "@ctrlplane/validators/conditions";

import {
clusterToTarget,
Expand Down
2 changes: 1 addition & 1 deletion apps/event-worker/src/target-scan/google.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { KubeConfig } from "@kubernetes/client-node";
import { GoogleAuth, Impersonated } from "google-auth-library";
import { SemVer } from "semver";

import { ReservedMetadataKey } from "@ctrlplane/validators/targets";
import { ReservedMetadataKey } from "@ctrlplane/validators/conditions";

import { omitNullUndefined } from "../utils.js";

Expand Down
85 changes: 85 additions & 0 deletions apps/webservice/src/app/[workspaceSlug]/_components/filter/ChoiceConditionRender.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
import { useState } from "react";
import { IconSelector } from "@tabler/icons-react";
import { capitalCase } from "change-case";

import { cn } from "@ctrlplane/ui";
import { Button } from "@ctrlplane/ui/button";
import {
Command,
CommandGroup,
CommandInput,
CommandItem,
CommandList,
} from "@ctrlplane/ui/command";
import { Popover, PopoverContent, PopoverTrigger } from "@ctrlplane/ui/popover";

type ChoiceConditionRenderProps = {
onSelect: (value: string) => void;
type: string;
selected: string | null;
options: { key: string; value: string; display: string }[];
className?: string;
};
Comment on lines +16 to +22
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 25, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider enhancing type safety and documentation.

While the type definition is functional, it could be improved for better maintainability and type safety.

Consider this enhanced version:

+/**
+ * Props for the ChoiceConditionRender component
+ * @template T - The type of the option key
+ */
-type ChoiceConditionRenderProps = {
+type ChoiceConditionRenderProps<T extends string = string> = {
+  /** Callback fired when an option is selected */
   onSelect: (value: string) => void;
+  /** The type of condition being rendered */
   type: string;
+  /** Currently selected option key */
   selected: string | null;
+  /** Available options to choose from */
-  options: { key: string; value: string; display: string }[];
+  options: Array<{
+    key: T;
+    value: string;
+    display: string;
+  }>;
   className?: string;
 };
📝 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 ChoiceConditionRenderProps = {
onSelect: (value: string) => void;
type: string;
selected: string | null;
options: { key: string; value: string; display: string }[];
className?: string;
};
/**
* Props for the ChoiceConditionRender component
* @template T - The type of the option key
*/
type ChoiceConditionRenderProps<T extends string = string> = {
/** Callback fired when an option is selected */
onSelect: (value: string) => void;
/** The type of condition being rendered */
type: string;
/** Currently selected option key */
selected: string | null;
/** Available options to choose from */
options: Array<{
key: T;
value: string;
display: string;
}>;
className?: string;
};


export const ChoiceConditionRender: React.FC<ChoiceConditionRenderProps> = ({
onSelect,
type,
selected,
options,
className,
}) => {
const [open, setOpen] = useState(false);

return (
<div className={cn("flex w-full items-center gap-2", className)}>
<div className="grid w-full grid-cols-12">
<div className="col-span-2 flex items-center rounded-l-md border bg-transparent px-3 text-sm text-muted-foreground">
{capitalCase(type)}
</div>
<div className="col-span-10">
<Popover open={open} onOpenChange={setOpen}>
<PopoverTrigger asChild>
<Button
variant="outline"
role="combobox"
aria-expanded={open}
className="w-full items-center justify-start gap-2 rounded-l-none rounded-r-md bg-transparent px-2 hover:bg-neutral-800/50"
>
<IconSelector className="h-4 w-4 text-muted-foreground" />
<span
className={cn(selected != null && "text-muted-foreground")}
>
{selected ?? `Select ${type}...`}
</span>
</Button>
</PopoverTrigger>
<PopoverContent align="start" className="w-[462px] p-0">
<Command>
<CommandInput placeholder={`Search ${type}...`} />
<CommandGroup>
<CommandList>
{options.length === 0 && (
<CommandItem disabled>No options to add</CommandItem>
)}
{options.map((option) => (
<CommandItem
key={option.key}
value={option.key}
onSelect={() => {
onSelect(option.key);
setOpen(false);
}}
>
{option.display}
</CommandItem>
))}
</CommandList>
</CommandGroup>
</Command>
</PopoverContent>
</Popover>
</div>
</div>
</div>
);
};
Comment on lines +24 to +85
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 25, 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 error handling and loading states.

The component implementation could benefit from additional robustness and accessibility improvements.

Consider these enhancements:

 export const ChoiceConditionRender: React.FC<ChoiceConditionRenderProps> = ({
   onSelect,
   type,
   selected,
   options,
   className,
+  isLoading,
 }) => {
   const [open, setOpen] = useState(false);
+  const handleSelect = useCallback((key: string) => {
+    onSelect(key);
+    setOpen(false);
+  }, [onSelect]);
+
+  // Validate that selected value exists in options
+  useEffect(() => {
+    if (selected && !options.some(opt => opt.key === selected)) {
+      console.warn(`Selected value "${selected}" not found in options`);
+    }
+  }, [selected, options]);

   return (
     <div className={cn("flex w-full items-center gap-2", className)}>
       <div className="grid w-full grid-cols-12">
         <div className="col-span-2 flex items-center rounded-l-md border bg-transparent px-3 text-sm text-muted-foreground">
           {capitalCase(type)}
         </div>
         <div className="col-span-10">
           <Popover open={open} onOpenChange={setOpen}>
             <PopoverTrigger asChild>
               <Button
                 variant="outline"
                 role="combobox"
                 aria-expanded={open}
+                aria-label={`Select ${type}`}
+                disabled={isLoading}
                 className="w-full items-center justify-start gap-2 rounded-l-none rounded-r-md bg-transparent px-2 hover:bg-neutral-800/50"
               >
                 <IconSelector className="h-4 w-4 text-muted-foreground" />
                 <span
                   className={cn(selected != null && "text-muted-foreground")}
                 >
-                  {selected ?? `Select ${type}...`}
+                  {isLoading ? "Loading..." : selected ?? `Select ${type}...`}
                 </span>
               </Button>
             </PopoverTrigger>
             <PopoverContent align="start" className="w-[462px] p-0">
               <Command>
                 <CommandInput placeholder={`Search ${type}...`} />
                 <CommandGroup>
                   <CommandList>
+                    {isLoading && (
+                      <CommandItem disabled>Loading options...</CommandItem>
+                    )}
                     {options.length === 0 && (
                       <CommandItem disabled>No options to add</CommandItem>
                     )}
                     {options.map((option) => (
                       <CommandItem
                         key={option.key}
                         value={option.key}
-                        onSelect={() => {
-                          onSelect(option.key);
-                          setOpen(false);
-                        }}
+                        onSelect={() => handleSelect(option.key)}
                       >
                         {option.display}
                       </CommandItem>
📝 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 ChoiceConditionRender: React.FC<ChoiceConditionRenderProps> = ({
onSelect,
type,
selected,
options,
className,
}) => {
const [open, setOpen] = useState(false);
return (
<div className={cn("flex w-full items-center gap-2", className)}>
<div className="grid w-full grid-cols-12">
<div className="col-span-2 flex items-center rounded-l-md border bg-transparent px-3 text-sm text-muted-foreground">
{capitalCase(type)}
</div>
<div className="col-span-10">
<Popover open={open} onOpenChange={setOpen}>
<PopoverTrigger asChild>
<Button
variant="outline"
role="combobox"
aria-expanded={open}
className="w-full items-center justify-start gap-2 rounded-l-none rounded-r-md bg-transparent px-2 hover:bg-neutral-800/50"
>
<IconSelector className="h-4 w-4 text-muted-foreground" />
<span
className={cn(selected != null && "text-muted-foreground")}
>
{selected ?? `Select ${type}...`}
</span>
</Button>
</PopoverTrigger>
<PopoverContent align="start" className="w-[462px] p-0">
<Command>
<CommandInput placeholder={`Search ${type}...`} />
<CommandGroup>
<CommandList>
{options.length === 0 && (
<CommandItem disabled>No options to add</CommandItem>
)}
{options.map((option) => (
<CommandItem
key={option.key}
value={option.key}
onSelect={() => {
onSelect(option.key);
setOpen(false);
}}
>
{option.display}
</CommandItem>
))}
</CommandList>
</CommandGroup>
</Command>
</PopoverContent>
</Popover>
</div>
</div>
</div>
);
};
export const ChoiceConditionRender: React.FC<ChoiceConditionRenderProps> = ({
onSelect,
type,
selected,
options,
className,
isLoading,
}) => {
const [open, setOpen] = useState(false);
const handleSelect = useCallback((key: string) => {
onSelect(key);
setOpen(false);
}, [onSelect]);
// Validate that selected value exists in options
useEffect(() => {
if (selected && !options.some(opt => opt.key === selected)) {
console.warn(`Selected value "${selected}" not found in options`);
}
}, [selected, options]);
return (
<div className={cn("flex w-full items-center gap-2", className)}>
<div className="grid w-full grid-cols-12">
<div className="col-span-2 flex items-center rounded-l-md border bg-transparent px-3 text-sm text-muted-foreground">
{capitalCase(type)}
</div>
<div className="col-span-10">
<Popover open={open} onOpenChange={setOpen}>
<PopoverTrigger asChild>
<Button
variant="outline"
role="combobox"
aria-expanded={open}
aria-label={`Select ${type}`}
disabled={isLoading}
className="w-full items-center justify-start gap-2 rounded-l-none rounded-r-md bg-transparent px-2 hover:bg-neutral-800/50"
>
<IconSelector className="h-4 w-4 text-muted-foreground" />
<span
className={cn(selected != null && "text-muted-foreground")}
>
{isLoading ? "Loading..." : selected ?? `Select ${type}...`}
</span>
</Button>
</PopoverTrigger>
<PopoverContent align="start" className="w-[462px] p-0">
<Command>
<CommandInput placeholder={`Search ${type}...`} />
<CommandGroup>
<CommandList>
{isLoading && (
<CommandItem disabled>Loading options...</CommandItem>
)}
{options.length === 0 && (
<CommandItem disabled>No options to add</CommandItem>
)}
{options.map((option) => (
<CommandItem
key={option.key}
value={option.key}
onSelect={() => handleSelect(option.key)}
>
{option.display}
</CommandItem>
))}
</CommandList>
</CommandGroup>
</Command>
</PopoverContent>
</Popover>
</div>
</div>
</div>
);
};

92 changes: 92 additions & 0 deletions apps/webservice/src/app/[workspaceSlug]/_components/filter/DateConditionRender.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
import type { DateValue } from "@internationalized/date";
import { ZonedDateTime } from "@internationalized/date";
import ms from "ms";

import { cn } from "@ctrlplane/ui";
import { DateTimePicker } from "@ctrlplane/ui/date-time-picker/date-time-picker";
import {
Select,
SelectContent,
SelectItem,
SelectTrigger,
SelectValue,
} from "@ctrlplane/ui/select";
import { DateOperator } from "@ctrlplane/validators/conditions";

const toZonedDateTime = (date: Date): ZonedDateTime => {
const timeZone = Intl.DateTimeFormat().resolvedOptions().timeZone;
const offset = -date.getTimezoneOffset() * ms("1m");
const year = date.getFullYear();
const month = date.getMonth() + 1;
const day = date.getDate();
const hour = date.getHours();
const minute = date.getMinutes();
const second = date.getSeconds();
const millisecond = date.getMilliseconds();

return new ZonedDateTime(
year,
month,
day,
timeZone,
offset,
hour,
minute,
second,
millisecond,
);
};

type Operator = "before" | "after" | "before-or-on" | "after-or-on";

type DateConditionRenderProps = {
setDate: (date: DateValue) => void;
setOperator: (operator: DateOperator) => void;
value: string;
operator: Operator;
type: string;
className?: string;
};
Comment on lines +40 to +49
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 25, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance type definitions with documentation and proper type inheritance

Consider these improvements:

  1. Add JSDoc comments to document the props interface
  2. Consider deriving the Operator type from DateOperator instead of redefining it
-type Operator = "before" | "after" | "before-or-on" | "after-or-on";
+type Operator = DateOperator;

+/**
+ * Props for the DateConditionRender component
+ * @property setDate - Callback to update the selected date
+ * @property setOperator - Callback to update the selected operator
+ * @property value - The current date value as ISO string
+ * @property operator - The current comparison operator
+ * @property type - The label for the condition type
+ * @property className - Optional CSS class names
+ */
 type DateConditionRenderProps = {
   setDate: (date: DateValue) => void;
   setOperator: (operator: DateOperator) => void;

Committable suggestion was skipped due to low confidence.


export const DateConditionRender: React.FC<DateConditionRenderProps> = ({
setDate,
setOperator,
value,
operator,
type,
className,
}) => (
<div className={cn("flex w-full items-center gap-2", className)}>
<div className="grid w-full grid-cols-12">
<div className="col-span-2 flex items-center rounded-l-md border bg-transparent px-3 text-sm text-muted-foreground">
{type}
</div>
<div className="col-span-3">
<Select value={operator} onValueChange={setOperator}>
<SelectTrigger className="rounded-none text-muted-foreground hover:bg-neutral-800/50">
<SelectValue
placeholder="Operator"
className="text-muted-foreground"
/>
</SelectTrigger>
<SelectContent className="text-muted-foreground">
<SelectItem value={DateOperator.Before}>before</SelectItem>
<SelectItem value={DateOperator.After}>after</SelectItem>
<SelectItem value={DateOperator.BeforeOrOn}>
before or on
</SelectItem>
<SelectItem value={DateOperator.AfterOrOn}>after or on</SelectItem>
</SelectContent>
</Select>
</div>
<div className="col-span-7">
<DateTimePicker
value={toZonedDateTime(new Date(value))}
onChange={setDate}
aria-label={type}
variant="filter"
/>
Comment on lines +83 to +88
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 25, 2024

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 for invalid date values

The toZonedDateTime function assumes value will always be a valid date string. Consider adding error handling to prevent runtime errors with invalid dates.

+const safeToZonedDateTime = (dateStr: string): ZonedDateTime => {
+  const date = new Date(dateStr);
+  if (isNaN(date.getTime())) {
+    // Return current date as fallback
+    return toZonedDateTime(new Date());
+  }
+  return toZonedDateTime(date);
+};

 <DateTimePicker
-  value={toZonedDateTime(new Date(value))}
+  value={safeToZonedDateTime(value)}
   onChange={setDate}

Committable suggestion was skipped due to low confidence.

</div>
</div>
</div>
);
67 changes: 29 additions & 38 deletions ...get-condition/MetadataConditionRender.tsx → ...onents/filter/MetadataConditionRender.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import type { MetadataCondition } from "@ctrlplane/validators/targets";
import type {
MetadataCondition,
MetadataOperatorType,
} from "@ctrlplane/validators/conditions";
import { useState } from "react";
import { useParams } from "next/navigation";

import { cn } from "@ctrlplane/ui";
import { Button } from "@ctrlplane/ui/button";
Expand All @@ -13,44 +15,33 @@ import {
SelectTrigger,
SelectValue,
} from "@ctrlplane/ui/select";
import { TargetOperator } from "@ctrlplane/validators/targets";
import { MetadataOperator } from "@ctrlplane/validators/conditions";

import type { TargetConditionRenderProps } from "./target-condition-props";
import { api } from "~/trpc/react";
import { useMatchSorter } from "~/utils/useMatchSorter";

type MetadataConditionRenderProps = {
condition: MetadataCondition;
onChange: (condition: MetadataCondition) => void;
metadataKeys: string[];
className?: string;
};
export const MetadataConditionRender: React.FC<
TargetConditionRenderProps<MetadataCondition>
> = ({ condition, onChange, className }) => {
const { workspaceSlug } = useParams<{ workspaceSlug: string }>();
const workspace = api.workspace.bySlug.useQuery(workspaceSlug);

MetadataConditionRenderProps
> = ({ condition, onChange, metadataKeys, className }) => {
const setKey = (key: string) => onChange({ ...condition, key });

const setValue = (value: string) =>
condition.operator !== TargetOperator.Null &&
condition.operator !== MetadataOperator.Null &&
onChange({ ...condition, value });

const setOperator = (
operator:
| TargetOperator.Equals
| TargetOperator.Like
| TargetOperator.Regex
| TargetOperator.Null,
) =>
operator === TargetOperator.Null
const setOperator = (operator: MetadataOperatorType) =>
operator === MetadataOperator.Null
? onChange({ ...condition, operator, value: undefined })
: onChange({ ...condition, operator, value: condition.value ?? "" });

const [open, setOpen] = useState(false);
const metadataKeys = api.target.metadataKeys.useQuery(
workspace.data?.id ?? "",
{ enabled: workspace.isSuccess && workspace.data != null },
);
const filteredMetadataKeys = useMatchSorter(
metadataKeys.data ?? [],
condition.key,
);

const filteredMetadataKeys = useMatchSorter(metadataKeys, condition.key);

return (
<div className={cn("flex w-full items-center gap-2", className)}>
Expand Down Expand Up @@ -92,10 +83,10 @@ export const MetadataConditionRender: React.FC<
value={condition.operator}
onValueChange={(
v:
| TargetOperator.Equals
| TargetOperator.Like
| TargetOperator.Regex
| TargetOperator.Null,
| MetadataOperator.Equals
| MetadataOperator.Like
| MetadataOperator.Regex
| MetadataOperator.Null,
) => setOperator(v)}
>
<SelectTrigger className="rounded-none text-muted-foreground hover:bg-neutral-800/50">
Expand All @@ -105,21 +96,21 @@ export const MetadataConditionRender: React.FC<
/>
</SelectTrigger>
<SelectContent className="text-muted-foreground">
<SelectItem value={TargetOperator.Equals}>Equals</SelectItem>
<SelectItem value={TargetOperator.Regex}>Regex</SelectItem>
<SelectItem value={TargetOperator.Like}>Like</SelectItem>
<SelectItem value={TargetOperator.Null}>Is Null</SelectItem>
<SelectItem value={MetadataOperator.Equals}>Equals</SelectItem>
<SelectItem value={MetadataOperator.Regex}>Regex</SelectItem>
<SelectItem value={MetadataOperator.Like}>Like</SelectItem>
<SelectItem value={MetadataOperator.Null}>Is Null</SelectItem>
</SelectContent>
</Select>
</div>

{condition.operator !== TargetOperator.Null ? (
{condition.operator !== MetadataOperator.Null ? (
<div className="col-span-4">
<Input
placeholder={
condition.operator === TargetOperator.Regex
condition.operator === MetadataOperator.Regex
? "^[a-zA-Z]+$"
: condition.operator === TargetOperator.Like
: condition.operator === MetadataOperator.Like
? "%value%"
: "Value"
}
Expand Down
2 changes: 1 addition & 1 deletion apps/webservice/src/app/[workspaceSlug]/_components/job-drawer/JobDrawer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {

import { Button, buttonVariants } from "@ctrlplane/ui/button";
import { Drawer, DrawerContent, DrawerTitle } from "@ctrlplane/ui/drawer";
import { ReservedMetadataKey } from "@ctrlplane/validators/targets";
import { ReservedMetadataKey } from "@ctrlplane/validators/conditions";

import { JobDropdownMenu } from "~/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/JobDropdownMenu";
import { api } from "~/trpc/react";
Expand Down
2 changes: 1 addition & 1 deletion apps/webservice/src/app/[workspaceSlug]/_components/job-drawer/JobMetadata.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { IconSparkles } from "@tabler/icons-react";

import { Input } from "@ctrlplane/ui/input";
import { ReservedMetadataKey } from "@ctrlplane/validators/targets";
import { ReservedMetadataKey } from "@ctrlplane/validators/conditions";

import type { Job } from "./Job";
import { useMatchSorterWithSearch } from "~/utils/useMatchSorter";
Expand Down
2 changes: 1 addition & 1 deletion apps/webservice/src/app/[workspaceSlug]/_components/job-drawer/JobProperties.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ import React from "react";
import { capitalCase } from "change-case";
import { format } from "date-fns";

import { ReservedMetadataKey } from "@ctrlplane/validators/conditions";
import { JobStatusReadable } from "@ctrlplane/validators/jobs";
import { ReservedMetadataKey } from "@ctrlplane/validators/targets";

import type { Job } from "./Job";
import { JobTableStatusIcon } from "../JobTableStatusIcon";
Expand Down
Loading
Loading