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.

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

fix: Settings page runbooks #242

Merged
merged 1 commit into from
Dec 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -7,31 +7,18 @@ 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 { useForm } from "@ctrlplane/ui/form";

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

const updateRunbookSchema = z.object({
name: z.string().min(1),
Expand Down Expand Up @@ -60,12 +47,12 @@ export const EditRunbookDialog: React.FC<{
});

const router = useRouter();
const onSubmit = form.handleSubmit(async (data) =>

const onSubmit = (data: EditRunbookFormSchema) =>
update
.mutateAsync({ id: runbook.id, data })
.then(() => router.refresh())
.then(() => setOpen(false)),
);
.then(() => setOpen(false));
Comment on lines +51 to +55
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 to the form submission

The promise chain should include error handling and user feedback for a better user experience.

Consider this improvement:

  const onSubmit = (data: EditRunbookFormSchema) =>
    update
      .mutateAsync({ id: runbook.id, data })
      .then(() => router.refresh())
-     .then(() => setOpen(false));
+     .then(() => {
+       setOpen(false);
+       // Consider adding a success toast/notification here
+     })
+     .catch((error) => {
+       console.error('Failed to update runbook:', error);
+       // Consider adding an error toast/notification here
+     });

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


const jobAgentId = form.watch("jobAgentId");
const jobAgent = jobAgents.find((j) => j.id === jobAgentId);
Expand All @@ -76,118 +63,13 @@ export const EditRunbookDialog: React.FC<{
<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>
<EditRunbookForm
form={form}
jobAgents={jobAgents}
jobAgent={jobAgent}
workspace={workspace}
onSubmit={onSubmit}
/>
</DialogContent>
</Dialog>
);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,160 @@
"use client";

import type { UseFormReturn } from "react-hook-form";
import { z } from "zod";

import * as SCHEMA from "@ctrlplane/db/schema";
import { Button } from "@ctrlplane/ui/button";
import {
Form,
FormControl,
FormField,
FormItem,
FormLabel,
FormMessage,
FormRootError,
} 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 { RunbookVariablesEditor } from "./create/RunbookVariableEditor";

export const updateRunbookSchema = z.object({
name: z.string().min(1),
description: z.string(),
variables: z.array(SCHEMA.createRunbookVariable),
jobAgentId: z.string().uuid({ message: "Must be a valid job agent ID" }),
jobAgentConfig: z.record(z.any()),
});
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

Strengthen type safety for jobAgentConfig validation

The current schema uses z.record(z.any()) for jobAgentConfig which is too permissive and could lead to runtime errors. Consider creating a more specific schema based on the possible agent configuration types.

Example improvement:

-  jobAgentConfig: z.record(z.any()),
+  jobAgentConfig: z.record(z.union([
+    z.string(),
+    z.number(),
+    z.boolean(),
+    z.array(z.string()),
+    // Add other specific types based on your agent configurations
+  ])),

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


export type EditRunbookFormSchema = z.infer<typeof updateRunbookSchema>;

export type EditRunbookFormProps = {
form: UseFormReturn<EditRunbookFormSchema>;
jobAgents: SCHEMA.JobAgent[];
jobAgent?: SCHEMA.JobAgent;
workspace: SCHEMA.Workspace;
onSubmit: (data: EditRunbookFormSchema) => void;
};

export const EditRunbookForm: React.FC<EditRunbookFormProps> = ({
form,
jobAgents,
jobAgent,
workspace,
onSubmit,
}) => (
<Form {...form}>
<form onSubmit={form.handleSubmit(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>
);
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
"use client";

import type { RouterOutputs } from "@ctrlplane/api";
import type React from "react";
import Link from "next/link";
import { useParams, usePathname } from "next/navigation";
Expand All @@ -18,9 +19,13 @@ import { nFormatter } from "../../_components/nFormatter";

type RunbookNavBarProps = {
totalJobs: number;
runbook: NonNullable<RouterOutputs["runbook"]["byId"]>;
};

export const RunbookNavBar: React.FC<RunbookNavBarProps> = ({ totalJobs }) => {
export const RunbookNavBar: React.FC<RunbookNavBarProps> = ({
totalJobs,
runbook,
}) => {
const { workspaceSlug, systemSlug, runbookId } = useParams<{
workspaceSlug: string;
systemSlug: string;
Expand Down Expand Up @@ -56,16 +61,18 @@ export const RunbookNavBar: React.FC<RunbookNavBarProps> = ({ totalJobs }) => {
</NavigationMenuLink>
</Link>
</NavigationMenuItem>
<NavigationMenuItem>
<Link href={settingsUrl} legacyBehavior passHref>
<NavigationMenuLink
className="group inline-flex h-9 w-max items-center justify-center rounded-md px-4 py-2 text-sm font-medium transition-colors hover:bg-accent/50 hover:text-accent-foreground focus:outline-none disabled:pointer-events-none disabled:opacity-50 data-[active]:bg-accent/50 data-[state=open]:bg-accent/50"
active={isSettingsActive}
>
Settings
</NavigationMenuLink>
</Link>
</NavigationMenuItem>
{runbook.runhooks.length === 0 && (
<NavigationMenuItem>
<Link href={settingsUrl} legacyBehavior passHref>
<NavigationMenuLink
className="group inline-flex h-9 w-max items-center justify-center rounded-md px-4 py-2 text-sm font-medium transition-colors hover:bg-accent/50 hover:text-accent-foreground focus:outline-none disabled:pointer-events-none disabled:opacity-50 data-[active]:bg-accent/50 data-[state=open]:bg-accent/50"
active={isSettingsActive}
>
Settings
</NavigationMenuLink>
</Link>
</NavigationMenuItem>
)}
Comment on lines +64 to +75
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Settings should remain accessible regardless of runhooks presence

The current implementation that hides the Settings menu when runhooks exist is problematic because:

  1. The settings page (RunbookSettingsPage) exists and is fully implemented without any conditions related to runhooks, suggesting it's meant to be accessible at all times
  2. The page contains important functionality like EditRunbook that users would need regardless of runhook status
  3. There's no documented reason or technical limitation that justifies hiding settings access when runhooks are present

Recommendations:

  • Remove the conditional rendering (runbook.runhooks.length === 0) and always show the Settings menu item
  • If there are specific settings that shouldn't be modified with runhooks, handle those restrictions within the settings page itself rather than hiding the entire navigation item
🔗 Analysis chain

Verify the UX decision to hide settings when runhooks exist

The current implementation hides the Settings menu item when runhooks are present. This might lead to:

  1. Reduced accessibility to settings when they might still be needed
  2. Confusion when the settings option disappears after adding runhooks
  3. Potential workflow disruption for users managing runbook configurations

Consider:

  • Keeping settings always accessible
  • Adding a tooltip explaining why settings are hidden (if this behavior must be maintained)
  • Documenting this behavior in user guides

Let's check if this behavior is consistent with other similar components:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for similar navigation patterns in other components
rg -t tsx "NavigationMenu.*Settings" --glob "!**/RunbookNavBar.tsx"

Length of output: 97


Script:

#!/bin/bash
# Search for similar navigation patterns with settings in other components
rg -t typescript -t javascriptreact -t typescriptreact "NavigationMenu.*Settings" --glob "!**/RunbookNavBar.tsx"

# Also search for other navigation components that might show/hide settings conditionally
rg -t typescript -t javascriptreact -t typescriptreact "Settings.*className.*(\{.*\})" -B 2 -A 2

Length of output: 283


Script:

#!/bin/bash
# Search for similar navigation patterns with settings
rg "NavigationMenu.*Settings" --type-add 'tsx:*.tsx' --type-add 'ts:*.ts' -t tsx -t ts --glob "!**/RunbookNavBar.tsx"

# Look for conditional rendering of settings in navigation
rg "Settings.*className.*\{.*\}" --type-add 'tsx:*.tsx' --type-add 'ts:*.ts' -t tsx -t ts -B 3 -A 3

# Search for other instances of runbook settings navigation
fd -e tsx -e ts . | xargs rg "runbook.*settings" -i

Length of output: 433


Script:

#!/bin/bash
# Check the settings page implementation to understand if it's meant to be accessible with runhooks
cat "apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/runbooks/[runbookId]/settings/page.tsx"

# Look for any other navigation bars in the runbook context to understand the pattern
rg -B 3 -A 3 "NavigationMenu" --type-add 'tsx:*.tsx' -t tsx -g "**/runbooks/**"

# Search for any documentation or comments about runbook settings access
rg "runbook.*settings?.*access" -i --type-add 'tsx:*.tsx' --type-add 'ts:*.ts' --type-add 'md:*.md' -t tsx -t ts -t md

Length of output: 7948

</NavigationMenuList>
</NavigationMenu>
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ export default async function RunbookLayout({
<SystemBreadcrumbNavbar params={params} />
</div>
</TopNav>
<RunbookNavBar totalJobs={total} />
<RunbookNavBar totalJobs={total} runbook={runbook} />

<div className="scrollbar-thin scrollbar-thumb-neutral-800 scrollbar-track-neutral-900 h-[calc(100vh-53px-49px)] overflow-auto">
{children}
Expand Down
Loading
Loading