fix: sync batch should not trigger some PR post-sync events#85
fix: sync batch should not trigger some PR post-sync events#85waltergalvao merged 1 commit intomainfrom
Conversation
WalkthroughRefactors the GitHub sync pull request worker to use strongly-typed JobData, introduces a handlePostSyncActions helper function, adds node_id validation, implements DataIntegrityException for critical errors, and adjusts control flow based on syncBatchId presence. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/api/src/app/github/workers/github-sync-pull-request.worker.ts (1)
73-85: RedundantinstallationIdguard, and helper defined after its call site.Two minor points:
handlePostSyncActionsis aconstdefined at line 73 but referenced at line 67 inside the worker callback. This is safe at runtime (the module is fully initialised before any job is processed), but it is unconventional — hoisting the helper abovesyncPullRequestWorkeris the standard ordering and easier to follow.The
installationIdnull-check at lines 80-85 is unreachable from the current call path:installation.idis already validated at lines 29-34, and the worker throws before ever calling this helper. The guard is only meaningful ifhandlePostSyncActionsis called from a future site that skips the worker-level validation. A brief comment (// defensive: called from contexts that may not pre-validate) or removing it would clarify intent.♻️ Suggested ordering fix
-export const syncPullRequestWorker = createWorker( - SweetQueue.GITHUB_SYNC_PULL_REQUEST, - async (job: Job<JobData>, token?: string) => { - ... - if (pullRequest) { - await handlePostSyncActions(job.data, pullRequest); - } - }, - { limiter: { max: 8, duration: 1000 } } -); - const handlePostSyncActions = async ( jobData: JobData, pullRequest: PullRequest ) => { ... }; + +export const syncPullRequestWorker = createWorker( + SweetQueue.GITHUB_SYNC_PULL_REQUEST, + async (job: Job<JobData>, token?: string) => { + ... + if (pullRequest) { + await handlePostSyncActions(job.data, pullRequest); + } + }, + { limiter: { max: 8, duration: 1000 } } +);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/github/workers/github-sync-pull-request.worker.ts` around lines 73 - 85, Move the helper handlePostSyncActions above syncPullRequestWorker to follow conventional ordering and improve readability, and then remove the redundant runtime guard that throws when installationId is missing (the worker already validates installation.id before calling this helper); if you want to retain a safety note instead of removing the check, replace the throw with a short defensive comment like "// defensive: called from contexts that may not pre-validate" next to the installationId extraction (jobData.installation?.id) so intent is clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/api/src/app/github/workers/github-sync-pull-request.worker.ts`:
- Around line 73-85: Move the helper handlePostSyncActions above
syncPullRequestWorker to follow conventional ordering and improve readability,
and then remove the redundant runtime guard that throws when installationId is
missing (the worker already validates installation.id before calling this
helper); if you want to retain a safety note instead of removing the check,
replace the throw with a short defensive comment like "// defensive: called from
contexts that may not pre-validate" next to the installationId extraction
(jobData.installation?.id) so intent is clear.
Greptile Summary
Refactored the PR sync worker to conditionally skip certain automation jobs when processing PRs as part of a batch sync operation. The code extracts post-sync actions into a separate
handlePostSyncActionsfunction that checks for the presence ofsyncBatchIdand skipsAUTOMATION_PR_SIZE_LABELERandALERT_MERGED_WITHOUT_APPROVALjobs during batch syncs.Key changes:
handlePostSyncActionsfunction for better code organizationAUTOMATION_PR_SIZE_LABELERwhensyncBatchIdis presentALERT_MERGED_WITHOUT_APPROVALwhensyncBatchIdis presentDataIntegrityExceptionfor missing installation ID in post-sync actionsJobDatatype alias to simplify type annotationsConfidence Score: 5/5
Important Files Changed
Last reviewed commit: daf76c1