fix: Use exists for job filtering #238
Conversation
WalkthroughThe pull request introduces significant 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 (6)
packages/db/src/schema/job.ts (6)
227-238: Refactor to reduce code duplication in condition buildingThe
existsqueries for differentJobFilterTypeconditions have similar patterns, especially with the selection and joining of tables. Consider extracting common query logic into helper functions to improve maintainability and readability.
234-237: Remove trailing comma in function callThere's a trailing comma after the last argument in the
and()function, which could lead to syntax issues or be against coding standards.Apply this diff to remove the trailing comma:
and( eq(release.deploymentId, cond.value), eq(releaseJobTrigger.jobId, job.id), - ), + )
246-248: Remove trailing comma in function callThere's a trailing comma after the last argument in the
and()function.Apply this diff:
and( eq(releaseJobTrigger.environmentId, cond.value), eq(releaseJobTrigger.jobId, job.id), - ), + )
251-260: Extract version condition logic into a helper functionSimilar to how
buildMetadataConditionandbuildCreatedAtConditionare separate functions, consider extracting theexistsquery forFilterType.Versioninto its own helper function to improve readability and reuse.
268-271: Remove trailing comma in function callThere's a trailing comma after the last argument in the
and()function.Apply this diff:
and( eq(releaseJobTrigger.jobId, job.id), eq(releaseJobTrigger.resourceId, cond.value), isNull(resource.deletedAt), - ), + )
275-286: Consider consistent use of joins forJobFilterType.ReleaseIn other conditions, there's an
innerJoinwith related tables (e.g.,release,resource). For consistency and potential future requirements, consider joining thereleasetable here as well.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
packages/db/src/schema/job.ts(1 hunks)
🔇 Additional comments (3)
packages/db/src/schema/job.ts (3)
262-274: Handle resource deletion status appropriately
When checking isNull(resource.deletedAt), confirm that this condition aligns with how deleted resources are managed in the system. This ensures that only active resources are included in the query results.
240-250:
Verify existence of environmentId in releaseJobTrigger
Ensure that the releaseJobTrigger table contains the environmentId field. If it doesn't, this query will fail or not behave as expected.
Run the following script to verify the environmentId column in releaseJobTrigger:
✅ Verification successful
environmentId field exists and is properly defined
The releaseJobTrigger table does contain the environmentId field, which is defined as a UUID column with a foreign key reference to the environment.id field and includes cascade deletion. The query in the code will work as expected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if `environmentId` exists in `releaseJobTrigger` table definition.
# Test: Search for `environmentId` in `releaseJobTrigger` table definition.
ast-grep --pattern $'export const releaseJobTrigger = pgTable($_, {
$$$,
environmentId: $_,
$$$
});'
Length of output: 107
Script:
#!/bin/bash
# Let's try a different approach to find the table definition
# First, let's find the file containing releaseJobTrigger definition
rg "releaseJobTrigger.*=.*pgTable" -l
# Then let's see the actual table definition
rg "releaseJobTrigger.*=.*pgTable" -A 20
Length of output: 1496
258-258: Ensure correct scope in buildVersionCondition
Verify that buildVersionCondition(cond) correctly references release.version. If release is not in scope within buildVersionCondition, there might be a reference error.
Run the following script to check the usage of release.version inside buildVersionCondition:
✅ Verification successful
Let me verify if release is properly defined and in scope. I'll check for its definition in both files.
Let me check for the table definition of release to ensure it's properly imported and defined.
Based on the verification results, I can now provide the final response:
buildVersionCondition correctly references release.version
The verification shows that:
- The
releasetable is properly imported injob.tsfrom./release.js buildVersionConditioncorrectly usesrelease.versionin its implementation- The
releasetable is properly defined as a pgTable inrelease.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Confirm `release.version` is used within `buildVersionCondition`.
# Test: Display the definition of `buildVersionCondition`.
rg -A 10 $'^const buildVersionCondition ='
# Expected: The function uses `release.version` in its conditions.
Length of output: 1786
Script:
#!/bin/bash
# Check for the definition of 'release' table/variable in both files
rg -B 5 "release\." packages/db/src/schema/{job,release}.ts
Length of output: 6695
Script:
#!/bin/bash
# Check for the release table definition
rg -B 5 "^export const release = " packages/db/src/schema/release.ts
Length of output: 164
Summary by CodeRabbit
New Features
Bug Fixes