fix: Always dispatch resources with changed vars #227
Conversation
WalkthroughThis pull request introduces updates to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/job-dispatch/src/resource/insert-resource-variables.ts (1)
69-75: Consider optimizing Set creation.While the current implementation is correct, we can optimize it by avoiding the intermediate array creation.
Here's a more efficient approach:
- const updatedResourceIds = [ - ...created.map((r) => r.resourceId), - ...deleted.map((r) => r.resourceId), - ...updated.map((r) => r.resourceId), - ]; - - return new Set(updatedResourceIds); + return new Set([ + ...created, + ...deleted, + ...updated + ].map(r => r.resourceId));Or even better, to avoid spreading arrays altogether:
- const updatedResourceIds = [ - ...created.map((r) => r.resourceId), - ...deleted.map((r) => r.resourceId), - ...updated.map((r) => r.resourceId), - ]; - - return new Set(updatedResourceIds); + const resourceIds = new Set<string>(); + for (const r of created) resourceIds.add(r.resourceId); + for (const r of deleted) resourceIds.add(r.resourceId); + for (const r of updated) resourceIds.add(r.resourceId); + return resourceIds;packages/job-dispatch/src/resource/upsert.ts (2)
65-65: Clarify the ignored result frominsertResourceMetadataIn the destructuring of the results from
Promise.all, the result ofinsertResourceMetadatais being ignored:const [, updatedVariableResourceIds] = await Promise.all([ insertResourceMetadata(tx, resourcesWithId), insertResourceVariables(tx, resourcesWithId), ]);If
insertResourceMetadatadoes not return a value or its result is not needed, consider the following:
- Remove it from the
Promise.allandawaitit separately to make it clear that its result is intentionally ignored.- Alternatively, if it returns a value that should be handled, ensure that it is appropriately captured and used.
This will improve code clarity and maintainability.
131-135: Review the filtering logic for removed resourcesIn the filtering of
removedIds:.filter( (id) => !deletedResourceIds.has(id) && !updatedVariableResourceIds.has(id), );Verify that excluding resource IDs in
updatedVariableResourceIdsfromremovedIdsis the intended behavior. Resources with updated variables might still need to have certain operations performed even if they are not deleted. If this logic is intentional, consider adding a comment to explain the reasoning for future maintainability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
packages/job-dispatch/src/resource/insert-resource-variables.ts(3 hunks)packages/job-dispatch/src/resource/upsert.ts(3 hunks)
🔇 Additional comments (2)
packages/job-dispatch/src/resource/insert-resource-variables.ts (2)
34-34: LGTM! Consistent return type handling.
The early return of an empty Set maintains consistency with the function's contract.
16-16: LGTM! Verify consumers of this function.
The explicit return type improves type safety and makes the contract clearer.
Let's verify the impact on existing consumers:
✅ Verification successful
Return type change is safe to proceed
The new Promise<Set<string>> return type is correctly handled in the only consumer (upsert.ts). The result is stored in updatedVariableResourceIds and used appropriately with the Set's has() method in two places:
- Line 89: Filtering resources for variable changes
- Line 134: Filtering removed resource IDs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all usages of insertResourceVariables to ensure they handle the new return type
ast-grep --pattern 'insertResourceVariables($$$)'
Length of output: 151
Script:
#!/bin/bash
# Let's check how the result is used in upsert.ts
cat packages/job-dispatch/src/resource/upsert.ts
Length of output: 5185
| const envVariableChangePromises = envsAfterInsert.map((env) => | ||
| dispatchJobsForAddedResources( | ||
| tx, | ||
| env.resources | ||
| .filter((r) => updatedVariableResourceIds.has(r.id)) | ||
| .map((r) => r.id), | ||
| env.id, | ||
| ), | ||
| ); | ||
| await Promise.all(envVariableChangePromises); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure proper handling when no resources have updated variables
The code dispatches jobs for resources with updated variables:
const envVariableChangePromises = envsAfterInsert.map((env) =>
dispatchJobsForAddedResources(
tx,
env.resources
.filter((r) => updatedVariableResourceIds.has(r.id))
.map((r) => r.id),
env.id,
),
);
await Promise.all(envVariableChangePromises);If updatedVariableResourceIds is empty, dispatchJobsForAddedResources may be called with an empty array of resource IDs. Confirm that dispatchJobsForAddedResources correctly handles an empty array to avoid unnecessary processing or potential errors.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/job-dispatch/src/resource/dispatch-resource.ts (1)
58-58: LGTM! Consider adding debug logging for early return.The early return optimization is a good addition that prevents unnecessary processing when no resources are provided.
Consider adding a debug log before the early return to maintain consistent logging throughout the function:
+ log.debug("No resources to dispatch, returning early"); if (resourceIds.length === 0) return;
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
apps/webservice/src/app/api/v1/resources/[resourceId]/route.ts (2)
Line range hint
44-47: Remove redundant.optional()calls in schema definitionThe schema fields have unnecessary double
.optional()calls which should be simplified.- name: z.string().optional().optional(), - version: z.string().optional().optional(), - kind: z.string().optional().optional(), - identifier: z.string().optional().optional(), + name: z.string().optional(), + version: z.string().optional(), + kind: z.string().optional(), + identifier: z.string().optional(),
96-97: Consider adding error handling for variable updatesGiven that
upsertResourcesnow handles variable changes and job dispatching differently, consider adding specific error handling for these scenarios.- const { all } = await upsertResources(db, [_.merge(resource, body)]); + try { + const { all } = await upsertResources(db, [_.merge(resource, body)]); + } catch (error) { + console.error('Failed to update resource:', error); + return NextResponse.json( + { error: 'Failed to update resource variables' }, + { status: 500 } + ); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
apps/webservice/src/app/api/v1/resources/[resourceId]/route.ts(1 hunks)
🔇 Additional comments (1)
apps/webservice/src/app/api/v1/resources/[resourceId]/route.ts (1)
96-99: Verify upsertResources response structure change
The change from updated to all property reflects an API change in the upsertResources function. The null check is a good addition for type safety.
Let's verify this change is consistent across the codebase:
Consider adding a more descriptive error message:
- if (res == null) throw new Error("Failed to update resource");
+ if (res == null) throw new Error(`Failed to update resource ${params.resourceId}`);✅ Verification successful
Let me verify the usage of upsertResources response handling across the codebase.
Let me check how the other files handle the response when not destructuring.
Change from updated to all property is consistently implemented
The codebase shows consistent usage of the all property from upsertResources response in files that need the result data. Two files explicitly destructure { all } while others either don't use the return value or store it directly. The null check is appropriately added where single resource access is needed.
apps/webservice/src/app/api/v1/resources/[resourceId]/route.ts: Uses{ all }with null checkapps/pty-proxy/src/controller/agent-socket.ts: Uses{ all }consistently- Other files either don't use the return value or store it directly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of upsertResources to ensure consistent handling
rg -A 3 "upsertResources\(" --type ts
Length of output: 1867
Script:
#!/bin/bash
# Let's check the implementation of these files to see how they handle the response
ast-grep --pattern 'const { $_ } = await upsertResources($$$)'
Length of output: 1076
Script:
#!/bin/bash
# Get more context around the other upsertResources calls
rg -B 2 -A 5 "await upsertResources\(" --type ts
Length of output: 3300
| .filter( | ||
| (id) => | ||
| !deletedResourceIds.has(id) && | ||
| !updatedVariableResourceIds.has(id), | ||
| ); |
There was a problem hiding this comment.
shouldn't we be doing this in the addedResources section?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/job-dispatch/src/resource/upsert.ts (2)
83-92: Add an early return optimization for empty variable updatesWhile the parallel dispatch logic is good, we should add an early return to avoid unnecessary processing when there are no updated variables.
Consider adding this optimization:
+ if (updatedVariableResourceIds.size === 0) { + log.debug("No resources with updated variables, skipping dispatch"); + return; + } const envVariableChangePromises = envsAfterInsert.map((env) => dispatchJobsForAddedResources( tx, env.resources .filter((r) => updatedVariableResourceIds.has(r.id)) .map((r) => r.id), env.id, ), ); await Promise.all(envVariableChangePromises);
Line range hint
26-155: Consider implementing a state machine for resource lifecycle managementThe current implementation handles multiple state transitions (variable updates, additions, removals) with implicit sequencing. Consider implementing a state machine pattern to:
- Make the resource lifecycle states explicit
- Ensure transitions are handled consistently
- Improve debugging and monitoring
- Make the code more maintainable
This would help in tracking which resources are in which state and why certain jobs are being dispatched.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
packages/job-dispatch/src/resource/upsert.ts(3 hunks)
🔇 Additional comments (2)
packages/job-dispatch/src/resource/upsert.ts (2)
65-68: LGTM: Clean destructuring of Promise results
The destructuring correctly captures the metadata insertion result and the set of updated variable resource IDs, which is used effectively in the subsequent operations.
123-125: LGTM: Prevents duplicate job dispatches
The filter correctly excludes resources that already had jobs dispatched due to variable updates, preventing duplicate processing. This answers @jsbroks's question - we handle variable updates separately to ensure proper sequencing and avoid double-dispatching jobs.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
PATCHmethod response structure for better resource update handling.