fix: Move events to job dispatch and use deleteResources utility function everywhere #222
Conversation
WalkthroughThe pull request includes significant modifications across various files, primarily focusing on dependency management, import adjustments, and Dockerfile optimizations. Key changes involve the removal of the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
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: 4
🧹 Outside diff range and nitpick comments (8)
packages/job-dispatch/tsconfig.json (1)
5-6: Consider reorganizing the TypeScript configurationHaving both
baseUrlandrootDirset to "." while including files outside the package directory might lead to unexpected module resolution behavior. Consider:
- Moving shared code to a separate package
- Adjusting the directory structure to better reflect the module boundaries
apps/jobs/src/expired-env-checker/index.ts (1)
Line range hint
28-31: Consider using deleteResources utility functionAccording to the PR objectives, we should use the deleteResources utility function everywhere. The current implementation directly deletes from the database.
Consider replacing the direct database deletion with the deleteResources utility function. This would maintain consistency with the PR objectives and ensure all resource cleanup is handled uniformly.
- await db - .delete(SCHEMA.environment) - .where(inArray(SCHEMA.environment.id, envIds)); + await deleteResources(envIds);Note: You'll need to import the deleteResources utility function.
apps/pty-proxy/src/controller/sockets.ts (1)
Line range hint
15-17: Consider optimizing stale connection checksThe current implementation performs checks every minute for all agents, which could impact performance as the number of agents grows. Each stale agent triggers a database query.
Consider these optimizations:
- Increase the check interval (e.g., to 5 minutes) while keeping the 15-minute stale threshold
- Batch process stale agents to reduce database operations
- Use a more efficient data structure (like a priority queue) to track agent timestamps
packages/api/src/router/runbook.ts (2)
38-47: Great simplification of the query implementation!The new implementation is more maintainable and leverages the type-safe query builder effectively. The inclusion of all necessary related data through the
withclauses ensures complete data retrieval while maintaining good performance characteristics by avoiding N+1 queries.Consider adding a comment documenting the shape of the returned data structure, as it might be helpful for consumers of this API.
+ // Returns runbooks with their associated runhooks, hooks, job agent, and variables .query(({ ctx, input }) =>
Consider using deleteResources utility function for runbook deletion
The runbook deletion currently uses direct database deletion:
ctx.db.delete(SCHEMA.runbook).where(eq(SCHEMA.runbook.id, input))Based on the codebase analysis:
- The
deleteResourcesutility from@ctrlplane/job-dispatchis used across the codebase for resource deletion- It handles additional responsibilities like event dispatching for deleted resources
- The runbook router already imports from
@ctrlplane/job-dispatchbut only usesdispatchRunbook🔗 Analysis chain
Line range hint
119-124: Consider using deleteResources utility functionBased on the PR objectives to "use deleteResources utility function everywhere", the delete operation might need to be updated to use this utility function instead of direct database deletion.
Let's verify the existence and usage pattern of the deleteResources utility:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the deleteResources utility function definition and its usage pattern rg -t typescript "deleteResources" -A 5Length of output: 74
Script:
#!/bin/bash # Search for deleteResources without file type restriction rg "deleteResources" -A 5 # Also search for similar delete utility patterns rg "delete.*Resources" -A 5 # Search for any delete-related utility functions rg "export.*delete|function.*delete" -A 5Length of output: 11251
Script:
#!/bin/bash # Let's check the implementation of runbook deletion to see if it's using direct DB deletion rg -A 10 "runbook.*delete" packages/api/src/router/runbook.ts # Also check if there are any resource-related operations for runbooks ast-grep --pattern 'from "@ctrlplane/job-dispatch"' packages/api/src/router/runbook.tsLength of output: 152
Script:
#!/bin/bash # Let's check the entire content of the runbook.ts file to understand the deletion implementation cat packages/api/src/router/runbook.ts # Also check for any imports from job-dispatch in this file rg -l "@ctrlplane/job-dispatch" packages/api/src/router/runbook.tsLength of output: 3998
packages/api/src/router/deployment.ts (1)
Line range hint
479-489: Consider enhancing error handling and atomicityWhile the event handling logic is correct, consider these improvements for better robustness:
- Add error logging for failed events
- Wrap the operation in a transaction for atomicity
.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)); + const results = await Promise.allSettled(events.map(handleEvent)); + results.forEach((result, index) => { + if (result.status === 'rejected') { + console.error(`Failed to handle event ${index}:`, result.reason); + } + }); return ctx.db .delete(deployment) .where(eq(deployment.id, input)) .returning() .then(takeFirst); + }); }),packages/job-dispatch/src/resource.ts (1)
347-352: Simplify asynchronous operations for better readabilityYou can simplify the code by combining the
Promise.allandawaitinto a single statement, which enhances readability and aligns with common asynchronous patterns.Refactored code:
export const deleteResources = async (tx: Tx, resources: Resource[]) => { + try { const events = ( await Promise.all(resources.map(getEventsForResourceDeleted)) ).flat(); await Promise.all(events.map(handleEvent)); const resourceIds = resources.map((r) => r.id); await tx.delete(resource).where(inArray(resource.id, resourceIds)); + } catch (err) { + log.error("Error in deleteResources", { error: err }); + throw err; + } };packages/api/src/router/environment.ts (1)
Line range hint
243-249: Handle rejections fromPromise.allSettledin event handlingIn the
deletemutation,Promise.allSettledis used to handle event promises, but the code does not process the results to handle any rejections. This could lead to silent failures if any of thehandleEventcalls are rejected. To improve error visibility and robustness, consider iterating over the settled promises and handling any rejected promises appropriately.Apply this diff to handle rejections:
.then(async (env) => { const events = await getEventsForEnvironmentDeleted(env); const handleEventPromises = events.map(handleEvent); - await Promise.allSettled(handleEventPromises); + const results = await Promise.allSettled(handleEventPromises); + results.forEach((result) => { + if (result.status === 'rejected') { + console.error('Event handling error in delete mutation:', result.reason); + } + }); return env; }),
📜 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 (21)
apps/jobs/Dockerfile(0 hunks)apps/jobs/package.json(0 hunks)apps/jobs/src/expired-env-checker/index.ts(1 hunks)apps/pty-proxy/src/controller/sockets.ts(2 hunks)apps/webservice/Dockerfile(0 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/EditHookDialog.tsx(2 hunks)packages/api/package.json(0 hunks)packages/api/src/router/deployment.ts(1 hunks)packages/api/src/router/environment.ts(1 hunks)packages/api/src/router/resources.ts(2 hunks)packages/api/src/router/runbook.ts(1 hunks)packages/db/src/schema/event.ts(1 hunks)packages/events/eslint.config.js(0 hunks)packages/events/package.json(0 hunks)packages/events/src/handlers/index.ts(0 hunks)packages/events/tsconfig.json(0 hunks)packages/job-dispatch/src/events/handlers/index.ts(1 hunks)packages/job-dispatch/src/events/triggers/index.ts(1 hunks)packages/job-dispatch/src/index.ts(1 hunks)packages/job-dispatch/src/resource.ts(3 hunks)packages/job-dispatch/tsconfig.json(1 hunks)
💤 Files with no reviewable changes (8)
- apps/jobs/Dockerfile
- apps/jobs/package.json
- apps/webservice/Dockerfile
- packages/api/package.json
- packages/events/eslint.config.js
- packages/events/package.json
- packages/events/src/handlers/index.ts
- packages/events/tsconfig.json
✅ Files skipped from review due to trivial changes (3)
- packages/db/src/schema/event.ts
- packages/job-dispatch/src/events/handlers/index.ts
- packages/job-dispatch/src/events/triggers/index.ts
🔇 Additional comments (14)
packages/job-dispatch/tsconfig.json (1)
8-8: Verify the necessity of including external utils directory
Including "../api/src/utils" from outside the package directory could lead to tight coupling and potential circular dependencies. Consider moving shared utilities to a common package or copying necessary utilities into this package.
Let's check what utilities are being used:
apps/jobs/src/expired-env-checker/index.ts (1)
4-7: Verify the imported functions exist in @ctrlplane/job-dispatch
The import change aligns with the PR objective to move events to job dispatch.
Let's verify that these functions exist in the new module with the same signature:
✅ Verification successful
Import changes are correctly implemented
The functions getEventsForEnvironmentDeleted and handleEvent are properly exported from @ctrlplane/job-dispatch with matching signatures. Found in:
packages/job-dispatch/src/events/triggers/environment-deleted.tspackages/job-dispatch/src/events/index.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the functions exist in @ctrlplane/job-dispatch
# Expected: Find function declarations in the new module
# Search for function declarations
ast-grep --pattern 'export $_ getEventsForEnvironmentDeleted' packages/job-dispatch
ast-grep --pattern 'export $_ handleEvent' packages/job-dispatch
Length of output: 152
Script:
#!/bin/bash
# Let's try a different approach to find these functions
# 1. First, let's check if the directory structure is correct
fd -t d "job-dispatch"
# 2. Then search for these function names in the codebase
rg "getEventsForEnvironmentDeleted" -l
rg "handleEvent" -l
# 3. Let's also check the package.json to verify the package name
fd "package.json" -x grep -l "@ctrlplane/job-dispatch" {}
Length of output: 729
Script:
#!/bin/bash
# Let's check the actual implementation of these functions
rg "getEventsForEnvironmentDeleted" -A 5 packages/job-dispatch/src/events/triggers/environment-deleted.ts
rg "export.*handleEvent" -A 5 packages/job-dispatch/src/events/index.ts
Length of output: 539
packages/job-dispatch/src/index.ts (1)
26-26: LGTM! Export aligns with centralization of event handling.
The addition of this export statement supports the PR's objective of moving event handling to the job-dispatch module.
apps/pty-proxy/src/controller/sockets.ts (1)
3-6: LGTM! Import changes align with PR objectives
The new imports support the transition to using the job dispatch system and database utilities.
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/EditHookDialog.tsx (2)
79-79: LGTM: Type safety improvement
The explicit type casting of hook.action to HookAction enhances type safety and makes the code more maintainable.
148-148: Verify empty state handling
The button text now always displays the value without a placeholder. Please verify that this handles the initial empty state gracefully.
Consider adding a fallback for empty state:
- {value}
+ {value || "Select action..."}✅ Verification successful
Empty state is properly handled - no changes needed
The code shows that:
- The
actionfield is initialized with the existing hook's action indefaultValues - The component is part of an edit dialog where a hook already exists
- The UI uses a Command pattern with proper placeholder text "Search action..." for the input field
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any default/initial state handling for the action field
ast-grep --pattern 'action: $value' # Search for default value assignments
rg -A 2 'defaultValues.*action' # Search for default value configurations
Length of output: 71
Script:
#!/bin/bash
# Let's look at the component implementation and surrounding context
rg -A 10 -B 10 "EditHookDialog" apps/webservice/src/app/[workspaceSlug]/\(app\)/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/EditHookDialog.tsx
# Also search for any Select or similar components that might be using this value
rg -A 5 "Select.*value.*action" apps/webservice/src/app/[workspaceSlug]/\(app\)/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/
Length of output: 1299
Script:
#!/bin/bash
# Let's search for the form initialization and default values
rg -A 20 "const.*form.*useForm" apps/webservice/src/app/[workspaceSlug]/\(app\)/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/EditHookDialog.tsx
# Let's also look at the Select component implementation
rg -A 10 -B 5 "value={.*action" apps/webservice/src/app/[workspaceSlug]/\(app\)/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/EditHookDialog.tsx
Length of output: 1196
Script:
#!/bin/bash
# Let's look for the defaultValues initialization
rg -B 10 "const form = useForm" apps/webservice/src/app/[workspaceSlug]/\(app\)/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/EditHookDialog.tsx
# And let's look at the Select component with proper escaping
rg -A 10 -B 5 'value=\{.*action\}' apps/webservice/src/app/[workspaceSlug]/\(app\)/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/EditHookDialog.tsx
Length of output: 1432
packages/api/src/router/resources.ts (2)
24-24: LGTM: Import statement follows conventions
The addition of deleteResources import aligns with the PR objective and maintains the existing import structure.
580-584: Verify event handling migration
Let's verify that all resource deletion events are properly migrated to the new system.
✅ Verification successful
Let me gather more information about the event handling implementation to ensure completeness of the migration.
Based on the comprehensive results, I can now provide the final response:
Resource deletion events are properly migrated
The migration from events to job dispatch is complete and properly implemented:
- The
deleteResourcesfunction injob-dispatchcorrectly handles resource deletion events - Events are properly triggered through
getEventsForResourceDeletedand processed viahandleEvent - The implementation includes proper handling of deployments and environments affected by resource deletion
- No references to the old
@ctrlplane/eventssystem remain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old event system
# and verify the new deleteResources implementation
# Check for any remaining references to @ctrlplane/events
echo "Checking for remaining references to @ctrlplane/events..."
rg "@ctrlplane/events"
# Check the implementation of deleteResources to ensure it handles all events
echo "Checking deleteResources implementation..."
rg -A 10 "export const deleteResources"
# Look for any remaining event handling patterns
echo "Checking for remaining event handling patterns..."
rg "getEventsForResourceDeleted|handleEvent"
Length of output: 2531
Script:
#!/bin/bash
# Check the implementation of event handling in the new system
echo "Checking event handling implementation..."
ast-grep --pattern 'export const getEventsForResourceDeleted = $_'
# Check for any resource deletion related code
echo "Checking resource deletion patterns..."
rg -A 5 "deleteResource|ResourceDeleted"
# Check the events directory structure
echo "Checking events directory structure..."
fd . packages/job-dispatch/src/events
Length of output: 13462
packages/api/src/router/deployment.ts (2)
40-43: LGTM: Import statement updated correctly
The change aligns with the PR objective to move events from '@ctrlplane/events' to '@ctrlplane/job-dispatch'.
40-43: Verify complete migration from @ctrlplane/events
Let's ensure all references to '@ctrlplane/events' have been migrated to '@ctrlplane/job-dispatch'.
Also applies to: 479-489
✅ Verification successful
Migration from @ctrlplane/events to @ctrlplane/job-dispatch is complete
The verification shows that:
- No references to '@ctrlplane/events' package remain in the codebase
- All event-related functions (
getEventsForDeploymentDeleted,handleEvent) are now properly imported from '@ctrlplane/job-dispatch' - The functions are consistently used across the codebase in:
- packages/api/src/router/deployment.ts
- packages/api/src/router/environment.ts
- apps/jobs/src/expired-env-checker/index.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to @ctrlplane/events
# Search for any remaining imports from @ctrlplane/events
echo "Checking for remaining @ctrlplane/events imports..."
rg "from ['\"]@ctrlplane/events['\"]"
# Search for any remaining package.json dependencies
echo "Checking package.json files for @ctrlplane/events dependency..."
fd "package.json" --exec jq -r '.dependencies["@ctrlplane/events"] // .devDependencies["@ctrlplane/events"] // empty'
Length of output: 523
Script:
#!/bin/bash
# Let's try a different approach to check for remaining references
# Search for any string containing @ctrlplane/events
echo "Checking for any references to @ctrlplane/events..."
rg "@ctrlplane/events"
# Check package.json files with a more resilient approach
echo -e "\nChecking package.json files..."
fd "package.json" --exec cat {} \; | grep -A 2 -B 2 "@ctrlplane/events"
# Look for any event-related imports that might need migration
echo -e "\nChecking for event-related function usage..."
rg "getEventsFor|handleEvent" -A 2 -B 2
Length of output: 6919
packages/job-dispatch/src/resource.ts (2)
25-25: Import statement is correct
The import statement correctly imports getEventsForResourceDeleted and handleEvent from "./events/index.js", which are used for event handling related to resource deletions.
324-324: Error handling is consistent when deleting resources
The catch block appropriately logs the error and rethrows it, ensuring that any issues during the deletion process are handled consistently.
packages/api/src/router/environment.ts (2)
29-33: Updated imports for event handling functions
The import statements have been correctly updated to import event handling functions from @ctrlplane/job-dispatch. This aligns with the refactoring efforts to remove the @ctrlplane/events dependency and maintains consistency across the codebase.
Line range hint 167-177: Handle rejections from Promise.allSettled in event handling
Similarly, in the update mutation, Promise.allSettled is used without processing the results for rejections. This could result in silent failures when handling events for removed resources. To ensure errors are not missed, consider handling the rejected promises.
Apply this diff to handle rejections:
if (removedResources.length > 0) {
// Existing code to generate events
const handleEventPromises = events.map(handleEvent);
- await Promise.allSettled(handleEventPromises);
+ const results = await Promise.allSettled(handleEventPromises);
+ results.forEach((result) => {
+ if (result.status === 'rejected') {
+ console.error('Event handling error in update mutation:', result.reason);
+ }
+ });
}| @@ -23,3 +23,4 @@ export * from "./policies/concurrency-policy.js"; | |||
| export * from "./policies/release-window.js"; | |||
| export * from "./environment-creation.js"; | |||
| export * from "./pending-job-checker.js"; | |||
| export * from "./events/index.js"; | |||
There was a problem hiding this comment.
💡 Codebase verification
Event module integration is properly structured but has a circular dependency
The events module is well-structured with proper exports and imports, but there's a circular dependency that needs attention:
packages/job-dispatch/src/events/handlers/resource-removed.tsimports from@ctrlplane/job-dispatchwhile being part of the same package- This creates a circular dependency as the main package exports the events module which contains this handler
Suggested fix:
- Move the
dispatchRunbookfunction to a separate utility module - Import it directly in
resource-removed.tsinstead of importing from the main package
🔗 Analysis chain
Verify the event module integration.
Let's ensure this export is properly integrated:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the events module integration and usage
# Check if the events/index.js file exists
echo "Checking for events/index.js file..."
fd --type f "index.js" packages/job-dispatch/src/events
# Check for potential circular dependencies
echo "Checking for potential circular dependencies..."
rg -l "from \"@ctrlplane/job-dispatch\"" packages/job-dispatch/src/events
# Check usage of the newly exported events
echo "Checking usage of exported events..."
rg --type ts "from \"@ctrlplane/job-dispatch\".*events" -A 3
Length of output: 523
Script:
#!/bin/bash
# Let's verify the events module structure and integration more thoroughly
# Check the content of events/index.ts (not .js)
echo "Checking for events/index.ts file..."
fd --type f "index.ts" packages/job-dispatch/src/events
echo "Checking content of events/index.ts if exists..."
fd --type f "index.ts" packages/job-dispatch/src/events --exec cat {}
echo "Checking for all files in events directory..."
fd . packages/job-dispatch/src/events
# Check the specific circular dependency found
echo "Checking content of resource-removed.ts..."
cat packages/job-dispatch/src/events/handlers/resource-removed.ts
# Check if events are imported elsewhere in the project
echo "Checking broader usage of events..."
rg "from.*events" packages/job-dispatch/src
Length of output: 4003
| db.query.resource | ||
| .findFirst({ where: eq(SCHEMA.resource.id, agentId) }) | ||
| .then((resource) => resource && deleteResources(db, [resource])) | ||
| .then(() => logger.info("Deleted stale agent resource", { agentId })); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and improve logging precision
While the new implementation correctly checks for resource existence before deletion, there are a few concerns:
- Missing error handling for the promise chain
- The success log message doesn't accurately reflect whether deletion succeeded
- Potential race condition between resource check and deletion
Consider this improved implementation:
db.query.resource
.findFirst({ where: eq(SCHEMA.resource.id, agentId) })
.then((resource) => resource && deleteResources(db, [resource]))
- .then(() => logger.info("Deleted stale agent resource", { agentId }));
+ .then((deleted) => {
+ if (deleted) {
+ logger.info("Deleted stale agent resource", { agentId });
+ } else {
+ logger.info("No resource found for stale agent", { agentId });
+ }
+ })
+ .catch((error) => {
+ logger.error("Failed to delete stale agent resource", {
+ agentId,
+ error: 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.
| db.query.resource | |
| .findFirst({ where: eq(SCHEMA.resource.id, agentId) }) | |
| .then((resource) => resource && deleteResources(db, [resource])) | |
| .then(() => logger.info("Deleted stale agent resource", { agentId })); | |
| db.query.resource | |
| .findFirst({ where: eq(SCHEMA.resource.id, agentId) }) | |
| .then((resource) => resource && deleteResources(db, [resource])) | |
| .then((deleted) => { | |
| if (deleted) { | |
| logger.info("Deleted stale agent resource", { agentId }); | |
| } else { | |
| logger.info("No resource found for stale agent", { agentId }); | |
| } | |
| }) | |
| .catch((error) => { | |
| logger.error("Failed to delete stale agent resource", { | |
| agentId, | |
| error: error.message | |
| }); | |
| }); |
| .mutation(async ({ ctx, input }) => | ||
| ctx.db.query.resource | ||
| .findMany({ where: inArray(schema.resource.id, input) }) | ||
| .then((resources) => deleteResources(ctx.db, resources)), | ||
| ), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider wrapping operations in a transaction and adding error handling
While the implementation correctly uses the new deleteResources utility, consider these improvements:
- Wrap the query and delete operations in a transaction to ensure atomicity
- Add error handling for cases where resources don't exist
- Consider performance optimization by selecting only necessary fields instead of fetching entire resources
Here's a suggested implementation:
.mutation(async ({ ctx, input }) =>
- ctx.db.query.resource
- .findMany({ where: inArray(schema.resource.id, input) })
- .then((resources) => deleteResources(ctx.db, resources)),
+ ctx.db.transaction(async (tx) => {
+ const resources = await tx.query.resource.findMany({
+ where: inArray(schema.resource.id, input),
+ columns: {
+ id: true,
+ kind: true,
+ name: true,
+ workspaceId: true
+ }
+ });
+
+ if (resources.length !== input.length) {
+ throw new Error('Some resources not found');
+ }
+
+ return deleteResources(tx, resources);
+ }),📝 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.query.resource | |
| .findMany({ where: inArray(schema.resource.id, input) }) | |
| .then((resources) => deleteResources(ctx.db, resources)), | |
| ), | |
| .mutation(async ({ ctx, input }) => | |
| ctx.db.transaction(async (tx) => { | |
| const resources = await tx.query.resource.findMany({ | |
| where: inArray(schema.resource.id, input), | |
| columns: { | |
| id: true, | |
| kind: true, | |
| name: true, | |
| workspaceId: true | |
| } | |
| }); | |
| if (resources.length !== input.length) { | |
| throw new Error('Some resources not found'); | |
| } | |
| return deleteResources(tx, resources); | |
| }), | |
| ), |
| export const deleteResources = async (tx: Tx, resources: Resource[]) => { | ||
| const eventsPromises = Promise.all( | ||
| resources.map(getEventsForResourceDeleted), | ||
| ); | ||
| const events = await eventsPromises.then((res) => res.flat()); | ||
| await Promise.all(events.map(handleEvent)); |
There was a problem hiding this comment.
Add error handling in deleteResources function
The deleteResources function lacks error handling around the asynchronous operations for fetching and handling events. If any of these promises are rejected, it could lead to unhandled promise rejections and potentially leave the system in an inconsistent state. To maintain robustness and consistency with the rest of the codebase, consider adding error handling.
Apply this diff to add error handling:
export const deleteResources = async (tx: Tx, resources: Resource[]) => {
+ try {
const eventsPromises = Promise.all(
resources.map(getEventsForResourceDeleted),
);
const events = await eventsPromises.then((res) => res.flat());
await Promise.all(events.map(handleEvent));
const resourceIds = resources.map((r) => r.id);
await tx.delete(resource).where(inArray(resource.id, resourceIds));
+ } catch (err) {
+ log.error("Error in deleteResources", { error: err });
+ throw err;
+ }
};Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores
@ctrlplane/eventsacross multiple packages.Documentation