fix: Refactor runhook creation #221
Conversation
WalkthroughThis pull request involves significant changes to several components related to job agent configurations and their management within a web service application. 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: 16
🧹 Outside diff range and nitpick comments (16)
packages/validators/src/events/hooks/runbook-variable.ts (2)
1-3: Consider restructuring to avoid circular dependencyThe comment indicates a circular dependency issue with the db package. This might suggest that the current code organization could be improved.
Consider:
- Moving shared types to a separate common/types package
- Splitting the dependency graph to have a clear hierarchical structure
- Using dependency injection or the mediator pattern to break the circular reference
Would you like me to help analyze the dependency structure to propose a specific solution?
4-8: Add JSDoc documentation for better maintainabilityThe type definition looks good, but adding documentation would improve maintainability and help other developers understand the purpose of each field.
Consider adding documentation like this:
+/** + * Represents a variable configuration for a runbook + */ export type RunbookVariable = { + /** Unique identifier for the variable */ key: string; + /** Human-readable name of the variable */ name: string; + /** Configuration settings for the variable */ config: RunbookVariableConfigType; };packages/validators/src/events/hooks/index.ts (1)
15-17: Consider consolidating hook action definitionsWhile the new enum provides better type safety, maintaining both
hookActionsListandHookActioncreates potential for inconsistency. Consider deriving one from the other to ensure they stay in sync.Here's a suggested approach:
-export const hookActionsList = ["deployment.resource.removed"]; -export const hookActions = z.enum(hookActionsList as [string, ...string[]]); export enum HookAction { DeploymentResourceRemoved = "deployment.resource.removed", } +export const hookActionsList = Object.values(HookAction); +export const hookActions = z.enum(hookActionsList as [string, ...string[]]);apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/page.tsx (1)
11-12: Consider adding error handling for API callsWhile the workspace existence check is good, consider adding try-catch blocks around API calls to handle potential network or server errors gracefully.
- const workspace = await api.workspace.bySlug(params.workspaceSlug); - if (!workspace) notFound(); + try { + const workspace = await api.workspace.bySlug(params.workspaceSlug); + if (!workspace) notFound(); + } catch (error) { + console.error('Failed to fetch workspace:', error); + throw new Error('Failed to load workspace configuration'); + }packages/validators/src/events/hooks/target.ts (2)
18-28: Consider enhancing type safety and documentationWhile the variables are well-structured, consider these improvements:
- Add type validation for the config objects
- Document the relationship between these variables and the Zod schemas above
Consider adding a type definition for the config objects:
type RunbookVariableConfig = | { type: "resource" } | { type: "string", inputType: "text" };And add JSDoc comments explaining the relationship:
/** * Variable definition for resource ID, corresponds to * the resource.id field in the resourceRemoved schema */ const resourceVar: RunbookVariable = { // ... existing code };
Line range hint
1-30: Well-structured event handling architectureThe file demonstrates good separation of concerns by:
- Defining clear validation schemas using Zod
- Maintaining type safety with TypeScript
- Organizing related variables for resource removal events
This structure aligns well with the PR's objective of refactoring runhook creation and the transition to job agent data.
packages/db/src/schema/event.ts (1)
29-33: Consider separating concerns in schema designThe current schema mixes runbook variables with job agent configuration. This could make it harder to maintain and evolve each feature independently.
Consider:
- Creating a separate junction table for variables if they're reusable across different hooks
- Moving job agent specific configuration to a dedicated table
- Adding a discriminator field to distinguish between runbook-based and job agent-based hooks
This would provide:
- Better data organization
- Easier migrations
- Independent feature evolution
- Clearer relationships between entities
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/HooksTable.tsx (1)
31-31: Consider improving table responsivenessThe table uses a fixed-width layout (
table-fixedclass) which might cause content overflow issues, especially in the runhooks column where multiple badges are displayed. Consider using a responsive layout or implementing horizontal scrolling for better mobile compatibility.- <Table className="table-fixed"> + <Table className="w-full"> + <div className="overflow-x-auto"> {/* table content */} + </div> </Table>apps/webservice/src/components/form/job-agent/JobAgentGitHubConfig.tsx (1)
Line range hint
31-75: Consider architectural improvements for better maintainability and user experience.The component could benefit from the following improvements:
- Error Handling: Add proper error states and user feedback for API failures
- Loading States: Consider implementing skeleton loaders instead of simple loading text
- Component Structure: Split into smaller sub-components for better maintainability:
RepositorySelectorWorkflowSelectorThis would improve code organization and make the component more maintainable.
Would you like me to provide example implementations for any of these improvements?
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/EditHookDialog.tsx (4)
60-61: Consider using a more specific type for jobAgentConfigInstead of using
z.record(z.any()), consider defining a more specific schema for the job agent configuration to ensure type safety and validation.- jobAgentConfig: z.record(z.any()).nullable(), + jobAgentConfig: z.object({ + // Add specific configuration fields based on your job agent types + }).nullable(),
172-177: Consider updating dialog header textThe dialog header still refers to "Runbook" while the codebase is transitioning to job agents. Consider updating the text to maintain consistency.
- <DialogTitle>Attach Runbook</DialogTitle> - <DialogDescription> - This runbook will trigger when the hook is triggered. + <DialogTitle>Attach Job Agent</DialogTitle> + <DialogDescription> + This job agent will be executed when the hook is triggered.
209-213: Consider extracting hardcoded style configurationThe githubFormStyleConfig contains hardcoded values. Consider moving these to a constants file or theme configuration for better maintainability.
221-230: Update button text to reflect job agent terminologyFor consistency with the codebase changes, update the button text from "Remove Runbook" to "Remove Job Agent".
- Remove Runbook + Remove Job Agentapps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/CreateHookDialog.tsx (1)
206-209: Consider Using Responsive Design for Fixed WidthsThe hardcoded widths in
githubFormStyleConfigmay not render well on different screen sizes. Consider using responsive units or CSS classes to improve scalability across devices.Example adjustment:
githubFormStyleConfig={{ - className: "flex flex-col gap-2 items-center w-[450px]", - buttonWidth: "w-[450px]", + className: "flex flex-col gap-2 items-center w-full max-w-md", + buttonWidth: "w-full", }}packages/api/src/router/deployment.ts (2)
301-303: Confirm proper deletion of associatedrunbookWhen deleting the existing
runbookassociated with therunhook, ensure that any cascading effects are handled, such as orphanedrunbookVariableentries.Consider adding
ON DELETE CASCADEconstraints in the database schema or explicitly deleting dependent records.
Line range hint
179-333: Consider wrapping transactional operations with error handlingAcross both the
createandupdatemethods, transactional operations involve multiple database interactions. Ensure that any exceptions are properly caught and handled to maintain data integrity.Implement try-catch blocks within the transaction to handle potential errors gracefully.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (16)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/_components/EditAgentConfigDialog.tsx(0 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/NavigationMenuAction.tsx(4 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/CreateHookDialog.tsx(3 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/EditHookDialog.tsx(4 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/HookActionsDropdown.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/HooksTable.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/page.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/runbooks/create/CreateRunbookForm.tsx(1 hunks)apps/webservice/src/components/form/job-agent/JobAgentConfig.tsx(3 hunks)apps/webservice/src/components/form/job-agent/JobAgentGitHubConfig.tsx(3 hunks)packages/api/src/router/deployment.ts(4 hunks)packages/db/src/schema/event.ts(2 hunks)packages/events/src/index.ts(1 hunks)packages/validators/src/events/hooks/index.ts(1 hunks)packages/validators/src/events/hooks/runbook-variable.ts(1 hunks)packages/validators/src/events/hooks/target.ts(2 hunks)
💤 Files with no reviewable changes (1)
- apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/_components/EditAgentConfigDialog.tsx
🔇 Additional comments (32)
packages/validators/src/events/hooks/runbook-variable.ts (1)
1-1: Verify the imported type exists
Let's ensure the imported type is properly defined and exported.
✅ Verification successful
Type import is correctly defined and used across the codebase
The type RunbookVariableConfigType is properly exported from packages/validators/src/variables/index.ts and is being used consistently across the codebase, including in the database schema and web components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify RunbookVariableConfigType exists and is exported
# Check if the type is exported from the variables module
ast-grep --pattern 'export type RunbookVariableConfigType = $_'
# Look for any references to this type
rg "RunbookVariableConfigType" --type ts
Length of output: 1122
packages/events/src/index.ts (1)
13-13: Verify that all events should be handled as resource removals.
The code now processes all events through handleResourceRemoved without any type checking. This could lead to runtime errors if events of different types are received.
Let's verify the event types and their handling:
✅ Verification successful
The code change is correct - resource removal is the only supported event type
Based on the verification results:
- The hook event schema (
hookEvent) is explicitly defined to only handle resource removal events - The
hookActionsListonly contains one action:"deployment.resource.removed" - This is formalized in the
HookActionenum which also only definesDeploymentResourceRemoved
The removal of the type checking is actually a simplification since there's only one possible event type. The code change aligns with the schema definition and doesn't risk any runtime errors.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for different event types and their handlers
# Check for event type definitions
echo "Checking event type definitions..."
rg -A 5 "type HookEvent"
# Check for other event handlers that might have been removed
echo "Checking for other event handlers..."
rg -l "handle.*Event" --type ts
# Check for event type usage in tests
echo "Checking event usage in tests..."
rg "HookEvent.*=" --type test
Length of output: 1476
Script:
#!/bin/bash
# Let's check the hook event schema and handlers more thoroughly
# Check the hook event schema
echo "Checking hook event schema..."
rg -A 10 "hookEvent = "
# Check for resource removal related code
echo "Checking resource removal handlers..."
rg -A 5 "handleResourceRemoved"
# Check for any other event action types
echo "Checking event action types..."
rg -B 2 -A 2 "hookActionsList"
# Check tests specifically for resource removal
echo "Checking resource removal tests..."
rg -l "resource.*removed" --type ts
Length of output: 7209
packages/validators/src/events/hooks/index.ts (3)
3-4: LGTM: Import statements are well-structured
The new imports properly support the type-safe mapping between hook actions and their variables.
19-21: LGTM: Well-structured type-safe mapping
The RunhookVariables constant provides a robust, type-safe mapping between hook actions and their variables. The Record type ensures exhaustiveness, making it maintainable as new hook actions are added.
1-21: Verify handling of removed type guard
The removal of isResourceRemoved type guard might affect type narrowing in other parts of the codebase. Let's verify its usage:
✅ Verification successful
Type guard removal is safe to proceed
The codebase uses Zod schema validation (resourceRemoved schema) and string literals for type checking instead of the removed type guard. All usages of "deployment.resource.removed" are:
- As a literal in the Zod schema definition
- As an enum value in
HookAction - As action strings in event triggers and handlers
The type safety is maintained through:
- Zod schema validation (
resourceRemovedschema) - String literal types (
as const) - Enum-based type checking (
HookAction.DeploymentResourceRemoved)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of the removed type guard
rg "isResourceRemoved"
# Search for potential type narrowing patterns that might need the guard
ast-grep --pattern 'if ($x instanceof ResourceRemoved)'
ast-grep --pattern 'if ($x.type === "deployment.resource.removed")'
Length of output: 150
Script:
#!/bin/bash
# Let's search for any references to the type or enum value
rg -A 2 "deployment\.resource\.removed" --type ts
# Check for any type narrowing patterns using the action string
ast-grep --pattern 'if ($x === "deployment.resource.removed")'
# Look for type assertions or type checks related to the resource removal
ast-grep --pattern 'as { type: "deployment.resource.removed" }'
# Search for any discriminated union type checks
ast-grep --pattern 'if ($x.type === HookAction.DeploymentResourceRemoved)'
Length of output: 2213
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/page.tsx (2)
14-14: Verify job agents type safety and error handling
The job agents fetching should include error handling and type validation.
Let's verify the type definitions and usage:
20-22: Verify HooksTable component interface
The changes align with the PR objective of refactoring from runbooks to job agents. Let's verify the HooksTable component accepts these new props.
✅ Verification successful
HooksTable component interface is correctly implemented
The HooksTable component properly accepts and uses all the required props:
hooks: Hook[]jobAgents: SCHEMA.JobAgent[]workspace: SCHEMA.Workspace
The component correctly renders the hooks data including the associated runbooks, confirming the interface aligns with the usage in the page component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify HooksTable component props
# Expected: Find component definition with updated props
# Check HooksTable component definition
ast-grep --pattern 'export function HooksTable({ hooks, jobAgents, workspace }: { $$$props$$$ }) {
$$$
}'
# Check for any remaining runbook references
rg -l 'runbook' --type ts --type tsx
Length of output: 174
Script:
#!/bin/bash
# Let's try a different approach to find the HooksTable component definition
# First, let's locate the file containing HooksTable
fd -e tsx -e ts "HooksTable"
# Then let's examine its content
rg "HooksTable.*{.*}.*{" --type ts --type-add "ts:*.{ts,tsx}"
# Also check for any runbook references with correct file type
rg "runbook" --type ts --type-add "ts:*.{ts,tsx}"
Length of output: 19174
Script:
#!/bin/bash
# Let's examine the HooksTable component definition
cat "apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/HooksTable.tsx"
Length of output: 2237
packages/validators/src/events/hooks/target.ts (2)
3-3: LGTM: Clean type import
The type-only import is correctly implemented and well-organized.
30-30: Verify the usage of ResourceRemovedVariables
The exported constant looks good, but let's verify its integration with the event handling system.
✅ Verification successful
ResourceRemovedVariables is properly integrated and used
The constant is correctly:
- Imported in
index.tsfromtarget.js - Used in the hook action mapping for
DeploymentResourceRemoved - Contains the expected variables (
resourceVaranddeploymentVar)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of ResourceRemovedVariables in event handling
# Check for imports of ResourceRemovedVariables
echo "Checking imports of ResourceRemovedVariables:"
rg --type typescript "import.*ResourceRemovedVariables.*from.*target"
# Check for usage in hook/event handling components
echo "Checking usage in hook-related components:"
rg --type typescript "ResourceRemovedVariables" -A 3
Length of output: 390
Script:
#!/bin/bash
# Let's try without typescript file type specification and use ripgrep's default behavior
# Check for imports of ResourceRemovedVariables
echo "Checking imports:"
rg "import.*ResourceRemovedVariables.*from.*target"
# Check for usage of ResourceRemovedVariables
echo -e "\nChecking usage:"
rg "ResourceRemovedVariables" -A 3
# Check the file content where ResourceRemovedVariables is defined
echo -e "\nChecking definition context:"
rg -B 5 -A 5 "export const ResourceRemovedVariables"
Length of output: 1454
apps/webservice/src/components/form/job-agent/JobAgentConfig.tsx (2)
Line range hint 19-38: Implementation looks good!
The component:
- Properly handles all job agent types
- Has good error handling
- Correctly passes the new style configuration
19-25: Verify all consumers of JobAgentConfig
Since this component has a new optional prop, let's verify that all consuming components are properly updated.
✅ Verification successful
Based on the search results, I can see all usages of JobAgentConfig component in the codebase. Let me analyze each usage:
- EditRunbookDialog.tsx: Does not pass
githubFormStyleConfig - CreateRunbookForm.tsx: Passes
githubFormStyleConfig - CreateHookDialog.tsx: Passes
githubFormStyleConfig - EditHookDialog.tsx: Passes
githubFormStyleConfig - JobAgentConfigForm.tsx: Does not pass
githubFormStyleConfig - JobAgentSection.tsx: Does not pass
githubFormStyleConfig
Since githubFormStyleConfig is marked as optional in the component props (as evident from the review snippet), it's perfectly fine for some components to not pass this prop. All consumers are properly using the component with the required props (workspace, jobAgent, value, onChange).
All consumers of JobAgentConfig are properly updated
The optional githubFormStyleConfig prop is correctly handled by all consuming components. Some components pass the prop while others don't, which is valid for an optional prop.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all usages of JobAgentConfig to ensure they're updated
rg -l "JobAgentConfig" | while read -r file; do
if [[ "$file" != "apps/webservice/src/components/form/job-agent/JobAgentConfig.tsx" ]]; then
echo "=== $file ==="
rg -A 5 "JobAgentConfig" "$file"
fi
done
Length of output: 8801
packages/db/src/schema/event.ts (2)
7-7: LGTM: Import statement follows best practices
The import statement correctly includes the file extension and is properly scoped.
29-33: Verify migration path and component updates
The schema changes from runbookIds to job agent fields require careful migration handling.
Let's verify the related changes:
✅ Verification successful
Let me gather more information about the schema changes and migrations.
Let me gather information about the database migrations to verify the schema changes.
Based on the analysis of the codebase and migrations, I can now provide a final response:
Schema changes are properly handled with existing migrations
The schema changes for jobAgentId and jobAgentConfig fields in the hook schema are safe because:
- These fields are optional in the schema definition:
jobAgentId: z.string().uuid().optional(),
jobAgentConfig: z.record(z.any()).optional(),-
The hook table in the database doesn't contain these fields - they are only part of the input validation schema used for creating/updating hooks.
-
The implementation in CreateHookDialog and EditHookDialog components properly handles these optional fields with null/empty defaults.
-
The changes are backward compatible as they only extend the schema without modifying existing fields.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing hooks table usage and related components
echo "Checking for hooks table usage..."
rg -l "runbookIds" --type typescript
echo "Checking for related component updates..."
fd -e tsx -e ts | xargs rg "createHook|updateHook"
echo "Checking for migration files..."
fd -e sql -e ts "migrate|migration"
Length of output: 1904
Script:
#!/bin/bash
# Check the migration files content and schema changes
echo "Checking migration files content..."
cat packages/db/migrate.ts
echo "Checking schema definition..."
cat packages/db/src/schema/event.ts
echo "Checking for jobAgentId and jobAgentConfig usage..."
rg "jobAgentId|jobAgentConfig" -A 3
Length of output: 55140
Script:
#!/bin/bash
# Check for migration files in drizzle folder
echo "Checking drizzle migration files..."
fd -e sql . packages/db/drizzle
echo "Checking migration file contents..."
find packages/db/drizzle -name "*.sql" -exec cat {} \;
Length of output: 73663
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/HookActionsDropdown.tsx (3)
28-29: LGTM!
The props destructuring correctly matches the updated type definition.
40-41: Verify EditHookDialog component interface
The props are correctly passed to EditHookDialog. Let's verify its interface has been updated to match.
#!/bin/bash
# Description: Verify EditHookDialog component's prop interface
# Find and show EditHookDialog's prop interface
ast-grep --pattern 'type EditHookDialogProps = {
$$$
}'
# Show the component definition
ast-grep --pattern 'export const EditHookDialog.*= ({
$$$
})'21-22: Verify prop changes propagation
The addition of jobAgents and workspace props aligns with the refactoring from runbooks to job agents. Let's verify all usages of this component have been updated accordingly.
✅ Verification successful
Props changes are properly propagated
The verification shows that the HookActionsDropdown component is used in HooksTable.tsx and all the new props (jobAgents and workspace) are correctly passed to the component:
<HookActionsDropdown
hook={hook}
jobAgents={jobAgents}
workspace={workspace}
>🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all instances where HookActionsDropdown is imported and used
# to ensure the new props are properly passed
# Find files importing HookActionsDropdown
echo "Files importing HookActionsDropdown:"
rg -l "import.*HookActionsDropdown" --type typescript --type typescriptreact
# Show usage context
echo "\nUsage context:"
rg -A 5 "<HookActionsDropdown" --type typescript --type typescriptreact
Length of output: 1406
Script:
#!/bin/bash
# Let's try a different approach to find the component usage
# Find files containing HookActionsDropdown
echo "Files containing HookActionsDropdown:"
rg --type tsx "HookActionsDropdown"
# Find the specific component usage with more context
echo "\nComponent usage context:"
rg --type tsx -B 2 -A 5 "<HookActionsDropdown.*>"
# Look for the component definition
echo "\nComponent definition:"
rg --type tsx -B 2 -A 5 "HookActionsDropdownProps.*=.*{"
Length of output: 531
Script:
#!/bin/bash
# Let's try without file type restrictions and fix the regex
# Find files containing HookActionsDropdown
echo "Files containing HookActionsDropdown:"
rg "HookActionsDropdown"
# Find the specific component usage with more context
echo "\nComponent usage context:"
rg -B 2 -A 5 "<HookActionsDropdown"
# Look for the component definition
echo "\nComponent definition:"
rg -B 2 -A 5 "type HookActionsDropdownProps"
Length of output: 4734
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/HooksTable.tsx (3)
26-30: LGTM! Component signature properly updated
The component signature has been correctly updated to match the new type definition, maintaining consistency with the refactoring changes.
63-67: Verify data model consistency
While the component now passes jobAgents and workspace props to HookActionsDropdown, the table body still renders data from hook.runhooks. This suggests a potential mismatch in the data model transition.
#!/bin/bash
# Search for runhooks references to ensure complete refactoring
echo "Checking for runhooks references in the codebase:"
rg "runhooks" --type tsx --type ts -A 2
echo "\nChecking Hook type definition:"
ast-grep --pattern 'type Hook = $_'22-23: Verify if "Runbooks" column header should be updated
While the props have been updated to use jobAgents, the table header still shows "Runbooks". Should this be updated to reflect the new terminology?
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/NavigationMenuAction.tsx (3)
4-4: LGTM: Import addition aligns with Next.js patterns
The addition of useParams is appropriate for accessing route parameters in a Next.js client component.
Line range hint 18-34: LGTM: Well-structured data fetching implementation
The implementation shows good practices:
- Proper type safety with
useParams<{ workspaceSlug: string }> - Efficient query chaining with conditional fetching
- Safe handling of null/undefined cases
57-62: Verify CreateHookDialog prop requirements
The component now receives jobAgents and workspace props instead of the previous runbook data. Let's verify the component's interface.
✅ Verification successful
Props are correctly defined and used in CreateHookDialog
The verification shows that:
- The component properly defines required props:
jobAgents: SCHEMA.JobAgent[]andworkspace: SCHEMA.Workspace - Props are correctly destructured in the component parameters
jobAgentsis actively used in the component for finding job agents and passed down to child components- The workspace prop is also properly passed down
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify CreateHookDialog component's prop interface and usage
# Check CreateHookDialog's prop interface
ast-grep --pattern 'interface $interface {
$$$
jobAgents: $_
workspace: $_
$$$
}'
# Check for any remaining runbook references that might need updating
rg -A 5 'runbook|Runbook' './apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/hooks'
Length of output: 7689
Script:
#!/bin/bash
# Let's check the CreateHookDialog component's implementation to verify prop usage
# Check CreateHookDialog's prop types and interface
ast-grep --pattern 'type $_ = {
$$$
jobAgents?: $_
workspace?: $_
$$$
}'
# Check the actual component implementation
ast-grep --pattern 'function CreateHookDialog({ jobAgents, workspace, $$ }: $_) {
$$$
}'
# Look for jobAgents usage in CreateHookDialog
rg -A 5 'jobAgents' './apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/CreateHookDialog.tsx'
Length of output: 1035
apps/webservice/src/components/form/job-agent/JobAgentGitHubConfig.tsx (1)
30-31: LGTM: Props update is clean and well-typed.
The integration of the new styleConfig prop is done correctly with proper TypeScript typing.
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/EditHookDialog.tsx (3)
5-5: LGTM: Type definitions properly updated for job agent integration
The changes to imports and prop types correctly reflect the transition from runbooks to job agents while maintaining type safety.
Also applies to: 51-52
83-95: LGTM: Form submission logic properly handles job agent data
The form submission correctly:
- Maps action variables using RunhookVariables
- Handles nullable fields appropriately
- Updates the UI state after successful submission
79-80: Verify migration path for existing hooks
The code handles existing runbook configurations through optional chaining, but we should verify the migration path for existing hooks.
✅ Verification successful
Based on the database schema and code analysis, I can now provide a final response:
Migration path for existing hooks is properly handled
The codebase has proper handling for the migration from runbooks to job agents:
- The database schema shows
jobAgentIdandjobAgentConfigare nullable fields in the runbook table, allowing for a smooth transition - The optional chaining in the code (
hook.runhook?.runbook.jobAgentId ?? null) safely handles both old and new configurations - The migration path is built into the schema design, with no breaking changes required for existing hooks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing hooks with runbook configurations that might need migration
# Search for hooks with runbook configurations
rg -A 5 "runbook\." "apps/webservice/src/app"
# Check database migrations
fd -e sql -e ts "migration" "apps/webservice/src/db"
Length of output: 13181
Script:
#!/bin/bash
# Let's check for database schema and migrations related to hooks and runbooks
# Search for hook and runbook related schema definitions
rg -A 10 "CREATE TABLE.*(?:hook|runbook|job)" .
# Search for migration files with hook or runbook related changes
rg -A 10 "ALTER TABLE.*(?:hook|runbook|job)" .
# Check for any TypeScript schema definitions
ast-grep --pattern 'export const $_ = pgTable("$_", {
$$$
})'
Length of output: 47287
packages/api/src/router/deployment.ts (6)
31-32: Imports for runbook and runbookVariable added appropriately
The addition of runbook and runbookVariable imports is necessary for the new functionality related to runbooks and their variables.
309-319: Ensure all required fields are set when inserting a new runbook during update
As with the creation method, confirm that the insertion includes all necessary fields.
Verify the runbook schema for required fields and ensure they are provided.
328-333: Ensure consistent creation of runhook during update
When inserting the updated runhook:
const updatedRh = await tx
.insert(runhook)
.values({ hookId: h.id, runbookId: rb.id })
.returning()
.then(takeFirst);Confirm that inserting a new runhook is the intended behavior and that it doesn't conflict with existing data integrity rules.
237-246: Set explicit fields when inserting into runbook
When inserting a new runbook, ensure that all necessary fields are explicitly set, including timestamps or default values that may not have defaults at the database level.
Check the runbook table schema to confirm that all required fields are populated, and modify the insertion accordingly.
#!/bin/bash
# Description: Verify required fields in the runbook table schema
# Get the table schema for runbook
psql -c "\d runbook"254-259: Ensure runhook creation aligns with data integrity constraints
When inserting into runhook:
const rh = await tx
.insert(runhook)
.values({ hookId: h.id, runbookId: rb.id })
.returning()
.then(takeFirst);Verify that there are no unique or foreign key constraints that might be violated, especially if a runhook for this hookId already exists.
Run the following script to check for existing runhook entries:
#!/bin/bash
# Description: Check for existing runhook entries with the same hookId
psql -c "
SELECT COUNT(*)
FROM runhook
WHERE hook_id = '${h.id}';
"If the count is greater than zero, consider updating the existing runhook instead of inserting a new one.
179-189: Verify the assumption of a single runhook per hook
In the list method of the hookRouter, you're mapping the runhooks array to a single runhook using row.runhooks[0] ?? null. This assumes that there is at most one runhook associated with each hook.
To ensure data integrity, run the following script to check if any hooks have multiple associated runhooks:
If the query returns any results, consider handling multiple runhooks appropriately or enforcing a one-to-one relationship.
| export const handleEvent = async (event: HookEvent) => { | ||
| await db.insert(SCHEMA.event).values(event); | ||
| if (isResourceRemoved(event)) return handleResourceRemoved(event); | ||
| throw new Error(`Unhandled event`); | ||
| return handleResourceRemoved(event); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for event processing.
The event handling could fail silently if handleResourceRemoved throws an error. Consider adding error handling to:
- Log failed events
- Handle different failure scenarios
- Ensure data consistency
Consider this improvement:
export const handleEvent = async (event: HookEvent) => {
await db.insert(SCHEMA.event).values(event);
- return handleResourceRemoved(event);
+ try {
+ return await handleResourceRemoved(event);
+ } catch (error) {
+ console.error(`Failed to process event: ${event.id}`, error);
+ throw error;
+ }
};📝 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.
| export const handleEvent = async (event: HookEvent) => { | |
| await db.insert(SCHEMA.event).values(event); | |
| if (isResourceRemoved(event)) return handleResourceRemoved(event); | |
| throw new Error(`Unhandled event`); | |
| return handleResourceRemoved(event); | |
| }; | |
| export const handleEvent = async (event: HookEvent) => { | |
| await db.insert(SCHEMA.event).values(event); | |
| try { | |
| return await handleResourceRemoved(event); | |
| } catch (error) { | |
| console.error(`Failed to process event: ${event.id}`, error); | |
| throw error; | |
| } | |
| }; |
| jobAgent?: JobAgent | null; | ||
| value: Record<string, any>; | ||
| onChange: (v: Record<string, any>) => void; | ||
| githubFormStyleConfig?: JobAgentGithubStyleCongig; |
There was a problem hiding this comment.
Fix the same typo in props type definition
The type name typo needs to be fixed here as well.
- githubFormStyleConfig?: JobAgentGithubStyleCongig;
+ githubFormStyleConfig?: JobAgentGithubStyleConfig;📝 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.
| githubFormStyleConfig?: JobAgentGithubStyleCongig; | |
| githubFormStyleConfig?: JobAgentGithubStyleConfig; |
|
|
||
| import { Input } from "@ctrlplane/ui/input"; | ||
|
|
||
| import type { JobAgentGithubStyleCongig } from "./JobAgentGitHubConfig"; |
There was a problem hiding this comment.
Fix typo in type name: JobAgentGithubStyleCongig → JobAgentGithubStyleConfig
There's a typo in the imported type name which should be corrected for better maintainability.
-import type { JobAgentGithubStyleCongig } from "./JobAgentGitHubConfig";
+import type { JobAgentGithubStyleConfig } from "./JobAgentGitHubConfig";📝 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.
| import type { JobAgentGithubStyleCongig } from "./JobAgentGitHubConfig"; | |
| import type { JobAgentGithubStyleConfig } from "./JobAgentGitHubConfig"; |
| .extend({ runbookIds: z.array(z.string().uuid()) }); | ||
| .extend({ | ||
| jobAgentId: z.string().uuid().optional(), | ||
| jobAgentConfig: z.record(z.any()).optional(), |
There was a problem hiding this comment.
Consider adding type safety to jobAgentConfig
Using z.record(z.any()) bypasses type safety and could lead to runtime issues. Consider defining a strict schema for the config structure.
Example improvement:
- jobAgentConfig: z.record(z.any()).optional(),
+ jobAgentConfig: z.object({
+ // Define expected configuration fields
+ command: z.string(),
+ environment: z.record(z.string()).optional(),
+ timeout: z.number().optional(),
+ // ... other specific config fields
+ }).optional(),Committable suggestion skipped: line range outside the PR's diff.
|
|
||
| return ( | ||
| <div className={cn("flex items-center gap-3", className)}> | ||
| <div className={cn("flex items-center gap-3", styleConfig?.className)}> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicate styles and ensure consistent scrollbar behavior.
The style configurations are duplicated across multiple elements, and there's inconsistent scrollbar behavior between the two CommandList components.
Consider these improvements:
- Extract common styles to constants:
const BUTTON_BASE_STYLES = "w-80 items-center justify-start gap-2 px-2";
const POPOVER_BASE_STYLES = "w-80 p-0";- Apply consistent scrollbar styling:
<CommandList>
+ className="scrollbar-thin scrollbar-track-neutral-800 scrollbar-thumb-neutral-700"Apply this refactor:
- <div className={cn("flex items-center gap-3", styleConfig?.className)}>
+ <div className={cn(CONTAINER_STYLES, styleConfig?.className)}>
{/* ... */}
- className={cn(
- "w-80 items-center justify-start gap-2 px-2",
- styleConfig?.buttonWidth,
- )}
+ className={cn(BUTTON_BASE_STYLES, styleConfig?.buttonWidth)}
{/* ... */}
- <PopoverContent className={cn("w-80 p-0", styleConfig?.buttonWidth)}>
+ <PopoverContent className={cn(POPOVER_BASE_STYLES, styleConfig?.buttonWidth)}>Also applies to: 84-87, 95-95, 131-134, 142-142
| const { jobAgentId, jobAgentConfig } = input; | ||
| if (jobAgentId == null || jobAgentConfig == null) | ||
| return { ...h, runhook: null }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure jobAgentId and jobAgentConfig validation is comprehensive
Currently, the check only tests for null values:
if (jobAgentId == null || jobAgentConfig == null)
return { ...h, runhook: null };Consider using a more robust validation to catch empty strings or undefined values.
Apply this diff to improve the validation:
- if (jobAgentId == null || jobAgentConfig == null)
+ if (!jobAgentId || !jobAgentConfig)| if (input.variables.length > 0) | ||
| await tx | ||
| .insert(runbookVariable) | ||
| .values(input.variables.map((v) => ({ ...v, runbookId: rb.id }))) | ||
| .returning(); | ||
|
|
There was a problem hiding this comment.
Handle cases where input.variables might be undefined
The current check assumes that input.variables is always defined:
if (input.variables.length > 0)
await tx
.insert(runbookVariable)
.values(input.variables.map((v) => ({ ...v, runbookId: rb.id })))
.returning();Modify the condition to handle undefined or null values:
- if (input.variables.length > 0)
+ if (input.variables && input.variables.length > 0)| const dep = await tx | ||
| .select() | ||
| .from(deployment) | ||
| .where(eq(deployment.id, h.scopeId)) | ||
| .then(takeFirst); |
There was a problem hiding this comment.
Handle the possibility of a missing deployment during update
Similar to the creation method, when updating, ensure that the deployment exists:
const dep = await tx
.select()
.from(deployment)
.where(eq(deployment.id, h.scopeId))
.then(takeFirst);Add a check to handle dep being null:
const dep = await tx
.select()
.from(deployment)
.where(eq(deployment.id, h.scopeId))
.then(takeFirst);
+ if (!dep) {
+ throw new Error(`Deployment with id ${h.scopeId} not found.`);
+ }| const { jobAgentId, jobAgentConfig } = input.data; | ||
| if (jobAgentId == null || jobAgentConfig == null) { | ||
| return { ...h, runhook: null }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate input data before proceeding with the update
Currently, the code checks if jobAgentId or jobAgentConfig are null:
if (jobAgentId == null || jobAgentConfig == null) {
return { ...h, runhook: null };
}Use a stricter validation to catch other falsy values:
- if (jobAgentId == null || jobAgentConfig == null) {
+ if (!jobAgentId || !jobAgentConfig) {| if (input.data.variables != null && input.data.variables.length > 0) | ||
| await tx | ||
| .insert(runbookVariable) | ||
| .values( | ||
| input.data.variables.map((v) => ({ ...v, runbookId: rb.id })), | ||
| ) | ||
| .returning(); |
There was a problem hiding this comment.
Handle variables being null or undefined during update
The current check may fail if input.data.variables is null:
if (input.data.variables != null && input.data.variables.length > 0)
await tx
.insert(runbookVariable)
.values(
input.data.variables.map((v) => ({ ...v, runbookId: rb.id })),
)
.returning();Modify the condition to safely handle undefined or null:
- if (input.data.variables != null && input.data.variables.length > 0)
+ if (Array.isArray(input.data.variables) && input.data.variables.length > 0)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/CreateHookDialog.tsx (1)
221-230: Add shouldDirty flag to form value updatesThe remove button's onClick handler should include the
shouldDirtyflag to maintain consistency with form state management.Apply this diff:
onClick={() => { - form.setValue("jobAgentId", null); - form.setValue("jobAgentConfig", {}); + form.setValue("jobAgentId", null, { shouldDirty: true }); + form.setValue("jobAgentConfig", {}, { shouldDirty: true }); }}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/CreateHookDialog.tsx(3 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/EditHookDialog.tsx(4 hunks)packages/db/src/schema/event.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/EditHookDialog.tsx
- packages/db/src/schema/event.ts
🔇 Additional comments (1)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/CreateHookDialog.tsx (1)
4-4: LGTM: Import and type changes align with job agent refactoring
The changes to imports and type definitions correctly support the transition from runbooks to job agents.
Also applies to: 7-7, 37-41, 43-44, 49-50
| jobAgentId: z.string().nullable(), | ||
| jobAgentConfig: z.record(z.any()).nullable(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider stronger typing for jobAgentConfig
The current schema uses z.record(z.any()) for jobAgentConfig which is too permissive and could lead to runtime errors if invalid configurations are submitted.
Consider defining a more specific schema based on the expected configuration structure:
- jobAgentConfig: z.record(z.any()).nullable(),
+ jobAgentConfig: z.object({
+ // Add expected configuration fields here
+ // For example:
+ // environment: z.record(z.string()),
+ // timeout: z.number().optional(),
+ }).nullable(),Also applies to: 61-66
| <JobAgentConfig | ||
| workspace={workspace} | ||
| jobAgent={jobAgent} | ||
| value={value ?? {}} | ||
| onChange={onChange} | ||
| githubFormStyleConfig={{ | ||
| className: "flex flex-col gap-2 items-center w-[450px]", | ||
| buttonWidth: "w-[450px]", | ||
| }} | ||
| /> |
There was a problem hiding this comment.
Add null check before rendering JobAgentConfig
The JobAgentConfig component receives a potentially undefined jobAgent prop which could cause runtime errors.
Apply this diff:
- <JobAgentConfig
- workspace={workspace}
- jobAgent={jobAgent}
- value={value ?? {}}
- onChange={onChange}
- githubFormStyleConfig={{
- className: "flex flex-col gap-2 items-center w-[450px]",
- buttonWidth: "w-[450px]",
- }}
- />
+ {jobAgent ? (
+ <JobAgentConfig
+ workspace={workspace}
+ jobAgent={jobAgent}
+ value={value ?? {}}
+ onChange={onChange}
+ githubFormStyleConfig={{
+ className: "flex flex-col gap-2 items-center w-[450px]",
+ buttonWidth: "w-[450px]",
+ }}
+ />
+ ) : (
+ <div className="text-muted-foreground">
+ Select a job agent to configure
+ </div>
+ )}📝 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.
| <JobAgentConfig | |
| workspace={workspace} | |
| jobAgent={jobAgent} | |
| value={value ?? {}} | |
| onChange={onChange} | |
| githubFormStyleConfig={{ | |
| className: "flex flex-col gap-2 items-center w-[450px]", | |
| buttonWidth: "w-[450px]", | |
| }} | |
| /> | |
| {jobAgent ? ( | |
| <JobAgentConfig | |
| workspace={workspace} | |
| jobAgent={jobAgent} | |
| value={value ?? {}} | |
| onChange={onChange} | |
| githubFormStyleConfig={{ | |
| className: "flex flex-col gap-2 items-center w-[450px]", | |
| buttonWidth: "w-[450px]", | |
| }} | |
| /> | |
| ) : ( | |
| <div className="text-muted-foreground"> | |
| Select a job agent to configure | |
| </div> | |
| )} |
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores