fix(github): preview limit check should not block updates to existing previews#4139
Open
mixelburg wants to merge 2 commits intoDokploy:canaryfrom
Open
fix(github): preview limit check should not block updates to existing previews#4139mixelburg wants to merge 2 commits intoDokploy:canaryfrom
mixelburg wants to merge 2 commits intoDokploy:canaryfrom
Conversation
… fails
When the deployment worker catches an error, the service status was left
permanently in 'running' state because the catch block only logged the
error without rolling back the status set at the beginning of the job.
Add status rollback in the catch block for all three applicationType
branches (application, compose, application-preview). Each rollback call
is wrapped in .catch(() => {}) so that a secondary DB failure cannot mask
the original error in the logs.
… previews The limit check in the GitHub webhook handler was running before findPreviewDeploymentByApplicationId(), so pushing a commit to an open PR whose app had already reached the preview limit would silently drop the deployment even though no new preview was being created. Moved the limit check inside the 'if (!previewDeploymentResult && shouldCreateDeployment)' block so it only fires when a NEW preview would be created. Existing previews always rebuild on push. Also fixed two secondary issues noted in Dokploy#4074: - The default for previewLimit was 0 (via || 0) instead of the DB schema default of 3; changed to ?? 3 so a configured value of 0 is respected. - Changed > to >= so the limit is enforced at exactly previewLimit previews, not previewLimit + 1.
dosubot
bot
added
size:S
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4074
What happened
The preview deployment limit check in the GitHub webhook handler ran before
findPreviewDeploymentByApplicationId(). Pushing a commit to an open PR whose application had already reached its preview limit would silently drop the rebuild event — the deployment was never queued even though no new preview was being created.Fix
Moved the limit check inside the
if (!previewDeploymentResult && shouldCreateDeployment)block so it only fires when a new preview deployment would be created. Updates to existing previews (i.e. new commits pushed to already-open PRs) always proceed regardless of the limit.Also fixed two secondary issues noted in the bug report:
const previewLimit = app?.previewLimit || 0was defaulting to0when the app's configured limit was falsy. The DB schema default is3. Changed to?? 3so a deliberately configured value of0is also respected.>to>=so the limit is enforced at exactlypreviewLimitpreviews rather than allowing one extra beyond the limit.Greptile Summary
This PR fixes two related bugs in the GitHub webhook preview deployment flow: (1) the preview limit check was running before the existing-preview lookup, blocking updates to already-open PRs, and (2) the deployment worker's catch block never updated statuses to
"error"on failure.The restructuring in
github.tsis logically correct — moving the limit guard insideif (!previewDeploymentResult && shouldCreateDeployment)ensures only new preview creation is gated by the limit, while commits to existing previews always proceed. The?? 3/>=corrections are also accurate fixes.Confidence Score: 5/5
This PR is safe to merge — both changes are targeted, correct fixes for well-described bugs with no new risk introduced.
All three sub-fixes (limit-check relocation,
?? 3default,>=comparison) are logically sound, and the new error-status handling in the worker's catch block mirrors the existing try-block pattern. No new logic paths or edge cases are introduced that could cause regressions.No files require special attention.
Reviews (1): Last reviewed commit: "fix(github): preview limit check should ..." | Re-trigger Greptile