init #208
init #208
Conversation
WalkthroughThis pull request introduces several significant changes across multiple packages, primarily focusing on the integration of an event handling system. Key modifications include the addition of a new dependency on Changes
Sequence Diagram(s)sequenceDiagram
participant API
participant Events
participant DB
participant Handlers
API->>DB: Delete Environment
DB-->>API: Confirm Deletion
API->>Events: getEventsForEnvironmentDeleted
Events->>DB: Query for Events
DB-->>Events: Return Events
API->>Handlers: handleEvent for each Event
Handlers-->>API: Event Handling Complete
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (9)
packages/validators/src/events/hooks/index.ts (2)
8-9: Add JSDoc documentation for the hook event types.Consider adding documentation to explain:
- The purpose and use cases of these hook events
- The relationship between targetRemoved and targetDeleted events
- Example usage in the broader system
Example documentation:
+/** + * Schema for hook events related to target lifecycle. + * Combines both target removal and deletion events for unified handling. + */ export const hookEvent = z.union([targetRemoved, targetDeleted]); + +/** + * Type representing all possible target-related hook events. + * @see targetRemoved + * @see targetDeleted + */ export type HookEvent = z.infer<typeof hookEvent>;
11-15: Consider enhancing type guard robustness and maintainability.The type guards are functionally correct, but could be improved for better maintainability and safety.
Consider these improvements:
+// Define action constants to avoid magic strings +const TARGET_ACTIONS = { + REMOVED: 'removed', + DELETED: 'deleted' +} as const; + // typeguards -export const isTargetRemoved = (event: HookEvent): event is TargetRemoved => - event.action === "removed"; -export const isTargetDeleted = (event: HookEvent): event is TargetDeleted => - event.action === "deleted"; +export const isTargetRemoved = ( + event: HookEvent | unknown +): event is TargetRemoved => { + return ( + typeof event === 'object' && + event !== null && + 'action' in event && + event.action === TARGET_ACTIONS.REMOVED + ); +}; + +export const isTargetDeleted = ( + event: HookEvent | unknown +): event is TargetDeleted => { + return ( + typeof event === 'object' && + event !== null && + 'action' in event && + event.action === TARGET_ACTIONS.DELETED + ); +};This refactor:
- Centralizes action strings for easier maintenance
- Adds runtime type checking for better error handling
- Accepts unknown input for safer type narrowing
packages/events/src/index.ts (1)
7-7: Consider removing the .js extension from imports.The
.jsextension in the import statement is unusual in TypeScript files. TypeScript's module resolution typically handles this automatically based on themoduleResolutioncompiler option.-import { handleTargetRemoved } from "./handlers/target-removed.js"; +import { handleTargetRemoved } from "./handlers/target-removed";packages/validators/src/events/hooks/target.ts (1)
4-8: Consider strengthening type safety for the target config field.The
configfield usingz.record(z.any())allows any value type, which could lead to runtime issues. Consider defining a more specific schema for the expected configuration structure if possible.Example of a more type-safe approach:
- config: z.record(z.any()), + config: z.record(z.union([z.string(), z.number(), z.boolean()])),packages/events/src/handlers/target-removed.ts (1)
8-10: Add error handling for payload extraction.While the destructuring is clean, consider adding validation to handle cases where the payload might be malformed.
export const handleTargetRemoved = async (event: TargetRemoved) => { + if (!event.payload?.target || !event.payload?.deployment) { + throw new Error('Invalid event payload: missing target or deployment'); + } const { target, deployment } = event.payload;apps/jobs/src/expired-env-checker/index.ts (1)
10-10: Ensure consistent time zones when comparing datesWhen using
new Date(), ensure that the time zone aligns withSCHEMA.environment.expiresAtto avoid potential discrepancies due to time zone differences.packages/db/src/schema/event.ts (2)
5-10: Consider adding timestamp columns to theeventtableIncluding
createdAtandupdatedAttimestamp columns can help track when events are created and modified, which is useful for auditing and debugging purposes.Apply the following changes to include timestamp columns:
export const event = pgTable("event", { id: uuid("id").primaryKey().defaultRandom(), event: text("event").notNull(), action: text("action").notNull(), payload: jsonb("payload").notNull(), + createdAt: timestamp("created_at").defaultNow().notNull(), + updatedAt: timestamp("updated_at").defaultNow().notNull(), });
32-37: Consider adding timestamp columns to thewebhooktableAdding
createdAtandupdatedAttimestamps to thewebhooktable can be useful for tracking when webhooks are created and modified.export const webhook = pgTable("webhook", { id: uuid("id").primaryKey().defaultRandom(), hookId: uuid("hook_id") .notNull() .references(() => hook.id, { onDelete: "cascade" }), url: text("url").notNull(), + createdAt: timestamp("created_at").defaultNow().notNull(), + updatedAt: timestamp("updated_at").defaultNow().notNull(), });packages/events/src/triggers/environment-deleted.ts (1)
35-40: Consider adding comments to clarify theremovedFromSystemFilterlogic.The construction of
removedFromSystemFilterinvolves complex logic with nested conditions. Adding inline comments to explain the purpose and reasoning behind this filter would improve readability and aid future maintenance.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
apps/jobs/package.json(1 hunks)apps/jobs/src/expired-env-checker/index.ts(1 hunks)packages/db/src/schema/event.ts(1 hunks)packages/db/src/schema/index.ts(1 hunks)packages/events/eslint.config.js(1 hunks)packages/events/package.json(1 hunks)packages/events/src/handlers/index.ts(1 hunks)packages/events/src/handlers/target-removed.ts(1 hunks)packages/events/src/index.ts(1 hunks)packages/events/src/triggers/environment-deleted.ts(1 hunks)packages/events/src/triggers/index.ts(1 hunks)packages/events/tsconfig.json(1 hunks)packages/validators/src/events/hooks/index.ts(1 hunks)packages/validators/src/events/hooks/target.ts(1 hunks)packages/validators/src/events/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- packages/events/eslint.config.js
- packages/events/package.json
- packages/events/src/handlers/index.ts
- packages/events/src/triggers/index.ts
- packages/events/tsconfig.json
- packages/validators/src/events/index.ts
🔇 Additional comments (16)
packages/validators/src/events/hooks/index.ts (2)
1-6: LGTM! Clean import structure and API design.
The imports are well-organized and the re-export pattern provides a clean public API.
1-15: Verify consistent event handling across the system.
Since this is part of a new event handling system, let's verify the consistency of event types and handling across the codebase.
✅ Verification successful
Event handling implementation is consistent across the system
The verification shows that the event system is well-structured and consistent:
- The event types are properly defined in
@ctrlplane/validators/eventsand correctly imported where needed - The event handling follows a consistent pattern:
- Events are defined using Zod schemas
- Type guards are properly implemented
- Event handlers are properly typed and imported
- The
actionliterals ("removed" and "deleted") are only used in the type guards, maintaining consistency - The event system is properly integrated with
@ctrlplane/eventspackage, with appropriate handlers and imports
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent event handling patterns
# Check for other event definitions and handlers
echo "Checking for event definitions and handlers..."
rg -t typescript "type.*Event.*=" --glob "!{node_modules,dist}/**"
# Check for uses of these specific events
echo "Checking for uses of target events..."
rg -t typescript "(TargetRemoved|TargetDeleted)" --glob "!{node_modules,dist}/**"
# Check for other action string literals that might need to be standardized
echo "Checking for action literals..."
rg -t typescript "action.*=.*['\"]" --glob "!{node_modules,dist}/**"
Length of output: 591
Script:
#!/bin/bash
# Description: Check for consistent event handling patterns with corrected commands
# Check for other event definitions and handlers
echo "Checking for event definitions and handlers..."
rg "type.*Event.*="
# Check for uses of these specific events
echo "Checking for uses of target events..."
rg "(TargetRemoved|TargetDeleted)"
# Check for other action string literals that might need to be standardized
echo "Checking for action literals..."
rg "action.*=.*['\"]"
# Check for event-related imports and exports
echo "Checking for event-related imports and exports..."
rg "import.*Event|export.*Event"
Length of output: 6432
packages/events/src/index.ts (2)
13-13: Consider adding validation before database insertion.
The event is directly inserted into the database without any validation of its structure or content.
#!/bin/bash
# Check if there are any validation utilities in the codebase
rg -A 5 "validate.*Event" 9-10: Review the use of wildcard exports.
Using wildcard exports (export *) can lead to naming conflicts and makes it harder to track which symbols are actually being used. Consider explicitly exporting only the necessary entities.
✅ Verification successful
Let me gather more information about the exports to make a proper assessment.
Wildcard exports are acceptable in this context
The wildcard exports are being used in a controlled manner here:
triggers/index.tsonly re-exports fromenvironment-deleted.jswhich exports a single functionhandlers/index.tsonly re-exports fromtarget-removed.jswhich exports a single function
Since each module exports just one clearly named function and the re-exports are used only in index files for organizing the public API, there's no risk of naming conflicts or unclear exports.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential naming conflicts in exports
rg -l "export \* from" ./packages/events/src/triggers
rg -l "export \* from" ./packages/events/src/handlers
Length of output: 230
Script:
#!/bin/bash
# Check the content of the index files and their exports
echo "=== Content of triggers/index.ts ==="
cat ./packages/events/src/triggers/index.ts
echo -e "\n=== Content of handlers/index.ts ==="
cat ./packages/events/src/handlers/index.ts
# List all export statements in the triggers directory
echo -e "\n=== Exports in triggers directory ==="
rg "export" ./packages/events/src/triggers/
# List all export statements in the handlers directory
echo -e "\n=== Exports in handlers directory ==="
rg "export" ./packages/events/src/handlers/
Length of output: 997
packages/validators/src/events/hooks/target.ts (2)
10-15: LGTM! Well-structured event schema.
The schema properly validates the event structure with literal types and includes necessary context (deployment and target information).
17-22: Verify the payload structure difference between removed and deleted events.
The targetDeleted event payload only includes target information, while targetRemoved includes both target and deployment context. Please confirm if this difference is intentional and aligns with the event handling requirements.
Let's check how these events are used in the codebase:
✅ Verification successful
Let me gather more context about these events by examining their definitions and usage patterns.
The difference in payload structure between removed and deleted events is intentional
The codebase shows a clear distinction between these events:
targetRemovedincludes both deployment and target info as it represents removing a target from a specific deploymenttargetDeletedonly includes target info as it represents the complete deletion of a target, independent of any deployment
This is further validated by:
- Both events are properly typed and exported
- They are combined in a union type
hookEvent - Separate typeguards exist for each event type
- The schema definitions are consistent with their semantic meaning
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usage of both event types to understand the context
echo "Searching for target removed event usage:"
rg -l "targetRemoved"
echo -e "\nSearching for target deleted event usage:"
rg -l "targetDeleted"
echo -e "\nSearching for event handlers:"
ast-grep --pattern 'function handle$_(event: $_) {
$$$
}'
Length of output: 558
Script:
#!/bin/bash
# Get the full content of the target events file
echo "Content of target.ts:"
cat packages/validators/src/events/hooks/target.ts
echo -e "\nSearching for actual usage of these events:"
rg "targetRemoved|targetDeleted" -A 5 -B 5
echo -e "\nChecking index.ts for event exports:"
cat packages/validators/src/events/hooks/index.ts
Length of output: 3892
packages/db/src/schema/index.ts (1)
22-22: LGTM!
The new export statement for the event module follows the established pattern and is consistent with other exports in the file.
apps/jobs/package.json (1)
17-17: LGTM! Verify usage of the new dependency.
The addition of @ctrlplane/events as a workspace dependency is correctly implemented and aligns with the transition to an event-driven architecture.
Let's verify the dependency usage:
✅ Verification successful
Dependency is properly imported and utilized ✅
The @ctrlplane/events package is correctly imported and effectively used in the expired-env-checker module. The code demonstrates proper usage of both imported functions:
getEventsForEnvironmentDeletedis used to generate events for expired environmentshandleEventis used to process these events asynchronously
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that @ctrlplane/events is being imported and used in the codebase
# Test: Search for imports of @ctrlplane/events
rg "from ['|\"]@ctrlplane/events" apps/jobs/src
Length of output: 175
Script:
#!/bin/bash
# Let's verify the actual usage of the imported functions
rg -A 5 "getEventsForEnvironmentDeleted|handleEvent" apps/jobs/src/expired-env-checker/index.ts
Length of output: 653
packages/events/src/handlers/target-removed.ts (2)
1-7: LGTM! Well-organized imports.
The imports are clean, necessary, and logically grouped with types separated from utilities.
1-29: Verify event handler registration and type definitions.
Let's ensure this handler is properly integrated into the event handling system.
✅ Verification successful
Based on the results, I can generate the final response as we have sufficient information about the event handler registration, type definitions, and potential duplicates.
Event handler is properly integrated and registered
The verification shows:
- Handler is correctly registered in the main event handler (
packages/events/src/index.ts) - Type definitions for
TargetRemovedexist and are properly exported - No duplicate handlers found for
target.removedevents - Clean integration with the event handling system through proper imports and type guards
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify event handler registration and type definitions
# Check if the handler is registered in the main event handler
echo "Checking event handler registration..."
rg -A 5 "handleTargetRemoved" "packages/events/src/index.ts"
# Verify TargetRemoved type definition
echo "Checking TargetRemoved type definition..."
rg -A 5 "TargetRemoved" "packages/validators/src/events"
# Check for other target removal event handlers to avoid duplicates
echo "Checking for duplicate handlers..."
rg -g "!target-removed.ts" "target.removed" "packages/events/src"
Length of output: 2313
apps/jobs/src/expired-env-checker/index.ts (3)
1-1: Imports from '@ctrlplane/db' are appropriate
The functions inArray and lte are correctly imported from @ctrlplane/db.
4-4: Event functions imported correctly from '@ctrlplane/events'
The imports for getEventsForEnvironmentDeleted and handleEvent are appropriately added from @ctrlplane/events.
17-18: Properly awaiting handling of events
The events are correctly processed by mapping over handleEvent and awaiting all the resulting promises.
packages/db/src/schema/event.ts (2)
23-28: Verify onDelete: "cascade" behavior for foreign keys in runhook
Ensure that cascading deletes are intended when a hook or runbook is deleted, as this will delete related runhook records. This behavior can lead to loss of important linkage data if not carefully managed.
33-35: Verify onDelete: "cascade" behavior for hookId foreign key in webhook
Confirm that deleting a hook should result in the deletion of associated webhook records. If you intend to preserve webhook entries independently of hook, consider changing the delete behavior.
packages/events/src/triggers/environment-deleted.ts (1)
11-66: Function getEventsForEnvironmentDeleted is well-implemented.
The function correctly handles the generation of events when an environment is deleted, ensuring that targets removed from the system are appropriately identified and processed. Error handling for null checks and empty results is thorough, and the use of database queries is efficient and effective.
| export const handleEvent = async (event: HookEvent) => { | ||
| await db.insert(SCHEMA.event).values(event); | ||
| if (isTargetRemoved(event)) return handleTargetRemoved(event); | ||
| throw new Error(`Unhandled event: ${event.event}`); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and improve observability.
The event handling implementation could benefit from:
- Error handling for database operations
- Transaction management
- Logging for event handling
- More descriptive error messages
Consider this implementation:
-export const handleEvent = async (event: HookEvent) => {
+export const handleEvent = async (event: HookEvent): Promise<void> => {
+ const trx = await db.transaction();
try {
- await db.insert(SCHEMA.event).values(event);
- if (isTargetRemoved(event)) return handleTargetRemoved(event);
- throw new Error(`Unhandled event: ${event.event}`);
+ await trx.insert(SCHEMA.event).values(event);
+
+ if (isTargetRemoved(event)) {
+ await handleTargetRemoved(event);
+ await trx.commit();
+ return;
+ }
+
+ await trx.rollback();
+ throw new Error(`Unhandled event type: ${event.event}. Expected types: 'target_removed'`);
+ } catch (error) {
+ await trx.rollback();
+ throw new Error(`Failed to handle event: ${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.
| export const handleEvent = async (event: HookEvent) => { | |
| await db.insert(SCHEMA.event).values(event); | |
| if (isTargetRemoved(event)) return handleTargetRemoved(event); | |
| throw new Error(`Unhandled event: ${event.event}`); | |
| }; | |
| export const handleEvent = async (event: HookEvent): Promise<void> => { | |
| const trx = await db.transaction(); | |
| try { | |
| await trx.insert(SCHEMA.event).values(event); | |
| if (isTargetRemoved(event)) { | |
| await handleTargetRemoved(event); | |
| await trx.commit(); | |
| return; | |
| } | |
| await trx.rollback(); | |
| throw new Error(`Unhandled event type: ${event.event}. Expected types: 'target_removed'`); | |
| } catch (error) { | |
| await trx.rollback(); | |
| throw new Error(`Failed to handle event: ${error.message}`); | |
| } | |
| }; |
| export * from "./invite.js"; | ||
| export * from "./target-group.js"; | ||
| export * from "./runbook-variables.js"; | ||
| export * from "./rbac.js"; |
There was a problem hiding this comment.
Remove duplicate export of runbook.js
The "./runbook.js" module is exported twice in this file. Remove one of the duplicate exports to maintain clean and efficient code.
export * from "./runbook-variables.js";
export * from "./rbac.js";
-export * from "./runbook.js";
export * from "./job-agent.js";Also applies to: 20-20
| const isSubscribedToTargetRemoved = and( | ||
| eq(SCHEMA.hook.scopeId, deployment.id), | ||
| eq(SCHEMA.hook.scopeType, "deployment"), | ||
| eq(SCHEMA.hook.event, "target.removed"), | ||
| ); | ||
| const runhooks = await db | ||
| .select() | ||
| .from(SCHEMA.runhook) | ||
| .innerJoin(SCHEMA.hook, eq(SCHEMA.runhook.hookId, SCHEMA.hook.id)) | ||
| .where(isSubscribedToTargetRemoved); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for database operations.
The query is well-constructed, but consider adding try-catch blocks to handle potential database errors gracefully.
const isSubscribedToTargetRemoved = and(
eq(SCHEMA.hook.scopeId, deployment.id),
eq(SCHEMA.hook.scopeType, "deployment"),
eq(SCHEMA.hook.event, "target.removed"),
);
- const runhooks = await db
- .select()
- .from(SCHEMA.runhook)
- .innerJoin(SCHEMA.hook, eq(SCHEMA.runhook.hookId, SCHEMA.hook.id))
- .where(isSubscribedToTargetRemoved);
+ let runhooks;
+ try {
+ runhooks = await db
+ .select()
+ .from(SCHEMA.runhook)
+ .innerJoin(SCHEMA.hook, eq(SCHEMA.runhook.hookId, SCHEMA.hook.id))
+ .where(isSubscribedToTargetRemoved);
+ } catch (error) {
+ throw new Error(`Failed to fetch runhooks: ${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.
| const isSubscribedToTargetRemoved = and( | |
| eq(SCHEMA.hook.scopeId, deployment.id), | |
| eq(SCHEMA.hook.scopeType, "deployment"), | |
| eq(SCHEMA.hook.event, "target.removed"), | |
| ); | |
| const runhooks = await db | |
| .select() | |
| .from(SCHEMA.runhook) | |
| .innerJoin(SCHEMA.hook, eq(SCHEMA.runhook.hookId, SCHEMA.hook.id)) | |
| .where(isSubscribedToTargetRemoved); | |
| const isSubscribedToTargetRemoved = and( | |
| eq(SCHEMA.hook.scopeId, deployment.id), | |
| eq(SCHEMA.hook.scopeType, "deployment"), | |
| eq(SCHEMA.hook.event, "target.removed"), | |
| ); | |
| let runhooks; | |
| try { | |
| runhooks = await db | |
| .select() | |
| .from(SCHEMA.runhook) | |
| .innerJoin(SCHEMA.hook, eq(SCHEMA.runhook.hookId, SCHEMA.hook.id)) | |
| .where(isSubscribedToTargetRemoved); | |
| } catch (error) { | |
| throw new Error(`Failed to fetch runhooks: ${error.message}`); | |
| } |
| const targetId = target.id; | ||
| const deploymentId = deployment.id; | ||
| const handleRunhooksPromises = runhooks.map(({ runhook }) => | ||
| dispatchRunbook(db, runhook.runbookId, { targetId, deploymentId }), | ||
| ); | ||
|
|
||
| await Promise.all(handleRunhooksPromises); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance runbook dispatch error handling and logging.
While parallel execution is efficient, consider:
- Handling individual runbook dispatch failures
- Adding logging for monitoring and debugging
- Implementing retry logic for failed dispatches
const targetId = target.id;
const deploymentId = deployment.id;
- const handleRunhooksPromises = runhooks.map(({ runhook }) =>
- dispatchRunbook(db, runhook.runbookId, { targetId, deploymentId }),
- );
+ const handleRunhooksPromises = runhooks.map(async ({ runhook }) => {
+ try {
+ await dispatchRunbook(db, runhook.runbookId, { targetId, deploymentId });
+ console.log(`Successfully dispatched runbook ${runhook.runbookId}`);
+ } catch (error) {
+ console.error(`Failed to dispatch runbook ${runhook.runbookId}:`, error);
+ throw error;
+ }
+ });
- await Promise.all(handleRunhooksPromises);
+ const results = await Promise.allSettled(handleRunhooksPromises);
+ const failures = results.filter((r): r is PromiseRejectedResult => r.status === 'rejected');
+ if (failures.length > 0) {
+ throw new Error(`Failed to dispatch ${failures.length} runbooks`);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const targetId = target.id; | |
| const deploymentId = deployment.id; | |
| const handleRunhooksPromises = runhooks.map(({ runhook }) => | |
| dispatchRunbook(db, runhook.runbookId, { targetId, deploymentId }), | |
| ); | |
| await Promise.all(handleRunhooksPromises); | |
| }; | |
| const targetId = target.id; | |
| const deploymentId = deployment.id; | |
| const handleRunhooksPromises = runhooks.map(async ({ runhook }) => { | |
| try { | |
| await dispatchRunbook(db, runhook.runbookId, { targetId, deploymentId }); | |
| console.log(`Successfully dispatched runbook ${runhook.runbookId}`); | |
| } catch (error) { | |
| console.error(`Failed to dispatch runbook ${runhook.runbookId}:`, error); | |
| throw error; | |
| } | |
| }); | |
| const results = await Promise.allSettled(handleRunhooksPromises); | |
| const failures = results.filter((r): r is PromiseRejectedResult => r.status === 'rejected'); | |
| if (failures.length > 0) { | |
| throw new Error(`Failed to dispatch ${failures.length} runbooks`); | |
| } |
| const eventPromises = expiredEnvironments.flatMap( | ||
| getEventsForEnvironmentDeleted, | ||
| ); | ||
| const events = (await Promise.all(eventPromises)).flat(); |
There was a problem hiding this comment.
Async handling issue with flatMap and promises
Using flatMap with async functions may not yield the expected results because flatMap does not await promises returned by the mapping function. This can lead to an array of unresolved promises.
Apply this diff to fix the issue:
- const eventPromises = expiredEnvironments.flatMap(
- getEventsForEnvironmentDeleted,
- );
- const events = (await Promise.all(eventPromises)).flat();
+ const eventPromises = await Promise.all(
+ expiredEnvironments.map(getEventsForEnvironmentDeleted)
+ );
+ const events = eventPromises.flat();This change ensures that all promises are resolved before flattening the array of events.
📝 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 eventPromises = expiredEnvironments.flatMap( | |
| getEventsForEnvironmentDeleted, | |
| ); | |
| const events = (await Promise.all(eventPromises)).flat(); | |
| const eventPromises = await Promise.all( | |
| expiredEnvironments.map(getEventsForEnvironmentDeleted) | |
| ); | |
| const events = eventPromises.flat(); |
packages/db/src/schema/event.ts
Outdated
| event: text("event").notNull(), | ||
| action: text("action").notNull(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define event and action as enums in the hook table
Using enums for the event and action fields can enforce valid values and enhance data integrity by preventing invalid event types or actions.
For example:
+import { pgEnum } from "drizzle-orm/pg-core";
+
+const eventEnum = pgEnum("event_enum", ["eventType1", "eventType2"]);
+const actionEnum = pgEnum("action_enum", ["actionType1", "actionType2"]);
+
export const hook = pgTable("hook", {
id: uuid("id").primaryKey().defaultRandom(),
- event: text("event").notNull(),
- action: text("action").notNull(),
+ event: eventEnum("event").notNull(),
+ action: actionEnum("action").notNull(),
name: text("name").notNull(),
scopeType: text("scope_type").notNull(),
scopeId: uuid("scope_id").notNull(),
});Replace ["eventType1", "eventType2"] and ["actionType1", "actionType2"] with the actual event and action types used in your application.
Committable suggestion skipped: line range outside the PR's diff.
| event: text("event").notNull(), | ||
| action: text("action").notNull(), | ||
| name: text("name").notNull(), | ||
| scopeType: text("scope_type").notNull(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define scopeType as an enum to enforce valid scope types
Using an enum for scopeType can prevent invalid values and improve data validation.
For example:
+const scopeTypeEnum = pgEnum("scope_type_enum", ["project", "environment", "user"]);
+
export const hook = pgTable("hook", {
id: uuid("id").primaryKey().defaultRandom(),
event: text("event").notNull(),
action: text("action").notNull(),
name: text("name").notNull(),
- scopeType: text("scope_type").notNull(),
+ scopeType: scopeTypeEnum("scope_type").notNull(),
scopeId: uuid("scope_id").notNull(),
});Replace ["project", "environment", "user"] with the actual scope types used in your application.
Committable suggestion skipped: line range outside the PR's diff.
| const isSubscribedToTargetRemoved = and( | ||
| eq(SCHEMA.hook.scopeId, deployment.id), | ||
| eq(SCHEMA.hook.scopeType, "deployment"), | ||
| eq(SCHEMA.hook.event, "target.removed"), |
There was a problem hiding this comment.
| eq(SCHEMA.hook.event, "target.removed"), | |
| eq(SCHEMA.hook.event, "target"), | |
| eq(SCHEMA.hook.action, "removed"), |
is what I had in mind
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
apps/jobs/src/expired-env-checker/index.ts (1)
Line range hint
22-25: Wrap operations in a transaction.The event handling and environment deletion should be atomic operations. If the deletion fails, we might end up with inconsistent state where events were processed but environments weren't deleted.
export const run = async () => { + const tx = await db.transaction(); + try { const expiredEnvironments = await db .select() .from(SCHEMA.environment) .where(lte(SCHEMA.environment.expiresAt, new Date())); if (expiredEnvironments.length === 0) return; const eventPromises = expiredEnvironments.flatMap( getEventsForEnvironmentDeleted, ); const events = (await Promise.allSettled(eventPromises)) .filter((result) => result.status === "fulfilled") .flatMap((result) => result.value); const handleEventsPromises = events.map(handleEvent); await Promise.allSettled(handleEventsPromises); const envIds = expiredEnvironments.map((env) => env.id); await db .delete(SCHEMA.environment) .where(inArray(SCHEMA.environment.id, envIds)); + await tx.commit(); + } catch (error) { + await tx.rollback(); + throw error; + } };packages/db/drizzle/0034_moaning_silk_fever.sql (2)
1-8: Consider adding indexes for better query performance.The
eventtable would benefit from indexes on frequently queried columns:
action: For filtering events by typecreated_at: For temporal queries and cleanup operationsCREATE INDEX idx_event_action ON event(action); CREATE INDEX idx_event_created_at ON event(created_at);
18-22: Optimize junction table structure.The
runhooktable could be optimized:
- Consider using a composite primary key (hook_id, runbook_id) instead of a UUID
- Add a unique constraint to prevent duplicate hook-runbook associations
CREATE TABLE IF NOT EXISTS "runhook" ( "hook_id" uuid NOT NULL, "runbook_id" uuid NOT NULL, PRIMARY KEY ("hook_id", "runbook_id") );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
apps/jobs/src/expired-env-checker/index.ts(1 hunks)packages/api/package.json(1 hunks)packages/api/src/router/environment.ts(2 hunks)packages/db/drizzle/0034_moaning_silk_fever.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/event.ts(1 hunks)packages/events/eslint.config.js(1 hunks)packages/events/package.json(1 hunks)packages/events/src/handlers/target-removed.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/db/drizzle/meta/_journal.json
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/db/src/schema/event.ts
- packages/events/eslint.config.js
- packages/events/package.json
- packages/events/src/handlers/target-removed.ts
🔇 Additional comments (6)
apps/jobs/src/expired-env-checker/index.ts (3)
1-4: LGTM!
The imports are well-organized and all dependencies are properly utilized.
13-15: Previous async handling issue with flatMap is still present.
16-20: LGTM! Robust error handling with Promise.allSettled.
The use of Promise.allSettled ensures that the failure of one event doesn't prevent others from being processed, and the filtering of fulfilled results is handled correctly.
packages/db/drizzle/0034_moaning_silk_fever.sql (1)
30-46: Review CASCADE DELETE behavior.
The current foreign key constraints use CASCADE DELETE, which means:
- Deleting a hook will automatically delete all associated runhooks and webhooks
- Deleting a runbook will delete associated runhooks
Please verify if this cascade behavior aligns with the business requirements, as it could lead to unintended data loss.
✅ Verification successful
The search results show that CASCADE DELETE is consistently used across the database schema for maintaining referential integrity, particularly in parent-child relationships. In the context of hooks and runbooks:
Cascade deletion is appropriate for this schema design
The CASCADE DELETE behavior in the new constraints aligns with the existing schema design patterns where:
- Child records (runhooks, webhooks) should not exist without their parent (hook)
- Associated records (runhooks) should not exist without their parent (runbook)
This is consistent with how other similar relationships are handled in the schema:
- Workspace deletions cascade to its dependent entities
- System deletions cascade to deployments, environments, and variable sets
- Release deletions cascade to their metadata and dependencies
The cascade behavior ensures referential integrity and prevents orphaned records, which is the correct design choice for these tightly coupled entities.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any existing delete triggers or constraints that might conflict
rg -A 5 "ON DELETE" packages/db/drizzle/
Length of output: 52498
packages/api/package.json (1)
24-24: LGTM! Verify the new dependency usage.
The addition of @ctrlplane/events as a workspace dependency is correctly configured and aligns with the event handling enhancements.
Let's verify the dependency usage:
✅ Verification successful
Dependency usage verified and properly integrated
The @ctrlplane/events package is correctly imported and utilized in packages/api/src/router/environment.ts, specifically for environment deletion events. The implementation shows both the import and actual usage of getEventsForEnvironmentDeleted function, which aligns with the intended purpose.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of @ctrlplane/events in the api package
# Expected: Find import statements and usage of the events package
# Search for imports of @ctrlplane/events
echo "Checking imports of @ctrlplane/events:"
rg "from ['\"@]ctrlplane/events" packages/api/
# Search for specific event-related function usage mentioned in the summary
echo -e "\nChecking usage of getEventsForEnvironmentDeleted:"
rg "getEventsForEnvironmentDeleted" packages/api/
Length of output: 529
packages/api/src/router/environment.ts (1)
26-26: LGTM: Import statement is correctly placed and utilized.
The new import for event handling functions is properly integrated and used within the delete mutation.
| .select() | ||
| .from(SCHEMA.environment) | ||
| .innerJoin( | ||
| SCHEMA.deployment, | ||
| eq(SCHEMA.deployment.systemId, SCHEMA.environment.systemId), | ||
| ) | ||
| .where(lte(SCHEMA.environment.expiresAt, new Date())) | ||
| .then(groupByEnvironment); | ||
| .where(lte(SCHEMA.environment.expiresAt, new Date())); | ||
| if (expiredEnvironments.length === 0) return; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for database operations.
The database query could fail due to various reasons (connection issues, permissions, etc.). Consider wrapping it in a try-catch block to handle potential errors gracefully.
export const run = async () => {
+ try {
const expiredEnvironments = await db
.select()
.from(SCHEMA.environment)
.where(lte(SCHEMA.environment.expiresAt, new Date()));
if (expiredEnvironments.length === 0) return;
+ } catch (error) {
+ console.error('Failed to fetch expired environments:', error);
+ throw error;
+ }Committable suggestion skipped: line range outside the PR's diff.
| CREATE TABLE IF NOT EXISTS "hook" ( | ||
| "id" uuid PRIMARY KEY DEFAULT gen_random_uuid () NOT NULL, | ||
| "action" text NOT NULL, | ||
| "name" text NOT NULL, | ||
| "scope_type" text NOT NULL, | ||
| "scope_id" uuid NOT NULL | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance data integrity constraints for the hook table.
Consider the following improvements:
- Add a unique constraint for name within a scope to prevent duplicates
- Add an index on scope_id for faster lookups
- Consider using an enum for scope_type to restrict valid values
-- Add after table creation
ALTER TABLE "hook" ADD CONSTRAINT "unique_hook_name_per_scope"
UNIQUE ("name", "scope_type", "scope_id");
CREATE INDEX idx_hook_scope ON hook(scope_type, scope_id);
-- Consider creating an enum type
CREATE TYPE scope_type_enum AS ENUM ('organization', 'project', 'environment');
ALTER TABLE "hook"
ALTER COLUMN "scope_type" TYPE scope_type_enum USING scope_type::scope_type_enum;| CREATE TABLE IF NOT EXISTS "webhook" ( | ||
| "id" uuid PRIMARY KEY DEFAULT gen_random_uuid () NOT NULL, | ||
| "hook_id" uuid NOT NULL, | ||
| "url" text NOT NULL | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation and uniqueness constraints for webhooks.
Consider adding:
- A unique constraint to prevent duplicate URLs for the same hook
- URL format validation using a CHECK constraint
ALTER TABLE "webhook"
ADD CONSTRAINT "unique_webhook_url_per_hook" UNIQUE ("hook_id", "url");
ALTER TABLE "webhook"
ADD CONSTRAINT "valid_url"
CHECK (url ~* '^https?://[^\s/$.?#].[^\s]*$');| ctx.db | ||
| .delete(environment) | ||
| .where(eq(environment.id, input)) | ||
| .returning() | ||
| .then(takeFirst) | ||
| .then(async (env) => { | ||
| const events = await getEventsForEnvironmentDeleted(env); | ||
| const handleEventPromises = events.map(handleEvent); | ||
| await Promise.allSettled(handleEventPromises); | ||
| return env; | ||
| }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving error handling and transaction safety.
The current implementation has several areas for improvement:
- Error handling is not explicit for failed event processing. While
Promise.allSettledensures all promises complete, it silently handles failures. - Removing the transaction wrapper could lead to inconsistent state if event handling fails after the deletion succeeds.
- The sequential nature of operations could impact performance.
Consider refactoring to:
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(async (env) => {
- const events = await getEventsForEnvironmentDeleted(env);
- const handleEventPromises = events.map(handleEvent);
- await Promise.allSettled(handleEventPromises);
- return env;
- }),
+ ctx.db.transaction(async (tx) => {
+ const env = await tx
+ .delete(environment)
+ .where(eq(environment.id, input))
+ .returning()
+ .then(takeFirst);
+
+ const events = await getEventsForEnvironmentDeleted(env);
+ const results = await Promise.allSettled(events.map(handleEvent));
+
+ // Check for any failed event handling
+ const failures = results
+ .filter((r): r is PromiseRejectedResult => r.status === 'rejected')
+ .map(r => r.reason);
+
+ if (failures.length > 0) {
+ console.error('Failed to process some events:', failures);
+ throw new Error('Failed to process all events after environment deletion');
+ }
+
+ return env;
+ }),This refactoring:
- Restores transaction safety
- Adds explicit error handling
- Logs failed events for debugging
- Maintains atomicity of the operation
📝 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(async (env) => { | |
| const events = await getEventsForEnvironmentDeleted(env); | |
| const handleEventPromises = events.map(handleEvent); | |
| await Promise.allSettled(handleEventPromises); | |
| return env; | |
| }), | |
| ctx.db.transaction(async (tx) => { | |
| const env = await tx | |
| .delete(environment) | |
| .where(eq(environment.id, input)) | |
| .returning() | |
| .then(takeFirst); | |
| const events = await getEventsForEnvironmentDeleted(env); | |
| const results = await Promise.allSettled(events.map(handleEvent)); | |
| // Check for any failed event handling | |
| const failures = results | |
| .filter((r): r is PromiseRejectedResult => r.status === 'rejected') | |
| .map(r => r.reason); | |
| if (failures.length > 0) { | |
| console.error('Failed to process some events:', failures); | |
| throw new Error('Failed to process all events after environment deletion'); | |
| } | |
| return env; | |
| }), |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (4)
packages/events/src/triggers/index.ts (2)
1-3: LGTM! Consider adding JSDoc documentation.The barrel file structure is clean and follows a consistent pattern. However, it would be helpful to add JSDoc documentation describing the available exports from each module.
Add documentation like this:
+/** + * @module triggers + * @description Exports event trigger handlers for environment, deployment, and target deletion events + */ + export * from "./environment-deleted.js"; export * from "./deployment-deleted.js"; export * from "./target-deleted.js";
1-3: Consider using explicit exports for better maintainability.While wildcard exports work, explicit exports can improve maintainability and enable better tree-shaking. This also makes it clearer what's being exported from each module.
Consider refactoring to:
-export * from "./environment-deleted.js"; -export * from "./deployment-deleted.js"; -export * from "./target-deleted.js"; +export { + getEventsForEnvironmentDeleted, + type EnvironmentDeletedEvent +} from "./environment-deleted.js"; +export { + getEventsForDeploymentDeleted, + type DeploymentDeletedEvent +} from "./deployment-deleted.js"; +export { + getEventsForTargetDeleted, + type TargetDeletedEvent +} from "./target-deleted.js";packages/events/src/triggers/target-deleted.ts (1)
20-26: Handle cases where no systems are foundIf no systems are associated with the target's workspace, the function will proceed without any systems to process. Consider adding a check to handle this scenario explicitly, which could improve readability and maintainability.
const systems = await db.query.system.findMany({ where: eq(SCHEMA.system.workspaceId, target.workspaceId), with: { environments: { where: isNotNull(SCHEMA.environment.targetFilter) }, deployments: true, }, }); + if (systems.length === 0) { + return []; + }packages/api/src/router/target.ts (1)
479-479: Remove unnecessary blank lineThere's an extra blank line at line 480 which could be removed to improve code readability.
await Promise.all( events.map(async (event) => { // Error handling as discussed previously await handleEvent(event); }) ); - return ctx.db .delete(schema.target) .where(inArray(schema.target.id, input)) .returning();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
apps/jobs/Dockerfile(1 hunks)apps/webservice/Dockerfile(1 hunks)packages/api/src/router/deployment.ts(2 hunks)packages/api/src/router/target.ts(2 hunks)packages/db/src/schema/index.ts(1 hunks)packages/events/src/triggers/deployment-deleted.ts(1 hunks)packages/events/src/triggers/index.ts(1 hunks)packages/events/src/triggers/target-deleted.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/db/src/schema/index.ts
🔇 Additional comments (12)
apps/jobs/Dockerfile (2)
30-30: LGTM! Package copy follows established pattern.
The addition of the events package copy command is correctly positioned with other package.json copies and follows the same pattern used for other internal packages.
30-30: Verify events package structure.
Let's ensure the events package is properly set up in the repository.
✅ Verification successful
Events package structure is properly set up and integrated
The verification confirms that:
- The events package exists with proper structure including source files, configuration, and TypeScript setup
- It's correctly referenced as a workspace dependency in the jobs package
- Contains necessary event handlers and triggers in the src directory
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the events package structure and dependencies
# Test 1: Check if events package.json exists
if [ ! -f "packages/events/package.json" ]; then
echo "Error: packages/events/package.json not found"
exit 1
fi
# Test 2: Check if events package is listed as a dependency in jobs package.json
cat apps/jobs/package.json | jq -r '.dependencies["@ctrlplane/events"]'
# Test 3: Check for other required events package files
fd -t f . packages/events -E "node_modules" -E ".turbo" -E "dist"
Length of output: 587
packages/events/src/triggers/deployment-deleted.ts (3)
1-9: LGTM! Imports are well-organized and properly typed.
The imports are appropriately scoped and include all necessary dependencies for the functionality.
22-25: LGTM! Environment filter processing is clean and type-safe.
The code effectively handles the extraction and validation of environment filters with proper type narrowing.
27-31: LGTM! System filter construction is well-structured.
The TargetCondition construction is correct and properly typed. However, let's verify that all possible environment filter types are handled correctly by the target matching logic.
✅ Verification successful
Based on the shell script results, I can now generate the final response since I have comprehensive information about the target matching implementation and filter type handling.
System filter construction is correctly implemented and all filter types are properly handled
The target matching implementation in packages/db/src/schema/target.ts correctly handles all possible filter types:
- Metadata conditions via
buildMetadataCondition - Kind conditions via direct equality check
- Name conditions via pattern matching with
like - Provider conditions via direct equality check
- Identifier conditions via
buildIdentifierCondition - Comparison conditions with proper AND/OR operator support
The system filter construction in the reviewed code follows this well-established pattern, using the comparison type with OR operator to combine environment filters.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the target matching implementation handles all filter types
# Search for target matching implementation and filter type handling
# Check target matching implementation
ast-grep --pattern 'targetMatchesMetadata($_, $_)'
# Check filter type handling
rg -A 5 'TargetFilterType\.'
Length of output: 25665
apps/webservice/Dockerfile (2)
39-39: LGTM! The events package.json is correctly added.
The addition follows the established pattern of copying package.json files before dependency installation.
39-39: Verify the events package setup is complete.
Since this is initializing a new events package, let's verify that all necessary files are present in the repository.
✅ Verification successful
The verification shows that the events package has a proper setup with essential files and configuration. Let's verify the content of the source files to ensure they are properly implemented:
Let's verify the implementation of the referenced handlers and triggers to ensure completeness:
Events package setup is complete and properly implemented
The events package has a complete and well-structured setup with:
- Proper package.json configuration with all necessary dependencies
- TypeScript configuration and build scripts
- Complete implementation of event handling system including:
- Main event handler with proper error handling
- Target removal handler with database operations
- Three trigger implementations for environment, deployment, and target deletion events
- Proper type definitions and imports from other workspace packages
The implementation shows a mature event system with proper error handling, database operations, and comprehensive trigger logic for various scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the events package structure and configuration
# Test 1: Check if the events package has the essential files
echo "Checking essential files for the events package..."
fd --type f --base-directory packages/events "^(tsconfig\.json|index\.ts|README\.md)$"
# Test 2: Verify package.json content
echo "Verifying package.json configuration..."
cat packages/events/package.json
# Test 3: Check for any TypeScript type definitions
echo "Checking for type definitions..."
fd --type f --base-directory packages/events "\.d\.ts$"
Length of output: 1539
Script:
#!/bin/bash
# Check the content of main source files
echo "Checking index.ts content..."
cat packages/events/src/index.ts
echo -e "\nChecking handlers/index.ts content..."
cat packages/events/src/handlers/index.ts
echo -e "\nChecking triggers/index.ts content..."
cat packages/events/src/triggers/index.ts
# Verify if there are any test files
echo -e "\nChecking for test files..."
fd --type f --base-directory packages/events "\.test\.(ts|js)$"
Length of output: 1254
Script:
#!/bin/bash
# Check handler implementation
echo "Checking target-removed handler..."
cat packages/events/src/handlers/target-removed.ts
echo -e "\nChecking trigger implementations..."
echo "Environment deleted trigger:"
cat packages/events/src/triggers/environment-deleted.ts
echo -e "\nDeployment deleted trigger:"
cat packages/events/src/triggers/deployment-deleted.ts
echo -e "\nTarget deleted trigger:"
cat packages/events/src/triggers/target-deleted.ts
Length of output: 6705
packages/api/src/router/deployment.ts (2)
34-34: LGTM: Clean import of event handling functions
The import statement is well-placed and the imported functions have clear, descriptive names that align with their usage in the delete mutation.
270-271: Verify error handling in event processing
The code uses Promise.allSettled which prevents event processing failures from blocking the deletion, but there's no handling of failed events. We should verify if this is the intended behavior.
Let's check how event failures are handled in other similar operations:
✅ Verification successful
Event handling failures are consistently handled across the codebase
Looking at the evidence:
- The
handleEventfunction inpackages/events/src/index.tsproperly logs events to the database and has error handling for unhandled event types - All event processing across the codebase (environments, targets, deployments) consistently uses
Promise.allSettledto prevent individual event failures from blocking the main operation - This pattern is intentionally used in critical cleanup operations where we want the main operation to succeed even if event processing fails
The current implementation follows the established pattern in the codebase and is the intended behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other uses of handleEvent to verify consistent error handling patterns
rg -A 5 "handleEvent.*Promise\.allSettled" --type ts
Length of output: 54
Script:
#!/bin/bash
# Let's first find the handleEvent function definition to understand its error handling
ast-grep --pattern 'const handleEvent = $_'
# Also search for other usages of handleEvent
rg "handleEvent" --type ts -B 2 -A 2
# And look for similar patterns with Promise.allSettled
rg "Promise\.allSettled" --type ts -B 2 -A 2
Length of output: 8083
packages/events/src/triggers/target-deleted.ts (2)
37-40: Verify the correctness of SCHEMA.targetMatchesMetadata usage
Ensure that the SCHEMA.targetMatchesMetadata function behaves as intended when used with the constructed systemFilter. It's crucial that the filter accurately matches the relevant targets, especially in the context of deleted targets.
Consider adding tests to confirm that the function correctly filters targets based on metadata.
46-49: Ensure all relevant deployments are included in events
Currently, the function maps deployments directly to events without filtering based on the matched target. Verify that this approach includes only the deployments that are relevant to the deleted target.
You may need to adjust the logic to associate deployments with the correct targets.
packages/api/src/router/target.ts (1)
18-18: Import statements correctly added
The import of getEventsForTargetDeleted and handleEvent from @ctrlplane/events is appropriate and properly integrates the event handling functionality.
| const targets = await db.query.target.findMany({ | ||
| where: SCHEMA.targetMatchesMetadata(db, systemFilter), | ||
| }); | ||
|
|
||
| return targets.map((target) => ({ | ||
| action: "target.removed", | ||
| payload: { deployment, target }, | ||
| })); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and consider performance optimization.
The target querying and event generation could be improved in two areas:
- Error handling for the database query is missing
- For large datasets, consider implementing batch processing
Here's how to add error handling:
- const targets = await db.query.target.findMany({
- where: SCHEMA.targetMatchesMetadata(db, systemFilter),
- });
-
- return targets.map((target) => ({
- action: "target.removed",
- payload: { deployment, target },
- }));
+ try {
+ const targets = await db.query.target.findMany({
+ where: SCHEMA.targetMatchesMetadata(db, systemFilter),
+ });
+
+ return targets.map((target) => ({
+ action: "target.removed",
+ payload: { deployment, target },
+ }));
+ } catch (error) {
+ console.error('Failed to query targets:', error);
+ throw new Error('Failed to generate target removal events');
+ }Consider implementing batch processing if the number of targets could be large:
- Use cursor-based pagination in the target query
- Process targets in chunks to avoid memory issues
- Consider adding a limit to the number of events generated per deployment
Committable suggestion skipped: line range outside the PR's diff.
| export const getEventsForDeploymentDeleted = async ( | ||
| deployment: SCHEMA.Deployment, | ||
| ): Promise<HookEvent[]> => { | ||
| const system = await db.query.system.findFirst({ | ||
| where: eq(SCHEMA.system.id, deployment.systemId), | ||
| with: { | ||
| environments: { where: isNotNull(SCHEMA.environment.targetFilter) }, | ||
| }, | ||
| }); | ||
| if (system == null) return []; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for the database query.
While the function handles the null case, it should also handle potential database errors that could occur during the query operation.
Consider wrapping the database operation in a try-catch block:
export const getEventsForDeploymentDeleted = async (
deployment: SCHEMA.Deployment,
): Promise<HookEvent[]> => {
+ try {
const system = await db.query.system.findFirst({
where: eq(SCHEMA.system.id, deployment.systemId),
with: {
environments: { where: isNotNull(SCHEMA.environment.targetFilter) },
},
});
if (system == null) return [];
+ } catch (error) {
+ console.error('Failed to query system:', error);
+ throw new Error('Failed to process deployment deletion events');
+ }📝 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 getEventsForDeploymentDeleted = async ( | |
| deployment: SCHEMA.Deployment, | |
| ): Promise<HookEvent[]> => { | |
| const system = await db.query.system.findFirst({ | |
| where: eq(SCHEMA.system.id, deployment.systemId), | |
| with: { | |
| environments: { where: isNotNull(SCHEMA.environment.targetFilter) }, | |
| }, | |
| }); | |
| if (system == null) return []; | |
| export const getEventsForDeploymentDeleted = async ( | |
| deployment: SCHEMA.Deployment, | |
| ): Promise<HookEvent[]> => { | |
| try { | |
| const system = await db.query.system.findFirst({ | |
| where: eq(SCHEMA.system.id, deployment.systemId), | |
| with: { | |
| environments: { where: isNotNull(SCHEMA.environment.targetFilter) }, | |
| }, | |
| }); | |
| if (system == null) return []; | |
| } catch (error) { | |
| console.error('Failed to query system:', error); | |
| throw new Error('Failed to process deployment deletion events'); | |
| } |
| .mutation(async ({ ctx, input }) => { | ||
| const dep = await ctx.db | ||
| .select() | ||
| .from(deployment) | ||
| .where(eq(deployment.id, input)) | ||
| .then(takeFirst); | ||
| const events = await getEventsForDeploymentDeleted(dep); | ||
| await Promise.allSettled(events.map(handleEvent)); | ||
| return ctx.db | ||
| .delete(deployment) | ||
| .where(eq(deployment.id, input)) | ||
| .returning() | ||
| .then(takeFirst), | ||
| ), | ||
| .then(takeFirst); | ||
| }), |
There was a problem hiding this comment.
Add transaction handling to ensure data consistency
The current implementation could lead to inconsistent state if event processing fails after fetching the deployment but before deletion. Consider wrapping the operations in a transaction.
Here's a suggested implementation:
.mutation(async ({ ctx, input }) => {
+ return ctx.db.transaction(async (tx) => {
const dep = await ctx.db
.select()
.from(deployment)
.where(eq(deployment.id, input))
.then(takeFirst);
const events = await getEventsForDeploymentDeleted(dep);
await Promise.allSettled(events.map(handleEvent));
return ctx.db
.delete(deployment)
.where(eq(deployment.id, input))
.returning()
.then(takeFirst);
+ });
}),Committable suggestion skipped: line range outside the PR's diff.
| const deploymentPromises = systems.map(async (s) => { | ||
| const filters = s.environments.map((e) => e.targetFilter).filter(isPresent); | ||
|
|
||
| const systemFilter: TargetCondition = { | ||
| type: TargetFilterType.Comparison, | ||
| operator: ComparisonOperator.Or, | ||
| conditions: filters, | ||
| }; | ||
|
|
||
| const matchedTarget = await db.query.target.findFirst({ | ||
| where: SCHEMA.targetMatchesMetadata(db, systemFilter), | ||
| }); | ||
| if (matchedTarget == null) return []; | ||
|
|
||
| return s.deployments; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize database queries to reduce the number of calls
Currently, for each system, the code executes a separate database query to find a matching target. This could lead to performance issues if there are many systems. Consider refactoring the code to minimize the number of database queries by batching them or restructuring the logic.
You might modify the code to collect all filters and perform a single query, like so:
const filters = systems.flatMap((s) =>
s.environments.map((e) => e.targetFilter).filter(isPresent)
);
const systemFilter: TargetCondition = {
type: TargetFilterType.Comparison,
operator: ComparisonOperator.Or,
conditions: filters,
};
const matchedTargets = await db.query.target.findMany({
where: SCHEMA.targetMatchesMetadata(db, systemFilter),
});
const deployments = systems
.filter((s) =>
matchedTargets.some(
(target) => target.workspaceId === s.workspaceId
)
)
.flatMap((s) => s.deployments);Committable suggestion skipped: line range outside the PR's diff.
| .mutation(async ({ ctx, input }) => { | ||
| const targets = await ctx.db.query.target.findMany({ | ||
| where: inArray(schema.target.id, input), | ||
| }); | ||
| const events = ( | ||
| await Promise.allSettled(targets.map(getEventsForTargetDeleted)) | ||
| ).flatMap((r) => (r.status === "fulfilled" ? r.value : [])); | ||
| await Promise.allSettled(events.map(handleEvent)); | ||
|
|
||
| return ctx.db | ||
| .delete(schema.target) | ||
| .where(inArray(schema.target.id, input)) | ||
| .returning(), | ||
| ), | ||
| .returning(); | ||
| }), |
There was a problem hiding this comment.
Ensure proper error handling when processing events during target deletion
The current implementation uses Promise.allSettled and ignores rejected promises when generating and handling events for target deletion. This could lead to silent failures, causing inconsistent state or missed critical actions.
Consider explicitly handling rejected promises to ensure failures are not silently ignored. Here's how you might modify the code:
.mutation(async ({ ctx, input }) => {
const targets = await ctx.db.query.target.findMany({
where: inArray(schema.target.id, input),
});
- const events = (
- await Promise.allSettled(targets.map(getEventsForTargetDeleted))
- ).flatMap((r) => (r.status === "fulfilled" ? r.value : []));
+ const events = await Promise.all(
+ targets.map(async (target) => {
+ try {
+ return await getEventsForTargetDeleted(target);
+ } catch (error) {
+ // Handle or log the error appropriately
+ console.error(`Failed to get events for target ${target.id}:`, error);
+ throw error;
+ }
+ })
+ );
- await Promise.allSettled(events.map(handleEvent));
+ await Promise.all(
+ events.map(async (event) => {
+ try {
+ await handleEvent(event);
+ } catch (error) {
+ // Handle or log the error appropriately
+ console.error(`Failed to handle event for target deletion:`, error);
+ throw error;
+ }
+ })
+ );
return ctx.db
.delete(schema.target)
.where(inArray(schema.target.id, input))
.returning();
}),This approach ensures that any errors during event generation or handling are caught and can be handled appropriately, possibly aborting the deletion if necessary.
Committable suggestion skipped: line range outside the PR's diff.
🛠️ Refactor suggestion
Wrap database operations in a transaction for consistency
The deletion process includes multiple database operations: fetching targets, handling events, and deleting targets. To maintain data consistency, especially in cases where an error might occur during event handling, it's advisable to wrap these operations in a transaction.
Consider modifying the code to use a transaction:
.mutation(async ({ ctx, input }) => {
+ return ctx.db.transaction(async (tx) => {
const targets = await tx.query.target.findMany({
where: inArray(schema.target.id, input),
});
const events = await Promise.all(
targets.map(async (target) => {
// Error handling as discussed previously
return await getEventsForTargetDeleted(target);
})
);
await Promise.all(
events.map(async (event) => {
// Error handling as discussed previously
await handleEvent(event);
})
);
return tx
.delete(schema.target)
.where(inArray(schema.target.id, input))
.returning();
+ });
}),Wrapping these operations in a transaction ensures that if any part of the process fails, all changes can be rolled back, maintaining the integrity of your data.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
packages/db/src/schema/event.ts (2)
5-12: Consider adding indexes and payload validation.The event table schema looks good, but consider these improvements for better performance and data integrity:
- Add an index on
actionfor faster event filtering- Add an index on
createdAtfor efficient time-based queries- Add JSON Schema validation for the payload structure
export const event = pgTable("event", { id: uuid("id").primaryKey().defaultRandom(), action: text("action").notNull(), payload: jsonb("payload").notNull(), createdAt: timestamp("created_at", { withTimezone: true }) .notNull() .defaultNow(), -}); +}, (table) => { + return { + actionIdx: index("event_action_idx").on(table.action), + createdAtIdx: index("event_created_at_idx").on(table.createdAt), + }; +});
22-30: Add index on runbookId for better query performance.The schema correctly implements the relationship between hooks and runbooks with appropriate cascade delete behavior. Consider adding an index on
runbookIdto improve query performance when looking up hooks by runbook.export const runhook = pgTable("runhook", { id: uuid("id").primaryKey().defaultRandom(), hookId: uuid("hook_id") .notNull() .references(() => hook.id, { onDelete: "cascade" }), runbookId: uuid("runbook_id") .notNull() .references(() => runbook.id, { onDelete: "cascade" }), -}); +}, (table) => { + return { + runbookIdx: index("runhook_runbook_idx").on(table.runbookId), + }; +});packages/db/drizzle/0035_moaning_supernaut.sql (3)
1-6: Consider adding indexes for performance optimizationThe event table might benefit from additional indexes:
- An index on
actionfor efficient event filtering- An index on
created_atfor timestamp-based queries and cleanup operationsCREATE INDEX IF NOT EXISTS idx_event_action ON event(action); CREATE INDEX IF NOT EXISTS idx_event_created_at ON event(created_at);
16-20: Consider renaming table and adding indexes
- The table name 'runhook' is not very descriptive. Consider 'hook_runbook_mapping' or 'hook_execution' to better reflect its purpose.
- Add indexes on foreign key columns for better JOIN performance.
CREATE INDEX IF NOT EXISTS idx_runhook_hook_id ON runhook(hook_id); CREATE INDEX IF NOT EXISTS idx_runhook_runbook_id ON runhook(runbook_id);
1-32: Consider operational aspects of the event systemImportant architectural considerations:
- Event Retention: Consider implementing a TTL strategy for the event table (e.g., using partitioning by created_at)
- Scaling: For high-volume event storage, consider implementing table partitioning
- Monitoring: Add necessary triggers or procedures to monitor table growth
Example partitioning strategy:
CREATE TABLE event_partition OF event PARTITION BY RANGE (created_at); CREATE TABLE event_y2024m01 PARTITION OF event_partition FOR VALUES FROM ('2024-01-01') TO ('2024-02-01');packages/events/src/triggers/target-deleted.ts (1)
46-49: Add type safety for event action stringsConsider defining an enum or union type for event actions to ensure type safety and prevent potential errors from typos.
+type DeploymentEventAction = "deployment.target.removed" | "deployment.target.added"; + return deployments.map((deployment) => ({ - action: "deployment.target.removed", + action: "deployment.target.removed" as DeploymentEventAction, payload: { target, deployment }, }));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
packages/api/src/router/target.ts(2 hunks)packages/db/drizzle/0035_moaning_supernaut.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/event.ts(1 hunks)packages/events/src/handlers/target-removed.ts(1 hunks)packages/events/src/triggers/deployment-deleted.ts(1 hunks)packages/events/src/triggers/environment-deleted.ts(1 hunks)packages/events/src/triggers/target-deleted.ts(1 hunks)packages/validators/src/events/hooks/index.ts(1 hunks)packages/validators/src/events/hooks/target.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/db/drizzle/meta/_journal.json
- packages/events/src/handlers/target-removed.ts
- packages/events/src/triggers/deployment-deleted.ts
- packages/events/src/triggers/environment-deleted.ts
- packages/validators/src/events/hooks/index.ts
- packages/validators/src/events/hooks/target.ts
🔇 Additional comments (7)
packages/db/src/schema/event.ts (1)
1-4: LGTM! Imports are well-structured.
The imports are correctly organized, using named imports and proper file extensions.
packages/db/drizzle/0035_moaning_supernaut.sql (1)
22-32: Verify foreign key dependencies and cascade behavior
- The CASCADE DELETE on both foreign keys could lead to unintended data loss. Please verify if this is the desired behavior.
- Need to verify the existence of the "public"."runbook" table as it's referenced but not created in this migration.
✅ Verification successful
Foreign key constraints and cascade behavior are correctly configured
The verification shows that:
- The
runbooktable exists and was created in the initial migration (0000_busy_firebird.sql) - The CASCADE DELETE behavior is consistent across all foreign key constraints referencing the
runbooktable:- runhook_runbook_id_runbook_id_fk (new constraint)
- runbook_job_trigger_runbook_id_runbook_id_fk
- runbook_variable_runbook_id_runbook_id_fk
The CASCADE DELETE is intentional and follows the established pattern in the schema where child records are automatically removed when their parent runbook is deleted.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if runbook table exists and its structure
rg -A 10 'CREATE TABLE.*runbook' --type sql
# Look for any existing foreign key references to runbook
rg 'REFERENCES.*runbook' --type sql
Length of output: 3163
packages/events/src/triggers/target-deleted.ts (3)
17-26: LGTM! Efficient database query design
The initial query is well-structured, efficiently fetching all required data in a single database call while properly scoping to the workspace and filtering for relevant environments.
28-44: Consider batching database queries for better performance
The current implementation executes a separate database query for each system, which could lead to performance issues with many systems.
The previous review comment about optimizing database queries by batching them together is still applicable here.
11-16: Verify the timing of function calls in the target deletion flow
The documentation correctly notes that this function must be called before the target is deleted. Let's verify this requirement is met in the implementation.
✅ Verification successful
Based on the search results, I can verify the timing requirement. Let's analyze the key findings:
- In
packages/api/src/router/target.ts, thegetEventsForTargetDeletedis called before the actual target deletion:
await Promise.allSettled(targets.map(getEventsForTargetDeleted))
// ... handle events ...
return ctx.db
.delete(schema.target)
.where(inArray(schema.target.id, input))
.returning();- The same pattern is followed in all target deletion flows:
- API routes (
apps/webservice/src/app/api/v1/targets/[targetId]/route.ts) - Target provider deletion (
packages/api/src/router/target-provider.ts) - Workspace-specific target deletion (
apps/webservice/src/app/api/v1/workspaces/[workspaceId]/targets/identifier/[identifier]/route.ts)
Timing requirement is correctly implemented
The documentation's timing requirement is properly enforced in the implementation. The getEventsForTargetDeleted function is consistently called before any target deletion operations across all code paths.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the order of operations in target deletion flow
# Expected: getEventsForTargetDeleted should be called before target deletion
# Search for target deletion implementations
rg -A 10 "target.*delete|delete.*target" --type ts
# Search for usage of getEventsForTargetDeleted
rg -A 10 "getEventsForTargetDeleted" --type ts
Length of output: 26187
packages/api/src/router/target.ts (2)
18-18: LGTM!
The import statement is well-structured and follows naming conventions.
481-494:
Critical: Improve error handling and data consistency
The current implementation has several critical issues that need to be addressed:
- Silent failure handling in event processing could lead to inconsistent state
- Lack of transaction management could result in partial updates
- Potential race conditions between target fetch and deletion
Consider implementing proper error handling and wrapping operations in a transaction. The previous review comments provide detailed suggestions for these improvements.
Additionally, to prevent race conditions, consider adding optimistic locking:
.mutation(async ({ ctx, input }) => {
+ return ctx.db.transaction(async (tx) => {
const targets = await ctx.db.query.target.findMany({
where: inArray(schema.target.id, input),
+ for: 'update', // Add FOR UPDATE lock
});
// ... event handling with proper error management as suggested in previous review
return ctx.db
.delete(schema.target)
.where(inArray(schema.target.id, input))
.returning();
+ });
}),| export const hook = pgTable("hook", { | ||
| id: uuid("id").primaryKey().defaultRandom(), | ||
| action: text("action").notNull(), | ||
| name: text("name").notNull(), | ||
| scopeType: text("scope_type").notNull(), | ||
| scopeId: uuid("scope_id").notNull(), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add index and unique constraint for better data integrity.
Consider these improvements:
- Add a composite index on (scopeType, scopeId) for faster hook lookups within a scope
- Add a unique constraint on (name, scopeType, scopeId) to prevent duplicate hook names within the same scope
export const hook = pgTable("hook", {
id: uuid("id").primaryKey().defaultRandom(),
action: text("action").notNull(),
name: text("name").notNull(),
scopeType: text("scope_type").notNull(),
scopeId: uuid("scope_id").notNull(),
-});
+}, (table) => {
+ return {
+ scopeIdx: index("hook_scope_idx").on(table.scopeType, table.scopeId),
+ nameUnique: uniqueIndex("hook_name_scope_unique_idx").on(
+ table.name,
+ table.scopeType,
+ table.scopeId
+ ),
+ };
+});📝 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 hook = pgTable("hook", { | |
| id: uuid("id").primaryKey().defaultRandom(), | |
| action: text("action").notNull(), | |
| name: text("name").notNull(), | |
| scopeType: text("scope_type").notNull(), | |
| scopeId: uuid("scope_id").notNull(), | |
| }); | |
| export const hook = pgTable("hook", { | |
| id: uuid("id").primaryKey().defaultRandom(), | |
| action: text("action").notNull(), | |
| name: text("name").notNull(), | |
| scopeType: text("scope_type").notNull(), | |
| scopeId: uuid("scope_id").notNull(), | |
| }, (table) => { | |
| return { | |
| scopeIdx: index("hook_scope_idx").on(table.scopeType, table.scopeId), | |
| nameUnique: uniqueIndex("hook_name_scope_unique_idx").on( | |
| table.name, | |
| table.scopeType, | |
| table.scopeId | |
| ), | |
| }; | |
| }); |
| CREATE TABLE IF NOT EXISTS "hook" ( | ||
| "id" uuid PRIMARY KEY DEFAULT gen_random_uuid() NOT NULL, | ||
| "action" text NOT NULL, | ||
| "name" text NOT NULL, | ||
| "scope_type" text NOT NULL, | ||
| "scope_id" uuid NOT NULL | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance data integrity and query performance
Consider the following improvements:
- Add a composite index for scope-based queries
- Add a CHECK constraint for valid scope_type values
- Consider adding a unique constraint on (name, scope_type, scope_id)
CREATE INDEX IF NOT EXISTS idx_hook_scope ON hook(scope_type, scope_id);
ALTER TABLE hook ADD CONSTRAINT chk_hook_scope_type
CHECK (scope_type IN ('environment', 'deployment', 'target'));
ALTER TABLE hook ADD CONSTRAINT unq_hook_name_scope
UNIQUE (name, scope_type, scope_id);
Summary by CodeRabbit
Release Notes
New Features
event,hook,runhook) to manage events and their relationships.Bug Fixes
Documentation
Chores