fix: only insert release different from latest #493
Conversation
WalkthroughThe code in the event worker responsible for evaluating release targets was updated. The database query fetching a release target now eagerly loads the latest associated version and variable releases, each limited to the most recent entry. The logic then compares these latest releases with the current releases determined by evaluation handlers. If both match, the function exits early, skipping further actions. Additionally, the insertion of new release records has been modified to remove the conflict-avoidance clause, meaning duplicate inserts are no longer automatically ignored. Changes
Suggested reviewers
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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
|
adityachoudhari26
changed the title
![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
🧹 Nitpick comments (1)
apps/event-worker/src/workers/evaluate-release-target.ts (1)
186-198: Consider narrowing eager‑loaded relations to reduce query overhead
resource,environment, anddeploymentare loaded in full but are never referenced later in this worker. Fetching the whole objects on every evaluation adds avoidable round‑trip time and JSON parsing cost, especially with the worker runningconcurrency: 10.If you only need their IDs (or nothing at all) trim the projection, e.g.:
- with: { - resource: true, - environment: true, - deployment: true, + with: { + resource: { columns: { id: true, workspaceId: true } }, // or remove if unusedLeaner queries will keep the hot‑path tight without affecting behaviour.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/event-worker/src/workers/evaluate-release-target.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/event-worker/src/workers/evaluate-release-target.ts
🧬 Code Graph Analysis (1)
apps/event-worker/src/workers/evaluate-release-target.ts (1)
packages/db/src/schema/release.ts (2)
releaseTarget(20-42)versionRelease(46-60)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (1)
apps/event-worker/src/workers/evaluate-release-target.ts (1)
186-198: Verify the relation alias for variable releasesThe query orders on
schema.variableSetRelease.createdAtwhile accessing the relation viavariableReleases.
If the relation is actually named differently (e.g.variableSetReleases) the ORM will return an empty array, making the downstream equality check always fail and producing redundant inserts.Please confirm the relation name in the generated schema and adjust the key if needed.
| const isSameVersionRelease = | ||
| releaseTarget.versionReleases.at(0)?.id === versionRelease.id; | ||
| const isSameVariableRelease = | ||
| releaseTarget.variableReleases.at(0)?.id === variableRelease.id; | ||
| if (isSameVersionRelease && isSameVariableRelease) return; | ||
|
|
There was a problem hiding this comment.
Guard against null variableRelease / versionRelease before dereferencing .id
handleVariableRelease does not explicitly guarantee a non‑null return.
If it yields null, the comparison below throws:
const isSameVariableRelease =
releaseTarget.variableReleases.at(0)?.id === variableRelease.id;A defensive rewrite prevents a runtime TypeError:
-const isSameVariableRelease =
- releaseTarget.variableReleases.at(0)?.id === variableRelease.id;
+const isSameVariableRelease =
+ variableRelease != null &&
+ releaseTarget.variableReleases.at(0)?.id === variableRelease.id;
+const isSameVersionRelease =
+ versionRelease != null &&
+ releaseTarget.versionReleases.at(0)?.id === versionRelease.id;Applying the same guard to versionRelease keeps the logic symmetrical and crash‑safe.
There was a problem hiding this comment.
this is wrong
There was a problem hiding this comment.
@adityachoudhari26 You're absolutely right, and I apologize for my incorrect suggestion. Looking more carefully at the code:
- There's already an explicit check
if (versionRelease == null)that returns early before reaching the comparisons - The
handleVariableRelease()function doesn't appear to have code paths that would return null
My suggestion to add null checks was unnecessary and would have added redundant code. Thank you for the correction.
…nt from latest
Summary by CodeRabbit