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: Identifier filter and other filter cleanup #180

Merged
merged 3 commits into from
Oct 28, 2024
Merged

fix: Identifier filter and other filter cleanup #180

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c3647ea
fix: Identifier filter + general filter cleanup
adityachoudhari26 Oct 28, 2024
f22b885
add comment
adityachoudhari26 Oct 28, 2024
46424ea
more cleanup
adityachoudhari26 Oct 28, 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
26 changes: 13 additions & 13 deletions ...ponents/filter/VersionConditionRender.tsx → ...mponents/filter/ColumnConditionRender.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type { VersionOperatorType } from "@ctrlplane/validators/conditions";
import type { ColumnOperatorType } from "@ctrlplane/validators/conditions";

import { cn } from "@ctrlplane/ui";
import { Input } from "@ctrlplane/ui/input";
Expand All @@ -9,24 +9,24 @@ import {
SelectTrigger,
SelectValue,
} from "@ctrlplane/ui/select";
import { VersionOperator } from "@ctrlplane/validators/conditions";
import { ColumnOperator } from "@ctrlplane/validators/conditions";

type VersionConditionRenderProps = {
operator: VersionOperatorType;
type ColumnConditionRenderProps = {
operator: ColumnOperatorType;
value: string;
setOperator: (operator: VersionOperatorType) => void;
setOperator: (operator: ColumnOperatorType) => void;
setValue: (value: string) => void;
title: string;
className?: string;
title?: string;
};

export const VersionConditionRender: React.FC<VersionConditionRenderProps> = ({
export const ColumnConditionRender: React.FC<ColumnConditionRenderProps> = ({
operator,
value,
setOperator,
setValue,
className,
title = "Version",
title,
}) => (
<div className={cn("flex w-full items-center gap-2", className)}>
<div className="grid w-full grid-cols-12">
Expand All @@ -39,18 +39,18 @@ export const VersionConditionRender: React.FC<VersionConditionRenderProps> = ({
<SelectValue placeholder="Operator" />
</SelectTrigger>
<SelectContent>
<SelectItem value={VersionOperator.Equals}>Equals</SelectItem>
<SelectItem value={VersionOperator.Like}>Like</SelectItem>
<SelectItem value={VersionOperator.Regex}>Regex</SelectItem>
<SelectItem value={ColumnOperator.Equals}>Equals</SelectItem>
<SelectItem value={ColumnOperator.Like}>Like</SelectItem>
<SelectItem value={ColumnOperator.Regex}>Regex</SelectItem>
</SelectContent>
</Select>
</div>
<div className="col-span-7">
<Input
placeholder={
operator === VersionOperator.Regex
operator === ColumnOperator.Regex
? "^[a-zA-Z]+$"
: operator === VersionOperator.Like
: operator === ColumnOperator.Like
? "%value%"
: "Value"
}
Expand Down
3 changes: 1 addition & 2 deletions ...ervice/src/app/[workspaceSlug]/_components/job-condition/JobComparisonConditionRender.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ import {
DateOperator,
FilterType,
MetadataOperator,
VersionOperator,
} from "@ctrlplane/validators/conditions";
import {
doesConvertingToComparisonRespectMaxDepth,
Expand Down Expand Up @@ -344,7 +343,7 @@ export const JobComparisonConditionRender: React.FC<
onClick={() =>
addCondition({
type: FilterType.Version,
operator: VersionOperator.Equals,
operator: ColumnOperator.Equals,
value: "",
})
}
Expand Down
2 changes: 1 addition & 1 deletion apps/webservice/src/app/[workspaceSlug]/_components/job-condition/JobConditionBadge.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ const StringifiedVersionCondition: React.FC<{
<span className="text-muted-foreground">
{operatorVerbs[condition.operator]}
</span>
<span className="text-white">{condition.value}</span>
<span className="text-white">{condition.value.replace(/%/g, "")}</span>
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

Add null check and document the purpose of removing % signs.

While removing % signs might be intentional, the code could be more robust and clearer:

  1. Add a null check to prevent runtime errors
  2. Document why % signs are being removed

Consider this safer implementation:

-    <span className="text-white">{condition.value.replace(/%/g, "")}</span>
+    <span className="text-white">
+      {condition.value?.replace(/%/g, "") ?? condition.value}
+    </span>

Also, add a comment explaining why % signs are being removed.

📝 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
<span className="text-white">{condition.value.replace(/%/g, "")}</span>
<span className="text-white">
{condition.value?.replace(/%/g, "") ?? condition.value}
</span>

</ConditionBadge>
);

Expand Down
8 changes: 4 additions & 4 deletions apps/webservice/src/app/[workspaceSlug]/_components/job-condition/VersionConditionRender.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,20 @@
import type {
ColumnOperatorType,
VersionCondition,
VersionOperatorType,
} from "@ctrlplane/validators/conditions";

import type { JobConditionRenderProps } from "./job-condition-props";
import { VersionConditionRender } from "../filter/VersionConditionRender";
import { ColumnConditionRender } from "../filter/ColumnConditionRender";

export const JobReleaseVersionConditionRender: React.FC<
JobConditionRenderProps<VersionCondition>
> = ({ condition, onChange, className }) => {
const setOperator = (operator: VersionOperatorType) =>
const setOperator = (operator: ColumnOperatorType) =>
onChange({ ...condition, operator });
const setValue = (value: string) => onChange({ ...condition, value });

return (
<VersionConditionRender
<ColumnConditionRender
operator={condition.operator}
value={condition.value}
setOperator={setOperator}
Expand Down
4 changes: 2 additions & 2 deletions ...rvice/src/app/[workspaceSlug]/_components/release-condition/ComparisonConditionRender.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ import {
SelectTrigger,
SelectValue,
} from "@ctrlplane/ui/select";
import { DateOperator } from "@ctrlplane/validators/conditions";
import { ColumnOperator, DateOperator } from "@ctrlplane/validators/conditions";
import {
doesConvertingToComparisonRespectMaxDepth,
isComparisonCondition,
Expand Down Expand Up @@ -289,7 +289,7 @@ export const ComparisonConditionRender: React.FC<
onClick={() =>
addCondition({
type: ReleaseFilterType.Version,
operator: ReleaseOperator.Equals,
operator: ColumnOperator.Equals,
value: "",
})
}
Expand Down
2 changes: 1 addition & 1 deletion ...ebservice/src/app/[workspaceSlug]/_components/release-condition/ReleaseConditionBadge.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ const StringifiedVersionCondition: React.FC<{
<span className="text-muted-foreground">
{operatorVerbs[condition.operator]}
</span>
<span className="text-white">{condition.value}</span>
<span className="text-white">{condition.value.replace(/%/g, "")}</span>
</ConditionBadge>
);

Expand Down
9 changes: 5 additions & 4 deletions ...e/src/app/[workspaceSlug]/_components/release-condition/ReleaseVersionConditionRender.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,27 @@
import type {
ColumnOperatorType,
VersionCondition,
VersionOperatorType,
} from "@ctrlplane/validators/conditions";
import React from "react";

import type { ReleaseConditionRenderProps } from "./release-condition-props";
import { VersionConditionRender } from "../filter/VersionConditionRender";
import { ColumnConditionRender } from "../filter/ColumnConditionRender";

export const ReleaseVersionConditionRender: React.FC<
ReleaseConditionRenderProps<VersionCondition>
> = ({ condition, onChange, className }) => {
const setOperator = (operator: VersionOperatorType) =>
const setOperator = (operator: ColumnOperatorType) =>
onChange({ ...condition, operator });
const setValue = (value: string) => onChange({ ...condition, value });

return (
<VersionConditionRender
<ColumnConditionRender
operator={condition.operator}
value={condition.value}
setOperator={setOperator}
setValue={setValue}
className={className}
title="Version"
/>
);
};
13 changes: 12 additions & 1 deletion ...ervice/src/app/[workspaceSlug]/_components/target-condition/ComparisonConditionRender.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import {
SelectTrigger,
SelectValue,
} from "@ctrlplane/ui/select";
import { ColumnOperator } from "@ctrlplane/validators/conditions";
import {
doesConvertingToComparisonRespectMaxDepth,
isComparisonCondition,
Expand Down Expand Up @@ -174,7 +175,6 @@ export const ComparisonConditionRender: React.FC<
key={index}
condition={subCond}
onChange={(c) => updateCondition(index, c)}
onRemove={() => removeCondition(index)}
depth={depth + 1}
className={cn(depth === 0 ? "col-span-11" : "col-span-10")}
/>
Expand Down Expand Up @@ -292,6 +292,17 @@ export const ComparisonConditionRender: React.FC<
>
Name
</DropdownMenuItem>
<DropdownMenuItem
onClick={() =>
addCondition({
type: TargetFilterType.Identifier,
operator: ColumnOperator.Like,
value: "",
})
}
>
Identifier
</DropdownMenuItem>
<DropdownMenuItem
onClick={() =>
addCondition({
Expand Down
24 changes: 24 additions & 0 deletions ...ervice/src/app/[workspaceSlug]/_components/target-condition/IdentifierConditionRender.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import type { ColumnOperatorType } from "@ctrlplane/validators/conditions";
import type { IdentifierCondition } from "@ctrlplane/validators/targets";

import type { TargetConditionRenderProps } from "./target-condition-props";
import { ColumnConditionRender } from "../filter/ColumnConditionRender";

export const IdentifierConditionRender: React.FC<
TargetConditionRenderProps<IdentifierCondition>
> = ({ condition, onChange, className }) => {
const setOperator = (operator: ColumnOperatorType) =>
onChange({ ...condition, operator });
const setValue = (value: string) => onChange({ ...condition, value });
Comment on lines +10 to +12
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 memoizing callback functions.

To optimize performance and prevent unnecessary re-renders, consider memoizing the callback functions using useCallback.

-  const setOperator = (operator: ColumnOperatorType) =>
-    onChange({ ...condition, operator });
-  const setValue = (value: string) => onChange({ ...condition, value });
+  const setOperator = useCallback(
+    (operator: ColumnOperatorType) => onChange({ ...condition, operator }),
+    [condition, onChange]
+  );
+  const setValue = useCallback(
+    (value: string) => onChange({ ...condition, value }),
+    [condition, onChange]
+  );

Committable suggestion was skipped due to low confidence.


return (
<ColumnConditionRender
operator={condition.operator}
value={condition.value}
setOperator={setOperator}
setValue={setValue}
className={className}
title="Identifier"
/>
);
Comment on lines +14 to +23
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 making the title configurable.

The "Identifier" title is hardcoded, which might limit component reusability. Consider making it a prop with "Identifier" as the default value.

 export const IdentifierConditionRender: React.FC<
-  TargetConditionRenderProps<IdentifierCondition>
+  TargetConditionRenderProps<IdentifierCondition> & {
+    title?: string;
+  }
- > = ({ condition, onChange, className }) => {
+ > = ({ condition, onChange, className, title = "Identifier" }) => {
   // ... rest of the component
   return (
     <ColumnConditionRender
       // ... other props
-      title="Identifier"
+      title={title}
     />
   );
 };

Committable suggestion was skipped due to low confidence.

};
17 changes: 17 additions & 0 deletions .../webservice/src/app/[workspaceSlug]/_components/target-condition/TargetConditionBadge.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import type { MetadataCondition } from "@ctrlplane/validators/conditions";
import type {
ComparisonCondition,
IdentifierCondition,
KindCondition,
NameCondition,
ProviderCondition,
Expand All @@ -18,6 +19,7 @@ import {
} from "@ctrlplane/ui/hover-card";
import {
isComparisonCondition,
isIdentifierCondition,
isKindCondition,
isMetadataCondition,
isNameCondition,
Expand Down Expand Up @@ -174,6 +176,18 @@ const StringifiedNameCondition: React.FC<{
</ConditionBadge>
);

const StringifiedIdentifierCondition: React.FC<{
condition: IdentifierCondition;
}> = ({ condition }) => (
<ConditionBadge>
<span className="text-white">Identifier</span>
<span className="text-muted-foreground">
{operatorVerbs[condition.operator]}
</span>
<span className="text-white">{condition.value.replace(/%/g, "")}</span>
</ConditionBadge>
);

const StringifiedProviderCondition: React.FC<{
condition: ProviderCondition;
}> = ({ condition }) => {
Expand Down Expand Up @@ -219,6 +233,9 @@ const StringifiedTargetCondition: React.FC<{
if (isNameCondition(condition))
return <StringifiedNameCondition condition={condition} />;

if (isIdentifierCondition(condition))
return <StringifiedIdentifierCondition condition={condition} />;

if (isProviderCondition(condition))
return <StringifiedProviderCondition condition={condition} />;
};
Expand Down
22 changes: 12 additions & 10 deletions ...webservice/src/app/[workspaceSlug]/_components/target-condition/TargetConditionRender.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import React from "react";

import {
isComparisonCondition,
isIdentifierCondition,
isKindCondition,
isMetadataCondition,
isNameCondition,
Expand All @@ -11,6 +12,7 @@ import {

import type { TargetConditionRenderProps } from "./target-condition-props";
import { ComparisonConditionRender } from "./ComparisonConditionRender";
import { IdentifierConditionRender } from "./IdentifierConditionRender";
import { KindConditionRender } from "./KindConditionRender";
import { NameConditionRender } from "./NameConditionRender";
import { ProviderConditionRender } from "./ProviderConditionRender";
Expand All @@ -22,14 +24,13 @@ import { TargetMetadataConditionRender } from "./TargetMetadataConditionRender";
*/
export const TargetConditionRender: React.FC<
TargetConditionRenderProps<TargetCondition>
> = ({ condition, onChange, onRemove, depth = 0, className }) => {
> = ({ condition, onChange, depth = 0, className }) => {
if (isComparisonCondition(condition))
return (
<ComparisonConditionRender
condition={condition}
onChange={onChange}
depth={depth}
onRemove={onRemove}
className={className}
/>
);
Expand All @@ -39,8 +40,6 @@ export const TargetConditionRender: React.FC<
<TargetMetadataConditionRender
condition={condition}
onChange={onChange}
onRemove={onRemove}
depth={depth}
className={className}
/>
);
Expand All @@ -50,8 +49,6 @@ export const TargetConditionRender: React.FC<
<KindConditionRender
condition={condition}
onChange={onChange}
onRemove={onRemove}
depth={depth}
className={className}
/>
);
Expand All @@ -61,8 +58,6 @@ export const TargetConditionRender: React.FC<
<NameConditionRender
condition={condition}
onChange={onChange}
onRemove={onRemove}
depth={depth}
className={className}
/>
);
Expand All @@ -72,8 +67,15 @@ export const TargetConditionRender: React.FC<
<ProviderConditionRender
condition={condition}
onChange={onChange}
onRemove={onRemove}
depth={depth}
className={className}
/>
);

if (isIdentifierCondition(condition))
return (
<IdentifierConditionRender
condition={condition}
onChange={onChange}
className={className}
/>
);
Expand Down
1 change: 0 additions & 1 deletion ...webservice/src/app/[workspaceSlug]/_components/target-condition/target-condition-props.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import type { TargetCondition } from "@ctrlplane/validators/targets";
export type TargetConditionRenderProps<T extends TargetCondition> = {
condition: T;
onChange: (condition: T) => void;
onRemove?: () => void;
depth?: number;
className?: string;
};
3 changes: 2 additions & 1 deletion ...systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowPolicyNode.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
AlertDialogTitle,
AlertDialogTrigger,
} from "@ctrlplane/ui/alert-dialog";
import { ColumnOperator } from "@ctrlplane/validators/conditions";
import { JobStatus } from "@ctrlplane/validators/jobs";
import {
ReleaseFilterType,
Expand Down Expand Up @@ -131,7 +132,7 @@ const useEvaluateReleaseFilterCheck = (
? [
{
type: ReleaseFilterType.Version,
operator: ReleaseOperator.Equals,
operator: ColumnOperator.Equals,
value: release.version,
},
check,
Expand Down
Loading
Loading