fix: Init handling lifecycle hooks #194
Conversation
WalkthroughThe pull request introduces significant modifications across various components related to job dispatching and lifecycle hooks management. Key changes include updates to 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
|
| }) | ||
| .where(eq(schema.job.id, job.id)), | ||
| ); | ||
| await dispatchJobsQueue.add(job.id, { jobId: job.id }); |
There was a problem hiding this comment.
idk why we were creating release variables for a runbook deployment but this was breaking dispatching runbooks
![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 (21)
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/LifecycleHooksPageContent.tsx (1)
14-24: Consider adding error boundaries and loading states.The component implementation is clean and follows React best practices. However, consider implementing:
- Error boundaries to gracefully handle potential runtime errors
- Loading states for when the data is being fetched
Example implementation:
export const LifecycleHooksPageContent: React.FC< LifecycleHooksPageContentProps > = ({ deployment, lifecycleHooks, runbooks, isLoading }) => { if (isLoading) { return <LoadingSpinner />; // Add a loading component } return lifecycleHooks.length === 0 ? ( <ErrorBoundary fallback={<ErrorMessage />}> <LifecycleHooksGettingStarted deployment={deployment} runbooks={runbooks} /> </ErrorBoundary> ) : ( <ErrorBoundary fallback={<ErrorMessage />}> <LifecycleHooksTable deploymentId={deployment.id} lifecycleHooks={lifecycleHooks} /> </ErrorBoundary> ); };packages/db/drizzle/0031_lively_meltdown.sql (1)
1-17: Consider adding created_at and updated_at timestamps.While the current schema is functional, consider adding timestamp columns for better auditing and debugging capabilities:
created_atwith DEFAULT NOW()updated_atwith DEFAULT NOW() and an update triggerHere's the suggested modification:
CREATE TABLE IF NOT EXISTS "deployment_lifecycle_hook" ( "id" uuid PRIMARY KEY DEFAULT gen_random_uuid() NOT NULL, "deployment_id" uuid NOT NULL, - "runbook_id" uuid NOT NULL + "runbook_id" uuid NOT NULL, + "created_at" timestamp with time zone DEFAULT NOW() NOT NULL, + "updated_at" timestamp with time zone DEFAULT NOW() NOT NULL ); + + CREATE TRIGGER set_timestamp + BEFORE UPDATE ON "deployment_lifecycle_hook" + FOR EACH ROW + EXECUTE PROCEDURE trigger_set_timestamp();apps/jobs/src/expired-env-checker/index.ts (1)
Line range hint
21-45: Consider adding resilience and observability features.While the current implementation is functional, consider these architectural improvements:
- Add retry logic for transient failures
- Add metrics for monitoring (e.g., number of environments processed, success/failure rates)
- Consider implementing a dead-letter queue for failed environments
Would you like help implementing these improvements?
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/LifecycleHooksTable.tsx (2)
15-18: Consider adding runtime validation for props.While TypeScript provides compile-time type safety, consider adding runtime validation for the props using libraries like
zodorprop-types, especially for critical data likedeploymentIdandlifecycleHooks.
11-13: Add JSDoc comments for type definitions.Consider adding documentation for the
LifecycleHooktype to explain the relationship between the lifecycle hook and its associated runbook.+/** + * Represents a deployment lifecycle hook with its associated runbook. + * @extends {SCHEMA.DeploymentLifecycleHook} + */ type LifecycleHook = SCHEMA.DeploymentLifecycleHook & { runbook: SCHEMA.Runbook; };apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/LifecycleHooksGettingStarted.tsx (3)
8-11: Consider making the type definition more specific.The component only uses
deployment.id, so the type could be more precise to better document the actual requirements.type LifecycleHooksGettingStartedProps = { - deployment: SCHEMA.Deployment; + deployment: Pick<SCHEMA.Deployment, 'id'>; runbooks: SCHEMA.Runbook[]; };
16-17: Consider extracting text content for internationalization.The static text content should be extracted to support multiple languages in the future.
Consider using a translation framework or at least extracting the strings to a constants file:
const LIFECYCLE_HOOKS_COPY = { title: 'Lifecycle Hooks', description: 'Lifecycle hooks allow you to run runbooks at specific points in a deployment\'s lifecycle.' } as const;Also applies to: 22-27
16-17: Review padding values for responsiveness.The fixed padding (p-20) might not work well on all screen sizes. Consider using responsive padding classes.
- <div className="h-full w-full p-20"> - <div className="container m-auto max-w-xl space-y-6 p-20"> + <div className="h-full w-full p-4 sm:p-8 md:p-12 lg:p-20"> + <div className="container m-auto max-w-xl space-y-6 p-4 sm:p-8 md:p-12 lg:p-20">apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/NavigationMenuAction.tsx (3)
24-24: Consider adding error handling for the runbooks queryWhile the query is correctly implemented, consider adding error handling to improve user experience when the runbooks fetch fails.
- const runbooksQ = api.runbook.bySystemId.useQuery(systemId); + const runbooksQ = api.runbook.bySystemId.useQuery(systemId, { + onError: (error) => { + // Show error notification or handle error appropriately + console.error('Failed to fetch runbooks:', error); + } + });
44-60: Consider flexible button width for internationalizationThe fixed width button (
w-36) might cause text overflow issues when translated to other languages.- className="w-36" + className="min-w-36"
63-71: Consider simplifying the conditional rendering logicThe multiple boolean checks could be simplified for better readability.
+ const isCreatingRelease = !isVariablesActive && + !isReleaseChannelsActive && + !isLifecycleHooksActive; + {isCreatingRelease && ( <CreateReleaseDialog deploymentId={deploymentId} systemId={systemId}> <Button size="sm" variant="secondary"> New Release </Button> </CreateReleaseDialog> )}apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/CreateLifecycleHookDialog.tsx (1)
1-38: Consider enhancing code documentation and import organization.The type definition and imports could be improved for better maintainability:
- Add JSDoc documentation for the
CreateLifecycleHookDialogPropsinterface to describe the purpose of each prop.- Consider grouping imports by category (React, UI components, utilities).
type CreateLifecycleHookDialogProps = { + /** The ID of the deployment to create a lifecycle hook for */ deploymentId: string; + /** Array of available runbooks that can be selected */ runbooks: SCHEMA.Runbook[]; + /** Trigger element to open the dialog */ children: React.ReactNode; };apps/event-worker/src/job-dispatch/github.ts (1)
42-56: Consider implementing a trigger registry pattern.The current implementation handles different trigger types (release and runbook) directly in the query. Consider implementing a trigger registry pattern where different trigger types can be registered and handled more modularly. This would make it easier to:
- Add new trigger types in the future
- Handle trigger-specific logic separately
- Test different trigger types in isolation
Example concept:
interface TriggerHandler { getQuery(): QueryBuilder; handleResult(result: any): Promise<void>; } class TriggerRegistry { private handlers: Map<string, TriggerHandler>; register(type: string, handler: TriggerHandler): void; async dispatch(job: Job): Promise<void>; }packages/db/src/schema/deployment.ts (1)
79-91: Consider adding additional fields for better lifecycle hook management.While the basic schema is solid, consider these enhancements for better hook management:
- Add a
typeorstageenum field to indicate when the hook should run (e.g., pre-deploy, post-deploy)- Add
created_atandupdated_attimestamps for auditing- Consider adding a unique constraint on
(deployment_id, runbook_id, type)to prevent duplicate hooks for the same stageHere's a suggested enhancement:
export const deploymentLifecycleHook = pgTable("deployment_lifecycle_hook", { id: uuid("id").primaryKey().defaultRandom(), deploymentId: uuid("deployment_id") .notNull() .references(() => deployment.id, { onDelete: "cascade", }), runbookId: uuid("runbook_id") .notNull() .references(() => runbook.id, { onDelete: "cascade", }), + type: text("type").notNull().$type<"pre-deploy" | "post-deploy">(), + createdAt: timestamp("created_at").notNull().defaultNow(), + updatedAt: timestamp("updated_at").notNull().defaultNow(), - }); + }, (t) => ({ + uniq: uniqueIndex().on(t.deploymentId, t.runbookId, t.type) + }));apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/LifecycleHookActionsDropdown.tsx (1)
84-107: Enhance accessibility and fix icon size prop.The dropdown implementation could benefit from some refinements:
Consider these improvements:
export const LifecycleHookActionsDropdown: React.FC< LifecycleHookActionsDropdownProps > = ({ lifecycleHook, children }) => { const [open, setOpen] = useState(false); return ( <DropdownMenu open={open} onOpenChange={setOpen}> - <DropdownMenuTrigger asChild>{children}</DropdownMenuTrigger> + <DropdownMenuTrigger asChild aria-label="Open lifecycle hook actions"> + {children} + </DropdownMenuTrigger> <DropdownMenuContent> <DeleteLifecycleHookDialog lifecycleHookId={lifecycleHook.id} onClose={() => setOpen(false)} > <DropdownMenuItem className="flex items-center gap-2" + // Prevent closing dropdown when opening dialog onSelect={(e) => e.preventDefault()} > Delete - <IconTrash size="icon" className="h-4 w-4 text-destructive" /> + <IconTrash size={16} className="text-destructive" /> </DropdownMenuItem> </DeleteLifecycleHookDialog> </DropdownMenuContent> </DropdownMenu> ); };apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/DeploymentNavBar.tsx (2)
123-132: Consider menu item placement.The lifecycle hooks menu item might be better placed next to related functionality. Consider grouping it with Channels or Variables, as these are typically related to deployment configuration.
Line range hint
108-122: Fix incorrect URL in Jobs menu item.The Jobs menu item is currently using
variablesUrlinstead of a jobs-specific URL. This needs to be fixed to prevent navigation issues.- <Link href={variablesUrl} legacyBehavior passHref> + <Link href={`/${workspaceSlug}/systems/${systemSlug}/deployments/${deploymentSlug}/jobs`} legacyBehavior passHref>packages/job-dispatch/src/lifecycle-hooks.ts (4)
13-23: Consider adding a log statement for the early return case.To aid in debugging and understanding the flow of the function, consider adding a log statement before the early return when
env.targetFilteris null or when no targets are found.if (env.targetFilter == null) { + console.log(`Skipping target handling for environment ${env.id} due to null targetFilter.`); return; } ... if (targets.length === 0) { + console.log(`No targets found for environment ${env.id}. Skipping target handling.`); return; }
38-47: Consider adding a log statement for the early return case.Similar to the previous comment, consider adding a log statement before the early return when no deployment lifecycle hooks are found.
if (deploymentLifecycleHooks.length === 0) { + console.log(`No deployment lifecycle hooks found for system ${system.id}. Skipping target handling.`); return; }
60-75: Consider extracting the complex query into a separate function.To improve readability and maintainability, consider extracting the complex query for
removedFromSystemTargetsinto a separate function. This will make the main function more concise and easier to understand.+const getRemovedFromSystemTargets = async ( + db: Tx, + envFilters: TargetCondition[], + targets: SCHEMA.Target[], +): Promise<SCHEMA.Target[]> => { + if (envFilters.length === 0) { + return targets; + } + + return db + .select() + .from(SCHEMA.target) + .where( + and( + SCHEMA.targetMatchesMetadata(db, { + type: TargetFilterType.Comparison, + operator: ComparisonOperator.Or, + not: true, + conditions: envFilters, + }), + inArray(SCHEMA.target.id, targets.map((t) => t.id)), + ), + ); +}; + const removedFromSystemTargets = await getRemovedFromSystemTargets( db, envFilters, targets, );
90-90: Consider adding error handling for thePromise.allcall.To ensure that any errors during the processing of lifecycle hooks are caught and handled appropriately, consider adding a
try-catchblock around thePromise.allcall.-await Promise.all(handleLifecycleHooksForTargets); +try { + await Promise.all(handleLifecycleHooksForTargets); +} catch (error) { + console.error('Error processing lifecycle hooks for targets:', error); + throw error; +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (21)
apps/event-worker/src/job-dispatch/github.ts(2 hunks)apps/jobs/src/expired-env-checker/index.ts(2 hunks)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]/lifecycle-hooks/CreateLifecycleHookDialog.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/LifecycleHookActionsDropdown.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/LifecycleHooksGettingStarted.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/LifecycleHooksPageContent.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/LifecycleHooksTable.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/page.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/release-channels/page.tsx(1 hunks)packages/api/src/router/deployment-lifecycle-hook.ts(1 hunks)packages/api/src/router/deployment.ts(2 hunks)packages/api/src/router/environment.ts(2 hunks)packages/api/src/router/runbook.ts(1 hunks)packages/db/drizzle/0031_lively_meltdown.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/deployment.ts(2 hunks)packages/job-dispatch/src/index.ts(1 hunks)packages/job-dispatch/src/job-dispatch.ts(1 hunks)packages/job-dispatch/src/lifecycle-hooks.ts(1 hunks)
🔇 Additional comments (42)
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/release-channels/page.tsx (2)
Line range hint 14-17: Verify release channels functionality remains unchanged.
While the parameter handling has been simplified, the core functionality of fetching and displaying release channels remains intact. The component maintains its responsibility of:
- Fetching deployment details
- Fetching associated release channels
- Rendering the ReleaseChannelsTable component
Let's verify the release channel API integration:
#!/bin/bash
# Description: Verify the release channel list API method
# Expected: Method should accept deploymentId and return release channels
# Search for the API method definition and its usage
ast-grep --pattern 'deployment.releaseChannel.list.byDeploymentId = async (deploymentId: string)'7-7: LGTM! Parameter handling simplification looks good.
The change to pass the entire params object directly to the API call is a clean improvement that reduces code verbosity while maintaining type safety through the TypeScript interface.
Let's verify the API method signature to ensure it accepts the complete params object:
Also applies to: 11-11
✅ Verification successful
Based on the code from both files, I can now generate the final response:
Parameter handling is correctly implemented and type-safe
The bySlug method in the deployment router accepts an object with workspaceSlug, systemSlug, and deploymentSlug parameters, which matches exactly how the params object is being passed in the page component. The implementation is type-safe through zod validation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the deployment.bySlug API method signature
# Expected: Method should accept an object with workspaceSlug, systemSlug, and deploymentSlug
# Search for the API method definition
ast-grep --pattern 'deployment.bySlug = async ({ workspaceSlug, systemSlug, deploymentSlug }: { workspaceSlug: string; systemSlug: string; deploymentSlug: string })'
Length of output: 317
Script:
#!/bin/bash
# Let's try a different approach to find the API method signature
# First, let's find files that might contain the deployment API definitions
fd "deployment" --type f
# Then let's use ripgrep to search for bySlug method definition
rg "bySlug" -A 5
Length of output: 46933
Script:
#!/bin/bash
# Let's look for the deployment API method definition in the router file
rg -A 10 "deployment.*bySlug.*=.*protectedProcedure" packages/api/src/router/deployment.ts
Length of output: 92
Script:
#!/bin/bash
# Let's try a different approach to find the deployment API method
# First, let's look at the deployment router file content
cat packages/api/src/router/deployment.ts
# Also check the file where the params are being used
cat apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/release-channels/page.tsx
Length of output: 15794
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/LifecycleHooksPageContent.tsx (2)
1-4: LGTM! Well-organized imports.
The imports are cleanly organized, separating external schema types from internal component imports.
6-12: Consider optimizing the type definition.
While the type definition is clear, consider extracting the extended lifecycle hook type into a reusable type definition if it's used elsewhere in the codebase.
Let's verify if this extended type is used in other files:
packages/db/drizzle/0031_lively_meltdown.sql (3)
1-5: LGTM: Table creation follows best practices.
The table creation is well-structured with:
- UUID as primary key with automatic generation
- NOT NULL constraints on all columns
- Clear naming convention
7-11: Verify the existence of the referenced deployment table and column.
The foreign key constraint looks correct, with appropriate CASCADE behavior for deletions. However, we should verify the existence of the referenced table and column.
#!/bin/bash
# Description: Verify the existence of the referenced deployment table and its id column
# Expected: Find the table creation SQL that confirms the structure
# Search for deployment table creation
rg -A 5 'CREATE TABLE.*"deployment".*' --type sql13-17: Verify the existence of the referenced runbook table and column.
The foreign key constraint looks correct, with appropriate CASCADE behavior for deletions. However, we should verify the existence of the referenced table and column.
✅ Verification successful
Let me try a different search pattern to find the runbook table creation.
Foreign key constraint is valid - table and column exist
The runbook table exists in the initial migration (0000_busy_firebird.sql) with the correct id column of type UUID, which matches the foreign key constraint's requirements. The constraint in migration 0031 is properly referencing this existing table and column.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of the referenced runbook table and its id column
# Expected: Find the table creation SQL that confirms the structure
# Search for runbook table creation
rg -A 5 'CREATE TABLE.*"runbook".*' --type sql
Length of output: 514
Script:
#!/bin/bash
# Search for runbook table creation with case-insensitive pattern
rg -i 'create table.*runbook' --type sql -A 5
# Also search in all migration files that might contain runbook table
fd '.*\.sql$' packages/db/drizzle/ --exec grep -l -i 'runbook' {} \;
Length of output: 1678
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/page.tsx (4)
1-4: LGTM! Clean and minimal imports.
The imports follow Next.js best practices and maintain good separation of concerns between server and client components.
6-10: LGTM! Well-structured component definition.
The component follows Next.js App Router conventions with proper TypeScript typing for route parameters.
19-26: LGTM! Clean component rendering.
The component maintains a clear separation between data fetching and presentation logic, passing all necessary props to the content component.
1-26: Verify integration with lifecycle hooks infrastructure.
Let's ensure proper integration with the backend infrastructure.
✅ Verification successful
Let me gather more information about the TRPC router implementation with the correct file type.
Integration with lifecycle hooks infrastructure is properly implemented
The verification shows complete integration with all necessary components:
- Database schema is properly defined in
packages/db/src/schema/deployment.tswith correct foreign key relationships - TRPC router implementation exists with required endpoints (create, delete, list)
- UI components are properly structured with:
- Main page component handling data fetching
- Table component for displaying hooks
- Creation dialog for new hooks
- Actions dropdown for managing existing hooks
- Getting started view for empty state
The implementation follows the expected patterns and includes all necessary integration points between frontend and backend infrastructure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the integration points for lifecycle hooks
# Check if the deployment_lifecycle_hook table schema exists
echo "Checking for deployment_lifecycle_hook table schema..."
rg -A 10 "deployment_lifecycle_hook" --type sql
# Verify TRPC router implementation
echo "Checking TRPC router implementation..."
rg -A 5 "deployment\.lifecycleHook\." --type typescript
# Verify error boundary implementation
echo "Checking for error boundary components..."
fd -e tsx -e ts "error" --exec rg -l "ErrorBoundary"
Length of output: 2046
Script:
#!/bin/bash
# Check TRPC router implementation for lifecycle hooks
echo "Checking TRPC router implementation..."
rg -A 5 "lifecycleHook" --type ts
# Check schema definition
echo "Checking schema definition..."
rg -A 5 "deployment_lifecycle_hook" --type ts
# Check for related components and their usage
echo "Checking lifecycle hook components..."
fd -e tsx "Lifecycle" --exec rg -l "export"
Length of output: 20106
packages/job-dispatch/src/index.ts (1)
24-24: LGTM! Verify lifecycle hooks implementation.
The export follows the established pattern and is appropriately placed in the file structure.
Let's verify the lifecycle hooks implementation:
✅ Verification successful
Let me try another verification approach to check for TypeScript source files since we're looking at a TypeScript project.
Let me check the complete implementation of the lifecycle hooks file to better understand its functionality.
Lifecycle hooks implementation verified and properly structured
The implementation is well-defined and follows good practices:
- Exports a properly typed async function for handling environment deletion lifecycle
- Correctly integrates with the database schema and validators
- Implements proper error handling and early returns
- Uses type-safe database operations and joins
- Properly dispatches runbooks for affected targets
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the lifecycle-hooks.js file exists and exports expected functionality
# Test 1: Check if the lifecycle-hooks.js file exists
echo "Checking for lifecycle-hooks.js file..."
fd --type f "lifecycle-hooks.js$" packages/job-dispatch/src
# Test 2: Verify the exports in lifecycle-hooks.js
echo -e "\nChecking exports in lifecycle-hooks.js..."
rg -A 5 "^export " packages/job-dispatch/src/lifecycle-hooks.js
# Test 3: Look for any related test files
echo -e "\nChecking for related test files..."
fd --type f "lifecycle-hooks.(test|spec).(ts|js)$" packages/job-dispatch/src
Length of output: 564
Script:
#!/bin/bash
# Check for TypeScript source files instead of JavaScript
echo "Checking for lifecycle-hooks.ts file..."
fd --type f "lifecycle-hooks.ts$" packages/job-dispatch/src
echo -e "\nChecking exports in lifecycle-hooks.ts..."
rg -A 5 "^export " packages/job-dispatch/src/lifecycle-hooks.ts
# Look for any references to lifecycle hooks in the codebase
echo -e "\nChecking for lifecycle hooks references..."
rg -g "*.ts" "lifecycle.*hooks?" packages/job-dispatch/src
Length of output: 855
Script:
#!/bin/bash
# Get the complete content of lifecycle-hooks.ts
echo "Content of lifecycle-hooks.ts:"
cat packages/job-dispatch/src/lifecycle-hooks.ts
# Check for any related interfaces or types
echo -e "\nChecking for related type definitions..."
ast-grep --pattern 'interface $_DeploymentLifecycle$_ {
$$$
}'
# Check for any usage of the exported function
echo -e "\nChecking for usage of handleTargetsFromEnvironmentToBeDeleted..."
rg -g "*.ts" "handleTargetsFromEnvironmentToBeDeleted" packages/job-dispatch/src
Length of output: 3292
apps/jobs/src/expired-env-checker/index.ts (1)
6-6: Verify the imported function exists in the job-dispatch module.
The import statement looks correct, but let's verify the function exists and is properly exported.
✅ Verification successful
The imported function exists and is properly exported from the job-dispatch module
The verification confirms that:
- The function is correctly exported from
packages/job-dispatch/src/lifecycle-hooks.ts - It's being used consistently across the codebase in both the API router and the expired-env-checker job
- The function signature matches the usage context, accepting
dbandenvparameters
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the imported function exists and is exported correctly
# Expected: Find the function declaration in the job-dispatch module
# Search for the function declaration
ast-grep --pattern 'export $_ handleTargetsFromEnvironmentToBeDeleted'
# Backup: Search for any occurrence of the function name
rg "handleTargetsFromEnvironmentToBeDeleted" -A 5
Length of output: 2072
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/LifecycleHooksTable.tsx (1)
29-36: Verify the dropdown implementation.
The actions dropdown is positioned absolutely within a flex container. This could lead to positioning issues in certain viewport sizes.
✅ Verification successful
Based on the extensive search results, I can now provide a definitive response about the dropdown implementation:
The dropdown positioning is properly handled and secure.
The LifecycleHookActionsDropdown component uses Radix UI's DropdownMenu primitive which provides robust positioning with proper portaling and floating UI behavior. The implementation includes:
- Proper content portaling via DropdownMenuPrimitive.Portal
- Built-in positioning logic with alignment controls
- Animation and transition states for smooth UI interactions
- Proper z-index management
- Accessibility features
The flex container usage is intentional and works correctly with the Radix UI dropdown implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the LifecycleHookActionsDropdown component handles positioning correctly
ast-grep --pattern 'const LifecycleHookActionsDropdown = $$$'
Length of output: 63
Script:
#!/bin/bash
# Let's try to find the LifecycleHookActionsDropdown component definition using ripgrep
rg "LifecycleHookActionsDropdown" -A 10
# Also search for any Dropdown components in the codebase to understand positioning patterns
rg "Dropdown.*position" -A 5
Length of output: 6675
Script:
#!/bin/bash
# Let's check the DropdownMenu component implementation and its positioning
rg "DropdownMenu" -A 5
# Also check for any UI component library imports that might be handling the dropdown positioning
rg "from.*@radix-ui/react-dropdown-menu" -A 2
Length of output: 220238
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/LifecycleHooksGettingStarted.tsx (1)
1-6: LGTM! Well-organized imports.
The imports are logically organized and all are being used within the component.
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/NavigationMenuAction.tsx (2)
5-5: LGTM: Imports are well-organized
The new imports are properly organized and necessary for the added functionality.
Also applies to: 10-11
20-22: LGTM: Improved path matching precision
The switch from includes() to endsWith() is a good improvement as it prevents false positives from partial path matches.
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/CreateLifecycleHookDialog.tsx (2)
46-46: Verify security measures for lifecycle hook creation.
Please ensure that proper security measures are in place for the mutation:
- Rate limiting to prevent abuse
- Permission checks for hook creation
- Validation of deployment ownership
#!/bin/bash
# Check for existing security measures in the API route
rg -A 10 "deployment\.lifecycleHook\.create"40-40: 🛠️ Refactor suggestion
Enhance schema validation for better data integrity.
The current schema only validates the presence of runbookId. Consider adding:
- Validation to ensure
runbookIdexists in the provided runbooks array - Additional fields for hook configuration (e.g., trigger conditions, description)
apps/event-worker/src/job-dispatch/github.ts (2)
3-3: LGTM: Import additions are appropriate.
The new imports (or, runbook, runbookJobTrigger) align with the expanded query functionality for handling both release and runbook job triggers.
Also applies to: 10-11
42-48: Query changes look good, but verify NULL handling.
The changes to use LEFT JOINs and the addition of runbook-related joins with OR conditions appropriately expand the job matching capabilities. However, we should verify the handling of NULL values that may now be returned due to the LEFT JOINs.
Let's verify the usage patterns to ensure proper NULL handling:
Also applies to: 53-56
✅ Verification successful
Let me gather more information about NULL handling in the codebase.
Let me check how the query results are processed after the joins.
Based on the code analysis, I can now generate the final response:
LEFT JOINs are properly handled with NULL checks
The code shows proper NULL handling through:
- Using
takeFirstOrNullto explicitly handle cases where no matching records are found - Subsequent NULL check
if (ghOrgResult == null)that updates job status toInvalidIntegration - Schema validation with
configSchema.safeParse()before the query execution
The changes to use LEFT JOINs are safe and well-implemented with appropriate error handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for NULL handling in related files
# Look for null checks or error handling related to these fields
rg -A 3 "environment\." "runbook\." --type typescript
# Check for similar patterns in other job dispatch implementations
fd -e ts -e js --full-path "job-dispatch"
# Look for test cases covering these scenarios
rg -l "test|describe|it" --type typescript | xargs rg -A 5 "dispatchGithubJob"
Length of output: 3284
Script:
#!/bin/bash
# Search for the schema definition and type information
ast-grep --pattern 'const configSchema = z.object({
$$$
})'
# Look for error handling patterns in the job dispatch implementation
rg -A 5 "if.*!.*\b(environment|runbook|releaseJobTrigger|runbookJobTrigger)\b" apps/event-worker/src/job-dispatch/github.ts
# Check how the query results are used
rg -A 10 "\.leftJoin.*environment" apps/event-worker/src/job-dispatch/github.ts
Length of output: 1146
Script:
#!/bin/bash
# Get the full context of the github.ts file to see how query results are handled
cat apps/event-worker/src/job-dispatch/github.ts
# Look for type definitions related to these tables
ast-grep --pattern 'interface $_ {
$$$
}'
# Search for any error handling or type checking after the query
rg -B 5 -A 10 "\.where\(" apps/event-worker/src/job-dispatch/github.ts
Length of output: 78125
packages/db/src/schema/deployment.ts (2)
8-8: LGTM: Import added for runbook schema reference.
The import is correctly placed and necessary for the foreign key reference in the new lifecycle hook table.
93-99: Update type definitions if adding suggested fields.
The current type definitions are correct, but if you implement the suggested schema improvements, remember to update the validation schema to include the new fields.
Let's check if similar lifecycle hook implementations exist in the codebase:
packages/api/src/router/runbook.ts (2)
39-39: LGTM! Correct alignment of procedure type with implementation.
The change from .mutation() to .query() better reflects the nature of this operation since it only performs read operations without any data modifications.
39-39: Verify frontend compatibility with the procedure type change.
Changing from mutation to query might require updates in the frontend code where this procedure is called, as TRPC clients typically handle mutations and queries differently.
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/LifecycleHookActionsDropdown.tsx (2)
1-31: LGTM! Well-structured type definitions and imports.
The type definitions are clear and properly typed with React.ReactNode for children props. The imports are well-organized and follow a logical grouping.
Also applies to: 79-82
1-107: Verify initialization handling aspects.
While the UI implementation for managing lifecycle hooks looks solid, the PR title mentions "Init handling lifecycle hooks". Could you provide more context about how this component relates to the initialization handling mentioned in the PR title?
Let's check for related initialization code:
packages/job-dispatch/src/job-dispatch.ts (1)
144-144: LGTM! Simplified runbook dispatch flow.
The removal of release variables handling for runbook dispatch is correct, as confirmed by the author's comment. This change fixes runbook dispatching by removing unnecessary deployment-specific logic.
Let's verify that runbooks are properly dispatched without release variables:
✅ Verification successful
Based on the results, I can now generate the final response:
Verified: Runbook dispatch correctly separates deployment and runbook concerns
The codebase analysis confirms:
createReleaseVariablesis only used in deployment-related code paths, specifically injob-variables-deployment.ts- Runbook dispatch is cleanly used in
lifecycle-hooks.tsandrouter/runbook.tswithout any deployment variable handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that runbook dispatch is separate from release variables
# Test 1: Check if createReleaseVariables is only used for deployments
rg -A 5 "createReleaseVariables"
# Test 2: Verify runbook dispatch patterns
ast-grep --pattern 'dispatchRunbook($$$)'
Length of output: 1916
packages/db/drizzle/meta/_journal.json (2)
221-227: LGTM! The journal entry is well-structured.
The new migration entry follows the established format and maintains sequential ordering with previous entries.
221-227: Verify deployment order for foreign key constraints.
Since this migration creates a new table with foreign key constraints to existing tables (deployment and runbook), ensure that the deployment process maintains referential integrity.
✅ Verification successful
Let me verify the migration order by checking the specific migration that adds the foreign key constraints.
Foreign key constraints are properly ordered and safe
The deployment order is correct as both referenced tables (deployment and runbook) were created in migration 0000_busy_firebird.sql, while the new table with foreign key constraints is created in migration 0031. This ensures referential integrity is maintained during deployment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the referenced tables exist in previous migrations
# Expected: Find references to deployment and runbook table creation
# Search for deployment table creation
echo "Checking deployment table creation:"
rg -B 2 -A 2 'CREATE TABLE.*deployment[^_]' packages/db/drizzle/
# Search for runbook table creation
echo "Checking runbook table creation:"
rg -B 2 -A 2 'CREATE TABLE.*runbook' packages/db/drizzle/
Length of output: 2163
Script:
#!/bin/bash
# Check the content of migration 0031 to see the foreign key constraints
echo "Checking migration 0031 content:"
find packages/db/drizzle -name "*0031*.sql" -exec cat {} \;
Length of output: 930
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/DeploymentNavBar.tsx (2)
56-56: LGTM! Improved path matching logic.
The changes improve the precision of active state detection by using endsWith instead of includes. The new lifecycle hooks URL and state variable follow consistent patterns.
Also applies to: 59-63
69-70: LGTM! Proper active state handling.
The settings active state correctly accounts for the new lifecycle hooks page, maintaining mutual exclusivity in navigation highlighting.
packages/api/src/router/environment.ts (1)
26-29: LGTM: Import changes are well-organized.
The new imports from '@ctrlplane/job-dispatch' are properly grouped with other external imports and align with the lifecycle hook handling requirements.
packages/api/src/router/deployment.ts (2)
38-38: LGTM: Import statement follows established patterns.
The import follows the existing pattern and naming conventions for router imports.
184-184: Verify integration with existing deployment operations.
The addition of lifecycleHook router looks good, but let's ensure proper integration with existing deployment workflows.
Let's verify the integration points:
✅ Verification successful
Integration with deployment operations verified successfully
The lifecycle hook integration is properly implemented with:
- Router composition in
deploymentRouterfollowing the established pattern - Complete schema definitions in
packages/db/schema/deployment.ts - Frontend components and UI integration in the webservice app
- Proper authorization checks and protected procedures in place
- Clear integration with the runbook system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for lifecycle hook usage in deployment-related operations
# Check for lifecycle hook references in deployment operations
echo "Checking lifecycle hook integration points..."
rg -l "lifecycleHook" --type ts
# Check for deployment lifecycle hook schema references
echo "Checking schema references..."
ast-grep --pattern 'type DeploymentLifecycleHook'
# Look for deployment lifecycle hook tests
echo "Checking test coverage..."
fd -e test.ts -e spec.ts | xargs rg "lifecycleHook"
Length of output: 1171
Script:
#!/bin/bash
# Let's check the implementation details and integration points
# Check the router implementation
echo "Checking deployment router implementation..."
rg "deploymentLifecycleHookRouter" -A 5 -B 5
# Check for lifecycle hook related procedures and handlers
echo "Checking lifecycle hook procedures..."
rg "lifecycleHook" -A 3 -B 3 packages/api/src/router/deployment.ts
# Check for related schema definitions
echo "Checking schema definitions..."
rg "DeploymentLifecycleHook" -A 5 -B 5
# Check for procedure calls from the frontend
echo "Checking frontend integration..."
rg "lifecycleHook.*mutation" -A 2 apps/webservice/src
Length of output: 10401
packages/job-dispatch/src/lifecycle-hooks.ts (7)
1-4: LGTM!
The imports are relevant and necessary for the functionality implemented in this file.
6-9: LGTM!
The imports are relevant and necessary for the functionality implemented in this file.
11-11: LGTM!
The import is relevant and necessary for dispatching runbooks.
25-36: LGTM!
The query to find the associated system with its non-null targetFilter environments (excluding the current environment) is well-structured and follows best practices.
49-58: LGTM!
The construction of the removedFromSystemFilter based on the system's environment filters is clear and concise.
77-88: LGTM!
The processing of lifecycle hooks for the removed targets using flatMap and dispatchRunbook is an efficient and readable approach.
1-91: Verify the usage of handleTargetsFromEnvironmentToBeDeleted in the codebase.
The handleTargetsFromEnvironmentToBeDeleted function is a crucial part of the environment deletion process. Ensure that it is being called appropriately in the relevant parts of the codebase, such as the environment deletion API endpoint or any cleanup scripts.
Run the following script to verify the function usage:
✅ Verification successful
Let me analyze the usage of handleTargetsFromEnvironmentToBeDeleted further by checking the specific contexts where it's being called.
Function is correctly used in environment deletion flows
The handleTargetsFromEnvironmentToBeDeleted function is properly integrated in two critical paths:
- In the API router (
packages/api/src/router/environment.ts): Called during manual environment deletion through the API - In the expired environment checker (
apps/jobs/src/expired-env-checker/index.ts): Called during automatic cleanup of expired environments
In both cases, the function is called with the correct parameters (db transaction and environment object) and is properly awaited before the actual environment deletion occurs, ensuring that target lifecycle hooks are processed before the environment is removed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of `handleTargetsFromEnvironmentToBeDeleted` in the codebase.
# Test: Search for the function usage. Expect: At least one occurrence in relevant files.
rg -A 5 $'handleTargetsFromEnvironmentToBeDeleted'
Length of output: 1999
Script:
#!/bin/bash
# Check the environment deletion API endpoint implementation
rg -B 10 -A 10 "handleTargetsFromEnvironmentToBeDeleted" packages/api/src/router/environment.ts
# Check the expired environment checker implementation
rg -B 10 -A 10 "handleTargetsFromEnvironmentToBeDeleted" apps/jobs/src/expired-env-checker/index.ts
Length of output: 2214
| const system = await api.system.bySlug(params); | ||
| const runbooks = await api.runbook.bySystemId(system.id); | ||
| const deployment = await api.deployment.bySlug(params); | ||
| if (!deployment) notFound(); | ||
| const lifecycleHooks = await api.deployment.lifecycleHook.list.byDeploymentId( | ||
| deployment.id, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing data fetching and error handling.
The current implementation has two areas for improvement:
- Performance: The sequential API calls create a waterfall effect. Consider using Promise.all for parallel execution.
- Error Handling: Only handles missing deployments. Consider adding error handling for system and runbook fetching.
Consider refactoring to:
- const system = await api.system.bySlug(params);
- const runbooks = await api.runbook.bySystemId(system.id);
- const deployment = await api.deployment.bySlug(params);
- if (!deployment) notFound();
- const lifecycleHooks = await api.deployment.lifecycleHook.list.byDeploymentId(
- deployment.id,
- );
+ try {
+ const system = await api.system.bySlug(params);
+ const [deployment, runbooks] = await Promise.all([
+ api.deployment.bySlug(params),
+ api.runbook.bySystemId(system.id),
+ ]);
+
+ if (!deployment) notFound();
+
+ const lifecycleHooks = await api.deployment.lifecycleHook.list.byDeploymentId(
+ deployment.id,
+ );
+
+ return { deployment, runbooks, lifecycleHooks };
+ } catch (error) {
+ console.error('Failed to fetch lifecycle hooks data:', error);
+ throw error; // Let Next.js error boundary handle it
+ }Committable suggestion skipped: line range outside the PR's diff.
| await Promise.all( | ||
| expiredEnvironments.map((env) => | ||
| handleTargetsFromEnvironmentToBeDeleted(db, env), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and logging for reliability.
The concurrent processing of environments is good for performance, but there are some reliability concerns:
- Missing error handling around Promise.all
- No logging of success/failure
- Potential race condition between target handling and environment deletion
Consider applying these improvements:
- await Promise.all(
- expiredEnvironments.map((env) =>
- handleTargetsFromEnvironmentToBeDeleted(db, env),
- ),
- );
+ try {
+ console.log(`Processing ${expiredEnvironments.length} expired environments`);
+ await Promise.all(
+ expiredEnvironments.map(async (env) => {
+ try {
+ await handleTargetsFromEnvironmentToBeDeleted(db, env);
+ console.log(`Successfully processed environment ${env.id}`);
+ } catch (error) {
+ console.error(`Failed to process environment ${env.id}:`, error);
+ throw error; // Re-throw to fail the entire batch
+ }
+ }),
+ );
+ } catch (error) {
+ console.error('Failed to process expired environments:', error);
+ throw error; // Prevent environment deletion on failure
+ }📝 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.
| await Promise.all( | |
| expiredEnvironments.map((env) => | |
| handleTargetsFromEnvironmentToBeDeleted(db, env), | |
| ), | |
| ); | |
| try { | |
| console.log(`Processing ${expiredEnvironments.length} expired environments`); | |
| await Promise.all( | |
| expiredEnvironments.map(async (env) => { | |
| try { | |
| await handleTargetsFromEnvironmentToBeDeleted(db, env); | |
| console.log(`Successfully processed environment ${env.id}`); | |
| } catch (error) { | |
| console.error(`Failed to process environment ${env.id}:`, error); | |
| throw error; // Re-throw to fail the entire batch | |
| } | |
| }), | |
| ); | |
| } catch (error) { | |
| console.error('Failed to process expired environments:', error); | |
| throw error; // Prevent environment deletion on failure | |
| } |
| export const LifecycleHooksTable: React.FC<LifecycleHooksTableProps> = ({ | ||
| lifecycleHooks, | ||
| }) => ( | ||
| <Table> | ||
| <TableBody> | ||
| {lifecycleHooks.map((lifecycleHook) => ( | ||
| <TableRow key={lifecycleHook.id}> | ||
| <TableCell>{lifecycleHook.runbook.name}</TableCell> | ||
| <TableCell> | ||
| <div className="flex justify-end"> | ||
| <LifecycleHookActionsDropdown lifecycleHook={lifecycleHook}> | ||
| <Button size="icon" variant="ghost" className="h-6 w-6"> | ||
| <IconDotsVertical size="icon" className="h-4 w-4" /> | ||
| </Button> | ||
| </LifecycleHookActionsDropdown> | ||
| </div> | ||
| </TableCell> | ||
| </TableRow> | ||
| ))} | ||
| </TableBody> | ||
| </Table> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance component implementation with best practices.
Several improvements could make this component more robust and accessible:
- Add accessibility attributes
- Implement error boundaries
- Add loading states
- Consider performance optimizations
Consider applying these changes:
+import { memo } from 'react';
+
-export const LifecycleHooksTable: React.FC<LifecycleHooksTableProps> = ({
+export const LifecycleHooksTable: React.FC<LifecycleHooksTableProps> = memo(({
lifecycleHooks,
-}) => (
+}) => {
+ if (lifecycleHooks.length === 0) {
+ return <div role="alert">No lifecycle hooks found</div>;
+ }
+
+ return (
<Table>
- <TableBody>
+ <TableBody role="rowgroup">
{lifecycleHooks.map((lifecycleHook) => (
- <TableRow key={lifecycleHook.id}>
- <TableCell>{lifecycleHook.runbook.name}</TableCell>
+ <TableRow key={lifecycleHook.id} role="row">
+ <TableCell role="cell">{lifecycleHook.runbook.name}</TableCell>
<TableCell>
<div className="flex justify-end">
<LifecycleHookActionsDropdown lifecycleHook={lifecycleHook}>
- <Button size="icon" variant="ghost" className="h-6 w-6">
+ <Button
+ size="icon"
+ variant="ghost"
+ className="h-6 w-6"
+ aria-label="Lifecycle hook actions"
+ >
<IconDotsVertical size="icon" className="h-4 w-4" />
</Button>
</LifecycleHookActionsDropdown>
</div>
</TableCell>
</TableRow>
))}
</TableBody>
</Table>
-);
+ );
+});
+
+LifecycleHooksTable.displayName = 'LifecycleHooksTable';📝 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 LifecycleHooksTable: React.FC<LifecycleHooksTableProps> = ({ | |
| lifecycleHooks, | |
| }) => ( | |
| <Table> | |
| <TableBody> | |
| {lifecycleHooks.map((lifecycleHook) => ( | |
| <TableRow key={lifecycleHook.id}> | |
| <TableCell>{lifecycleHook.runbook.name}</TableCell> | |
| <TableCell> | |
| <div className="flex justify-end"> | |
| <LifecycleHookActionsDropdown lifecycleHook={lifecycleHook}> | |
| <Button size="icon" variant="ghost" className="h-6 w-6"> | |
| <IconDotsVertical size="icon" className="h-4 w-4" /> | |
| </Button> | |
| </LifecycleHookActionsDropdown> | |
| </div> | |
| </TableCell> | |
| </TableRow> | |
| ))} | |
| </TableBody> | |
| </Table> | |
| ); | |
| import { memo } from 'react'; | |
| export const LifecycleHooksTable: React.FC<LifecycleHooksTableProps> = memo(({ | |
| lifecycleHooks, | |
| }) => { | |
| if (lifecycleHooks.length === 0) { | |
| return <div role="alert">No lifecycle hooks found</div>; | |
| } | |
| return ( | |
| <Table> | |
| <TableBody role="rowgroup"> | |
| {lifecycleHooks.map((lifecycleHook) => ( | |
| <TableRow key={lifecycleHook.id} role="row"> | |
| <TableCell role="cell">{lifecycleHook.runbook.name}</TableCell> | |
| <TableCell> | |
| <div className="flex justify-end"> | |
| <LifecycleHookActionsDropdown lifecycleHook={lifecycleHook}> | |
| <Button | |
| size="icon" | |
| variant="ghost" | |
| className="h-6 w-6" | |
| aria-label="Lifecycle hook actions" | |
| > | |
| <IconDotsVertical size="icon" className="h-4 w-4" /> | |
| </Button> | |
| </LifecycleHookActionsDropdown> | |
| </div> | |
| </TableCell> | |
| </TableRow> | |
| ))} | |
| </TableBody> | |
| </Table> | |
| ); | |
| }); | |
| LifecycleHooksTable.displayName = 'LifecycleHooksTable'; |
| <div className="relative -ml-1 text-neutral-500"> | ||
| <IconWebhook className="h-10 w-10" strokeWidth={0.5} /> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add aria-label to IconWebhook for accessibility.
The icon should have an aria-label for screen readers.
- <IconWebhook className="h-10 w-10" strokeWidth={0.5} />
+ <IconWebhook className="h-10 w-10" strokeWidth={0.5} aria-label="Lifecycle Hooks Icon" />📝 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="relative -ml-1 text-neutral-500"> | |
| <IconWebhook className="h-10 w-10" strokeWidth={0.5} /> | |
| </div> | |
| <div className="relative -ml-1 text-neutral-500"> | |
| <IconWebhook className="h-10 w-10" strokeWidth={0.5} aria-label="Lifecycle Hooks Icon" /> | |
| </div> |
| <Button size="sm" variant="secondary"> | ||
| Documentation | ||
| </Button> |
There was a problem hiding this comment.
Documentation button lacks navigation functionality.
The Documentation button doesn't have an href or onClick handler, making it non-functional.
- <Button size="sm" variant="secondary">
+ <Button
+ size="sm"
+ variant="secondary"
+ asChild
+ >
+ <a href="/docs/lifecycle-hooks" target="_blank" rel="noopener noreferrer">
Documentation
+ </a>
</Button>📝 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.
| <Button size="sm" variant="secondary"> | |
| Documentation | |
| </Button> | |
| <Button | |
| size="sm" | |
| variant="secondary" | |
| asChild | |
| > | |
| <a href="/docs/lifecycle-hooks" target="_blank" rel="noopener noreferrer"> | |
| Documentation | |
| </a> | |
| </Button> |
| const DeleteLifecycleHookDialog: React.FC<DeleteLifecycleHookDialogProps> = ({ | ||
| lifecycleHookId, | ||
| onClose, | ||
| children, | ||
| }) => { | ||
| const [open, setOpen] = useState(false); | ||
| const router = useRouter(); | ||
| const deleteLifecycleHook = api.deployment.lifecycleHook.delete.useMutation(); | ||
|
|
||
| const onDelete = async () => | ||
| deleteLifecycleHook | ||
| .mutateAsync(lifecycleHookId) | ||
| .then(() => router.refresh()) | ||
| .then(() => setOpen(false)); | ||
|
|
||
| return ( | ||
| <AlertDialog | ||
| open={open} | ||
| onOpenChange={(o) => { | ||
| setOpen(o); | ||
| if (!o) onClose(); | ||
| }} | ||
| > | ||
| <AlertDialogTrigger asChild>{children}</AlertDialogTrigger> | ||
| <AlertDialogContent> | ||
| <AlertDialogHeader> | ||
| <AlertDialogTitle>Delete Lifecycle Hook?</AlertDialogTitle> | ||
| <AlertDialogDescription> | ||
| This action cannot be undone. | ||
| </AlertDialogDescription> | ||
| </AlertDialogHeader> | ||
| <AlertDialogFooter> | ||
| <AlertDialogCancel>Cancel</AlertDialogCancel> | ||
| <div className="flex-grow" /> | ||
| <AlertDialogAction | ||
| className={buttonVariants({ variant: "destructive" })} | ||
| onClick={onDelete} | ||
| > | ||
| Delete | ||
| </AlertDialogAction> | ||
| </AlertDialogFooter> | ||
| </AlertDialogContent> | ||
| </AlertDialog> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and loading states for better UX.
The delete dialog implementation needs improvements in error handling and user feedback:
- Add loading state during deletion
- Handle and display error messages
- Optimize the refresh strategy
Consider applying these improvements:
const DeleteLifecycleHookDialog: React.FC<DeleteLifecycleHookDialogProps> = ({
lifecycleHookId,
onClose,
children,
}) => {
const [open, setOpen] = useState(false);
+ const [error, setError] = useState<string | null>(null);
const router = useRouter();
const deleteLifecycleHook = api.deployment.lifecycleHook.delete.useMutation();
- const onDelete = async () =>
- deleteLifecycleHook
- .mutateAsync(lifecycleHookId)
- .then(() => router.refresh())
- .then(() => setOpen(false));
+ const onDelete = async () => {
+ try {
+ await deleteLifecycleHook.mutateAsync(lifecycleHookId);
+ setOpen(false);
+ // Consider using more granular data updates instead of full page refresh
+ router.refresh();
+ } catch (err) {
+ setError(err instanceof Error ? err.message : 'Failed to delete lifecycle hook');
+ }
+ };
return (
<AlertDialog
open={open}
onOpenChange={(o) => {
setOpen(o);
+ setError(null);
if (!o) onClose();
}}
>
<AlertDialogTrigger asChild>{children}</AlertDialogTrigger>
<AlertDialogContent>
<AlertDialogHeader>
<AlertDialogTitle>Delete Lifecycle Hook?</AlertDialogTitle>
<AlertDialogDescription>
This action cannot be undone.
</AlertDialogDescription>
+ {error && (
+ <p className="text-sm text-destructive mt-2">{error}</p>
+ )}
</AlertDialogHeader>
<AlertDialogFooter>
<AlertDialogCancel>Cancel</AlertDialogCancel>
<div className="flex-grow" />
<AlertDialogAction
className={buttonVariants({ variant: "destructive" })}
onClick={onDelete}
+ disabled={deleteLifecycleHook.isLoading}
>
- Delete
+ {deleteLifecycleHook.isLoading ? 'Deleting...' : 'Delete'}
</AlertDialogAction>
</AlertDialogFooter>
</AlertDialogContent>
</AlertDialog>
);
};📝 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 DeleteLifecycleHookDialog: React.FC<DeleteLifecycleHookDialogProps> = ({ | |
| lifecycleHookId, | |
| onClose, | |
| children, | |
| }) => { | |
| const [open, setOpen] = useState(false); | |
| const router = useRouter(); | |
| const deleteLifecycleHook = api.deployment.lifecycleHook.delete.useMutation(); | |
| const onDelete = async () => | |
| deleteLifecycleHook | |
| .mutateAsync(lifecycleHookId) | |
| .then(() => router.refresh()) | |
| .then(() => setOpen(false)); | |
| return ( | |
| <AlertDialog | |
| open={open} | |
| onOpenChange={(o) => { | |
| setOpen(o); | |
| if (!o) onClose(); | |
| }} | |
| > | |
| <AlertDialogTrigger asChild>{children}</AlertDialogTrigger> | |
| <AlertDialogContent> | |
| <AlertDialogHeader> | |
| <AlertDialogTitle>Delete Lifecycle Hook?</AlertDialogTitle> | |
| <AlertDialogDescription> | |
| This action cannot be undone. | |
| </AlertDialogDescription> | |
| </AlertDialogHeader> | |
| <AlertDialogFooter> | |
| <AlertDialogCancel>Cancel</AlertDialogCancel> | |
| <div className="flex-grow" /> | |
| <AlertDialogAction | |
| className={buttonVariants({ variant: "destructive" })} | |
| onClick={onDelete} | |
| > | |
| Delete | |
| </AlertDialogAction> | |
| </AlertDialogFooter> | |
| </AlertDialogContent> | |
| </AlertDialog> | |
| ); | |
| }; | |
| const DeleteLifecycleHookDialog: React.FC<DeleteLifecycleHookDialogProps> = ({ | |
| lifecycleHookId, | |
| onClose, | |
| children, | |
| }) => { | |
| const [open, setOpen] = useState(false); | |
| const [error, setError] = useState<string | null>(null); | |
| const router = useRouter(); | |
| const deleteLifecycleHook = api.deployment.lifecycleHook.delete.useMutation(); | |
| const onDelete = async () => { | |
| try { | |
| await deleteLifecycleHook.mutateAsync(lifecycleHookId); | |
| setOpen(false); | |
| // Consider using more granular data updates instead of full page refresh | |
| router.refresh(); | |
| } catch (err) { | |
| setError(err instanceof Error ? err.message : 'Failed to delete lifecycle hook'); | |
| } | |
| }; | |
| return ( | |
| <AlertDialog | |
| open={open} | |
| onOpenChange={(o) => { | |
| setOpen(o); | |
| setError(null); | |
| if (!o) onClose(); | |
| }} | |
| > | |
| <AlertDialogTrigger asChild>{children}</AlertDialogTrigger> | |
| <AlertDialogContent> | |
| <AlertDialogHeader> | |
| <AlertDialogTitle>Delete Lifecycle Hook?</AlertDialogTitle> | |
| <AlertDialogDescription> | |
| This action cannot be undone. | |
| </AlertDialogDescription> | |
| {error && ( | |
| <p className="text-sm text-destructive mt-2">{error}</p> | |
| )} | |
| </AlertDialogHeader> | |
| <AlertDialogFooter> | |
| <AlertDialogCancel>Cancel</AlertDialogCancel> | |
| <div className="flex-grow" /> | |
| <AlertDialogAction | |
| className={buttonVariants({ variant: "destructive" })} | |
| onClick={onDelete} | |
| disabled={deleteLifecycleHook.isLoading} | |
| > | |
| {deleteLifecycleHook.isLoading ? 'Deleting...' : 'Delete'} | |
| </AlertDialogAction> | |
| </AlertDialogFooter> | |
| </AlertDialogContent> | |
| </AlertDialog> | |
| ); | |
| }; |
| ctx.db | ||
| .delete(environment) | ||
| .where(eq(environment.id, input)) | ||
| .returning() | ||
| .then(takeFirst) | ||
| .then((env) => handleTargetsFromEnvironmentToBeDeleted(ctx.db, env)), |
There was a problem hiding this comment.
💡 Codebase verification
Transaction wrapper is necessary for environment deletion
The implementation of handleTargetsFromEnvironmentToBeDeleted in packages/job-dispatch/src/lifecycle-hooks.ts confirms our concerns:
- The function performs database queries to find and handle targets based on environment's filter
- It's used in multiple places including the expired environment checker
- The function accepts a transaction (
Tx) parameter, indicating it was designed to work within transactions - The implementation shows it can fail during target querying/handling
The original review comment's suggestion to use transactions is correct and necessary to maintain data consistency. Without a transaction:
- If target handling fails, we'll have a deleted environment but potentially inconsistent target state
- The expired environment checker already handles this better by processing targets before deletion
Recommended changes from the original review should be implemented:
- Wrap operations in a transaction
- Add proper error handling
- Return consistent results
🔗 Analysis chain
Critical: Improve error handling and atomicity in delete mutation.
The current implementation has several potential issues:
- No transaction wrapper means we could end up in an inconsistent state if target handling fails after successful environment deletion
- The promise chain could silently swallow errors
- The atomicity of the delete operation is compromised
- The return value and error handling strategy is unclear
Consider wrapping the operations in a transaction for atomicity and adding proper error handling:
delete: protectedProcedure
.meta({
authorizationCheck: ({ canUser, input }) =>
canUser
.perform(Permission.SystemDelete)
.on({ type: "environment", id: input }),
})
.input(z.string().uuid())
.mutation(({ ctx, input }) =>
- ctx.db
- .delete(environment)
- .where(eq(environment.id, input))
- .returning()
- .then(takeFirst)
- .then((env) => handleTargetsFromEnvironmentToBeDeleted(ctx.db, env)),
+ ctx.db.transaction(async (tx) => {
+ const deletedEnv = await tx
+ .delete(environment)
+ .where(eq(environment.id, input))
+ .returning()
+ .then(takeFirst);
+
+ if (!deletedEnv) {
+ throw new Error(`Environment ${input} not found`);
+ }
+
+ try {
+ await handleTargetsFromEnvironmentToBeDeleted(tx, deletedEnv);
+ return deletedEnv;
+ } catch (error) {
+ console.error('Failed to handle targets for deleted environment:', error);
+ throw error;
+ }
+ }),
),Let's verify the implementation of handleTargetsFromEnvironmentToBeDeleted to understand its error handling:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the implementation of handleTargetsFromEnvironmentToBeDeleted
ast-grep --pattern 'export function handleTargetsFromEnvironmentToBeDeleted($_) {
$$$
}'
Length of output: 92
Script:
#!/bin/bash
# Let's try different search patterns to find the implementation
# First, let's search for the function name using ripgrep
rg -A 10 "handleTargetsFromEnvironmentToBeDeleted"
# Also search for any async function declaration in case it's defined differently
ast-grep --pattern 'async function handleTargetsFromEnvironmentToBeDeleted($_) {
$$$
}'
# And search for const declaration in case it's defined as an arrow function
ast-grep --pattern 'const handleTargetsFromEnvironmentToBeDeleted = async $_'
Length of output: 3473
| authorizationCheck: async ({ canUser, ctx, input }) => { | ||
| const hook = await ctx.db | ||
| .select() | ||
| .from(SCHEMA.deploymentLifecycleHook) | ||
| .where(eq(SCHEMA.deploymentLifecycleHook.id, input)) | ||
| .then(takeFirst); | ||
|
|
||
| return canUser | ||
| .perform(Permission.DeploymentUpdate) | ||
| .on({ type: "deployment", id: hook.deploymentId }); | ||
| }, |
There was a problem hiding this comment.
Handle potential undefined hook in delete procedure
If no deploymentLifecycleHook is found with the provided input ID, the hook variable will be undefined, leading to an error when accessing hook.deploymentId. To prevent this, add a check to handle the case when hook is undefined.
Apply this diff to fix the issue:
const hook = await ctx.db
.select()
.from(SCHEMA.deploymentLifecycleHook)
.where(eq(SCHEMA.deploymentLifecycleHook.id, input))
.then(takeFirst);
+ if (!hook) {
+ throw new Error("DeploymentLifecycleHook not found");
+ }
return canUser
.perform(Permission.DeploymentUpdate)
.on({ type: "deployment", id: hook.deploymentId });📝 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.
| authorizationCheck: async ({ canUser, ctx, input }) => { | |
| const hook = await ctx.db | |
| .select() | |
| .from(SCHEMA.deploymentLifecycleHook) | |
| .where(eq(SCHEMA.deploymentLifecycleHook.id, input)) | |
| .then(takeFirst); | |
| return canUser | |
| .perform(Permission.DeploymentUpdate) | |
| .on({ type: "deployment", id: hook.deploymentId }); | |
| }, | |
| authorizationCheck: async ({ canUser, ctx, input }) => { | |
| const hook = await ctx.db | |
| .select() | |
| .from(SCHEMA.deploymentLifecycleHook) | |
| .where(eq(SCHEMA.deploymentLifecycleHook.id, input)) | |
| .then(takeFirst); | |
| if (!hook) { | |
| throw new Error("DeploymentLifecycleHook not found"); | |
| } | |
| return canUser | |
| .perform(Permission.DeploymentUpdate) | |
| .on({ type: "deployment", id: hook.deploymentId }); | |
| }, |
| create: protectedProcedure | ||
| .input(SCHEMA.createDeploymentLifecycleHook) | ||
| .meta({ | ||
| authorizationCheck: ({ canUser, input }) => | ||
| canUser.perform(Permission.DeploymentUpdate).on({ | ||
| type: "deployment", | ||
| id: input.deploymentId, | ||
| }), | ||
| }) | ||
| .mutation(async ({ ctx, input }) => | ||
| ctx.db.insert(SCHEMA.deploymentLifecycleHook).values(input), | ||
| ), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure proper permission checks for runbookId in create procedure
While the authorizationCheck verifies that the user has permission to update the deployment, it does not check whether the user has access to the specified runbookId. This could lead to situations where a user links a runbook they shouldn't have access to. It's important to verify that the user has the necessary permissions for both the deployment and the runbook.
Consider modifying the authorizationCheck as follows:
authorizationCheck: async ({ canUser, ctx, input }) => {
- return canUser.perform(Permission.DeploymentUpdate).on({
+ const hasDeploymentPermission = canUser.perform(Permission.DeploymentUpdate).on({
type: "deployment",
id: input.deploymentId,
});
+
+ const hasRunbookPermission = await canUser
+ .perform(Permission.RunbookGet)
+ .on({ type: "runbook", id: input.runbookId });
+
+ return hasDeploymentPermission && hasRunbookPermission;
},📝 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(SCHEMA.createDeploymentLifecycleHook) | |
| .meta({ | |
| authorizationCheck: ({ canUser, input }) => | |
| canUser.perform(Permission.DeploymentUpdate).on({ | |
| type: "deployment", | |
| id: input.deploymentId, | |
| }), | |
| }) | |
| .mutation(async ({ ctx, input }) => | |
| ctx.db.insert(SCHEMA.deploymentLifecycleHook).values(input), | |
| ), | |
| create: protectedProcedure | |
| .input(SCHEMA.createDeploymentLifecycleHook) | |
| .meta({ | |
| authorizationCheck: async ({ canUser, ctx, input }) => { | |
| const hasDeploymentPermission = canUser.perform(Permission.DeploymentUpdate).on({ | |
| type: "deployment", | |
| id: input.deploymentId, | |
| }); | |
| const hasRunbookPermission = await canUser | |
| .perform(Permission.RunbookGet) | |
| .on({ type: "runbook", id: input.runbookId }); | |
| return hasDeploymentPermission && hasRunbookPermission; | |
| }, | |
| }) | |
| .mutation(async ({ ctx, input }) => | |
| ctx.db.insert(SCHEMA.deploymentLifecycleHook).values(input), | |
| ), |
| list: createTRPCRouter({ | ||
| byDeploymentId: protectedProcedure | ||
| .input(z.string().uuid()) | ||
| .meta({ | ||
| authorizationCheck: ({ canUser, input }) => | ||
| canUser | ||
| .perform(Permission.DeploymentGet) | ||
| .on({ type: "deployment", id: input }), | ||
| }) | ||
| .query(async ({ ctx, input }) => | ||
| ctx.db | ||
| .select() | ||
| .from(SCHEMA.deploymentLifecycleHook) | ||
| .innerJoin( | ||
| SCHEMA.runbook, | ||
| eq(SCHEMA.deploymentLifecycleHook.runbookId, SCHEMA.runbook.id), | ||
| ) | ||
| .where(eq(SCHEMA.deploymentLifecycleHook.deploymentId, input)) | ||
| .then((rows) => | ||
| rows.map((r) => ({ | ||
| ...r.deployment_lifecycle_hook, | ||
| runbook: r.runbook, | ||
| })), | ||
| ), | ||
| ), | ||
| }), |
There was a problem hiding this comment.
Incorrect use of createTRPCRouter inside list
The list property is currently initialized as a router using createTRPCRouter, which may not be necessary and could lead to unexpected behavior. Since byDeploymentId seems to be the only procedure under list, it would be clearer and more appropriate to define listByDeploymentId as a single procedure directly under the router.
Apply the following diff to refactor the code:
-export const deploymentLifecycleHookRouter = createTRPCRouter({
- list: createTRPCRouter({
- byDeploymentId: protectedProcedure
+export const deploymentLifecycleHookRouter = createTRPCRouter({
+ listByDeploymentId: protectedProcedure
.input(z.string().uuid())
.meta({
authorizationCheck: ({ canUser, input }) =>
canUser
.perform(Permission.DeploymentGet)
.on({ type: "deployment", id: input }),
})
.query(async ({ ctx, input }) =>
ctx.db
.select()
.from(SCHEMA.deploymentLifecycleHook)
.innerJoin(
SCHEMA.runbook,
eq(SCHEMA.deploymentLifecycleHook.runbookId, SCHEMA.runbook.id),
)
.where(eq(SCHEMA.deploymentLifecycleHook.deploymentId, input))
.then((rows) =>
rows.map((r) => ({
...r.deployment_lifecycle_hook,
runbook: r.runbook,
})),
),
),📝 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.
| list: createTRPCRouter({ | |
| byDeploymentId: protectedProcedure | |
| .input(z.string().uuid()) | |
| .meta({ | |
| authorizationCheck: ({ canUser, input }) => | |
| canUser | |
| .perform(Permission.DeploymentGet) | |
| .on({ type: "deployment", id: input }), | |
| }) | |
| .query(async ({ ctx, input }) => | |
| ctx.db | |
| .select() | |
| .from(SCHEMA.deploymentLifecycleHook) | |
| .innerJoin( | |
| SCHEMA.runbook, | |
| eq(SCHEMA.deploymentLifecycleHook.runbookId, SCHEMA.runbook.id), | |
| ) | |
| .where(eq(SCHEMA.deploymentLifecycleHook.deploymentId, input)) | |
| .then((rows) => | |
| rows.map((r) => ({ | |
| ...r.deployment_lifecycle_hook, | |
| runbook: r.runbook, | |
| })), | |
| ), | |
| ), | |
| }), | |
| export const deploymentLifecycleHookRouter = createTRPCRouter({ | |
| listByDeploymentId: protectedProcedure | |
| .input(z.string().uuid()) | |
| .meta({ | |
| authorizationCheck: ({ canUser, input }) => | |
| canUser | |
| .perform(Permission.DeploymentGet) | |
| .on({ type: "deployment", id: input }), | |
| }) | |
| .query(async ({ ctx, input }) => | |
| ctx.db | |
| .select() | |
| .from(SCHEMA.deploymentLifecycleHook) | |
| .innerJoin( | |
| SCHEMA.runbook, | |
| eq(SCHEMA.deploymentLifecycleHook.runbookId, SCHEMA.runbook.id), | |
| ) | |
| .where(eq(SCHEMA.deploymentLifecycleHook.deploymentId, input)) | |
| .then((rows) => | |
| rows.map((r) => ({ | |
| ...r.deployment_lifecycle_hook, | |
| runbook: r.runbook, | |
| })), | |
| ), | |
| ), | |
| }); |
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (23)
packages/db/drizzle/0031_lively_meltdown.sql (1)
1-5: Consider adding a created_at timestamp column.The table structure looks good with proper constraints and UUID usage. However, for audit purposes, it would be beneficial to track when lifecycle hooks are created.
CREATE TABLE IF NOT EXISTS "deployment_lifecycle_hook" ( "id" uuid PRIMARY KEY DEFAULT gen_random_uuid() NOT NULL, "deployment_id" uuid NOT NULL, - "runbook_id" uuid NOT NULL + "runbook_id" uuid NOT NULL, + "created_at" timestamp with time zone DEFAULT CURRENT_TIMESTAMP NOT NULL );apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/LifecycleHooksPageContent.tsx (1)
14-31: Consider adding loading state and explicit empty state check.While the implementation is clean, consider these improvements for better user experience and robustness:
export const LifecycleHooksPageContent: React.FC< LifecycleHooksPageContentProps > = ({ deployment, lifecycleHooks, runbooks }) => { + if (!lifecycleHooks) { + return <div>Loading...</div>; + } + - if (lifecycleHooks.length === 0) + if (Array.isArray(lifecycleHooks) && lifecycleHooks.length === 0) return ( <LifecycleHooksGettingStarted deployment={deployment}apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/LifecycleHooksTable.tsx (3)
11-18: Add JSDoc documentation for type definitions.Consider adding JSDoc comments to improve code documentation and IDE support.
+/** + * Represents a deployment lifecycle hook with its associated runbook. + */ type LifecycleHook = SCHEMA.DeploymentLifecycleHook & { runbook: SCHEMA.Runbook; }; +/** + * Props for the LifecycleHooksTable component. + */ type LifecycleHooksTableProps = { deploymentId: string; lifecycleHooks: LifecycleHook[]; };
20-22: Remove unused deploymentId prop.The
deploymentIdprop is defined in the component props but never used in the implementation.export const LifecycleHooksTable: React.FC<LifecycleHooksTableProps> = ({ - deploymentId, lifecycleHooks, }) => {
1-2: Add error boundary for client-side component.Since this is a client-side component (indicated by "use client"), consider wrapping it with an error boundary to handle potential runtime errors gracefully.
Consider creating and using an error boundary:
// components/ErrorBoundary.tsx 'use client'; class LifecycleHooksErrorBoundary extends React.Component { constructor(props) { super(props); this.state = { hasError: false }; } static getDerivedStateFromError(error) { return { hasError: true }; } render() { if (this.state.hasError) { return <div>Something went wrong loading the lifecycle hooks.</div>; } return this.props.children; } }Then wrap your component:
<LifecycleHooksErrorBoundary> <LifecycleHooksTable {...props} /> </LifecycleHooksErrorBoundary>apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/LifecycleHooksGettingStarted.tsx (1)
16-18: Consider standardizing padding values.The component uses multiple padding values (p-20). Consider using design system tokens or standardized values for consistent spacing.
-<div className="h-full w-full p-20"> - <div className="container m-auto max-w-xl space-y-6 p-20"> +<div className="h-full w-full p-8"> + <div className="container m-auto max-w-xl space-y-6 p-8">apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/NavigationMenuAction.tsx (2)
44-60: Consider UI/UX improvements for the lifecycle hooks button.While the loading state implementation is good, there are opportunities for improvement:
- The fixed width (
w-36) might cause layout shifts- The transition between loading and text states could be smoother
<Button size="sm" variant="secondary" disabled={runbooksQ.isLoading} - className="w-36" + className="min-w-36 inline-flex items-center justify-center" > - {runbooksQ.isLoading && ( - <IconLoader2 className="h-3 w-3 animate-spin" /> - )} - {!runbooksQ.isLoading && "New Lifecycle Hook"} + <span className="inline-flex items-center gap-2"> + {runbooksQ.isLoading && ( + <IconLoader2 className="h-3 w-3 animate-spin" /> + )} + <span>{runbooksQ.isLoading ? "Loading..." : "New Lifecycle Hook"}</span> + </span> </Button>
63-71: Consider extracting dialog rendering logic for better maintainability.The conditional rendering logic is becoming complex. Consider extracting it into a separate function or using a more declarative approach.
+ const getActiveDialog = () => { + if (isVariablesActive) { + return ( + <CreateVariableDialog deploymentId={deploymentId}> + <Button size="sm" variant="secondary">New Variable</Button> + </CreateVariableDialog> + ); + } + if (isReleaseChannelsActive) { + return ( + <CreateReleaseChannelDialog deploymentId={deploymentId}> + <Button size="sm" variant="secondary">New Release Channel</Button> + </CreateReleaseChannelDialog> + ); + } + if (isLifecycleHooksActive) { + return ( + <CreateLifecycleHookDialog + deploymentId={deploymentId} + runbooks={runbooksQ.data ?? []} + > + {/* ... button implementation ... */} + </CreateLifecycleHookDialog> + ); + } + return ( + <CreateReleaseDialog deploymentId={deploymentId} systemId={systemId}> + <Button size="sm" variant="secondary">New Release</Button> + </CreateReleaseDialog> + ); + }; + return ( <div> - {/* ... existing conditions ... */} + {getActiveDialog()} </div> );apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/CreateLifecycleHookDialog.tsx (2)
40-40: Consider enhancing the schema validation.The current schema only validates that
runbookIdis a string. Consider adding:
- Format validation to ensure the ID matches your expected format
- Custom error messages for better user experience
-const schema = z.object({ runbookId: z.string() }); +const schema = z.object({ + runbookId: z.string({ + required_error: "Please select a runbook", + invalid_type_error: "Runbook selection is invalid", + }).min(1, "Please select a runbook"), +});
77-82: Handle empty runbooks state.The component should handle cases where no runbooks are available.
<SelectContent> + {runbooks.length === 0 ? ( + <SelectItem value="" disabled> + No runbooks available + </SelectItem> + ) : ( {runbooks.map((runbook) => ( <SelectItem key={runbook.id} value={runbook.id}> {runbook.name} </SelectItem> ))} + )} </SelectContent>apps/event-worker/src/job-dispatch/github.ts (1)
53-56: Consider query performance optimizationThe combination of multiple left joins and OR conditions might impact query performance as the number of jobs and triggers grows. Consider:
- Adding appropriate indexes on
jobIdcolumns- Monitoring query execution time
- If performance becomes an issue, consider splitting into two separate queries (one for each trigger type) and union the results
packages/db/src/schema/deployment.ts (1)
79-91: Consider adding additional fields for better lifecycle management.The table structure is solid with proper foreign key relationships and cascade behaviors. However, consider adding:
- Timestamps (
created_at,updated_at) for audit purposes- A
typeorstagefield to differentiate between pre/post deployment hooks- A
statusfield to track hook execution stateHere's a suggested enhancement:
export const deploymentLifecycleHook = pgTable("deployment_lifecycle_hook", { id: uuid("id").primaryKey().defaultRandom(), deploymentId: uuid("deployment_id") .notNull() .references(() => deployment.id, { onDelete: "cascade", }), runbookId: uuid("runbook_id") .notNull() .references(() => runbook.id, { onDelete: "cascade", }), + type: text("type").notNull(), + status: text("status").default("pending").notNull(), + createdAt: timestamp("created_at").defaultNow().notNull(), + updatedAt: timestamp("updated_at").defaultNow().notNull(), });packages/api/src/router/runbook.ts (2)
Line range hint
39-57: Add error handling and consider performance optimizations.The current implementation has several areas for improvement:
- Error handling for database operations is missing
- No pagination which could lead to performance issues with large datasets
- The in-memory grouping could be optimized by handling it at the database level
Consider this implementation:
.query(({ ctx, input }) => - ctx.db + ctx.db.transaction(async (tx) => { + const limit = 100; // or use input parameter + try { + const result = await tx .select() .from(SCHEMA.runbook) .leftJoin( SCHEMA.runbookVariable, eq(SCHEMA.runbookVariable.runbookId, SCHEMA.runbook.id), ) .leftJoin( SCHEMA.jobAgent, eq(SCHEMA.runbook.jobAgentId, SCHEMA.jobAgent.id), ) .where(eq(SCHEMA.runbook.systemId, input)) + .limit(limit); + + return _.chain(result) .groupBy((rb) => rb.runbook.id) .map((rb) => ({ ...rb[0]!.runbook, variables: rb.map((v) => v.runbook_variable).filter(isPresent), jobAgent: rb[0]!.job_agent, })) .value(); + } catch (error) { + throw new Error(`Failed to fetch runbooks: ${error.message}`); + } + })
Line range hint
39-57: Consider adding type safety for the transformed result.The current implementation lacks type safety for the transformed result structure.
Consider adding a type definition:
type RunbookWithDetails = { id: string; // ... other runbook fields variables: Array<typeof SCHEMA.runbookVariable.$inferSelect>; jobAgent: typeof SCHEMA.jobAgent.$inferSelect | null; }; // Then use it in the query: .query(({ ctx, input }): Promise<RunbookWithDetails[]> => { // ... rest of the implementation })apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/LifecycleHookActionsDropdown.tsx (2)
96-102: Improve accessibility and keyboard navigation.The dropdown menu item has the following issues:
preventDefault()inonSelectbreaks keyboard navigation- Missing accessibility attributes
Consider these improvements:
<DropdownMenuItem className="flex items-center gap-2" - onSelect={(e) => e.preventDefault()} + aria-label="Delete lifecycle hook" > Delete <IconTrash size="icon" className="h-4 w-4 text-destructive" /> </DropdownMenuItem>
84-107: Add loading state handling.The component should handle loading states during deletion to prevent multiple delete requests and provide visual feedback to users.
Consider tracking the loading state and disabling the dropdown during deletion:
export const LifecycleHookActionsDropdown: React.FC< LifecycleHookActionsDropdownProps > = ({ lifecycleHook, children }) => { const [open, setOpen] = useState(false); + const [isDeleting, setIsDeleting] = useState(false); + + const handleDelete = async () => { + setIsDeleting(true); + try { + // ... deletion logic + } finally { + setIsDeleting(false); + } + }; + return ( - <DropdownMenu open={open} onOpenChange={setOpen}> + <DropdownMenu open={open} onOpenChange={setOpen} disabled={isDeleting}> {/* ... rest of the component */} </DropdownMenu> ); };apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/DeploymentNavBar.tsx (1)
Line range hint
114-122: Fix incorrect URL in Jobs navigation item.The Jobs menu item is currently linking to the variables URL instead of a jobs-specific URL.
Apply this fix:
- <Link href={variablesUrl} legacyBehavior passHref> + <Link href={`/${workspaceSlug}/systems/${systemSlug}/deployments/${deploymentSlug}/jobs`} legacyBehavior passHref>packages/api/src/router/deployment-lifecycle-hook.ts (2)
9-35: Consider adding pagination to thelistprocedure.As the number of deployment lifecycle hooks grows, returning all hooks associated with a deployment in a single query may become inefficient and impact performance.
To address this, consider implementing pagination using
limitandoffsetparameters in thelistprocedure. This will allow clients to retrieve hooks in smaller, manageable chunks.Here's an example of how you can modify the
listprocedure to support pagination:list: createTRPCRouter({ byDeploymentId: protectedProcedure - .input(z.string().uuid()) + .input(z.object({ + deploymentId: z.string().uuid(), + limit: z.number().min(1).max(100).optional().default(10), + offset: z.number().min(0).optional().default(0), + })) .meta({ authorizationCheck: ({ canUser, input }) => canUser .perform(Permission.DeploymentGet) - .on({ type: "deployment", id: input }), + .on({ type: "deployment", id: input.deploymentId }), }) .query(async ({ ctx, input }) => ctx.db .select() .from(SCHEMA.deploymentLifecycleHook) .innerJoin( SCHEMA.runbook, eq(SCHEMA.deploymentLifecycleHook.runbookId, SCHEMA.runbook.id), ) - .where(eq(SCHEMA.deploymentLifecycleHook.deploymentId, input)) + .where(eq(SCHEMA.deploymentLifecycleHook.deploymentId, input.deploymentId)) + .limit(input.limit) + .offset(input.offset) .then((rows) => rows.map((r) => ({ ...r.deployment_lifecycle_hook, runbook: r.runbook, })), ), ), }),
1-70: Consider adding logging for better observability.To help with debugging and monitoring the behavior of the deployment lifecycle hook router, consider adding logging statements at key points in the code.
For example, you can log:
- When a new hook is created
- When a hook is deleted
- Any errors that occur during the execution of the procedures
This will provide valuable insights into the runtime behavior of the router and aid in troubleshooting issues.
Here's an example of how you can add logging to the
createanddeleteprocedures:+ import { getLogger } from "../logger"; + + const logger = getLogger("deploymentLifecycleHookRouter"); + // ... create: protectedProcedure .input(SCHEMA.createDeploymentLifecycleHook) // ... .mutation(async ({ ctx, input }) => { + logger.info(`Creating new deployment lifecycle hook for deployment ${input.deploymentId}`); const result = await ctx.db.insert(SCHEMA.deploymentLifecycleHook).values(input); + logger.info(`Created deployment lifecycle hook with ID ${result.id}`); + return result; }), // ... delete: protectedProcedure .input(z.string().uuid()) // ... .mutation(async ({ ctx, input }) => { + logger.info(`Deleting deployment lifecycle hook with ID ${input}`); const result = await ctx.db .delete(SCHEMA.deploymentLifecycleHook) .where(eq(SCHEMA.deploymentLifecycleHook.id, input)); + logger.info(`Deleted deployment lifecycle hook with ID ${input}`); + return result; }),Make sure to handle any sensitive information in the logs appropriately.
packages/job-dispatch/src/lifecycle-hooks.ts (4)
17-17: Use strict equality checks for nullIn line 17, consider using strict equality
=== nullinstead of== nullfor better type safety and to avoid unexpected type coercion.
25-36: Handle the case where the system is not foundCurrently, if the system is not found (
system == null), the function returns silently. Consider adding logging or error handling to assist with debugging if this situation is unexpected.
60-75: Simplify conditional assignment ofremovedFromSystemTargetsThe conditional assignment of
removedFromSystemTargetscould be streamlined for better readability. Consider refactoring this block.Apply this diff to simplify the logic:
const removedFromSystemTargets = - envFilters.length > 0 - ? await db - .select() - .from(SCHEMA.target) - .where( - and( - SCHEMA.targetMatchesMetadata(db, removedFromSystemFilter), - inArray( - SCHEMA.target.id, - targets.map((t) => t.id), - ), - ), - ) - : targets; + await (envFilters.length > 0 + ? db + .select() + .from(SCHEMA.target) + .where( + and( + SCHEMA.targetMatchesMetadata(db, removedFromSystemFilter), + inArray(SCHEMA.target.id, targets.map((t) => t.id)), + ), + ) + : Promise.resolve(targets));
77-88: Improve readability by refactoring nestedflatMapThe nested
flatMapon lines 77-88 can be refactored to enhance readability. Using nested loops can make the code more understandable.Apply this diff to refactor the code:
const handleLifecycleHooksForTargets = removedFromSystemTargets.flatMap((t) => deploymentLifecycleHooks.map((dlh) => { const values: Record<string, string> = { targetId: t.id, deploymentId: dlh.deploymentId, environmentId: env.id, systemId: system.id, }; return dispatchRunbook(db, dlh.runbookId, values); }), ); +const handleLifecycleHooksForTargets = []; +for (const t of removedFromSystemTargets) { + for (const dlh of deploymentLifecycleHooks) { + const values: Record<string, string> = { + targetId: t.id, + deploymentId: dlh.deploymentId, + environmentId: env.id, + systemId: system.id, + }; + handleLifecycleHooksForTargets.push(dispatchRunbook(db, dlh.runbookId, values)); + } +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (21)
apps/event-worker/src/job-dispatch/github.ts(2 hunks)apps/jobs/src/expired-env-checker/index.ts(2 hunks)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]/lifecycle-hooks/CreateLifecycleHookDialog.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/LifecycleHookActionsDropdown.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/LifecycleHooksGettingStarted.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/LifecycleHooksPageContent.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/LifecycleHooksTable.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/page.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/release-channels/page.tsx(1 hunks)packages/api/src/router/deployment-lifecycle-hook.ts(1 hunks)packages/api/src/router/deployment.ts(2 hunks)packages/api/src/router/environment.ts(2 hunks)packages/api/src/router/runbook.ts(1 hunks)packages/db/drizzle/0031_lively_meltdown.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/deployment.ts(2 hunks)packages/job-dispatch/src/index.ts(1 hunks)packages/job-dispatch/src/job-dispatch.ts(1 hunks)packages/job-dispatch/src/lifecycle-hooks.ts(1 hunks)
🔇 Additional comments (36)
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/release-channels/page.tsx (2)
7-11: LGTM! Clean params handling.
The simplified approach of passing the entire params object is cleaner and reduces unnecessary destructuring while maintaining type safety.
7-11: Verify consistent params handling across similar components.
Since this change is part of a broader refactoring effort, let's ensure this pattern is consistently applied across other page components.
✅ Verification successful
Based on the search results, I can now generate the final response since I have enough information to verify the params handling pattern across page components.
Params handling is consistently applied across page components
The codebase shows a consistent pattern where page components use the params object directly without destructuring its properties in the function parameters. This matches the pattern in the file under review. Examples:
apps/webservice/src/app/[workspaceSlug]/systems/page.tsxapps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/variable-sets/page.tsxapps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/release-channels/page.tsx
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for inconsistent params handling patterns across page components
# Look for page components that might need similar refactoring
# Search for page components with similar parameter destructuring pattern
ast-grep --pattern 'export default async function $FUNC({ params: { workspaceSlug, systemSlug, deploymentSlug } }: { params: $_ }) {
$$$
}'
# Search for components already using the new pattern
ast-grep --pattern 'export default async function $FUNC({ params }: { params: $_ }) {
$$$
}'
Length of output: 132545
packages/db/drizzle/0031_lively_meltdown.sql (2)
7-11: LGTM! Well-structured foreign key constraint.
The constraint is properly defined with appropriate CASCADE behavior and error handling for safe migration reruns.
13-17: Verify CASCADE DELETE behavior for runbook references.
The current constraint will automatically delete lifecycle hooks when their associated runbook is deleted. Please verify if this is the intended behavior, as runbooks might be meant to serve as reusable templates.
Consider using ON DELETE RESTRICT instead if lifecycle hooks should prevent runbook deletion, or ON DELETE SET NULL if the hooks should be preserved (would require making runbook_id nullable).
✅ Verification successful
Based on the results, I can now generate a final response since I have sufficient information about the runbook relationships in the database schema.
CASCADE DELETE behavior for runbook references is consistent across the schema
The CASCADE DELETE behavior is consistently applied across all runbook relationships in the database:
- deployment_lifecycle_hook -> runbook
- runbook_job_trigger -> runbook
- runbook_variable -> runbook
This consistent pattern suggests that runbooks are designed to be atomic units where all associated data (triggers, variables, and lifecycle hooks) should be removed when the runbook itself is deleted. The CASCADE behavior appears to be an intentional design choice rather than an oversight.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if runbooks are referenced elsewhere to understand their lifecycle
# This will help determine the appropriate foreign key behavior
# Check for other foreign key references to runbook
rg -A 5 'REFERENCES.*runbook.*id'
# Look for runbook-related code patterns
ast-grep --pattern 'class $_ {
$$$
runbook($$$) {
$$$
}
$$$
}'
Length of output: 1647
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/page.tsx (3)
1-5: LGTM! Imports are well-organized.
The imports follow a clean structure with external dependencies first, followed by internal imports.
19-25: LGTM! Clean component rendering.
The component properly passes all required data to the content component.
6-10: LGTM! Component signature follows Next.js conventions.
The component is correctly typed as an async Server Component with proper parameter typing.
Let's verify the consistency of these parameter types across the codebase:
✅ Verification successful
Parameter types are consistent across related pages
Based on examining the variables/page.tsx file and finding other related pages in the same directory structure, we can confirm that:
- The parameter typing
{ params: { workspaceSlug: string; systemSlug: string; deploymentSlug: string } }is consistently used - The parameters are used correctly in API calls (e.g.,
api.deployment.bySlug(params)) - The same parameter structure is shared across multiple pages in the deployment section (
variables,lifecycle-hooks,releases, etc.)
The component signature follows Next.js conventions and maintains type consistency throughout the deployment-related pages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify parameter type consistency across similar page components
# Expected: Similar page components should use consistent parameter typing
# Search for similar page component parameter types
ast-grep --pattern 'export default $_ function $_Page({
params,
}: {
params: { workspaceSlug: string; systemSlug: string; deploymentSlug: string };
})'
Length of output: 157
Script:
#!/bin/bash
# Let's try a more flexible pattern to find page components with similar params
ast-grep --pattern 'export default $_ function $_({
params
}: {
params: { $$$; workspaceSlug: string; $$$; systemSlug: string; $$$; deploymentSlug: string; $$$}
})'
# Also search for any page components with these slug params to ensure we catch variations
rg -l "params.*workspaceSlug.*systemSlug.*deploymentSlug" --type typescript
Length of output: 283
Script:
#!/bin/bash
# Let's try searching for files with .ts or .tsx extension containing these params
rg -l "params.*workspaceSlug.*systemSlug.*deploymentSlug" -g "*.ts" -g "*.tsx"
# Also search for type definitions of these params
rg "type.*\{.*workspaceSlug.*systemSlug.*deploymentSlug" -g "*.ts" -g "*.tsx" -A 2
Length of output: 932
Script:
#!/bin/bash
# Let's examine one of these files to verify the parameter type consistency
rg "export default.*function.*params" -A 5 apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/variables/page.tsx
# Also check the layout file as it might define shared types
rg "export default.*function.*params" -A 5 apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/layout.tsx
Length of output: 303
Script:
#!/bin/bash
# Let's try to read the content of these files directly
cat apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/variables/page.tsx
# And also check for any shared type definitions in the directory
rg "type.*Params" -g "*.ts" -g "*.tsx" apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/
Length of output: 3785
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/LifecycleHooksPageContent.tsx (2)
1-4: LGTM! Clean and well-organized imports.
The imports are well-structured with type-only imports for schema and clear local component imports.
6-12: Verify runbook references integrity.
While the type structure is clean, it's important to ensure that all lifecycle hooks have valid runbook references to prevent runtime issues.
Let's verify the schema relationships:
packages/job-dispatch/src/index.ts (1)
24-24: LGTM! Verify the lifecycle-hooks module.
The new export follows the established patterns in terms of style, file extension, and organization.
Let's verify the existence and content of the lifecycle-hooks module:
✅ Verification successful
The lifecycle-hooks module exists and exports the expected function
The file exists as lifecycle-hooks.ts and exports the handleTargetsFromEnvironmentToBeDeleted function. The export in index.ts correctly re-exports this module, following the TypeScript convention where .js extension is used in imports even for TypeScript files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the lifecycle-hooks module exists and contains the expected exports
# Test 1: Check if the file exists
fd --type f "lifecycle-hooks.js" --exec echo "File exists: {}"
# Test 2: Examine the exports
rg -A 5 "export" "lifecycle-hooks.js"
Length of output: 157
Script:
#!/bin/bash
# Let's check for both .ts and .js files since this is a TypeScript project
fd "lifecycle-hooks\.(ts|js)" --exec echo "File exists: {}"
# Check the src directory specifically since the import is relative to src
fd "lifecycle-hooks\.(ts|js)" packages/job-dispatch/src --exec echo "File exists: {}"
# Let's also check for any files containing "lifecycle" in their name
fd "lifecycle.*\.(ts|js)" packages/job-dispatch/src --exec echo "File exists: {}"
Length of output: 409
Script:
#!/bin/bash
# Let's examine the content of the lifecycle-hooks.ts file
cat packages/job-dispatch/src/lifecycle-hooks.ts
# Also check if there are any exports
rg "export" packages/job-dispatch/src/lifecycle-hooks.ts
Length of output: 2843
apps/jobs/src/expired-env-checker/index.ts (1)
6-6: LGTM! Import changes align with the refactoring goals.
The new import from @ctrlplane/job-dispatch provides a dedicated function for handling target cleanup during environment deletion, which is a better separation of concerns.
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/LifecycleHooksGettingStarted.tsx (2)
1-11: LGTM! Clean imports and well-typed props.
The imports are well-organized and the props type definition is properly typed with schema types.
13-15: LGTM! Well-typed component declaration.
The component follows React best practices with proper type annotations and prop destructuring.
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/NavigationMenuAction.tsx (3)
5-5: LGTM: Import statements are well-organized and necessary.
The new imports support the lifecycle hooks feature with loading state visualization and data fetching capabilities.
Also applies to: 10-11
20-22: Improved path matching precision.
Using endsWith() instead of includes() provides more accurate route matching and prevents potential false positives.
24-24: Verify error handling for runbooks query.
While the query implementation is correct, consider adding error handling for failed queries to improve user experience.
- const runbooksQ = api.runbook.bySystemId.useQuery(systemId);
+ const runbooksQ = api.runbook.bySystemId.useQuery(systemId, {
+ onError: (error) => {
+ console.error('Failed to fetch runbooks:', error);
+ // Consider adding a toast notification here
+ }
+ });apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/CreateLifecycleHookDialog.tsx (1)
1-38: LGTM: Well-organized imports and clear type definitions.
The imports are logically grouped and the props interface is properly typed with clear documentation.
apps/event-worker/src/job-dispatch/github.ts (2)
3-3: LGTM: Import additions support new query logic
The new imports (or, runbook, runbookJobTrigger) are correctly added to support the enhanced query logic for handling both release and runbook job triggers.
Also applies to: 10-11
42-48: Verify NULL handling with left joins
The switch to left joins is appropriate for supporting both release and runbook triggers. However, please verify that the code handling ghOrgResult properly accounts for potential NULL values in the joined fields.
Let's verify the usage of these fields:
✅ Verification successful
Left joins are safely implemented with proper null handling
The code shows appropriate null handling:
- The query result is properly checked with
if (ghOrgResult == null) - After the null check, only the
github_organizationfield is accessed, which comes from the inner joined table - The left-joined tables' fields (environment, triggers) are not directly accessed in the code, suggesting they are only used in the WHERE clause for filtering
- The OR condition in the WHERE clause safely handles cases where either trigger type might be null
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there's proper NULL handling for the joined fields
# Look for any direct access to potentially null fields from environment or triggers
rg -A 3 "ghOrgResult\." apps/event-worker/src/job-dispatch/
# Check for any error handling related to these fields
rg -A 3 "if.*null.*environment|if.*null.*trigger" apps/event-worker/src/job-dispatch/
Length of output: 474
Script:
#!/bin/bash
# Let's check the full context of the query and its result handling
# First, let's see the full query context
rg -B 10 -A 20 "leftJoin.*environment" apps/event-worker/src/job-dispatch/github.ts
# Then, let's check how the results are processed
rg -B 5 -A 10 "const ghOrg = ghOrgResult" apps/event-worker/src/job-dispatch/github.ts
# Also check for any type definitions or interfaces related to this query
ast-grep --pattern 'interface $_ {
$$$
environment?: $_
$$$
}'
Length of output: 1694
Script:
#!/bin/bash
# Let's check how the query results are used further down in the file
rg -A 20 "const ghOrg = ghOrgResult" apps/event-worker/src/job-dispatch/github.ts
# Also check for any type safety checks or null checks after the query
rg -B 2 -A 5 "if.*ghOrgResult" apps/event-worker/src/job-dispatch/github.ts
# Look for any usage of environment or trigger fields
rg "ghOrgResult\.(environment|releaseJobTrigger|runbookJobTrigger)" apps/event-worker/src/job-dispatch/github.ts
Length of output: 1162
packages/db/src/schema/deployment.ts (3)
8-8: LGTM: Import statement is correctly placed and necessary.
The import of runbook schema is required for the foreign key reference in the new lifecycle hook table.
93-95: LGTM: Insert schema correctly omits auto-generated fields.
Note: If you implement the suggested additional fields from the previous comment, remember to update this insert schema to include the new required fields.
97-99: LGTM: Type export follows established patterns.
The type inference is correctly set up and will automatically reflect any schema changes.
packages/api/src/router/runbook.ts (2)
39-39: LGTM! Correct change from mutation to query.
The change from mutation to query is appropriate as this endpoint is fetching data rather than modifying it.
39-39: Verify frontend handling of the query change.
Since this endpoint changed from mutation to query, ensure that all frontend components are updated to use useQuery instead of useMutation.
✅ Verification successful
Frontend is already using useQuery for bySystemId endpoint
The verification shows that the frontend is correctly using useQuery for the bySystemId endpoint in all instances, particularly in the NavigationMenuAction component mentioned in the original context. No changes are needed as the frontend is already aligned with the API's query implementation.
Key examples found:
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/NavigationMenuAction.tsx:const runbooksQ = api.runbook.bySystemId.useQuery(systemId);- Multiple other components also correctly use
useQuerywith this endpoint
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining mutation calls to bySystemId
rg "useMutation.*bySystemId" --type ts
Length of output: 40
Script:
#!/bin/bash
# Let's first find where bySystemId is used in frontend components
rg "bySystemId" --type ts -A 3
# Also search for any usage of this specific query in frontend
rg "useQuery.*bySystemId" --type ts -A 3
Length of output: 15676
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/LifecycleHookActionsDropdown.tsx (1)
1-25: LGTM! Well-organized imports.
The imports are logically grouped and all dependencies are necessary for the component's functionality.
packages/job-dispatch/src/job-dispatch.ts (1)
144-144:
Verify the impact of skipping release variable creation.
The direct dispatch of the job without calling createReleaseVariables is inconsistent with the behavior in DispatchBuilder.dispatch(). This could lead to issues if the job requires these variables during execution.
Let's verify the usage of release variables in runbook jobs:
Consider one of these approaches:
- Restore the
createReleaseVariablescall with proper error handling - Document why release variables are not needed for runbook jobs
- Update
DispatchBuilder.dispatch()to be consistent if variables are truly not needed
packages/db/drizzle/meta/_journal.json (1)
221-227: LGTM! The migration journal entry is well-structured.
The new entry follows the established format and maintains sequential integrity with previous migrations.
Let's verify the corresponding migration file exists and contains the expected schema changes:
✅ Verification successful
Migration journal entry and corresponding migration file verified successfully
The verification confirms:
- Migration file
0031_lively_meltdown.sqlexists in the expected location - File contains the creation of
deployment_lifecycle_hooktable as expected
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the migration file exists and contains deployment_lifecycle_hook table creation
# Test 1: Check if the migration file exists
fd -t f "0031_lively_meltdown.sql" packages/db/drizzle
# Test 2: Verify the migration file content includes table creation
rg -l "CREATE TABLE.*deployment_lifecycle_hook" packages/db/drizzle/0031_lively_meltdown.sql
Length of output: 239
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/DeploymentNavBar.tsx (3)
56-56: LGTM! Improved active state detection logic.
The changes improve the precision of active state detection by using endsWith() instead of includes(). The lifecycle hooks integration is properly handled in both URL construction and active state management.
Also applies to: 59-64, 69-70
123-132: LGTM! Lifecycle hooks navigation item properly implemented.
The new navigation item follows the established pattern and maintains consistent styling.
56-70: Verify integration with lifecycle hooks components.
The navigation changes look good, but let's verify the integration with related components mentioned in the summary.
Also applies to: 123-132
✅ Verification successful
Navigation changes are properly integrated with lifecycle hooks components
The verification confirms proper integration:
- All required lifecycle hooks components exist and are properly located
- Components have correct import relationships
- Router integration is present in the API layer
- Navigation paths align with the component structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and proper integration of related lifecycle hooks components
# Check for the existence of related components
echo "Checking for related lifecycle hooks components..."
rg -l "CreateLifecycleHookDialog|LifecycleHooksGettingStarted|LifecycleHooksPageContent"
# Verify the router integration
echo "Checking for lifecycle hooks router..."
rg -l "deployment-lifecycle-hook"
# Check for proper imports of these components
echo "Checking for proper imports..."
rg "import.*CreateLifecycleHookDialog|import.*LifecycleHooksGettingStarted|import.*LifecycleHooksPageContent"
Length of output: 2003
packages/api/src/router/environment.ts (2)
26-29: LGTM: Import changes align with lifecycle hooks handling.
The addition of handleTargetsFromEnvironmentToBeDeleted import is well-placed alongside the existing job dispatch functionality.
328-333: Verify lifecycle hooks implementation consistency.
Let's verify the implementation of lifecycle hooks across the codebase.
✅ Verification successful
Let me gather more specific information about the implementation of handleTargetsFromEnvironmentToBeDeleted and its transaction handling.
Let me check one more crucial piece - the actual implementation of handleTargetsFromEnvironmentToBeDeleted to verify its transaction handling.
Based on the gathered information, I can now provide the final response:
Environment deletion transaction handling is correctly implemented
The environment deletion process is properly handled:
- The main deletion operation is wrapped in a transaction in
environment.ts - The
handleTargetsFromEnvironmentToBeDeletedfunction receives a transaction object (db: Tx) as parameter - The function is called using
.then()chaining, which maintains transaction context - No concurrent database operations are performed outside the transaction scope
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent patterns in lifecycle hook implementations
# Test 1: Find all lifecycle hook handler implementations
echo "Searching for lifecycle hook handlers..."
rg -l "lifecycle.*hook" --type ts
# Test 2: Check for similar transaction patterns in other handlers
echo "Checking transaction patterns in handlers..."
ast-grep --pattern 'ctx.db.transaction(async ($_) => { $$$ })'
# Test 3: Look for other direct uses of handleTargetsFromEnvironmentToBeDeleted
echo "Checking usage of handleTargetsFromEnvironmentToBeDeleted..."
rg "handleTargetsFromEnvironmentToBeDeleted" --type ts
Length of output: 26968
Script:
#!/bin/bash
# Check the implementation of handleTargetsFromEnvironmentToBeDeleted
echo "Checking handleTargetsFromEnvironmentToBeDeleted implementation..."
ast-grep --pattern 'handleTargetsFromEnvironmentToBeDeleted = async ($_, $_) => { $$$ }'
# Look for transaction patterns specifically in environment.ts
echo "Checking transaction patterns in environment.ts..."
rg -A 10 "ctx\.db\.transaction" packages/api/src/router/environment.ts
# Check for any other environment deletion related handlers
echo "Looking for environment deletion handlers..."
rg -A 5 "delete.*environment" --type ts
Length of output: 8599
Script:
#!/bin/bash
# Get the implementation of handleTargetsFromEnvironmentToBeDeleted from job-dispatch
echo "Getting handleTargetsFromEnvironmentToBeDeleted implementation..."
rg -A 20 "handleTargetsFromEnvironmentToBeDeleted = async" packages/job-dispatch/src/lifecycle-hooks.ts
# Check if there are any other database operations in the same file
echo "Checking other database operations in lifecycle-hooks.ts..."
rg "\.delete\(|\.update\(|\.insert\(" packages/job-dispatch/src/lifecycle-hooks.ts
Length of output: 1046
packages/api/src/router/deployment.ts (2)
38-38: LGTM: Clean import integration
The import follows the established pattern and is properly grouped with other router imports.
184-184: LGTM: Router integration looks good
The lifecycle hook router integration follows the existing pattern of router composition and is placed logically with other sub-routers.
Let's verify that the new router properly implements authorization checks:
✅ Verification successful
Authorization checks properly implemented
The lifecycle hook router correctly implements authorization checks for all its procedures:
list.byDeploymentId: RequiresDeploymentGetpermission on the deploymentcreate: RequiresDeploymentUpdatepermission on the deploymentdelete: Includes authorization check with database validation
The implementation follows the established pattern using protectedProcedure with proper meta.authorizationCheck definitions for each endpoint.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify authorization checks in the lifecycle hook router
# Search for authorization checks in the new router
ast-grep --pattern 'meta({
authorizationCheck: $$$
})'
Length of output: 58
Script:
#!/bin/bash
# Let's try a different approach to find authorization checks in the router
# First, let's locate the deployment lifecycle hook router file
fd deploymentLifecycleHook
# Then search for authorization patterns commonly used in routers
rg -l "authorize|isAuthorized|requireAuth|protectedProcedure|middleware" $(fd deploymentLifecycleHook)
# Also check the actual router implementation
rg -A 5 "router|procedure" $(fd deploymentLifecycleHook)
Length of output: 142657
Script:
#!/bin/bash
# Let's check the deployment lifecycle hook router implementation for authorization checks
# First locate the router file
fd -t f deployment-lifecycle-hook
# Then examine its contents for authorization patterns
rg -A 5 "protectedProcedure|procedure|meta|auth" $(fd -t f deployment-lifecycle-hook)
Length of output: 1359
packages/api/src/router/deployment-lifecycle-hook.ts (2)
1-70: LGTM! The router implementation looks solid and well-structured.
The deploymentLifecycleHookRouter is properly defined with clear separation of concerns for listing, creating, and deleting deployment lifecycle hooks. The input validation, authorization checks, and database queries are implemented correctly.
The router integrates well with the existing schema and follows best practices for modular design.
50-69: Verify the impact of deleting a deployment lifecycle hook.
Deleting a deployment lifecycle hook may have unintended consequences if other parts of the system rely on its existence. Before allowing deletion, consider the following:
- Check if the hook is currently in use by any active or scheduled deployments.
- Verify if deleting the hook would break any existing deployment workflows.
- Ensure that deleting the hook does not leave any orphaned records in other tables.
To help verify the impact, you can run the following script:
Replace <hook_id> with the actual ID of the hook being deleted.
If any of these checks indicate potential issues, consider implementing additional safeguards or warning the user before allowing the deletion.
✅ Verification successful
Deletion of deployment lifecycle hooks appears safe
The code analysis shows that deployment lifecycle hooks are independent entities with proper access control. The deletion operation:
- Is protected by authorization checks ensuring only users with
DeploymentUpdatepermission can delete hooks - Only removes the specific hook record without cascading effects
- Shows no evidence of being referenced by active deployments or other critical operations
The hook appears to be a configuration entity that can be safely removed without orphaning records or breaking existing deployments.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the hook is used by any active deployments
rg -A 5 $'deploymentLifecycleHookId.*=.*<hook_id>' packages/api/src/router/deployment.ts
# Check if the hook is referenced in any deployment workflows
rg -A 5 $'deploymentLifecycleHookId.*=.*<hook_id>' packages/api/src/router/deployment-workflow.ts
# Check for any potential orphaned records in other tables
rg -A 5 $'deploymentLifecycleHookId.*=.*<hook_id>'
Length of output: 327
Script:
#!/bin/bash
# Let's check the schema and table relationships first
rg -A 10 "deploymentLifecycleHook" packages/api/src/schema/
# Check for any foreign key constraints or references
rg "deploymentLifecycleHook.*foreign" packages/api/src/schema/
# Look for any usage of the hook in deployment-related files
rg -l "deploymentLifecycleHook" packages/api/src/
# Check for any cascade delete or related operations
ast-grep --pattern 'delete($$$).where(eq(SCHEMA.deploymentLifecycleHook$$$))'
Length of output: 475
Script:
#!/bin/bash
# Let's check how deployment lifecycle hooks are used in deployment.ts
rg -A 5 "deploymentLifecycleHook" packages/api/src/router/deployment.ts
# Check the schema definition in the deployment.ts
rg "createTable.*deployment" packages/api/src/router/deployment.ts
# Look for any references or relationships in deployment-lifecycle-hook.ts
rg -A 5 "createTable|references|foreign|relation" packages/api/src/router/deployment-lifecycle-hook.ts
# Check for any hook execution or usage patterns
ast-grep --pattern 'from($$$deploymentLifecycleHook$$$).where($$$)'
Length of output: 739
| const system = await api.system.bySlug(params); | ||
| const runbooks = await api.runbook.bySystemId(system.id); | ||
| const deployment = await api.deployment.bySlug(params); | ||
| if (!deployment) notFound(); | ||
| const lifecycleHooks = await api.deployment.lifecycleHook.list.byDeploymentId( | ||
| deployment.id, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing data fetching and error handling.
The current implementation has several areas for improvement:
- Sequential API calls could be parallelized for better performance
- Missing error handling for system and runbooks fetch
- Potential race conditions in data dependencies
Consider refactoring to this pattern:
- const system = await api.system.bySlug(params);
- const runbooks = await api.runbook.bySystemId(system.id);
- const deployment = await api.deployment.bySlug(params);
- if (!deployment) notFound();
- const lifecycleHooks = await api.deployment.lifecycleHook.list.byDeploymentId(
- deployment.id,
- );
+ const [system, deployment] = await Promise.all([
+ api.system.bySlug(params).catch(() => notFound()),
+ api.deployment.bySlug(params).catch(() => notFound()),
+ ]);
+
+ if (!deployment || !system) notFound();
+
+ const [runbooks, lifecycleHooks] = await Promise.all([
+ api.runbook.bySystemId(system.id),
+ api.deployment.lifecycleHook.list.byDeploymentId(deployment.id),
+ ]);📝 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 system = await api.system.bySlug(params); | |
| const runbooks = await api.runbook.bySystemId(system.id); | |
| const deployment = await api.deployment.bySlug(params); | |
| if (!deployment) notFound(); | |
| const lifecycleHooks = await api.deployment.lifecycleHook.list.byDeploymentId( | |
| deployment.id, | |
| ); | |
| const [system, deployment] = await Promise.all([ | |
| api.system.bySlug(params).catch(() => notFound()), | |
| api.deployment.bySlug(params).catch(() => notFound()), | |
| ]); | |
| if (!deployment || !system) notFound(); | |
| const [runbooks, lifecycleHooks] = await Promise.all([ | |
| api.runbook.bySystemId(system.id), | |
| api.deployment.lifecycleHook.list.byDeploymentId(deployment.id), | |
| ]); |
| await Promise.all( | ||
| expiredEnvironments.map((env) => | ||
| handleTargetsFromEnvironmentToBeDeleted(db, env), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
The shell script results show that Promise.allSettled is consistently used across the codebase for handling concurrent operations, particularly in similar scenarios involving multiple asynchronous tasks. The implementation patterns found in other files (like kubernetes-job-agent/src/index.ts and apps/event-worker/src/target-scan/gke.ts) demonstrate robust error handling with detailed logging.
Consider using Promise.allSettled with proper error handling, following established patterns
- await Promise.all(
- expiredEnvironments.map((env) =>
- handleTargetsFromEnvironmentToBeDeleted(db, env),
- ),
- );
+ const results = await Promise.allSettled(
+ expiredEnvironments.map((env) =>
+ handleTargetsFromEnvironmentToBeDeleted(db, env)
+ .catch(error => {
+ console.error(`Failed to process environment ${env.id}:`, error);
+ return { environmentId: env.id, error };
+ })
+ ),
+ );
+
+ const failures = results.filter(
+ (result): result is PromiseRejectedResult => result.status === 'rejected'
+ ).length;
+
+ if (failures > 0) {
+ console.error(`Failed to process ${failures} out of ${expiredEnvironments.length} environments`);
+ }🔗 Analysis chain
Consider adding error handling for concurrent operations.
While using Promise.all for concurrent processing is efficient, it has some potential issues:
- It will fail fast if any single environment processing fails
- Multiple concurrent database operations might impact system performance
Consider this more robust implementation:
- await Promise.all(
- expiredEnvironments.map((env) =>
- handleTargetsFromEnvironmentToBeDeleted(db, env),
- ),
- );
+ const results = await Promise.allSettled(
+ expiredEnvironments.map((env) =>
+ handleTargetsFromEnvironmentToBeDeleted(db, env)
+ .catch(error => {
+ console.error(`Failed to handle targets for environment ${env.id}:`, error);
+ return { environmentId: env.id, error };
+ })
+ ),
+ );
+
+ const failures = results.filter(
+ (result): result is PromiseRejectedResult => result.status === 'rejected'
+ );
+
+ if (failures.length > 0) {
+ console.error(`Failed to process ${failures.length} environments`);
+ }Let's verify the potential impact of concurrent operations:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other uses of Promise.all with database operations
rg -l "Promise\.all.*db\." --type ts
# Look for any existing error handling patterns in similar operations
ast-grep --pattern 'Promise.allSettled($$$)'
Length of output: 6178
| <Table> | ||
| <TableBody> | ||
| {lifecycleHooks.map((lifecycleHook) => ( | ||
| <TableRow key={lifecycleHook.id}> | ||
| <TableCell>{lifecycleHook.runbook.name}</TableCell> | ||
| <TableCell> | ||
| <div className="flex justify-end"> | ||
| <LifecycleHookActionsDropdown lifecycleHook={lifecycleHook}> | ||
| <Button size="icon" variant="ghost" className="h-6 w-6"> | ||
| <IconDotsVertical size="icon" className="h-4 w-4" /> | ||
| </Button> | ||
| </LifecycleHookActionsDropdown> | ||
| </div> | ||
| </TableCell> | ||
| </TableRow> | ||
| ))} | ||
| </TableBody> | ||
| </Table> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance table accessibility and structure.
The table implementation could be improved by:
- Adding table headers for better structure
- Including empty state handling
- Adding proper ARIA attributes
<Table>
+ <TableHeader>
+ <TableRow>
+ <TableHead>Runbook Name</TableHead>
+ <TableHead className="w-[100px]">Actions</TableHead>
+ </TableRow>
+ </TableHeader>
<TableBody>
+ {lifecycleHooks.length === 0 ? (
+ <TableRow>
+ <TableCell colSpan={2} className="text-center text-muted-foreground">
+ No lifecycle hooks found
+ </TableCell>
+ </TableRow>
+ ) : (
{lifecycleHooks.map((lifecycleHook) => (
<TableRow key={lifecycleHook.id}>
<TableCell>{lifecycleHook.runbook.name}</TableCell>
<TableCell>
<div className="flex justify-end">
<LifecycleHookActionsDropdown lifecycleHook={lifecycleHook}>
- <Button size="icon" variant="ghost" className="h-6 w-6">
+ <Button
+ size="icon"
+ variant="ghost"
+ className="h-6 w-6"
+ aria-label={`Actions for ${lifecycleHook.runbook.name}`}
+ >
<IconDotsVertical size="icon" className="h-4 w-4" />
</Button>
</LifecycleHookActionsDropdown>
</div>
</TableCell>
</TableRow>
- ))}
+ ))}
+ )}
</TableBody>
</Table>📝 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.
| <Table> | |
| <TableBody> | |
| {lifecycleHooks.map((lifecycleHook) => ( | |
| <TableRow key={lifecycleHook.id}> | |
| <TableCell>{lifecycleHook.runbook.name}</TableCell> | |
| <TableCell> | |
| <div className="flex justify-end"> | |
| <LifecycleHookActionsDropdown lifecycleHook={lifecycleHook}> | |
| <Button size="icon" variant="ghost" className="h-6 w-6"> | |
| <IconDotsVertical size="icon" className="h-4 w-4" /> | |
| </Button> | |
| </LifecycleHookActionsDropdown> | |
| </div> | |
| </TableCell> | |
| </TableRow> | |
| ))} | |
| </TableBody> | |
| </Table> | |
| <Table> | |
| <TableHeader> | |
| <TableRow> | |
| <TableHead>Runbook Name</TableHead> | |
| <TableHead className="w-[100px]">Actions</TableHead> | |
| </TableRow> | |
| </TableHeader> | |
| <TableBody> | |
| {lifecycleHooks.length === 0 ? ( | |
| <TableRow> | |
| <TableCell colSpan={2} className="text-center text-muted-foreground"> | |
| No lifecycle hooks found | |
| </TableCell> | |
| </TableRow> | |
| ) : ( | |
| lifecycleHooks.map((lifecycleHook) => ( | |
| <TableRow key={lifecycleHook.id}> | |
| <TableCell>{lifecycleHook.runbook.name}</TableCell> | |
| <TableCell> | |
| <div className="flex justify-end"> | |
| <LifecycleHookActionsDropdown lifecycleHook={lifecycleHook}> | |
| <Button | |
| size="icon" | |
| variant="ghost" | |
| className="h-6 w-6" | |
| aria-label={`Actions for ${lifecycleHook.runbook.name}`} | |
| > | |
| <IconDotsVertical size="icon" className="h-4 w-4" /> | |
| </Button> | |
| </LifecycleHookActionsDropdown> | |
| </div> | |
| </TableCell> | |
| </TableRow> | |
| )) | |
| )} | |
| </TableBody> | |
| </Table> |
| <CreateLifecycleHookDialog | ||
| deploymentId={deployment.id} | ||
| runbooks={runbooks} | ||
| > | ||
| <Button size="sm">Create Lifecycle Hook</Button> | ||
| </CreateLifecycleHookDialog> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle empty runbooks state.
The CreateLifecycleHookDialog should be disabled when there are no available runbooks to prevent users from starting a creation flow that can't be completed.
<CreateLifecycleHookDialog
deploymentId={deployment.id}
runbooks={runbooks}
+ disabled={runbooks.length === 0}
>
- <Button size="sm">Create Lifecycle Hook</Button>
+ <Button size="sm" disabled={runbooks.length === 0}>
+ {runbooks.length === 0
+ ? "No runbooks available"
+ : "Create Lifecycle Hook"
+ }
+ </Button>
</CreateLifecycleHookDialog>📝 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.
| <CreateLifecycleHookDialog | |
| deploymentId={deployment.id} | |
| runbooks={runbooks} | |
| > | |
| <Button size="sm">Create Lifecycle Hook</Button> | |
| </CreateLifecycleHookDialog> | |
| <CreateLifecycleHookDialog | |
| deploymentId={deployment.id} | |
| runbooks={runbooks} | |
| disabled={runbooks.length === 0} | |
| > | |
| <Button size="sm" disabled={runbooks.length === 0}> | |
| {runbooks.length === 0 | |
| ? "No runbooks available" | |
| : "Create Lifecycle Hook" | |
| } | |
| </Button> | |
| </CreateLifecycleHookDialog> |
| <Button size="sm" variant="secondary"> | ||
| Documentation | ||
| </Button> |
There was a problem hiding this comment.
Add functionality to Documentation button.
The Documentation button is currently non-functional. Consider adding an href or onClick handler to navigate to the relevant documentation.
-<Button size="sm" variant="secondary">
+<Button
+ size="sm"
+ variant="secondary"
+ asChild
+>
+ <a
+ href="/docs/lifecycle-hooks"
+ target="_blank"
+ rel="noopener noreferrer"
+ >
Documentation
+ </a>
</Button>📝 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.
| <Button size="sm" variant="secondary"> | |
| Documentation | |
| </Button> | |
| <Button | |
| size="sm" | |
| variant="secondary" | |
| asChild | |
| > | |
| <a | |
| href="/docs/lifecycle-hooks" | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| > | |
| Documentation | |
| </a> | |
| </Button> |
| const onDelete = async () => | ||
| deleteLifecycleHook | ||
| .mutateAsync(lifecycleHookId) | ||
| .then(() => router.refresh()) | ||
| .then(() => setOpen(false)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and improve promise chain.
The deletion logic has the following issues:
- Missing error handling for the mutation
- Inconsistent behavior: onClose is not called after successful deletion
- Promise chain can be simplified
Consider this improvement:
- const onDelete = async () =>
- deleteLifecycleHook
- .mutateAsync(lifecycleHookId)
- .then(() => router.refresh())
- .then(() => setOpen(false));
+ const onDelete = async () => {
+ try {
+ await deleteLifecycleHook.mutateAsync(lifecycleHookId);
+ router.refresh();
+ setOpen(false);
+ onClose();
+ } catch (error) {
+ console.error('Failed to delete lifecycle hook:', error);
+ // Consider adding a toast notification here
+ }
+ };Committable suggestion skipped: line range outside the PR's diff.
| const job = await createTriggeredRunbookJob(db, runbook, values); | ||
| await createReleaseVariables(db, job.id) | ||
| .then(() => dispatchJobsQueue.add(job.id, { jobId: job.id })) | ||
| .catch((error) => | ||
| db | ||
| .update(schema.job) | ||
| .set({ | ||
| status: JobStatus.Failure, | ||
| message: `Failed to create release variables: ${error.message}`, | ||
| }) | ||
| .where(eq(schema.job.id, job.id)), | ||
| ); | ||
| await dispatchJobsQueue.add(job.id, { jobId: job.id }); | ||
| return job; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for job dispatch.
The function lacks error handling for the dispatch operation. If the queue addition fails, there's no status update to reflect the failure.
Consider wrapping the dispatch in a try-catch:
const job = await createTriggeredRunbookJob(db, runbook, values);
- await dispatchJobsQueue.add(job.id, { jobId: job.id });
+ try {
+ await dispatchJobsQueue.add(job.id, { jobId: job.id });
+ await db
+ .update(schema.job)
+ .set({ status: JobStatus.InProgress })
+ .where(eq(schema.job.id, job.id));
+ } catch (error) {
+ await db
+ .update(schema.job)
+ .set({
+ status: JobStatus.Failure,
+ message: `Failed to dispatch job: ${error.message}`,
+ })
+ .where(eq(schema.job.id, job.id));
+ throw error;
+ }
return job;📝 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 job = await createTriggeredRunbookJob(db, runbook, values); | |
| await createReleaseVariables(db, job.id) | |
| .then(() => dispatchJobsQueue.add(job.id, { jobId: job.id })) | |
| .catch((error) => | |
| db | |
| .update(schema.job) | |
| .set({ | |
| status: JobStatus.Failure, | |
| message: `Failed to create release variables: ${error.message}`, | |
| }) | |
| .where(eq(schema.job.id, job.id)), | |
| ); | |
| await dispatchJobsQueue.add(job.id, { jobId: job.id }); | |
| return job; | |
| const job = await createTriggeredRunbookJob(db, runbook, values); | |
| try { | |
| await dispatchJobsQueue.add(job.id, { jobId: job.id }); | |
| await db | |
| .update(schema.job) | |
| .set({ status: JobStatus.InProgress }) | |
| .where(eq(schema.job.id, job.id)); | |
| } catch (error) { | |
| await db | |
| .update(schema.job) | |
| .set({ | |
| status: JobStatus.Failure, | |
| message: `Failed to dispatch job: ${error.message}`, | |
| }) | |
| .where(eq(schema.job.id, job.id)); | |
| throw error; | |
| } | |
| return job; |
| ctx.db | ||
| .delete(environment) | ||
| .where(eq(environment.id, input)) | ||
| .returning() | ||
| .then(takeFirst) | ||
| .then((env) => handleTargetsFromEnvironmentToBeDeleted(ctx.db, env)), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider transaction safety and error handling improvements.
The current implementation has potential issues:
- Moving target cleanup outside the transaction could lead to inconsistency if the cleanup fails.
- No explicit error handling for the target cleanup process.
- The
.then()chain could be more readable with async/await.
Consider refactoring to:
- ctx.db
- .delete(environment)
- .where(eq(environment.id, input))
- .returning()
- .then(takeFirst)
- .then((env) => handleTargetsFromEnvironmentToBeDeleted(ctx.db, env)),
+ ctx.db.transaction(async (tx) => {
+ const deletedEnv = await tx
+ .delete(environment)
+ .where(eq(environment.id, input))
+ .returning()
+ .then(takeFirst);
+
+ if (!deletedEnv) {
+ throw new Error('Environment not found');
+ }
+
+ try {
+ await handleTargetsFromEnvironmentToBeDeleted(tx, deletedEnv);
+ return deletedEnv;
+ } catch (error) {
+ console.error('Failed to cleanup targets:', error);
+ throw error;
+ }
+ }),This refactoring:
- Maintains transactional integrity
- Adds proper error handling
- Improves code readability with async/await
- Ensures atomic operations
📝 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.
| ctx.db | |
| .delete(environment) | |
| .where(eq(environment.id, input)) | |
| .returning() | |
| .then(takeFirst) | |
| .then((env) => handleTargetsFromEnvironmentToBeDeleted(ctx.db, env)), | |
| ctx.db.transaction(async (tx) => { | |
| const deletedEnv = await tx | |
| .delete(environment) | |
| .where(eq(environment.id, input)) | |
| .returning() | |
| .then(takeFirst); | |
| if (!deletedEnv) { | |
| throw new Error('Environment not found'); | |
| } | |
| try { | |
| await handleTargetsFromEnvironmentToBeDeleted(tx, deletedEnv); | |
| return deletedEnv; | |
| } catch (error) { | |
| console.error('Failed to cleanup targets:', error); | |
| throw error; | |
| } | |
| }), |
| }), | ||
| ); | ||
|
|
||
| await Promise.all(handleLifecycleHooksForTargets); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for asynchronous operations
When using Promise.all, consider wrapping it in a try...catch block to handle any potential rejections from the dispatched runbooks.
Apply this diff to add error handling:
-await Promise.all(handleLifecycleHooksForTargets);
+try {
+ await Promise.all(handleLifecycleHooksForTargets);
+} catch (error) {
+ // Handle errors appropriately, such as logging or retrying
+ console.error('Error dispatching runbooks:', error);
+}Committable suggestion skipped: line range outside the PR's diff.
| const deploymentLifecycleHooks = await db | ||
| .select() | ||
| .from(SCHEMA.deploymentLifecycleHook) | ||
| .innerJoin( | ||
| SCHEMA.deployment, | ||
| eq(SCHEMA.deploymentLifecycleHook.deploymentId, SCHEMA.deployment.id), | ||
| ) | ||
| .where(eq(SCHEMA.deployment.systemId, system.id)) | ||
| .then((rows) => rows.map((r) => r.deployment_lifecycle_hook)); | ||
| if (deploymentLifecycleHooks.length === 0) return; |
There was a problem hiding this comment.
💡 Codebase verification
Property access in mapping function needs correction
Based on the schema definition found in packages/db/src/schema/deployment.ts, the table is defined as deploymentLifecycleHook and the property should match the Drizzle ORM's naming convention. The current access r.deployment_lifecycle_hook appears incorrect as it uses snake_case instead of the expected camelCase format from the ORM's type inference.
- Change
r.deployment_lifecycle_hooktor.deploymentLifecycleHookto match the Drizzle ORM's type inference
🔗 Analysis chain
Verify the property access in the mapping function
Ensure that r.deployment_lifecycle_hook on line 46 refers to the correct property from the joined tables. If the property name is different, it might cause issues when mapping the rows.
Run the following script to check the structure of the joined result:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the properties of the rows returned by the deploymentLifecycleHooks query.
# Expected: Confirm that 'deployment_lifecycle_hook' is the correct property name.
# Since we cannot execute code, please manually verify that the alias or property names used in the query match the expected structure.
Length of output: 0
Script:
#!/bin/bash
# Let's check the schema definition to verify the table structure and column names
rg -A 10 "deploymentLifecycleHook = " --type ts
# Also check for any type definitions or interfaces related to this
rg -A 5 "interface.*DeploymentLifecycleHook" --type ts
rg -A 5 "type.*DeploymentLifecycleHook" --type ts
Length of output: 2058
Script:
#!/bin/bash
# Let's check the drizzle ORM's select query result type by looking at similar queries in the codebase
rg -B 2 -A 2 "from\(SCHEMA\.deploymentLifecycleHook\).*innerJoin" --type ts
# Also check for any other similar mappings of deployment_lifecycle_hook
rg "\.deployment_lifecycle_hook\)" --type ts
Length of output: 236
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
packages/api/src/router/deployment-lifecycle-hook.ts (1)
27-32: Consider improving type safety in the data transformation.The current implementation assumes the shape of the joined data. Consider using a more explicit type definition or zod schema for the return type to ensure type safety.
+ type JoinedRow = { + deployment_lifecycle_hook: DeploymentLifecycleHook; + runbook: Runbook; + }; .then((rows) => - rows.map((r) => ({ + rows.map((r: JoinedRow) => ({ ...r.deployment_lifecycle_hook, runbook: r.runbook, })), ),apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/CreateLifecycleHookDialog.tsx (2)
40-44: Consider enhancing the validation schema.The current schema only validates the presence of
runbookId. Consider adding more validation rules:
- Ensure runbookId exists in the provided runbooks array
- Add custom error messages for better user feedback
-const schema = z.object({ runbookId: z.string() }); +const schema = z.object({ + runbookId: z.string() + .refine( + (id) => runbooks.some(runbook => runbook.id === id), + "Please select a valid runbook" + ) +});
57-97: Enhance dialog UX with empty state and loading indicators.Consider the following improvements:
- Add empty state handling for when no runbooks are available
- Add loading indicator in dialog content during submission
- Add visual feedback when form validation fails
<DialogContent> <DialogHeader> <DialogTitle>Create Lifecycle Hook</DialogTitle> </DialogHeader> <Form {...form}> <form onSubmit={onSubmit} className="space-y-4"> + {runbooks.length === 0 ? ( + <div className="text-center text-gray-500"> + No runbooks available. Please create a runbook first. + </div> + ) : ( <FormField control={form.control} name="runbookId" render={({ field: { value, onChange } }) => ( <FormItem> <FormLabel>Runbook</FormLabel> <Select value={value} onValueChange={onChange}> <SelectTrigger> <SelectValue placeholder="Select a runbook" /> </SelectTrigger> <SelectContent> {runbooks.map((runbook) => ( <SelectItem key={runbook.id} value={runbook.id}> {runbook.name} </SelectItem> ))} </SelectContent> </Select> + {form.formState.errors.runbookId && ( + <div className="text-sm text-red-500"> + {form.formState.errors.runbookId.message} + </div> + )} </FormItem> )} /> + )} <DialogFooter> <Button type="submit" disabled={createLifecycleHook.isPending}> - Create + {createLifecycleHook.isPending ? ( + <> + <span className="loading loading-spinner" /> + Creating... + </> + ) : ( + 'Create' + )} </Button> </DialogFooter> </form> </Form> </DialogContent>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/CreateLifecycleHookDialog.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/LifecycleHookActionsDropdown.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/LifecycleHooksPageContent.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/LifecycleHooksTable.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/page.tsx(1 hunks)packages/api/src/router/deployment-lifecycle-hook.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/LifecycleHookActionsDropdown.tsx
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/LifecycleHooksPageContent.tsx
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/LifecycleHooksTable.tsx
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/page.tsx
🔇 Additional comments (4)
packages/api/src/router/deployment-lifecycle-hook.ts (1)
1-8: LGTM: Imports and router setup are correct.
All necessary dependencies are properly imported, and the router is correctly configured.
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/lifecycle-hooks/CreateLifecycleHookDialog.tsx (3)
1-38: LGTM! Well-structured imports and type definitions.
The component is properly marked as client-side, imports are organized logically, and the props interface is well-defined with appropriate types.
45-55:
Critical: Implement proper error handling.
The form submission logic needs improvement. Previous review comments about error handling and loading state management are still valid and should be addressed.
32-32: Verify TRPC router implementation.
Let's verify that the TRPC router and mutation are properly implemented.
Also applies to: 46-47
✅ Verification successful
TRPC router implementation is properly integrated
The verification confirms that:
- The TRPC router for lifecycle hooks is properly implemented in
packages/api/src/router/deployment-lifecycle-hook.ts - The router includes the required
createmutation with proper authorization checks - The implementation is correctly imported and exposed through the deployment router
- The component's usage of
api.deployment.lifecycleHook.create.useMutation()matches the server-side implementation
The integration is complete and correct.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify TRPC router implementation for lifecycle hooks
# Test 1: Check if the TRPC router exists and has the create mutation
echo "Checking TRPC router implementation..."
rg -l "lifecycleHook.*create.*useMutation" --type ts
# Test 2: Check the router implementation
echo "Checking router implementation details..."
rg -A 10 "deployment.*lifecycleHook.*create.*procedure" --type ts
Length of output: 438
Script:
#!/bin/bash
# Let's try a broader search to find the TRPC router implementation
# Search for lifecycle hook router files
echo "Searching for lifecycle hook router files..."
fd -e ts router
# Search for lifecycle hook procedure definitions
echo "Searching for lifecycle hook procedures..."
rg -A 5 "lifecycleHook|deployment.*procedure" --type ts
# Search for TRPC router imports and usage
echo "Checking TRPC router imports and usage..."
ast-grep --pattern 'import { $_ } from "~/trpc/react"'
Length of output: 34526
Script:
#!/bin/bash
# Let's check the deployment lifecycle hook router implementation in the API package
# Search for the router implementation
echo "Searching for lifecycle hook router implementation..."
rg -A 10 "deploymentLifecycleHookRouter" packages/api/src/router/
# Search for create procedure
echo "Searching for create procedure..."
rg -A 10 "lifecycleHook.*create.*procedure" packages/api/src/router/
Length of output: 2849
Script:
#!/bin/bash
# Let's check the specific file for create procedure
echo "Checking deployment-lifecycle-hook router file..."
cat packages/api/src/router/deployment-lifecycle-hook.ts
# Also check for any related schema definitions
echo "Checking for related schema definitions..."
rg -A 5 "DeploymentLifecycleHook.*schema" packages/api/src/
Length of output: 2405
| .mutation(async ({ ctx, input }) => | ||
| ctx.db.insert(SCHEMA.deploymentLifecycleHook).values(input), | ||
| ), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for the insert operation.
The database insert operation should include error handling to gracefully handle potential failures (e.g., unique constraint violations).
.mutation(async ({ ctx, input }) =>
- ctx.db.insert(SCHEMA.deploymentLifecycleHook).values(input),
+ ctx.db.insert(SCHEMA.deploymentLifecycleHook).values(input).catch((error) => {
+ throw new Error(`Failed to create deployment lifecycle hook: ${error.message}`);
+ }),
),📝 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.
| .mutation(async ({ ctx, input }) => | |
| ctx.db.insert(SCHEMA.deploymentLifecycleHook).values(input), | |
| ), | |
| .mutation(async ({ ctx, input }) => | |
| ctx.db.insert(SCHEMA.deploymentLifecycleHook).values(input).catch((error) => { | |
| throw new Error(`Failed to create deployment lifecycle hook: ${error.message}`); | |
| }), | |
| ), |
| .mutation(async ({ ctx, input }) => | ||
| ctx.db | ||
| .delete(SCHEMA.deploymentLifecycleHook) | ||
| .where(eq(SCHEMA.deploymentLifecycleHook.id, input)), | ||
| ), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify successful deletion.
The delete operation should confirm that a row was actually deleted to distinguish between "not found" and "successfully deleted" cases.
.mutation(async ({ ctx, input }) =>
- ctx.db
+ const result = await ctx.db
.delete(SCHEMA.deploymentLifecycleHook)
- .where(eq(SCHEMA.deploymentLifecycleHook.id, input)),
+ .where(eq(SCHEMA.deploymentLifecycleHook.id, input))
+ .returning();
+
+ if (result.length === 0) {
+ throw new Error("DeploymentLifecycleHook not found or already deleted");
+ }
+ return result[0];
),📝 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.
| .mutation(async ({ ctx, input }) => | |
| ctx.db | |
| .delete(SCHEMA.deploymentLifecycleHook) | |
| .where(eq(SCHEMA.deploymentLifecycleHook.id, input)), | |
| ), | |
| .mutation(async ({ ctx, input }) => | |
| const result = await ctx.db | |
| .delete(SCHEMA.deploymentLifecycleHook) | |
| .where(eq(SCHEMA.deploymentLifecycleHook.id, input)) | |
| .returning(); | |
| if (result.length === 0) { | |
| throw new Error("DeploymentLifecycleHook not found or already deleted"); | |
| } | |
| return result[0]; | |
| ), |
| export const deploymentLifecycleHook = pgTable("deployment_lifecycle_hook", { | ||
| id: uuid("id").primaryKey().defaultRandom(), | ||
| deploymentId: uuid("deployment_id") | ||
| .notNull() | ||
| .references(() => deployment.id, { | ||
| onDelete: "cascade", | ||
| }), | ||
| runbookId: uuid("runbook_id") | ||
| .notNull() | ||
| .references(() => runbook.id, { | ||
| onDelete: "cascade", | ||
| }), | ||
| }); |
There was a problem hiding this comment.
that implies it does all types of hooks
not just runbook hooks
what would the tables be called for webhooks?
There was a problem hiding this comment.
should be also include the type of hook this is? target delete, deployment create, deployment delete, env delete? deployment update?
There was a problem hiding this comment.
we can add this stuff in once we use more than one type of hook
|
|
||
| import { dispatchRunbook } from "./job-dispatch.js"; | ||
|
|
||
| export const handleTargetsFromEnvironmentToBeDeleted = async ( |
There was a problem hiding this comment.
can you have llm generate a jsdocs description for this function
| systemId: system.id, | ||
| }; | ||
|
|
||
| return dispatchRunbook(db, dlh.runbookId, values); |
There was a problem hiding this comment.
what happens if you add these and they are not defined as variables on the runbook? Should we not add them to the job?
There was a problem hiding this comment.
this function directly creates the variables on the job - so technically the runbook can have 0 variables defined, if its set as an exit hook, then the ids will all be set as variabels for the resulting job
| const system = await api.system.bySlug(params); | ||
| const runbooks = await api.runbook.bySystemId(system.id); | ||
| const deployment = await api.deployment.bySlug(params); | ||
| if (!deployment) notFound(); | ||
| const lifecycleHooks = await api.deployment.lifecycleHook.byDeploymentId( | ||
| deployment.id, | ||
| ); |
There was a problem hiding this comment.
the deployment endpoint returns the system
| const deployment = await api.deployment.bySlug(params); | ||
| if (!deployment) notFound(); |
There was a problem hiding this comment.
this check should be first, don't query extra data if you don't need too
| id: input.deploymentId, | ||
| }), | ||
| }) | ||
| .mutation(async ({ ctx, input }) => |
| import { dispatchRunbook } from "./job-dispatch.js"; | ||
|
|
||
| export const handleTargetsFromEnvironmentToBeDeleted = async ( | ||
| db: Tx, |
There was a problem hiding this comment.
because we are doing dispatch, I don't see why we need to pass in a tx
There was a problem hiding this comment.
this could lead to race conditions like before
| .perform(Permission.DeploymentGet) | ||
| .on({ type: "deployment", id: input }), | ||
| }) | ||
| .query(async ({ ctx, input }) => |
There was a problem hiding this comment.
| .query(async ({ ctx, input }) => | |
| .query(({ ctx, input }) => |
Summary by CodeRabbit
Release Notes
New Features
LifecycleHooksGettingStartedcomponent for user onboarding.Improvements
Bug Fixes
Documentation