Skip to content

fix: Init runbooks ui #241

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
Nov 29, 2024
Merged

fix: Init runbooks ui #241

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
7799a20
fix: Init runbooks ui
adityachoudhari26 Nov 29, 2024
e5136b3
nformatter
adityachoudhari26 Nov 29, 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/webservice/src/app/[workspaceSlug]/(app)/(job)/jobs/JobTable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export const JobTable: React.FC<JobTableProps> = ({ workspaceId }) => {
const { filter, setFilter } = useFilter<JobCondition>();
const { setJobId } = useJobDrawer();
const allReleaseJobTriggers = api.job.config.byWorkspaceId.list.useQuery(
{ workspaceId },
{ workspaceId, limit: 0 },
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 29, 2024

Choose a reason for hiding this comment

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

💡 Codebase verification

🛠️ Refactor suggestion

Based on the search results and analysis of the codebase, I can now generate the final response:

Consider using pagination instead of limit: 0 for better performance

Setting limit: 0 to fetch all records is not the optimal approach. The codebase shows that pagination is already implemented in other routers (as seen in resources.ts, system.ts, and release.ts) with a pattern that returns both items and total. This pattern should be used here instead.

  • apps/webservice/src/app/[workspaceSlug]/(app)/(job)/jobs/JobTable.tsx: Replace the unlimited query with a paginated one that only fetches the total count
  • Other components like TargetPageContent.tsx and JobHistoryChart.tsx show similar patterns of using limit: 0 which should also be reviewed

Example implementation:

-const allReleaseJobTriggers = api.job.config.byWorkspaceId.list.useQuery(
-  { workspaceId, limit: 0 },
-  { refetchInterval: 60_000, placeholderData: (prev) => prev },
-);
+const jobsTotal = api.job.config.byWorkspaceId.list.useQuery(
+  { workspaceId, limit: 1, page: 0 },
+  { refetchInterval: 60_000, select: (data) => data.total },
+);
🔗 Analysis chain

Consider optimizing the data fetching strategy

The current implementation fetches all records with limit: 0 when only the total count is needed for the "NoFilterMatch" component. This could be inefficient for large datasets.

Consider these improvements:

  1. Add a separate endpoint to fetch only the count
  2. Or modify the existing endpoint to accept a countOnly parameter
  3. Or use the same query instance with different parameters to avoid potential UI inconsistencies

Example implementation:

-const allReleaseJobTriggers = api.job.config.byWorkspaceId.list.useQuery(
-  { workspaceId, limit: 0 },
-  { refetchInterval: 60_000, placeholderData: (prev) => prev },
-);
+const jobCount = api.job.config.byWorkspaceId.count.useQuery(
+  { workspaceId },
+  { refetchInterval: 60_000, placeholderData: (prev) => prev },
+);

Let's verify if there are any existing count-only endpoints or if other components are using similar patterns:

Let me analyze the results and generate additional verification scripts to check for count-specific endpoints in the codebase.

The results will help us determine if there are existing count endpoints or if we need to suggest creating new ones.

Let me try one more search pattern to look for any count-related implementations.

Let me try one final search to specifically look for the job router implementation.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for similar query patterns in the codebase
rg -A 2 "byWorkspaceId.list.useQuery" 

# Search for any count-specific endpoints
ast-grep --pattern 'export const count = $$$'

Length of output: 7204

Script:

#!/bin/bash
# Search for count-related router endpoints
rg -A 5 "router.*count" 

# Search for count-related procedures in TRPC
rg -A 5 "procedure.*count"

# Check if there are any existing count implementations
ast-grep --pattern 'count = async ($$$) => {
  $$$
}'

Length of output: 110

Script:

#!/bin/bash
# Search for count-related implementations with broader patterns
rg -i "count.*=" -A 5

# Look for any pagination or total count related code
rg -i "total.*items|pagination" -A 5

# Check the specific router file for the jobs endpoint
fd -e ts -e tsx "job.*router" --exec cat {}

Length of output: 36347

Script:

#!/bin/bash
# Search for job router implementation
rg -A 10 "job.*router.*=" 

# Search for job-related TRPC procedures
rg -A 10 "job.*procedure" 

# Look for any count-related endpoints in job routes
rg -A 10 "job.*count"

Length of output: 995

{ refetchInterval: 60_000, placeholderData: (prev) => prev },
);

Expand Down
348 changes: 348 additions & 0 deletions ...p/[workspaceSlug]/(app)/_components/job-condition/RunbookJobComparisonConditionRender.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,348 @@
import type {
ComparisonCondition,
JobCondition,
} from "@ctrlplane/validators/jobs";
import type React from "react";
import {
IconChevronDown,
IconCopy,
IconDots,
IconEqualNot,
IconPlus,
IconRefresh,
IconTrash,
} from "@tabler/icons-react";
import { capitalCase } from "change-case";

import { cn } from "@ctrlplane/ui";
import { Button } from "@ctrlplane/ui/button";
import {
DropdownMenu,
DropdownMenuContent,
DropdownMenuGroup,
DropdownMenuItem,
DropdownMenuTrigger,
} from "@ctrlplane/ui/dropdown-menu";
import {
Select,
SelectContent,
SelectGroup,
SelectItem,
SelectTrigger,
SelectValue,
} from "@ctrlplane/ui/select";
import {
ColumnOperator,
ComparisonOperator,
DateOperator,
FilterType,
MetadataOperator,
} from "@ctrlplane/validators/conditions";
import {
doesConvertingToComparisonRespectMaxDepth,
isComparisonCondition,
JobFilterType,
JobStatus,
} from "@ctrlplane/validators/jobs";

import type { JobConditionRenderProps } from "./job-condition-props";
import { JobConditionRender } from "./JobConditionRender";

export const RunbookJobComparisonConditionRender: React.FC<
JobConditionRenderProps<ComparisonCondition>
> = ({ condition, onChange, depth = 0, className }) => {
const setOperator = (
operator: ComparisonOperator.And | ComparisonOperator.Or,
) => onChange({ ...condition, operator });

const updateCondition = (index: number, changedCondition: JobCondition) =>
onChange({
...condition,
conditions: condition.conditions.map((c, i) =>
i === index ? changedCondition : c,
),
});

const addCondition = (changedCondition: JobCondition) =>
onChange({
...condition,
conditions: [...condition.conditions, changedCondition],
});

const removeCondition = (index: number) =>
onChange({
...condition,
conditions: condition.conditions.filter((_, i) => i !== index),
});

const convertToComparison = (index: number) => {
const cond = condition.conditions[index];
if (!cond) return;

if (!doesConvertingToComparisonRespectMaxDepth(depth + 1, cond)) return;

const newComparisonCondition: ComparisonCondition = {
type: FilterType.Comparison,
operator: ComparisonOperator.And,
conditions: [cond],
};

const newCondition = {
...condition,
conditions: condition.conditions.map((c, i) =>
i === index ? newComparisonCondition : c,
),
};
onChange(newCondition);
};

const convertToNotComparison = (index: number) => {
const cond = condition.conditions[index];
if (!cond) return;

if (isComparisonCondition(cond)) {
const currentNot = cond.not ?? false;
const newNotSubcondition = {
...cond,
not: !currentNot,
};
const newCondition = {
...condition,
conditions: condition.conditions.map((c, i) =>
i === index ? newNotSubcondition : c,
),
};
onChange(newCondition);
return;
}

const newNotComparisonCondition: ComparisonCondition = {
type: FilterType.Comparison,
operator: ComparisonOperator.And,
not: true,
conditions: [cond],
};

const newCondition = {
...condition,
conditions: condition.conditions.map((c, i) =>
i === index ? newNotComparisonCondition : c,
),
};
onChange(newCondition);
};

const clear = () => onChange({ ...condition, conditions: [] });

const not = condition.not ?? false;

return (
<div
className={cn(
"space-y-4 rounded-md border p-2",
className,
depth === 0 ? "bg-neutral-950" : "bg-neutral-800/10",
)}
>
{condition.conditions.length === 0 && (
<span className="text-sm text-muted-foreground">
{not ? "Empty not group" : "No conditions"}
</span>
)}
<div className="space-y-2">
{condition.conditions.map((subCond, index) => (
<div key={index} className="flex items-start gap-2">
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 29, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Avoid using array index as key in list rendering

Using the array index index as a key in list rendering can lead to issues with component state and may cause unnecessary re-renders if the list changes order. It's better to use a unique identifier for each item if available.

If subCond has a unique identifier, consider using it as the key:

- <div key={index} className="flex items-start gap-2">
+ <div key={getUniqueKeyForCondition(subCond)} className="flex items-start gap-2">

Ensure that getUniqueKeyForCondition returns a unique and stable identifier for each condition.

Committable suggestion skipped: line range outside the PR's diff.

<div className="grid flex-grow grid-cols-12 gap-2">
{index !== 1 && (
<div
className={cn(
"col-span-2 flex justify-end px-1 pt-1 text-muted-foreground",
depth === 0 ? "col-span-1" : "col-span-2",
)}
>
{index !== 0 && capitalCase(condition.operator)}
{index === 0 && !condition.not && "When"}
{index === 0 && condition.not && "Not"}
</div>
)}
{index === 1 && (
<Select value={condition.operator} onValueChange={setOperator}>
<SelectTrigger
className={cn(
"col-span-2 text-muted-foreground hover:bg-neutral-700/50",
depth === 0 ? "col-span-1" : "col-span-2",
)}
>
<SelectValue />
</SelectTrigger>
<SelectContent>
<SelectGroup>
<SelectItem value={ComparisonOperator.And}>
And
</SelectItem>
<SelectItem value={ComparisonOperator.Or}>Or</SelectItem>
</SelectGroup>
</SelectContent>
</Select>
)}
<JobConditionRender
key={index}
condition={subCond}
onChange={(c) => updateCondition(index, c)}
depth={depth + 1}
className={cn(depth === 0 ? "col-span-11" : "col-span-10")}
/>
</div>

<DropdownMenu>
<DropdownMenuTrigger asChild>
<Button
variant="ghost"
size="icon"
className="col-span-1 h-6 w-6 text-muted-foreground"
>
<IconDots className="h-4 w-4" />
</Button>
</DropdownMenuTrigger>
<DropdownMenuContent
align="end"
className="text-muted-foreground"
>
<DropdownMenuItem
onClick={() => removeCondition(index)}
className="flex items-center gap-2"
>
<IconTrash className="h-4 w-4 text-red-400" />
Remove
</DropdownMenuItem>
<DropdownMenuItem
onClick={() => addCondition(subCond)}
className="flex items-center gap-2"
>
<IconCopy className="h-4 w-4" />
Duplicate
</DropdownMenuItem>
{doesConvertingToComparisonRespectMaxDepth(
depth + 1,
subCond,
) && (
<DropdownMenuItem
onClick={() => convertToComparison(index)}
className="flex items-center gap-2"
>
<IconRefresh className="h-4 w-4" />
Turn into group
</DropdownMenuItem>
)}
{(isComparisonCondition(subCond) ||
doesConvertingToComparisonRespectMaxDepth(
depth + 1,
subCond,
)) && (
<DropdownMenuItem
onClick={() => convertToNotComparison(index)}
className="flex items-center gap-2"
>
<IconEqualNot className="h-4 w-4" />
Negate condition
</DropdownMenuItem>
)}
</DropdownMenuContent>
</DropdownMenu>
</div>
))}
</div>

<div className="flex">
<DropdownMenu>
<DropdownMenuTrigger
className="w-max focus-visible:outline-none"
asChild
>
<Button
type="button"
variant="outline"
className={cn(
"flex items-center gap-1 bg-inherit px-2 text-muted-foreground hover:bg-neutral-800/50",
depth === 0 && "border-neutral-800/70",
)}
>
<IconPlus className="h-4 w-4" /> Add Condition{" "}
<IconChevronDown className="h-4 w-4" />
</Button>
</DropdownMenuTrigger>
<DropdownMenuContent align="start" className="text-muted-foreground">
<DropdownMenuGroup>
<DropdownMenuItem
onClick={() =>
addCondition({
type: FilterType.Metadata,
operator: MetadataOperator.Equals,
key: "",
value: "",
})
}
>
Metadata
</DropdownMenuItem>
<DropdownMenuItem
onClick={() =>
addCondition({
type: FilterType.CreatedAt,
operator: DateOperator.Before,
value: new Date().toISOString(),
})
}
>
Created at
</DropdownMenuItem>
<DropdownMenuItem
onClick={() =>
addCondition({
type: JobFilterType.Status,
operator: ColumnOperator.Equals,
value: JobStatus.Completed,
})
}
>
Status
</DropdownMenuItem>
{depth < 2 && (
<DropdownMenuItem
onClick={() =>
addCondition({
type: FilterType.Comparison,
operator: ComparisonOperator.And,
conditions: [],
not: false,
})
}
>
Filter group
</DropdownMenuItem>
)}
{depth < 2 && (
<DropdownMenuItem
onClick={() =>
addCondition({
type: FilterType.Comparison,
operator: ComparisonOperator.And,
not: true,
conditions: [],
})
}
>
Not group
</DropdownMenuItem>
)}
</DropdownMenuGroup>
</DropdownMenuContent>
</DropdownMenu>
<div className="flex-grow" />
<Button variant="outline" type="button" onClick={clear}>
Clear
</Button>
</div>
</div>
);
};
Loading
Loading