fix: Check if removed from entire system #226
Conversation
WalkthroughThe pull request introduces several modifications 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: 0
🧹 Outside diff range and nitpick comments (3)
packages/job-dispatch/src/resource/dispatch-resource.ts (3)
161-182: Consider adding a log message when no filters are foundIn the
getNotInSystemFilterfunction, iffilters.length === 0, the function returnsnullwithout any log message. For better debugging and traceability, consider adding a log statement to indicate that no applicable filters were found for the system.Apply this diff to add the log message:
if (filters.length === 0) return null; + log.info("No resource filters found in environments for system", { systemId });
205-208: Refine log message for clarity when 'notInSystemFilter' is nullThe log message
"No system found for environment"may be misleading whennotInSystemFilterisnullbecause it could be due to the absence of applicable filters rather than the system not being found. To improve clarity, consider updating the log message to reflect the actual reason.Apply this diff to update the log message:
if (notInSystemFilter == null) { - log.warn("No system found for environment", { envId }); + log.warn("No applicable resource filters found for system", { envId, systemId }); return; }
193-196: Ensure consistent parameter documentationIn the
dispatchEventsForRemovedResourcesfunction signature, ensure that the JSDoc comments accurately reflect the updated parameters. The parameterenvshould be documented, and any outdated references toenvIdshould be updated.Apply this diff to update the documentation:
/** * Dispatches hook events for resources that were removed from an environment * @param db - Database transaction * @param resourceIds - IDs of the resources that were removed - * @param envId - ID of the environment the resources were removed from + * @param env - Object containing the environment ID and system ID */ export const dispatchEventsForRemovedResources = async (
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
packages/job-dispatch/src/resource/dispatch-resource.ts(3 hunks)packages/job-dispatch/src/resource/upsert.ts(1 hunks)packages/job-dispatch/src/resource/utils.ts(1 hunks)
🔇 Additional comments (2)
packages/job-dispatch/src/resource/utils.ts (1)
80-80: LGTM! The addition of systemId supports system-wide resource removal checks.
The change to include systemId in the environment selection is aligned with the PR's objective of checking if resources are removed from the entire system.
Let's verify how this systemId is used in related files:
✅ Verification successful
LGTM! The systemId addition is correctly integrated with the system-wide resource removal checks.
The search results confirm that the systemId is properly used in the resource removal workflow:
- In
dispatch-resource.ts, it's used bygetNotInSystemFilterto check if resources are removed from the entire system - The
dispatchEventsForRemovedResourcesfunction uses this systemId to verify system-wide resource removal before dispatching events - The change is integrated with environment queries and joins across the codebase, maintaining data consistency
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of systemId in resource removal logic
# Expected: Find references showing how systemId is used to check system-wide resource removal
# Search for functions that use systemId in resource removal context
rg -A 5 "systemId.*resource.*remov"
# Search for the dispatchEventsForRemovedResources function that was updated
ast-grep --pattern 'function dispatchEventsForRemovedResources($_) {
$$$
}'
Length of output: 117
Script:
#!/bin/bash
# Let's try a broader search to understand the context better
# Search for systemId usage in the job-dispatch package
rg -A 5 "systemId" packages/job-dispatch/
# Search for functions related to resource removal
rg -A 5 "resource.*remov|remov.*resource" packages/job-dispatch/
# Look for the getNotInSystemFilter function mentioned in the PR summary
ast-grep --pattern 'function getNotInSystemFilter($_) {
$$$
}'
Length of output: 20567
packages/job-dispatch/src/resource/upsert.ts (1)
128-128: LGTM! The change aligns with the PR objective.
The update to pass the full environment object instead of just the ID enables checking if resources are removed from the entire system.
Let's verify the integration with related changes:
✅ Verification successful
Function signature and usage are correctly aligned across the codebase
The verification confirms:
- The
dispatchEventsForRemovedResourcesfunction correctly expects the full environment object with bothidandsystemIdproperties - The function call in
upsert.tspasses the completeenvobject - The environment-related utilities in
utils.tshandle thesystemIdfield
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the function signature change and its usage
# Expected: The function should accept full environment object in all implementations
# Check the function signature in dispatch-resource.ts
rg -A 2 "dispatchEventsForRemovedResources.*=.*async"
# Check all calls to this function
rg "dispatchEventsForRemovedResources\("
# Verify environment query includes systemId
rg "getEnvironmentsByResourceWithIdentifiers.*select"
Length of output: 531
Script:
#!/bin/bash
# Let's check the complete function signature and implementation
ast-grep --pattern 'const dispatchEventsForRemovedResources = async ($$$) {
$$$
}'
# Check the environment parameter type and usage
rg -B 2 -A 5 "dispatchEventsForRemovedResources.*=.*async"
# Check environment-related queries in utils
fd utils.ts --exec rg -l "systemId.*environment"
Length of output: 950
Summary by CodeRabbit
New Features
Bug Fixes
Documentation