fix: Init hooks ui #216
fix: Init hooks ui #216
Conversation
WalkthroughThe changes introduce a comprehensive set of features for managing deployment hooks within the application. This includes the addition of a navigation item for "Hooks" in the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (22)
packages/validators/src/events/hooks/index.ts (1)
21-21: Consider adding runtime validation for action valuesWhile the implementation is type-safe, consider adding runtime validation to ensure the extracted actions are non-empty strings and match the expected format (e.g., 'deployment.target.*').
-export const hookActions = z.enum(hookActionsList as [string, ...string[]]); +// Validate that we have at least one action +if (hookActionsList.length === 0) { + throw new Error('hookActionsList must not be empty'); +} + +// Validate action format +const isValidAction = (action: string) => /^deployment\.target\.[a-z]+$/.test(action); +if (!hookActionsList.every(isValidAction)) { + throw new Error('Invalid action format detected'); +} + +export const hookActions = z.enum(hookActionsList as [string, ...string[]]);packages/db/src/schema/event.ts (2)
26-31: Consider enhancing schema validationWhile the basic schema structure is good, consider adding these validations to improve data integrity:
- Validation for the
namefield (e.g., min/max length, allowed characters)- Validation for the
actionfield (e.g., enum of allowed actions)- Validation for the
scopeTypefield (e.g., enum of allowed scope types)Example enhancement:
export const createHook = createInsertSchema(hook) .omit({ id: true }) + .extend({ + name: z.string().min(1).max(100).regex(/^[a-zA-Z0-9-_\s]+$/), + action: z.enum(['pre_deploy', 'post_deploy']), + scopeType: z.enum(['deployment', 'environment']), runbookIds: z.array(z.string().uuid()), + });
Line range hint
1-52: Consider the relationship between events and hooksThe
eventtable exists in the same file but doesn't have any direct relationship with the hook functionality. Consider whether:
- Events should track hook executions
- Hook actions should generate events
- These should be separated into different schema files if truly unrelated
This could improve the system's observability and maintain a clear audit trail of hook executions.
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/HookActionsDropdown.tsx (3)
1-23: LGTM! Consider adding JSDoc comments for type definitions.The imports and type definitions are well-organized and properly typed. The component is correctly marked for client-side rendering.
Consider adding JSDoc comments to document the
HookActionsDropdownPropsinterface:+/** + * Props for the HookActionsDropdown component + * @property {Hook} hook - The hook object containing hook details + * @property {SCHEMA.Runbook[]} runbooks - Array of available runbooks + * @property {React.ReactNode} children - React nodes to render as dropdown trigger + */ type HookActionsDropdownProps = { hook: Hook; runbooks: SCHEMA.Runbook[]; children: React.ReactNode; };
30-30: Consider using controlled state for better dialog-dropdown coordination.The current implementation might lead to state inconsistencies if dialogs are closed via escape key or clicking outside.
Consider implementing a useEffect to reset the dropdown state when dialogs close:
const [open, setOpen] = useState(false); +const [dialogOpen, setDialogOpen] = useState(false); + +useEffect(() => { + if (dialogOpen) { + setOpen(false); + } +}, [dialogOpen]);
32-60: Consider adding error boundaries for dialog components.The component should gracefully handle potential errors in the dialog components.
Consider wrapping the DropdownMenu content with an error boundary:
+import { ErrorBoundary } from '@ctrlplane/ui/error-boundary'; return ( <DropdownMenu open={open} onOpenChange={setOpen}> <DropdownMenuTrigger asChild>{children}</DropdownMenuTrigger> + <ErrorBoundary fallback={<div>Something went wrong</div>}> <DropdownMenuContent> {/* ... existing content ... */} </DropdownMenuContent> + </ErrorBoundary> </DropdownMenu> );apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/HooksTable.tsx (4)
1-16: Consider grouping IconDots with UI componentsFor better organization, consider moving the
IconDotsimport to be grouped with other UI-related imports.-import { IconDots } from "@tabler/icons-react"; - import { Badge } from "@ctrlplane/ui/badge"; import { Button } from "@ctrlplane/ui/button"; +import { IconDots } from "@tabler/icons-react";
18-23: Add JSDoc comments to describe type structuresConsider adding JSDoc comments to document the structure and purpose of these types, especially the
Hooktype which is derived from the API output.+/** Represents a deployment hook as returned by the API */ type Hook = RouterOutputs["deployment"]["hook"]["list"][number]; +/** Props for the HooksTable component */ type HooksTableProps = { + /** Array of hooks to display in the table */ hooks: Hook[]; + /** Array of available runbooks that can be associated with hooks */ runbooks: SCHEMA.Runbook[]; };
44-56: Improve runbooks cell overflow handlingThe current overflow handling might truncate important information. Consider adding a tooltip or expandable view for overflow content.
<TableCell> - <div className="flex items-center gap-2 overflow-hidden"> + <div className="flex items-center gap-2 flex-wrap"> {hook.runhooks.map((rh) => ( <Badge key={rh.id} variant="outline" - className="flex items-center gap-2" + className="flex items-center gap-2 mb-1" + title={rh.runbook.name} > {rh.runbook.name} </Badge> ))} </div> </TableCell>
58-63: Add tooltip to actions buttonThe actions button icon alone might not be clear enough for users. Consider adding a tooltip.
- <Button variant="ghost" size="icon" className="h-6 w-6"> + <Button + variant="ghost" + size="icon" + className="h-6 w-6" + title="Hook actions" + aria-label="Hook actions" + > <IconDots className="h-4 w-4" /> </Button>apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/DeleteHookDialog.tsx (1)
45-75: Enhance accessibility of the dialog.The dialog implementation is solid, but could benefit from improved accessibility.
Consider these enhancements:
<AlertDialog open={open} onOpenChange={(o) => { setOpen(o); if (!o) onClose(); }} > - <AlertDialogTrigger asChild>{children}</AlertDialogTrigger> + <AlertDialogTrigger asChild aria-label="Delete hook">{children}</AlertDialogTrigger> <AlertDialogContent> <AlertDialogHeader> <AlertDialogTitle> Are you sure you want to delete this hook? </AlertDialogTitle> <AlertDialogDescription> This action cannot be undone. </AlertDialogDescription> </AlertDialogHeader> <AlertDialogFooter> - <AlertDialogCancel>Cancel</AlertDialogCancel> + <AlertDialogCancel aria-label="Cancel deletion">Cancel</AlertDialogCancel> <AlertDialogAction onClick={onDelete} disabled={deleteHook.isPending} className={buttonVariants({ variant: "destructive" })} + aria-label="Confirm deletion" > - Delete + {deleteHook.isPending ? "Deleting..." : "Delete"} </AlertDialogAction> </AlertDialogFooter> </AlertDialogContent> </AlertDialog>apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/NavigationMenuAction.tsx (1)
27-29: Consider handling loading and error states for runbooks query.While the query implementation is correct, the UI might benefit from handling loading and error states, similar to how release channels are handled. This would prevent showing the "New Hook" button when the runbooks data is still loading or when there's an error.
- const runbooksQ = api.runbook.bySystemId.useQuery(systemId); - const runbooks = runbooksQ.data ?? []; + const runbooksQ = api.runbook.bySystemId.useQuery(systemId); + const runbooks = runbooksQ.data ?? []; + + if (runbooksQ.isError) { + return null; // or show an error state + }packages/validators/src/auth/index.ts (1)
96-100: LGTM! Consider grouping hook permissions with other resource permissions.The new hook permissions follow the established naming conventions and provide a complete set of CRUD operations. However, for better organization, consider moving these permissions to group them with other resource-specific permissions (like ResourceCreate, ResourceList, etc.) rather than adding them at the end.
ResourceViewUpdate = "resourceView.update", ResourceViewDelete = "resourceView.delete", + HookCreate = "hook.create", + HookGet = "hook.get", + HookList = "hook.list", + HookUpdate = "hook.update", + HookDelete = "hook.delete", ResourceMetadataGroupList = "resourceMetadataGroup.list",apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/DeploymentNavBar.tsx (2)
64-70: Consider simplifying the settings active state logicWhile the current implementation works correctly, it might become harder to maintain as more navigation items are added. Consider refactoring to a more scalable approach.
Here's a suggested improvement:
- const isSettingsActive = - !isReleasesActive && - !isVariablesActive && - !isJobsActive && - !isReleaseChannelsActive && - !isHooksActive; + const isSettingsActive = pathname === overviewUrl;This approach:
- Is more direct and explicit
- Reduces the need to update this check when adding new routes
- Makes the code more maintainable
Line range hint
111-121: Fix incorrect URL in Jobs navigation itemThe Jobs navigation item is currently using
variablesUrlinstead of a dedicatedjobsUrl.Here's how to fix it:
+ const jobsUrl = `/${workspaceSlug}/systems/${systemSlug}/deployments/${deploymentSlug}/jobs`; <NavigationMenuItem> - <Link href={variablesUrl} legacyBehavior passHref> + <Link href={jobsUrl} 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={isJobsActive} > Jobs </NavigationMenuLink> </Link> </NavigationMenuItem>apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/CreateHookDialog.tsx (3)
49-53: Consider enhancing the name field validation.The current validation only checks for minimum length. Consider adding:
- Maximum length constraint to prevent UI overflow and DB issues
- Pattern validation to restrict special characters if needed
const schema = z.object({ - name: z.string().min(1), + name: z.string().min(1).max(100).regex(/^[a-zA-Z0-9-_ ]+$/), action: hookActions, runbookIds: z.array(z.object({ id: z.string().uuid() })), });
67-80: Enhance form submission UX with loading and success states.The form submission could benefit from better user feedback:
- Show loading state during submission
- Display success message before closing
- Simplify promise chain
const onSubmit = form.handleSubmit((data) => createHook .mutateAsync({ ...data, scopeType: "deployment", scopeId: deploymentId, runbookIds: data.runbookIds.map((r) => r.id), }) - .then(() => utils.deployment.hook.list.invalidate(deploymentId)) - .then(() => router.refresh()) - .then(() => setOpen(false)), + .then(async () => { + await utils.deployment.hook.list.invalidate(deploymentId); + router.refresh(); + toast.success('Hook created successfully'); + setOpen(false); + }) + .catch((error) => { + toast.error(error.message); + }) );
82-90: Optimize performance with memoization.The runbook filtering logic could be memoized to prevent unnecessary recalculations:
+ const selectedRunbookIds = useMemo(() => + form.watch("runbookIds").map((r) => r.id), + [form.watch("runbookIds")] + ); + + const unselectedRunbooks = useMemo(() => + runbooks.filter((r) => !selectedRunbookIds.includes(r.id)), + [runbooks, selectedRunbookIds] + ); - const selectedRunbookIds = form.watch("runbookIds").map((r) => r.id); - const unselectedRunbooks = runbooks.filter( - (r) => !selectedRunbookIds.includes(r.id), - );apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/EditHookDialog.tsx (2)
50-54: Consider enhancing the schema validation.The current schema could be strengthened to ensure more robust validation:
- Add
.trim()to the name field to prevent whitespace-only names- Add
.max()to the name field to prevent excessively long names- Consider adding
.nonempty()to runbookIds array to ensure at least one runbook is selectedconst schema = z.object({ - name: z.string().min(1), + name: z.string().trim().min(1).max(100), action: hookActions, - runbookIds: z.array(z.object({ id: z.string().uuid() })), + runbookIds: z.array(z.object({ id: z.string().uuid() })).nonempty("At least one runbook is required"), });
97-167: Add loading state indicators for better UX.Consider enhancing the user experience by showing loading states:
<DialogContent> - <DialogHeader>Edit Hook</DialogHeader> + <DialogHeader> + {updateHook.isPending ? 'Saving Hook...' : 'Edit Hook'} + </DialogHeader> <Form {...form}> <form onSubmit={onSubmit} className="space-y-4"> <FormField control={form.control} name="name" render={({ field }) => ( <FormItem> <FormLabel>Name</FormLabel> <FormControl> - <Input {...field} /> + <Input {...field} disabled={updateHook.isPending} /> </FormControl> <FormMessage /> </FormItem> )} />packages/api/src/router/deployment.ts (2)
186-198: Refactor repetitive authorization checks into a helper functionThe
authorizationChecklogic in thebyId,update, anddeleteprocedures is similar. Consider creating a helper function to reduce code duplication and improve maintainability.Example refactoring:
+const authorizeHookAccess = async (canUser, ctx, hookId, permission) => { + const h = await ctx.db + .select() + .from(hook) + .where(eq(hook.id, hookId)) + .then(takeFirstOrNull); + if (h == null || h.scopeType !== "deployment") return false; + return canUser.perform(permission).on({ + type: "deployment", + id: h.scopeId, + }); +}; // In 'byId' procedure .meta({ authorizationCheck: async ({ canUser, ctx, input }) => - // existing code + authorizeHookAccess(canUser, ctx, input, Permission.HookGet), }) // In 'update' procedure .meta({ authorizationCheck: async ({ canUser, ctx, input }) => - // existing code + authorizeHookAccess(canUser, ctx, input.id, Permission.HookUpdate), }) // In 'delete' procedure .meta({ authorizationCheck: async ({ canUser, ctx, input }) => - // existing code + authorizeHookAccess(canUser, ctx, input, Permission.HookDelete), })Also applies to: 236-248, 275-287
166-292: Recommend adding unit tests for 'hookRouter' proceduresTo ensure the new
hookRouterfunctions correctly and to prevent future regressions, consider adding unit tests covering all CRUD operations and edge cases.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (14)
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/DeploymentNavBar.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/NavigationMenuAction.tsx(3 hunks)apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/CreateHookDialog.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/DeleteHookDialog.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/EditHookDialog.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/HookActionsDropdown.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/HooksTable.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/page.tsx(1 hunks)packages/api/src/router/deployment.ts(3 hunks)packages/api/src/router/runbook.ts(1 hunks)packages/db/src/schema/event.ts(3 hunks)packages/db/src/schema/runbook.ts(2 hunks)packages/validators/src/auth/index.ts(1 hunks)packages/validators/src/events/hooks/index.ts(1 hunks)
🔇 Additional comments (26)
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/page.tsx (3)
1-4: LGTM! Clean and focused imports.
The imports are well-organized and include only the necessary dependencies.
6-10: LGTM! Well-structured component definition.
The component follows Next.js server component conventions with proper TypeScript types.
4-4: Verify HooksTable component interface.
Let's ensure the HooksTable component exists and accepts the correct props.
Also applies to: 16-16
✅ Verification successful
Let me gather more information about the HooksTable component interface.
HooksTable component interface is correctly implemented
The HooksTable component exists and properly accepts the required props:
hooksprop of typeHook[](derived from RouterOutputs)runbooksprop of typeSCHEMA.Runbook[]
The component correctly uses these props to render a table displaying hook details and associated runbooks.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify HooksTable component existence and interface
# Check if HooksTable component exists
fd "HooksTable.tsx" --type f
# Check component interface
ast-grep --pattern 'interface $interface {
hooks: $_;
runbooks: $_;
}'
Length of output: 216
Script:
#!/bin/bash
# Check the content of HooksTable.tsx to verify its interface and usage
cat "apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/HooksTable.tsx"
Length of output: 2100
packages/validators/src/events/hooks/index.ts (2)
18-20: LGTM: Robust extraction of action values from schemas
The implementation correctly extracts action values from the hook event schemas using type-safe operations.
16-21: Verify the impact on existing hook actions
Let's ensure that all existing hook actions in the codebase are compatible with the new enum.
✅ Verification successful
The previous search revealed some hook actions in use (deployment.target.removed and deployment.target.deleted), but we need to verify these against the schema definition to ensure all actions are properly included in the enum. Let's gather this additional context.
We need to see the complete schema definitions to verify that the enum creation from hookActionsList includes all the necessary actions we found (deployment.target.removed and deployment.target.deleted).
Hook actions enum is correctly derived from schema definitions
The verification confirms that the hookActions enum is correctly derived from the schema definitions:
- The
hookEventunion includes bothtargetRemovedandtargetDeletedschemas - These schemas define the actions
deployment.target.removedanddeployment.target.deletedrespectively - The
hookActionsListcorrectly extracts these actions from the schema using.options.map() - All hook action usages found in the codebase match these defined actions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for hook action usage in the codebase
# Expected: All hook actions should match the ones defined in hookActionsList
# Find all string literals that look like hook actions
echo "Searching for hook action usage..."
rg -g '*.{ts,tsx}' '"deployment\.target\.[a-z]+"' -A 2
# Find all references to hookActions enum
echo "Searching for hookActions enum usage..."
rg -g '*.{ts,tsx}' 'hookActions\.' -A 2
Length of output: 2108
Script:
#!/bin/bash
# Let's verify the hook actions defined in the schema
echo "Checking hook event schema definitions..."
ast-grep --pattern 'const hookEvent = z.discriminatedUnion("action", [
$$$
]);'
# Let's also check for any other hook action literals that might be defined
echo "Checking for other hook action literals..."
rg -g '*.{ts,tsx}' 'z\.literal\("deployment\.target\.[a-z]+"\)' -A 1
Length of output: 711
Script:
#!/bin/bash
# Let's check the actual content of the hooks schema file to see the full definition
echo "Checking the hooks schema file..."
cat packages/validators/src/events/hooks/target.ts
# Also check the index file where the enum is defined
echo "Checking the index file with enum definition..."
cat packages/validators/src/events/hooks/index.ts
Length of output: 1626
packages/db/src/schema/event.ts (2)
1-5: LGTM: Imports are well-structured and necessary
The new imports are appropriate for the schema definitions and type inference requirements.
32-34: LGTM: Relations are well-defined with proper referential integrity
The relationships between hooks, runhooks, and runbooks are correctly established with appropriate cascade delete rules.
Let's verify there are no orphaned records in the existing data:
Also applies to: 46-52
✅ Verification successful
Relations are properly defined with correct cascade rules
The verification confirms that:
- The hook-to-runhook relation is correctly implemented with cascade delete
- The runhook table has proper foreign key constraints to both hook and runbook tables
- All foreign keys are set up with
ON DELETE cascadeensuring referential integrity - The schema matches the TypeScript definitions in
packages/db/src/schema/event.ts
The migration files and schema definitions show that orphaned records are prevented by the database's referential integrity constraints.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any SQL files that might contain existing data migrations
# to ensure they handle potential orphaned records
# Look for migration files
fd -e sql | while read -r file; do
echo "Checking $file for potential data migrations..."
# Look for INSERT/UPDATE statements that might affect these tables
rg -i "insert into (hook|runhook|runbook)|update (hook|runhook|runbook)" "$file"
done
Length of output: 12260
Script:
#!/bin/bash
# Let's check the table creation statements to verify the foreign key constraints and cascade rules
# First, check the table creation statements
fd -e sql | while read -r file; do
echo "=== Checking $file for table definitions ==="
rg -i "CREATE TABLE.*?(hook|runhook|runbook)|FOREIGN KEY.*?(hook|runhook|runbook)" "$file"
done
# Also check the schema definition in TypeScript for completeness
echo -e "\n=== Checking TypeScript schema definitions ==="
rg -i "references|foreignKey" "packages/db/src/schema"
Length of output: 23182
packages/db/src/schema/runbook.ts (2)
2-2: LGTM: Required imports for relationship definition
The new imports are necessary and properly used for defining the relationship between runbooks and runhooks.
Also applies to: 7-7
37-39: Verify foreign key constraints in the runhook table
The one-to-many relationship between runbook and runhooks is correctly defined. However, let's verify that the corresponding foreign key in the runhook table is properly configured with appropriate cascade behavior.
✅ Verification successful
Foreign key constraints are properly configured ✅
The runhook table has the correct foreign key setup with runbookId referencing runbook.id and appropriate cascade delete behavior configured. The relationship is well-defined on both sides:
runbookRelationsestablishes the one-to-many relationshiprunhooktable hasrunbookIdas foreign key withonDelete: "cascade"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the foreign key configuration in the runhook table
# Expected: Should find a foreign key reference to runbook.id with appropriate ON DELETE behavior
# Search for runhook table definition and its foreign key constraints
ast-grep --pattern 'export const runhook = pgTable($_, {
$$$
runbookId: $$$
$$$
})'
Length of output: 706
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/HookActionsDropdown.tsx (1)
25-61: Verify integration with HooksTable component.
The component implementation looks good, but let's verify its integration with the HooksTable component.
✅ Verification successful
Integration with HooksTable component is properly implemented
The HookActionsDropdown is correctly integrated within the HooksTable component:
- It's properly imported in HooksTable.tsx
- Receives required props:
hookandrunbooks - Used as an action button in the table rows with correct children (Button with IconDots)
- The HooksTable itself is properly used in the page component with the necessary props
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify HookActionsDropdown integration in HooksTable
# Check how HookActionsDropdown is used in HooksTable
rg -l "HookActionsDropdown" --type typescript
Length of output: 80
Script:
#!/bin/bash
# Search for HookActionsDropdown usage in all files
rg "HookActionsDropdown" -A 5
# Also search for files containing "HooksTable" to find the table component
rg "HooksTable" -A 5
Length of output: 6644
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/DeleteHookDialog.tsx (1)
1-25: LGTM! Well-structured imports and type definitions.
The component's props interface is well-defined and the imports are clean and purposeful.
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/NavigationMenuAction.tsx (4)
10-10: LGTM!
The import follows the established pattern and maintains proper code organization.
21-21: LGTM!
The isHooksActive state follows the established pattern for feature-specific pathname checks.
51-57: LGTM!
The hooks UI implementation maintains consistency with other features and properly handles the required props.
59-59: LGTM! Verify mutual exclusivity of UI states.
The condition correctly prevents overlapping UI states between variables, release channels, hooks, and releases.
✅ Verification successful
Mutual exclusivity of UI states is correctly implemented
The implementation ensures that only one dialog can be shown at a time through:
- State variables derived from URL path segments (
isVariablesActive,isReleaseChannelsActive,isHooksActive) - Mutually exclusive conditional rendering of dialogs
- The release dialog is only shown when no other feature is active
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that only one dialog can be shown at a time by checking route definitions
# Check for any overlapping route patterns in the app directory
rg -A 5 "page\.(tsx|ts)" apps/webservice/src/app/
Length of output: 51
Script:
#!/bin/bash
# Let's check the component's implementation to verify state management
ast-grep --pattern 'const [isVariablesActive, isReleaseChannelsActive, isHooksActive] = $_'
# Also search for any related state updates
rg -B2 -A2 "setIs(Variables|ReleaseChannels|Hooks)Active" apps/webservice/src/app/
Length of output: 178
Script:
#!/bin/bash
# Let's check the component file directly to understand the state management
rg -B5 -A5 "isVariablesActive|isReleaseChannelsActive|isHooksActive" "apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/NavigationMenuAction.tsx"
# Also check for any dialog or modal components that might be controlled by these states
rg -B2 -A2 "Modal|Dialog" "apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/NavigationMenuAction.tsx"
Length of output: 3542
packages/api/src/router/runbook.ts (2)
Line range hint 39-58: LGTM! The query implementation looks solid with a few optimization opportunities.
The conversion from mutation to query is appropriate since this endpoint retrieves data rather than modifying it. The implementation correctly:
- Joins the necessary tables using appropriate foreign key relationships
- Groups the results by runbook ID
- Includes associated variables and job agent data
However, there are a few optimization opportunities to consider:
- The
leftJoinwithjobAgentcould be changed to aninnerJoinif a jobAgent is required for every runbook - Consider adding an index on
systemIdif not already present, as it's used in the WHERE clause
- .leftJoin(
- SCHEMA.jobAgent,
- eq(SCHEMA.runbook.jobAgentId, SCHEMA.jobAgent.id),
- )
+ .innerJoin(
+ SCHEMA.jobAgent,
+ eq(SCHEMA.runbook.jobAgentId, SCHEMA.jobAgent.id),
+ )Let's verify if jobAgent is required for runbooks:
39-39: Verify all clients are updated for the breaking change
Converting bySystemId from a mutation to a query is a breaking change that requires updates to all clients using this endpoint. The AI summary mentions this is used in CreateHookDialog, but there might be other clients.
Let's find all usages:
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/DeploymentNavBar.tsx (2)
57-57: LGTM: URL and state variables are well-structured!
The new hooks-related variables follow the established patterns for URL construction and active state detection.
Also applies to: 63-63
122-131: LGTM: Hooks navigation item is well-implemented!
The new Hooks navigation item follows the established patterns for styling and structure.
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/CreateHookDialog.tsx (1)
1-47: LGTM! Well-structured imports and type definitions.
The imports are properly organized and the props interface is well-typed with clear requirements.
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/EditHookDialog.tsx (2)
56-72: LGTM!
The component initialization and state management are well-structured, following React best practices with proper separation of concerns.
86-95: LGTM!
The runbook management logic is well-implemented with proper use of useFieldArray and efficient filtering of unselected runbooks.
packages/api/src/router/deployment.ts (4)
166-181: Good implementation of the 'list' procedure with proper authorization checks
The list procedure effectively retrieves hooks associated with a deployment and includes appropriate authorization checks to ensure secure access.
183-203: Proper 'byId' procedure with thorough authorization validation
The byId procedure correctly fetches a specific hook by its ID and includes comprehensive authorization checks, enhancing security.
233-269: Ensure errors within transactions are properly handled in 'update' procedure
In the update procedure, confirm that any potential errors during the transaction are appropriately managed to prevent partial updates.
Run the following script to verify error handling:
#!/bin/bash
# Description: Check for error handling within the transaction in the 'update' procedure
# Test: Look for try-catch blocks or error management code
rg -A 10 'ctx\.db\.transaction' packages/api/src/router/deployment.ts | rg 'try|catch'204-231: Handle database transaction errors in 'create' procedure
Ensure that any errors occurring during the transaction in the create procedure are properly caught and handled to maintain data integrity.
Run the following script to check for error handling in the transaction:
| const deployment = await api.deployment.bySlug(params); | ||
| if (!deployment) notFound(); | ||
|
|
||
| const hooks = await api.deployment.hook.list(deployment.id); | ||
| const runbooks = await api.runbook.bySystemId(deployment.systemId); | ||
| return <HooksTable hooks={hooks} runbooks={runbooks} />; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling and optimizing data fetching.
The current implementation has a few areas for improvement:
- Error handling is limited to deployment not found scenarios. Consider handling potential errors from
hook.listandrunbook.bySystemIdcalls. - The API calls are sequential, which may impact page load time.
Consider this improvement:
export default async function HooksPage({
params,
}: {
params: { workspaceSlug: string; systemSlug: string; deploymentSlug: string };
}) {
const deployment = await api.deployment.bySlug(params);
if (!deployment) notFound();
- const hooks = await api.deployment.hook.list(deployment.id);
- const runbooks = await api.runbook.bySystemId(deployment.systemId);
+ try {
+ const [hooks, runbooks] = await Promise.all([
+ api.deployment.hook.list(deployment.id),
+ api.runbook.bySystemId(deployment.systemId),
+ ]);
+ return <HooksTable hooks={hooks} runbooks={runbooks} />;
+ } catch (error) {
+ // Consider adding proper error boundary or error state handling
+ throw error;
+ }
- return <HooksTable hooks={hooks} runbooks={runbooks} />;
}📝 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.
| const deployment = await api.deployment.bySlug(params); | |
| if (!deployment) notFound(); | |
| const hooks = await api.deployment.hook.list(deployment.id); | |
| const runbooks = await api.runbook.bySystemId(deployment.systemId); | |
| return <HooksTable hooks={hooks} runbooks={runbooks} />; | |
| } | |
| const deployment = await api.deployment.bySlug(params); | |
| if (!deployment) notFound(); | |
| try { | |
| const [hooks, runbooks] = await Promise.all([ | |
| api.deployment.hook.list(deployment.id), | |
| api.runbook.bySystemId(deployment.systemId), | |
| ]); | |
| return <HooksTable hooks={hooks} runbooks={runbooks} />; | |
| } catch (error) { | |
| // Consider adding proper error boundary or error state handling | |
| throw error; | |
| } | |
| } |
| <DropdownMenuItem | ||
| onSelect={(e) => e.preventDefault()} | ||
| className="flex cursor-pointer items-center gap-2" | ||
| > | ||
| <IconEdit className="h-4 w-4" /> | ||
| Edit | ||
| </DropdownMenuItem> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility for dropdown menu items.
The dropdown menu items need proper ARIA attributes and roles for better accessibility.
Apply these improvements to both menu items:
<DropdownMenuItem
onSelect={(e) => e.preventDefault()}
className="flex cursor-pointer items-center gap-2"
+ role="menuitem"
+ aria-label="Edit hook"
>
- <IconEdit className="h-4 w-4" />
+ <IconEdit className="h-4 w-4" aria-hidden="true" />
Edit
</DropdownMenuItem>Also applies to: 50-56
| <TableRow> | ||
| <TableHead>Name</TableHead> | ||
| <TableHead>Event</TableHead> | ||
| <TableHead>Runbooks</TableHead> | ||
| <TableHead /> | ||
| </TableRow> | ||
| </TableHeader> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance table accessibility
Add ARIA labels and roles to improve accessibility for screen readers.
- <TableRow>
+ <TableRow role="row">
<TableHead>Name</TableHead>
<TableHead>Event</TableHead>
<TableHead>Runbooks</TableHead>
- <TableHead />
+ <TableHead aria-label="Actions" />
</TableRow>📝 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.
| <TableRow> | |
| <TableHead>Name</TableHead> | |
| <TableHead>Event</TableHead> | |
| <TableHead>Runbooks</TableHead> | |
| <TableHead /> | |
| </TableRow> | |
| </TableHeader> | |
| <TableRow role="row"> | |
| <TableHead>Name</TableHead> | |
| <TableHead>Event</TableHead> | |
| <TableHead>Runbooks</TableHead> | |
| <TableHead aria-label="Actions" /> | |
| </TableRow> | |
| </TableHeader> |
| export const HooksTable: React.FC<HooksTableProps> = ({ hooks, runbooks }) => ( | ||
| <Table className="table-fixed"> | ||
| <TableHeader> | ||
| <TableRow> | ||
| <TableHead>Name</TableHead> | ||
| <TableHead>Event</TableHead> | ||
| <TableHead>Runbooks</TableHead> | ||
| <TableHead /> | ||
| </TableRow> | ||
| </TableHeader> | ||
| <TableBody> | ||
| {hooks.map((hook) => ( | ||
| <TableRow key={hook.id}> | ||
| <TableCell>{hook.name}</TableCell> | ||
| <TableCell> | ||
| <span className="rounded-md border-x border-y px-1 font-mono text-red-400"> | ||
| {hook.action} | ||
| </span> | ||
| </TableCell> | ||
| <TableCell> | ||
| <div className="flex items-center gap-2 overflow-hidden"> | ||
| {hook.runhooks.map((rh) => ( | ||
| <Badge | ||
| key={rh.id} | ||
| variant="outline" | ||
| className="flex items-center gap-2" | ||
| > | ||
| {rh.runbook.name} | ||
| </Badge> | ||
| ))} | ||
| </div> | ||
| </TableCell> | ||
| <TableCell className="text-right"> | ||
| <HookActionsDropdown hook={hook} runbooks={runbooks}> | ||
| <Button variant="ghost" size="icon" className="h-6 w-6"> | ||
| <IconDots className="h-4 w-4" /> | ||
| </Button> | ||
| </HookActionsDropdown> | ||
| </TableCell> | ||
| </TableRow> | ||
| ))} | ||
| </TableBody> | ||
| </Table> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add empty state handling and error boundaries
The table currently doesn't handle the case when there are no hooks to display. Consider adding an empty state message and wrapping the component in an error boundary.
export const HooksTable: React.FC<HooksTableProps> = ({ hooks, runbooks }) => (
+ <>
+ {hooks.length === 0 ? (
+ <div className="text-center py-4 text-gray-500">
+ No hooks configured for this deployment
+ </div>
+ ) : (
<Table className="table-fixed">
{/* ... existing code ... */}
</Table>
+ )}
+ </>
);Committable suggestion skipped: line range outside the PR's diff.
| const onDelete = () => | ||
| deleteHook | ||
| .mutateAsync(hookId) | ||
| .then(() => utils.deployment.hook.list.invalidate(hookId)) | ||
| .then(() => router.refresh()) | ||
| .then(() => setOpen(false)) | ||
| .then(() => onClose()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve delete handler implementation.
The current implementation has several areas for improvement:
- The promise chain could be simplified using async/await
- Error handling should be added
- The cache invalidation might be using hookId incorrectly as it's typically used with query parameters
Consider this enhancement:
- const onDelete = () =>
- deleteHook
- .mutateAsync(hookId)
- .then(() => utils.deployment.hook.list.invalidate(hookId))
- .then(() => router.refresh())
- .then(() => setOpen(false))
- .then(() => onClose());
+ const onDelete = async () => {
+ try {
+ await deleteHook.mutateAsync(hookId);
+ await utils.deployment.hook.list.invalidate(); // Remove hookId parameter
+ router.refresh();
+ setOpen(false);
+ onClose();
+ } catch (error) {
+ console.error('Failed to delete hook:', error);
+ // Consider adding a toast notification here
+ }
+ };📝 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.
| const onDelete = () => | |
| deleteHook | |
| .mutateAsync(hookId) | |
| .then(() => utils.deployment.hook.list.invalidate(hookId)) | |
| .then(() => router.refresh()) | |
| .then(() => setOpen(false)) | |
| .then(() => onClose()); | |
| const onDelete = async () => { | |
| try { | |
| await deleteHook.mutateAsync(hookId); | |
| await utils.deployment.hook.list.invalidate(); // Remove hookId parameter | |
| router.refresh(); | |
| setOpen(false); | |
| onClose(); | |
| } catch (error) { | |
| console.error('Failed to delete hook:', error); | |
| // Consider adding a toast notification here | |
| } | |
| }; |
| <IconSelector className="h-4 w-4" /> | ||
| {value === "" ? "Select action..." : value} | ||
| </Button> | ||
| </PopoverTrigger> | ||
| <PopoverContent align="start" className="w-[462px] p-0"> | ||
| <Command> | ||
| <CommandInput placeholder="Search action..." /> | ||
| <CommandList> | ||
| {hookActionsList.map((action) => ( | ||
| <CommandItem | ||
| key={action} | ||
| value={action} | ||
| onSelect={() => { | ||
| onChange(action); | ||
| setActionsOpen(false); | ||
| }} | ||
| > | ||
| {action} | ||
| </CommandItem> | ||
| ))} | ||
| </CommandList> | ||
| </Command> | ||
| </PopoverContent> | ||
| </Popover> | ||
| </FormControl> | ||
| </FormItem> | ||
| )} | ||
| /> | ||
|
|
||
| <div className="flex flex-col gap-2"> | ||
| <Label>Runbooks</Label> | ||
| <div className="flex flex-wrap gap-2"> | ||
| {fields.map((field, index) => ( | ||
| <FormField | ||
| key={field.id} | ||
| control={form.control} | ||
| name={`runbookIds.${index}.id`} | ||
| render={({ field }) => { | ||
| const runbook = runbooks.find( | ||
| (r) => r.id === field.value, | ||
| ); | ||
| return ( | ||
| <Badge | ||
| variant="outline" | ||
| className="flex items-center gap-2" | ||
| > | ||
| {runbook?.name ?? ""} | ||
| <IconX | ||
| className="h-4 w-4 cursor-pointer" | ||
| onClick={() => remove(index)} | ||
| /> | ||
| </Badge> | ||
| ); | ||
| }} | ||
| /> | ||
| ))} | ||
| </div> | ||
| </div> | ||
|
|
||
| <Popover open={runbooksOpen} onOpenChange={setRunbooksOpen}> | ||
| <PopoverTrigger asChild> | ||
| <Button variant="outline" className="flex items-center gap-2"> | ||
| <IconPlus className="h-4 w-4" /> | ||
| Add Runbook | ||
| </Button> | ||
| </PopoverTrigger> | ||
| <PopoverContent align="start" className="p-0"> | ||
| <Command> | ||
| <CommandInput placeholder="Search runbook..." /> | ||
| <CommandList> | ||
| {unselectedRunbooks.map((runbook) => ( | ||
| <CommandItem | ||
| key={runbook.id} | ||
| onSelect={() => { | ||
| append({ id: runbook.id }); | ||
| setRunbooksOpen(false); | ||
| }} | ||
| > | ||
| {runbook.name} | ||
| </CommandItem> | ||
| ))} | ||
| </CommandList> | ||
| </Command> | ||
| </PopoverContent> | ||
| </Popover> | ||
|
|
||
| <DialogFooter> | ||
| <Button | ||
| type="submit" | ||
| disabled={createHook.isPending || !form.formState.isDirty} | ||
| > | ||
| Create | ||
| </Button> | ||
| </DialogFooter> | ||
| </form> | ||
| </Form> | ||
| </DialogContent> | ||
| </Dialog> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility and responsive design.
Consider the following improvements:
- Add aria-labels to interactive elements
- Improve keyboard navigation
- Enhance mobile responsiveness
<Dialog open={open} onOpenChange={setOpen}>
- <DialogTrigger asChild>{children}</DialogTrigger>
+ <DialogTrigger asChild aria-label="Open create hook dialog">{children}</DialogTrigger>
<DialogContent>
<DialogHeader>
<DialogTitle>Create Hook</DialogTitle>
<DialogDescription>
Trigger actions for events in this deployment.
</DialogDescription>
</DialogHeader>
- <Form {...form}>
+ <Form {...form} className="max-w-full">
<form onSubmit={onSubmit} className="space-y-4">
// ... existing form fields ...
<div className="flex flex-col gap-2">
- <div className="flex flex-wrap gap-2">
+ <div className="flex flex-wrap gap-2 max-h-[200px] overflow-y-auto">
// ... existing badge list ...
</div>
</div>
// ... rest of the form ...
</form>
</Form>
</DialogContent>
</Dialog>Committable suggestion skipped: line range outside the PR's diff.
| const form = useForm({ schema, defaultValues }); | ||
| const onSubmit = form.handleSubmit((data) => | ||
| updateHook | ||
| .mutateAsync({ | ||
| id: hook.id, | ||
| data: { ...data, runbookIds: data.runbookIds.map((r) => r.id) }, | ||
| }) | ||
| .then(() => utils.deployment.hook.list.invalidate(hook.scopeId)) | ||
| .then(() => router.refresh()) | ||
| .then(() => setOpen(false)) | ||
| .then(() => onClose()), | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and simplify the promise chain.
The form submission logic needs error handling and could be simplified:
const onSubmit = form.handleSubmit((data) =>
updateHook
.mutateAsync({
id: hook.id,
data: { ...data, runbookIds: data.runbookIds.map((r) => r.id) },
})
- .then(() => utils.deployment.hook.list.invalidate(hook.scopeId))
- .then(() => router.refresh())
- .then(() => setOpen(false))
- .then(() => onClose()),
+ .then(async () => {
+ await utils.deployment.hook.list.invalidate(hook.scopeId);
+ router.refresh();
+ setOpen(false);
+ onClose();
+ })
+ .catch((error) => {
+ console.error('Failed to update hook:', error);
+ // Consider adding a toast notification here
+ }),
);📝 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.
| const form = useForm({ schema, defaultValues }); | |
| const onSubmit = form.handleSubmit((data) => | |
| updateHook | |
| .mutateAsync({ | |
| id: hook.id, | |
| data: { ...data, runbookIds: data.runbookIds.map((r) => r.id) }, | |
| }) | |
| .then(() => utils.deployment.hook.list.invalidate(hook.scopeId)) | |
| .then(() => router.refresh()) | |
| .then(() => setOpen(false)) | |
| .then(() => onClose()), | |
| ); | |
| const form = useForm({ schema, defaultValues }); | |
| const onSubmit = form.handleSubmit((data) => | |
| updateHook | |
| .mutateAsync({ | |
| id: hook.id, | |
| data: { ...data, runbookIds: data.runbookIds.map((r) => r.id) }, | |
| }) | |
| .then(async () => { | |
| await utils.deployment.hook.list.invalidate(hook.scopeId); | |
| router.refresh(); | |
| setOpen(false); | |
| onClose(); | |
| }) | |
| .catch((error) => { | |
| console.error('Failed to update hook:', error); | |
| // Consider adding a toast notification here | |
| }), | |
| ); |
| <div className="flex flex-col gap-2"> | ||
| <Label>Runbooks</Label> | ||
| <div className="flex flex-wrap gap-2"> | ||
| {fields.map((field, index) => ( | ||
| <FormField | ||
| key={field.id} | ||
| control={form.control} | ||
| name={`runbookIds.${index}.id`} | ||
| render={({ field }) => { | ||
| const runbook = runbooks.find( | ||
| (r) => r.id === field.value, | ||
| ); | ||
| return ( | ||
| <Badge | ||
| variant="outline" | ||
| className="flex items-center gap-2" | ||
| > | ||
| {runbook?.name ?? ""} | ||
| <IconX | ||
| className="h-4 w-4 cursor-pointer" | ||
| onClick={() => remove(index)} | ||
| /> | ||
| </Badge> | ||
| ); | ||
| }} | ||
| /> | ||
| ))} | ||
| </div> | ||
| </div> | ||
|
|
||
| <Popover open={runbooksOpen} onOpenChange={setRunbooksOpen}> | ||
| <PopoverTrigger asChild> | ||
| <Button variant="outline" className="flex items-center gap-2"> | ||
| <IconPlus className="h-4 w-4" /> | ||
| Add Runbook | ||
| </Button> | ||
| </PopoverTrigger> | ||
| <PopoverContent align="start" className="p-0"> | ||
| <Command> | ||
| <CommandInput placeholder="Search runbook..." /> | ||
| <CommandList> | ||
| {unselectedRunbooks.map((runbook) => ( | ||
| <CommandItem | ||
| key={runbook.id} | ||
| onSelect={() => { | ||
| append({ id: runbook.id }); | ||
| setRunbooksOpen(false); | ||
| }} | ||
| > | ||
| {runbook.name} | ||
| </CommandItem> | ||
| ))} | ||
| </CommandList> | ||
| </Command> | ||
| </PopoverContent> | ||
| </Popover> | ||
| <DialogFooter> | ||
| <Button | ||
| type="submit" | ||
| disabled={updateHook.isPending || !form.formState.isDirty} | ||
| > | ||
| Save | ||
| </Button> | ||
| </DialogFooter> | ||
| </form> | ||
| </Form> | ||
| </DialogContent> | ||
| </Dialog> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility for runbook removal.
The remove runbook button (IconX) needs proper accessibility attributes:
<IconX
className="h-4 w-4 cursor-pointer"
+ role="button"
+ aria-label={`Remove runbook ${runbook?.name}`}
onClick={() => remove(index)}
/>📝 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.
| <div className="flex flex-col gap-2"> | |
| <Label>Runbooks</Label> | |
| <div className="flex flex-wrap gap-2"> | |
| {fields.map((field, index) => ( | |
| <FormField | |
| key={field.id} | |
| control={form.control} | |
| name={`runbookIds.${index}.id`} | |
| render={({ field }) => { | |
| const runbook = runbooks.find( | |
| (r) => r.id === field.value, | |
| ); | |
| return ( | |
| <Badge | |
| variant="outline" | |
| className="flex items-center gap-2" | |
| > | |
| {runbook?.name ?? ""} | |
| <IconX | |
| className="h-4 w-4 cursor-pointer" | |
| onClick={() => remove(index)} | |
| /> | |
| </Badge> | |
| ); | |
| }} | |
| /> | |
| ))} | |
| </div> | |
| </div> | |
| <Popover open={runbooksOpen} onOpenChange={setRunbooksOpen}> | |
| <PopoverTrigger asChild> | |
| <Button variant="outline" className="flex items-center gap-2"> | |
| <IconPlus className="h-4 w-4" /> | |
| Add Runbook | |
| </Button> | |
| </PopoverTrigger> | |
| <PopoverContent align="start" className="p-0"> | |
| <Command> | |
| <CommandInput placeholder="Search runbook..." /> | |
| <CommandList> | |
| {unselectedRunbooks.map((runbook) => ( | |
| <CommandItem | |
| key={runbook.id} | |
| onSelect={() => { | |
| append({ id: runbook.id }); | |
| setRunbooksOpen(false); | |
| }} | |
| > | |
| {runbook.name} | |
| </CommandItem> | |
| ))} | |
| </CommandList> | |
| </Command> | |
| </PopoverContent> | |
| </Popover> | |
| <DialogFooter> | |
| <Button | |
| type="submit" | |
| disabled={updateHook.isPending || !form.formState.isDirty} | |
| > | |
| Save | |
| </Button> | |
| </DialogFooter> | |
| </form> | |
| </Form> | |
| </DialogContent> | |
| </Dialog> | |
| ); | |
| }; | |
| <div className="flex flex-col gap-2"> | |
| <Label>Runbooks</Label> | |
| <div className="flex flex-wrap gap-2"> | |
| {fields.map((field, index) => ( | |
| <FormField | |
| key={field.id} | |
| control={form.control} | |
| name={`runbookIds.${index}.id`} | |
| render={({ field }) => { | |
| const runbook = runbooks.find( | |
| (r) => r.id === field.value, | |
| ); | |
| return ( | |
| <Badge | |
| variant="outline" | |
| className="flex items-center gap-2" | |
| > | |
| {runbook?.name ?? ""} | |
| <IconX | |
| className="h-4 w-4 cursor-pointer" | |
| role="button" | |
| aria-label={`Remove runbook ${runbook?.name}`} | |
| onClick={() => remove(index)} | |
| /> | |
| </Badge> | |
| ); | |
| }} | |
| /> | |
| ))} | |
| </div> | |
| </div> | |
| <Popover open={runbooksOpen} onOpenChange={setRunbooksOpen}> | |
| <PopoverTrigger asChild> | |
| <Button variant="outline" className="flex items-center gap-2"> | |
| <IconPlus className="h-4 w-4" /> | |
| Add Runbook | |
| </Button> | |
| </PopoverTrigger> | |
| <PopoverContent align="start" className="p-0"> | |
| <Command> | |
| <CommandInput placeholder="Search runbook..." /> | |
| <CommandList> | |
| {unselectedRunbooks.map((runbook) => ( | |
| <CommandItem | |
| key={runbook.id} | |
| onSelect={() => { | |
| append({ id: runbook.id }); | |
| setRunbooksOpen(false); | |
| }} | |
| > | |
| {runbook.name} | |
| </CommandItem> | |
| ))} | |
| </CommandList> | |
| </Command> | |
| </PopoverContent> | |
| </Popover> | |
| <DialogFooter> | |
| <Button | |
| type="submit" | |
| disabled={updateHook.isPending || !form.formState.isDirty} | |
| > | |
| Save | |
| </Button> | |
| </DialogFooter> | |
| </form> | |
| </Form> | |
| </DialogContent> | |
| </Dialog> | |
| ); | |
| }; |
| update: protectedProcedure | ||
| .input(z.object({ id: z.string().uuid(), data: updateHook })) | ||
| .meta({ | ||
| authorizationCheck: async ({ canUser, ctx, input }) => { | ||
| const h = await ctx.db | ||
| .select() | ||
| .from(hook) | ||
| .where(eq(hook.id, input.id)) | ||
| .then(takeFirstOrNull); | ||
| if (h == null) return false; | ||
| if (h.scopeType !== "deployment") return false; | ||
| return canUser.perform(Permission.HookUpdate).on({ | ||
| type: "deployment", | ||
| id: h.scopeId, | ||
| }); | ||
| }, | ||
| }) | ||
| .mutation(({ ctx, input }) => | ||
| ctx.db.transaction(async (tx) => { | ||
| const h = await tx | ||
| .update(hook) | ||
| .set(input.data) | ||
| .where(eq(hook.id, input.id)) | ||
| .returning() | ||
| .then(takeFirst); | ||
|
|
||
| if (input.data.runbookIds == null) return h; | ||
|
|
||
| await tx.delete(runhook).where(eq(runhook.hookId, input.id)); | ||
| if (input.data.runbookIds.length === 0) return h; | ||
| const rhInserts = input.data.runbookIds.map((id) => ({ | ||
| hookId: h.id, | ||
| runbookId: id, | ||
| })); | ||
| const runhooks = await tx.insert(runhook).values(rhInserts).returning(); | ||
| return { ...h, runhooks }; | ||
| }), |
There was a problem hiding this comment.
Ensure 'runbookIds' are validated in the 'update' procedure
In the update procedure, input.data.runbookIds should be validated to confirm it's an array before accessing its properties. This prevents potential runtime errors if runbookIds is undefined or not an array.
Update the updateHook schema to include validation for runbookIds:
const updateHook = z.object({
// existing fields
+ runbookIds: z.array(z.string().uuid()).optional(),
});Committable suggestion skipped: line range outside the PR's diff.
| create: protectedProcedure | ||
| .input(createHook) | ||
| .meta({ | ||
| authorizationCheck: ({ canUser, input }) => | ||
| canUser | ||
| .perform(Permission.HookCreate) | ||
| .on({ type: "deployment", id: input.scopeId }), | ||
| }) | ||
| .mutation(async ({ ctx, input }) => | ||
| ctx.db.transaction(async (tx) => { | ||
| const h = await tx | ||
| .insert(hook) | ||
| .values(input) | ||
| .returning() | ||
| .then(takeFirst); | ||
| if (input.runbookIds.length === 0) return h; | ||
| const rhInserts = input.runbookIds.map((id) => ({ | ||
| hookId: h.id, | ||
| runbookId: id, | ||
| })); | ||
| const runhooks = await tx | ||
| .insert(runhook) | ||
| .values(rhInserts) | ||
| .returning() | ||
| .then((r) => r.map((rh) => rh.runbookId)); | ||
| return { ...h, runhooks }; | ||
| }), | ||
| ), |
There was a problem hiding this comment.
Validate 'runbookIds' before accessing 'length'
In the create procedure, accessing input.runbookIds.length assumes that runbookIds is always defined and is an array. To prevent potential runtime errors, add validation to ensure runbookIds is an array.
Apply this diff to validate runbookIds:
protectedProcedure
.input(createHook)
+ .refine((data) => Array.isArray(data.runbookIds), {
+ message: "runbookIds must be an array",
+ path: ["runbookIds"],
+ })
.meta({
authorizationCheck: ({ canUser, input }) =>
canUser📝 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.
| create: protectedProcedure | |
| .input(createHook) | |
| .meta({ | |
| authorizationCheck: ({ canUser, input }) => | |
| canUser | |
| .perform(Permission.HookCreate) | |
| .on({ type: "deployment", id: input.scopeId }), | |
| }) | |
| .mutation(async ({ ctx, input }) => | |
| ctx.db.transaction(async (tx) => { | |
| const h = await tx | |
| .insert(hook) | |
| .values(input) | |
| .returning() | |
| .then(takeFirst); | |
| if (input.runbookIds.length === 0) return h; | |
| const rhInserts = input.runbookIds.map((id) => ({ | |
| hookId: h.id, | |
| runbookId: id, | |
| })); | |
| const runhooks = await tx | |
| .insert(runhook) | |
| .values(rhInserts) | |
| .returning() | |
| .then((r) => r.map((rh) => rh.runbookId)); | |
| return { ...h, runhooks }; | |
| }), | |
| ), | |
| create: protectedProcedure | |
| .input(createHook) | |
| .refine((data) => Array.isArray(data.runbookIds), { | |
| message: "runbookIds must be an array", | |
| path: ["runbookIds"], | |
| }) | |
| .meta({ | |
| authorizationCheck: ({ canUser, input }) => | |
| canUser | |
| .perform(Permission.HookCreate) | |
| .on({ type: "deployment", id: input.scopeId }), | |
| }) | |
| .mutation(async ({ ctx, input }) => | |
| ctx.db.transaction(async (tx) => { | |
| const h = await tx | |
| .insert(hook) | |
| .values(input) | |
| .returning() | |
| .then(takeFirst); | |
| if (input.runbookIds.length === 0) return h; | |
| const rhInserts = input.runbookIds.map((id) => ({ | |
| hookId: h.id, | |
| runbookId: id, | |
| })); | |
| const runhooks = await tx | |
| .insert(runhook) | |
| .values(rhInserts) | |
| .returning() | |
| .then((r) => r.map((rh) => rh.runbookId)); | |
| return { ...h, runhooks }; | |
| }), | |
| ), |
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes