Add previewUrl to vcs comment from vcs controller #10396
Conversation
📝 WalkthroughWalkthroughAdds distributed locking in app/controllers/api/vcs.php for GitHub VCS comment updates by creating Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PHPStan (2.1.17)Note: Using configuration file /phpstan.neon. If the excluded path can sometimes exist, append (?) parameters: ✨ Finishing Touches
🧪 Generate unit tests
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
![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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
app/controllers/api/vcs.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/controllers/api/vcs.php (2)
src/Appwrite/Event/Database.php (2)
getCollection(137-140)getDocument(159-162)src/Appwrite/Vcs/Comment.php (4)
Comment(10-226)parseComment(214-225)addBuild(31-48)generateComment(50-199)
🔇 Additional comments (1)
app/controllers/api/vcs.php (1)
274-274: Confirm intended preview rule source (deployment vs branch/commit).Right now
previewRuleIdpoints to the deployment preview rule. Is that the URL we want to surface in PR comments over the branch/commit preview rules when those exist? If branch/commit previews are preferred, we should derivepreviewRuleIdaccordingly.
✨ Benchmark results
⚡ Benchmark Comparison
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (2)
1560-1580: Preview URL derivation: add guard for empty _APP_DOMAIN_SITES, simplify rule check, keep logic minimal
- Guard missing _APP_DOMAIN_SITES to avoid computing invalid domains.
empty($branchRule)is redundant for Documents;isEmpty()suffices.Apply this diff:
- $sitesDomain = System::getEnv('_APP_DOMAIN_SITES', ''); + $sitesDomain = System::getEnv('_APP_DOMAIN_SITES', ''); + if ($sitesDomain === '') { + return ''; + } @@ - $branchRule = Authorization::skip(fn () => $dbForPlatform->getDocument('rules', $ruleId)); - - if (!empty($branchRule) && !$branchRule->isEmpty()) { + $branchRule = Authorization::skip(fn () => $dbForPlatform->getDocument('rules', $ruleId)); + if (!$branchRule->isEmpty()) { return "{$protocol}://" . $branchRule->getAttribute('domain', ''); }
1564-1571: Normalize branch to a DNS-safe label and de-duplicate algorithm across code pathsBranch names can contain chars not valid in host labels (e.g.,
/,_, uppercase). Recommend:
- Centralize the “branch prefix + 7-char hash” algorithm into a helper to prevent drift with the rule-creation code in this file and in app/controllers/api/vcs.php.
- Normalize to lowercase and replace disallowed chars with
-, collapsing repeats.Example helper to add to this class (and use for both rule creation and lookup):
private function deriveBranchPrefix(string $branch): string { $prefix = substr($branch, 0, 16); if (strlen($branch) > 16) { $remaining = substr($branch, 16); $prefix .= '-' . substr(hash('sha256', $remaining), 0, 7); } // Normalize to DNS-safe label $label = strtolower(preg_replace('/[^a-z0-9-]/', '-', $prefix)); $label = trim(preg_replace('/-+/', '-', $label), '-'); return $label; }Then here:
- $branchPrefix = substr($providerBranch, 0, 16); - if (strlen($providerBranch) > 16) { - $remainingChars = substr($providerBranch, 16); - $branchPrefix .= '-' . substr(hash('sha256', $remainingChars), 0, 7); - } + $branchPrefix = $this->deriveBranchPrefix($providerBranch);Note: apply the same helper where the preview rule is created to avoid mismatched IDs/domains.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (1)
src/Appwrite/Event/Database.php (1)
getDocument(159-162)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Tokens)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (VCS)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (Migrations)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: Unit Test
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: E2E General Test
- GitHub Check: Benchmark
- GitHub Check: scan
🔇 Additional comments (1)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (1)
1558-1583: LGTM on adding previewUrl into the GitHub comment flowUsing the comment lock + updating comment with
addBuild(..., $previewUrl)is a solid integration. With the small nits above, this is good to ship.
| 'sites' => (function () use ($deployment, $project, $dbForPlatform, $protocol, $resource) { | ||
| $providerBranch = $deployment->getAttribute('providerBranch', ''); | ||
| if (!empty($providerBranch)) { | ||
| $sitesDomain = System::getEnv('_APP_DOMAIN_SITES', ''); | ||
| $branchPrefix = substr($providerBranch, 0, 16); | ||
| if (strlen($providerBranch) > 16) { | ||
| $remainingChars = substr($providerBranch, 16); | ||
| $branchPrefix .= '-' . substr(hash('sha256', $remainingChars), 0, 7); | ||
| } | ||
| $resourceProjectHash = substr(hash('sha256', $resource->getId() . $project->getId()), 0, 7); | ||
| $domain = "branch-{$branchPrefix}-{$resourceProjectHash}.{$sitesDomain}"; | ||
| $ruleId = md5($domain); | ||
|
|
||
| $branchRule = Authorization::skip(fn () => $dbForPlatform->getDocument('rules', $ruleId)); | ||
|
|
||
| if (!empty($branchRule) && !$branchRule->isEmpty()) { | ||
| return "{$protocol}://" . $branchRule->getAttribute('domain', ''); | ||
| } | ||
| } | ||
| return ''; | ||
| })(), |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify consistency of preview domain algorithm across codebase
Ensure the exact same derivation is used everywhere (rule creation, lookup here, and in VCS controller), or preview links may intermittently fail.
Run from repo root:
Expected:
- All derivations use the same normalization and hashing.
- No occurrences of
->foreach(; only->forEach(.
🏁 Script executed:
#!/bin/bash
# Find all occurrences of branch-based preview domain derivation and ruleId usage
rg -nP -C3 '(branch-|_APP_DOMAIN_SITES|deploymentVcsProviderBranch|md5\(\$domain\)|deriveBranchPrefix\()' --type=php
# Sanity-check any accidental use of `$dbForPlatform->foreach` (case typo) vs `forEach`
rg -nP '\->foreach\(' --type=php -C2Length of output: 61073
Replace all incorrect ->foreach( calls with ->forEach(
Search the codebase for ->foreach( and update each to the correct camel-case method.
🤖 Prompt for AI Agents
In src/Appwrite/Platform/Modules/Functions/Workers/Builds.php around lines 1560
to 1580, the review requests replacing incorrect method calls named "->foreach("
with the correct camel-case "->forEach("; search the repository for any
occurrences of "->foreach(" and update them to "->forEach(" (preserving the same
arguments and whitespace), run unit/static checks to ensure no other
casing-sensitive calls remain, and commit the changes.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/controllers/api/vcs.php (1)
366-382: Fix rule existence check to avoid posting an invalid "https://" preview URL.Document is truthy even when empty. Guard with isEmpty() and the domain value before composing previewUrl; also tweak the catch comment.
- if ($resource->getCollection() === 'sites' && !empty($latestCommentId) && !empty($previewRuleId)) { + if ($resource->getCollection() === 'sites' && !empty($latestCommentId) && !empty($previewRuleId)) { try { - $rule = Authorization::skip(fn () => $dbForPlatform->getDocument('rules', $previewRuleId)); - - $protocol = System::getEnv('_APP_OPTIONS_FORCE_HTTPS') === 'disabled' ? 'http' : 'https'; - $previewUrl = !empty($rule) ? ("{$protocol}://" . $rule->getAttribute('domain', '')) : ''; + $rule = Authorization::skip(fn () => $dbForPlatform->getDocument('rules', $previewRuleId)); + $protocol = System::getEnv('_APP_OPTIONS_FORCE_HTTPS') === 'disabled' ? 'http' : 'https'; + $domain = !$rule->isEmpty() ? $rule->getAttribute('domain', '') : ''; + $previewUrl = $domain !== '' ? "{$protocol}://{$domain}" : ''; - if (!empty($previewUrl)) { + if ($previewUrl !== '') { $comment = new Comment(); $comment->parseComment($github->getComment($owner, $repositoryName, $latestCommentId)); $comment->addBuild($project, $resource, $resourceType, $commentStatus, $deploymentId, $action, $previewUrl); $github->updateComment($owner, $repositoryName, $latestCommentId, $comment->generateComment()); } } catch (\Throwable $th) { - // Ignore, rule already exists; will be updated by builds worker + // Ignore; preview URL will be updated by the builds worker } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
app/controllers/api/vcs.php(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: Unit Test
- GitHub Check: Benchmark
- GitHub Check: scan
🔇 Additional comments (1)
app/controllers/api/vcs.php (1)
373-378: Comment::addBuild is already idempotent—entries are keyed byprojectId_resourceIdand use direct assignment, so repeated calls overwrite instead of appending duplicates.
| $previewRuleId = $ruleId; | ||
| Authorization::skip( |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Prefer branch preview domain for previewRuleId to keep URL stable across deployments.
Initialize previewRuleId, then set it to the branch rule when available; fall back to commit rule, then to the ephemeral deployment rule.
- $previewRuleId = $ruleId;
+ $previewRuleId = ''; // will prefer branch rule below, then commit, else fallback to this deployment ruleAdd the following in the Branch Preview block (after computing $ruleId = md5($domain); and also inside the Duplicate catch):
// after: $ruleId = md5($domain);
$previewRuleId = $ruleId;Add the following in the Commit Preview block (after computing $ruleId = md5($domain); and also inside its Duplicate catch):
// Prefer branch; only use commit if branch wasn't set
if (empty($previewRuleId)) {
$previewRuleId = $ruleId;
}🏁 Script executed:
#!/bin/bash
rg -n "previewRuleId" -C3 app/controllers/api/vcs.php
rg -n "md5" -C3 app/controllers/api/vcs.phpLength of output: 2337
🏁 Script executed:
#!/bin/bash
rg -nP --type=php 'branchPrefix' -C3 app/controllers/api/vcs.php
rg -nP --type=php 'VCS commit preview' -C3 app/controllers/api/vcs.php
rg -nP --type=php 'catch' -C5 app/controllers/api/vcs.phpLength of output: 10635
Prefer branch or commit preview rule before falling back to the ephemeral deployment rule
Refactor the $previewRuleId assignment so that branch previews override commit previews, which in turn override the initial deployment preview:
At the top of the Deployment preview block (around line 274), capture the fallback and reset for later:
- $previewRuleId = $ruleId;
+ $deploymentRuleId = $ruleId;
+ $previewRuleId = ''; // will be set below: branch > commit > deployment fallbackIn the Branch Preview section (after \$ruleId = md5($domain); and inside its catch (Duplicate $err)):
$previewRuleId = $ruleId;In the Commit Preview section (after \$ruleId = md5($domain); and inside its catch (Duplicate $err)):
if (empty($previewRuleId)) {
$previewRuleId = $ruleId;
}Immediately before constructing the preview URL (just before the if ($resource->getCollection() === 'sites'…) at line 366):
if (empty($previewRuleId)) {
$previewRuleId = $deploymentRuleId;
}This guarantees the preview URL always uses the branch rule if present, otherwise the commit rule, and finally the deployment rule.
🤖 Prompt for AI Agents
In app/controllers/api/vcs.php around lines 274-275, the preview rule selection
currently defaults to the deployment rule too early; refactor so branch preview
overrides commit preview which overrides deployment: at the top of the
Deployment preview block save the deployment rule id into a variable for
fallback, in the Branch Preview catch (after $ruleId = md5($domain);) assign
$previewRuleId = $ruleId to ensure branch wins, in the Commit Preview catch
(after $ruleId = md5($domain);) set $previewRuleId = $ruleId only if
$previewRuleId is empty so commits only set when no branch exists, and
immediately before building the preview URL set $previewRuleId =
$deploymentRuleId if it is still empty so deployment is the final fallback.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/controllers/api/vcs.php (1)
415-451: Guard preview URL construction: avoid posting "https://" without a domain!empty($rule) is always true for Document; check isEmpty() and ensure domain exists before composing.
Apply:
- try { - $rule = Authorization::skip(fn () => $dbForPlatform->getDocument('rules', $previewRuleId)); - - $protocol = System::getEnv('_APP_OPTIONS_FORCE_HTTPS') === 'disabled' ? 'http' : 'https'; - $previewUrl = !empty($rule) ? ("{$protocol}://" . $rule->getAttribute('domain', '')) : ''; + try { + $rule = Authorization::skip(fn () => $dbForPlatform->getDocument('rules', $previewRuleId)); + + $protocol = System::getEnv('_APP_OPTIONS_FORCE_HTTPS') === 'disabled' ? 'http' : 'https'; + $domain = $rule->isEmpty() ? '' : $rule->getAttribute('domain', ''); + $previewUrl = $domain !== '' ? "{$protocol}://{$domain}" : '';
🧹 Nitpick comments (2)
app/controllers/api/vcs.php (2)
323-323: Choose a stable preview URL: prefer branch rule, then commit, then deployment fallbackRight now previewRuleId is set to the ephemeral deployment rule; per discussion, branch URLs are preferable (Netlify/Vercel-style), with commit as secondary and deployment as fallback.
Apply at deployment preview creation:
- $previewRuleId = $ruleId; + $deploymentRuleId = $ruleId; // fallback only + $previewRuleId = ''; // will prefer branch > commit > deploymentThen:
- In the Branch Preview block, after computing $ruleId: set $previewRuleId = $ruleId;
- In the Commit Preview block, set if (empty($previewRuleId)) { $previewRuleId = $ruleId; }
- Just before using $previewRuleId (see Line 415 block), fallback: if (empty($previewRuleId)) { $previewRuleId = $deploymentRuleId; }
415-451: Ensure previewRuleId fallback is applied before fetching the ruleIf branch/commit didn’t set previewRuleId, fall back to the deploymentRuleId before getDocument.
Apply:
- try { + try { + if (empty($previewRuleId) && !empty($deploymentRuleId)) { + $previewRuleId = $deploymentRuleId; + } $rule = Authorization::skip(fn () => $dbForPlatform->getDocument('rules', $previewRuleId));
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
app/controllers/api/vcs.php(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/controllers/api/vcs.php (2)
src/Appwrite/Vcs/Comment.php (4)
Comment(10-226)parseComment(214-225)addBuild(31-48)generateComment(50-199)src/Appwrite/Platform/Modules/Proxy/Http/Rules/API/Create.php (1)
action(72-199)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Benchmark
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
| $retries = 0; | ||
|
|
||
| while (true) { | ||
| $retries++; | ||
|
|
||
| try { | ||
| $dbForPlatform->createDocument('vcsCommentLocks', new Document([ | ||
| '$id' => $latestCommentId | ||
| ])); | ||
| break; | ||
| } catch (\Throwable $err) { | ||
| if ($retries >= 9) { | ||
| throw $err; | ||
| } | ||
|
|
||
| \sleep(1); | ||
| } | ||
| } | ||
|
|
||
| // Wrap in try/finally to ensure lock file gets deleted | ||
| try { | ||
| $comment = new Comment(); | ||
| $comment->parseComment($github->getComment($owner, $repositoryName, $latestCommentId)); | ||
| $comment->addBuild($project, $resource, $resourceType, $commentStatus, $deploymentId, $action, ''); | ||
|
|
||
| $latestCommentId = \strval($github->updateComment($owner, $repositoryName, $latestCommentId, $comment->generateComment())); | ||
| $latestCommentId = \strval($github->updateComment($owner, $repositoryName, $latestCommentId, $comment->generateComment())); | ||
| } finally { | ||
| $dbForPlatform->deleteDocument('vcsCommentLocks', $latestCommentId); | ||
| } |
There was a problem hiding this comment.
Fix lock-release bug: deleting the wrong lock ID after updateComment mutates latestCommentId
updateComment may return a different ID. You overwrite latestCommentId, then attempt to delete the lock with the new value, leaking the original lock and blocking future updates.
Apply:
- // Wrap in try/finally to ensure lock file gets deleted
+ // Wrap in try/finally to ensure lock doc gets deleted
try {
+ $lockedCommentId = $latestCommentId; // keep original for lock cleanup
$comment = new Comment();
$comment->parseComment($github->getComment($owner, $repositoryName, $latestCommentId));
$comment->addBuild($project, $resource, $resourceType, $commentStatus, $deploymentId, $action, '');
- $latestCommentId = \strval($github->updateComment($owner, $repositoryName, $latestCommentId, $comment->generateComment()));
+ $github->updateComment($owner, $repositoryName, $latestCommentId, $comment->generateComment());
} finally {
- $dbForPlatform->deleteDocument('vcsCommentLocks', $latestCommentId);
+ try { $dbForPlatform->deleteDocument('vcsCommentLocks', $lockedCommentId); } catch (\Throwable $e) { /* no-op */ }
}Optional: guard against empty IDs to avoid creating a lock with an empty key.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $retries = 0; | |
| while (true) { | |
| $retries++; | |
| try { | |
| $dbForPlatform->createDocument('vcsCommentLocks', new Document([ | |
| '$id' => $latestCommentId | |
| ])); | |
| break; | |
| } catch (\Throwable $err) { | |
| if ($retries >= 9) { | |
| throw $err; | |
| } | |
| \sleep(1); | |
| } | |
| } | |
| // Wrap in try/finally to ensure lock file gets deleted | |
| try { | |
| $comment = new Comment(); | |
| $comment->parseComment($github->getComment($owner, $repositoryName, $latestCommentId)); | |
| $comment->addBuild($project, $resource, $resourceType, $commentStatus, $deploymentId, $action, ''); | |
| $latestCommentId = \strval($github->updateComment($owner, $repositoryName, $latestCommentId, $comment->generateComment())); | |
| $latestCommentId = \strval($github->updateComment($owner, $repositoryName, $latestCommentId, $comment->generateComment())); | |
| } finally { | |
| $dbForPlatform->deleteDocument('vcsCommentLocks', $latestCommentId); | |
| } | |
| // Wrap in try/finally to ensure lock doc gets deleted | |
| try { | |
| $lockedCommentId = $latestCommentId; // keep original for lock cleanup | |
| $comment = new Comment(); | |
| $comment->parseComment($github->getComment($owner, $repositoryName, $latestCommentId)); | |
| $comment->addBuild($project, $resource, $resourceType, $commentStatus, $deploymentId, $action, ''); | |
| // Perform the update but don’t overwrite $latestCommentId here | |
| $github->updateComment($owner, $repositoryName, $latestCommentId, $comment->generateComment()); | |
| } finally { | |
| // Clean up the original lock, ignore any delete errors | |
| try { | |
| $dbForPlatform->deleteDocument('vcsCommentLocks', $lockedCommentId); | |
| } catch (\Throwable $e) { | |
| /* no-op */ | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/controllers/api/vcs.php around lines 138 to 166, the code overwrites
$latestCommentId with the return from updateComment and then deletes a lock
using the new ID, leaking the original lock; preserve the original lock id in a
separate variable before calling updateComment (e.g. $lockId =
$latestCommentId), use that $lockId when creating and deleting the
'vcsCommentLocks' document inside the retry/finally blocks, and only overwrite
$latestCommentId with the updated value after the lock is deleted; additionally,
validate/guard against empty IDs when creating the lock (do not attempt to
create a lock if $lockId is empty, and handle that as an error).
| $retries = 0; | ||
|
|
||
| while (true) { | ||
| $retries++; | ||
|
|
||
| try { | ||
| $dbForPlatform->createDocument('vcsCommentLocks', new Document([ | ||
| '$id' => $latestCommentId | ||
| ])); | ||
| break; | ||
| } catch (\Throwable $err) { | ||
| if ($retries >= 9) { | ||
| throw $err; | ||
| } | ||
|
|
||
| \sleep(1); | ||
| } | ||
| } | ||
|
|
||
| // Wrap in try/finally to ensure lock file gets deleted | ||
| try { | ||
| $comment = new Comment(); | ||
| $comment->parseComment($github->getComment($owner, $repositoryName, $latestCommentId)); | ||
| $comment->addBuild($project, $resource, $resourceType, $commentStatus, $deploymentId, $action, ''); | ||
|
|
||
| $latestCommentId = \strval($github->updateComment($owner, $repositoryName, $latestCommentId, $comment->generateComment())); | ||
| } finally { | ||
| $dbForPlatform->deleteDocument('vcsCommentLocks', $latestCommentId); | ||
| } |
There was a problem hiding this comment.
Same lock-release bug in branch-comments loop
The same mutation of latestCommentId risks leaking the lock per iteration.
Apply:
- // Wrap in try/finally to ensure lock file gets deleted
+ // Wrap in try/finally to ensure lock doc gets deleted
try {
+ $lockedCommentId = $latestCommentId;
$comment = new Comment();
$comment->parseComment($github->getComment($owner, $repositoryName, $latestCommentId));
$comment->addBuild($project, $resource, $resourceType, $commentStatus, $deploymentId, $action, '');
- $latestCommentId = \strval($github->updateComment($owner, $repositoryName, $latestCommentId, $comment->generateComment()));
+ $github->updateComment($owner, $repositoryName, $latestCommentId, $comment->generateComment());
} finally {
- $dbForPlatform->deleteDocument('vcsCommentLocks', $latestCommentId);
+ try { $dbForPlatform->deleteDocument('vcsCommentLocks', $lockedCommentId); } catch (\Throwable $e) { /* no-op */ }
}Minor: consider small jitter in the retry loop to reduce thundering herd.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $retries = 0; | |
| while (true) { | |
| $retries++; | |
| try { | |
| $dbForPlatform->createDocument('vcsCommentLocks', new Document([ | |
| '$id' => $latestCommentId | |
| ])); | |
| break; | |
| } catch (\Throwable $err) { | |
| if ($retries >= 9) { | |
| throw $err; | |
| } | |
| \sleep(1); | |
| } | |
| } | |
| // Wrap in try/finally to ensure lock file gets deleted | |
| try { | |
| $comment = new Comment(); | |
| $comment->parseComment($github->getComment($owner, $repositoryName, $latestCommentId)); | |
| $comment->addBuild($project, $resource, $resourceType, $commentStatus, $deploymentId, $action, ''); | |
| $latestCommentId = \strval($github->updateComment($owner, $repositoryName, $latestCommentId, $comment->generateComment())); | |
| } finally { | |
| $dbForPlatform->deleteDocument('vcsCommentLocks', $latestCommentId); | |
| } | |
| // Wrap in try/finally to ensure lock doc gets deleted | |
| try { | |
| $lockedCommentId = $latestCommentId; | |
| $comment = new Comment(); | |
| $comment->parseComment($github->getComment($owner, $repositoryName, $latestCommentId)); | |
| $comment->addBuild($project, $resource, $resourceType, $commentStatus, $deploymentId, $action, ''); | |
| $github->updateComment($owner, $repositoryName, $latestCommentId, $comment->generateComment()); | |
| } finally { | |
| try { | |
| $dbForPlatform->deleteDocument('vcsCommentLocks', $lockedCommentId); | |
| } catch (\Throwable $e) { | |
| /* no-op */ | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
app/controllers/api/vcs.php (4)
332-334: Prefer branch (then commit) preview domain; fall back to deployment rule.Use stable branch preview URLs when available, else commit, else the ephemeral deployment rule. This keeps the PR comment’s preview link consistent across deployments.
Apply:
- $previewRuleId = $ruleId; + $deploymentRuleId = $ruleId; // fallback + $previewRuleId = ''; // will prefer branch > commit > deployment @@ // VCS branch preview if (!empty($providerBranch)) { @@ - try { + try { Authorization::skip( fn () => $dbForPlatform->createDocument('rules', new Document([ @@ ])) ); + $previewRuleId = $ruleId; // prefer branch domain } catch (Duplicate $err) { - // Ignore, rule already exists; will be updated by builds worker + $previewRuleId = $ruleId; // prefer branch domain } } @@ // VCS commit preview if (!empty($providerCommitHash)) { @@ - try { + try { Authorization::skip( fn () => $dbForPlatform->createDocument('rules', new Document([ @@ ])) ); + if (empty($previewRuleId)) { $previewRuleId = $ruleId; } } catch (Duplicate $err) { - // Ignore, rule already exists; will be updated by builds worker + if (empty($previewRuleId)) { $previewRuleId = $ruleId; } } } @@ - if ($resource->getCollection() === 'sites' && !empty($latestCommentId) && !empty($previewRuleId)) { + // final fallback to deployment rule + if (empty($previewRuleId ?? '')) { $previewRuleId = $deploymentRuleId; } + if ($resource->getCollection() === 'sites' && !empty($latestCommentId) && !empty($previewRuleId)) {Also applies to: 355-390, 392-421, 423-424
451-455: Guard against empty rule/domain to avoid posting an invalid “https://”.
!empty($rule)is always truthy for a Document. UseisEmpty()and verify domain is non-empty before composing the URL.Apply:
- $protocol = System::getEnv('_APP_OPTIONS_FORCE_HTTPS') === 'disabled' ? 'http' : 'https'; - $previewUrl = !empty($rule) ? ("{$protocol}://" . $rule->getAttribute('domain', '')) : ''; + $protocol = System::getEnv('_APP_OPTIONS_FORCE_HTTPS') === 'disabled' ? 'http' : 'https'; + $domain = !$rule->isEmpty() ? $rule->getAttribute('domain', '') : ''; + $previewUrl = $domain !== '' ? "{$protocol}://{$domain}" : '';
139-171: Fix lock-release bug and avoid mutating the lock key.
updateCommentmay return a different ID. You overwrite$latestCommentIdand then try to delete the lock using the new value, leaking the original lock and blocking future updates. Keep a stable$lockId, don’t reassign$latestCommentId, and always delete using$lockIdwith a safe try/catch.Apply:
- $retries = 0; + $retries = 0; + $lockId = $latestCommentId; $lockAcquired = false; @@ - $dbForPlatform->createDocument('vcsCommentLocks', new Document([ - '$id' => $latestCommentId + $dbForPlatform->createDocument('vcsCommentLocks', new Document([ + '$id' => $lockId ])); @@ - $comment = new Comment(); + $comment = new Comment(); $comment->parseComment($github->getComment($owner, $repositoryName, $latestCommentId)); $comment->addBuild($project, $resource, $resourceType, $commentStatus, $deploymentId, $action, ''); - - $latestCommentId = \strval($github->updateComment($owner, $repositoryName, $latestCommentId, $comment->generateComment())); + // Do not overwrite $latestCommentId; use a stable lock key. + $github->updateComment($owner, $repositoryName, $latestCommentId, $comment->generateComment()); } finally { - $dbForPlatform->deleteDocument('vcsCommentLocks', $latestCommentId); + try { if ($lockId !== '') { $dbForPlatform->deleteDocument('vcsCommentLocks', $lockId); } } catch (\Throwable $e) { /* no-op */ } }
210-242: Same lock key mutation/leak in branch-comments loop.Mirror the fix: preserve a
$lockId, don’t reassign$latestCommentId, and delete using$lockId.Apply:
- $retries = 0; + $retries = 0; + $lockId = $latestCommentId; $lockAcquired = false; @@ - $dbForPlatform->createDocument('vcsCommentLocks', new Document([ - '$id' => $latestCommentId + $dbForPlatform->createDocument('vcsCommentLocks', new Document([ + '$id' => $lockId ])); @@ - $comment = new Comment(); + $comment = new Comment(); $comment->parseComment($github->getComment($owner, $repositoryName, $latestCommentId)); $comment->addBuild($project, $resource, $resourceType, $commentStatus, $deploymentId, $action, ''); - - $latestCommentId = \strval($github->updateComment($owner, $repositoryName, $latestCommentId, $comment->generateComment())); + $github->updateComment($owner, $repositoryName, $latestCommentId, $comment->generateComment()); } finally { - $dbForPlatform->deleteDocument('vcsCommentLocks', $latestCommentId); + try { if ($lockId !== '') { $dbForPlatform->deleteDocument('vcsCommentLocks', $lockId); } } catch (\Throwable $e) { /* no-op */ } }
🧹 Nitpick comments (1)
app/controllers/api/vcs.php (1)
428-444: Reduce thundering herd and request blocking in lock backoff.Current backoff sleeps for 1s up to 9s per request. Add small jitter to spread contention; consider a shorter base delay.
Example:
- \sleep(1); + \usleep(\random_int(200_000, 800_000)); // 200–800ms jitter
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
app/controllers/api/vcs.php(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/controllers/api/vcs.php (1)
src/Appwrite/Vcs/Comment.php (4)
Comment(10-226)parseComment(214-225)addBuild(31-48)generateComment(50-199)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Benchmark
- GitHub Check: scan
🔇 Additional comments (1)
app/controllers/api/vcs.php (1)
18-18: LGTM: Console logger import is appropriate.
2 participants
What does this PR do?
Update
previewUrleven when builds worker hasn't picked up the job.Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Screenshots may also be helpful.)
Related PRs and Issues
Checklist