Conversation
WalkthroughThe buildDeployment method in the Builds worker was updated to enhance build command composition and log handling for 'sites' resources when the runtime version is not 'v2'. The changes introduce separator-based log parsing for SSR detection, enforce runtime container removal, and streamline deployment document updates and event triggers. Additionally, test assertions were refined and reordered for deployment build logs and status in related e2e tests. Changes
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (20)
🔇 Additional comments (2)
✨ 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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
🛑 Comments failed to post (26)
app/config/template-runtimes.php (1)
3-4: 🛠️ Refactor suggestion
Remove or implement the TODO
The// TODO: Remove, replace with runtimes.php directlycomment indicates this file is temporary. Please either remove the stub or link toruntimes.phpnow to prevent confusion.🤖 Prompt for AI Agents
In app/config/template-runtimes.php at lines 3 to 4, the TODO comment indicates this file is temporary and should be removed or replaced. To fix this, either delete this stub file entirely if it is no longer needed, or update it to directly include or require the runtimes.php file as intended, ensuring the code uses runtimes.php directly to avoid confusion.app/controllers/api/proxy.php (1)
217-227:
⚠️ Potential issueCNAME validation condition inverted – deployments may never verify
DNSvalidators are added only when the CNAME is not known or is a
test domain:if (!$targetCNAME->isKnown() || $targetCNAME->isTest()) { $validators[] = new DNS($targetCNAME->get(), DNS::RECORD_CNAME); }For a normal production value (e.g.
proxy.appwrite.io)isKnown() === true
andisTest() === false, so no CNAME validator is registered.
Projects that rely solely on CNAME records will therefore always fail
verification, because later we short-circuit with an error when the
validators array is empty or when A / AAAA records are absent.- if (!$targetCNAME->isKnown() || $targetCNAME->isTest()) { + if ($targetCNAME->isKnown() && !$targetCNAME->isTest()) { $validators[] = new DNS($targetCNAME->get(), DNS::RECORD_CNAME); }Please fix the condition (or remove it entirely) so valid CNAME targets are
actually validated.🤖 Prompt for AI Agents
In app/controllers/api/proxy.php around lines 217 to 227, the condition that adds the CNAME DNS validator is inverted, causing valid production CNAME targets to skip validation. To fix this, change the condition to add the CNAME validator when the target CNAME is known and not a test domain, or remove the condition entirely to always add the CNAME validator. This ensures that valid CNAME targets are properly validated and verification does not fail incorrectly.app/controllers/api/migrations.php (2)
358-399: 🛠️ Refactor suggestion
Streaming decryption/decompression to avoid OOM on large files
The current flow loads the entire source into memory:
$source = $deviceForFiles->read($path); // decrypt // decompress $deviceForImports->write($newPath, $source, 'text/csv');A single 100 MB CSV decompressed from GZIP → ~1 GB RAM peak.
Consider processing the file in chunks:
- Create a read stream from
$deviceForFiles.- Pipe through incremental decrypt/compress filters (
OpenSSL,Zstd,
GZIPhave stream variants).- Write to
$deviceForImportsusing->writeStream().This keeps memory constant, unblocks very large imports, and aligns with the
“no encryption/compression above 20 MB” comment.🤖 Prompt for AI Agents
In app/controllers/api/migrations.php between lines 358 and 399, the current code reads the entire file into memory before decrypting and decompressing, which causes high memory usage for large files. To fix this, refactor the code to use streaming: create a read stream from $deviceForFiles, then pipe it through streaming decrypt and decompress filters provided by OpenSSL, Zstd, and GZIP, and finally write the output using $deviceForImports->writeStream(). This approach processes the file in chunks, keeping memory usage constant and supporting large file imports efficiently.
339-356:
⚠️ Potential issuePermission bypass threat: regular project members can import from any bucket
Authorization::skip()is used to fetch the bucket & file, but the only
access gate afterwards is:if ($bucket->isEmpty() || (!$isAPIKey && !$isPrivilegedUser)) { throw new Exception(Exception::STORAGE_BUCKET_NOT_FOUND); }A non-privileged project member (role
user:{uid}) is authorised by
default SDK rules yet does not satisfy either$isAPIKeyor
$isPrivilegedUser; they will hit a 404 even for their own buckets.
Conversely, if you ever change the condition to allow them through, the
earlierskip()means ACLs are no longer enforced and users could import
from buckets they don’t own.Recommendation:
- Query bucket & file without
Authorization::skip().- Keep the explicit privilege check for additional hardening.
- $bucket = Authorization::skip(fn () => $dbForProject->getDocument('buckets', $bucketId)); + $bucket = $dbForProject->getDocument('buckets', $bucketId);Repeat for the file query.
🤖 Prompt for AI Agents
In app/controllers/api/migrations.php around lines 339 to 356, the use of Authorization::skip() to fetch the bucket and file bypasses ACL checks, allowing unauthorized access. To fix this, remove Authorization::skip() when querying the bucket and file so that normal authorization checks apply. Then, retain the explicit privilege checks ($isAPIKey and $isPrivilegedUser) after fetching to enforce additional access control. This ensures only authorized users can access their own buckets and files without bypassing ACLs.app/config/frameworks.php (1)
11-16: 🛠️ Refactor suggestion
Ensure
getVersions()handles missing runtime keys gracefully
$templateRuntimes['NODE'](and later['FLUTTER']) is accessed without verifying that the key exists in the configuration.
If the key is missing or mis-typed, PHP will throw a warning and the whole config load will fail, blocking the server from booting.-function getVersions(array $versions, string $prefix) +function getVersions(?array $versions, string $prefix) { - return array_map(function ($version) use ($prefix) { + if ($versions === null) { + return []; // fail-safe: no versions instead of hard error + } + return array_map(function ($version) use ($prefix) { return $prefix . '-' . $version; }, $versions); }📝 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.function getVersions(?array $versions, string $prefix) { if ($versions === null) { return []; // fail-safe: no versions instead of hard error } return array_map(function ($version) use ($prefix) { return $prefix . '-' . $version; }, $versions); }🤖 Prompt for AI Agents
In app/config/frameworks.php around lines 11 to 16, the getVersions() function is used with keys like 'NODE' and 'FLUTTER' from $templateRuntimes without checking if these keys exist. To fix this, add checks before accessing these keys to verify they exist in the array, and handle the case where they are missing by providing a default value or skipping the operation. This will prevent PHP warnings and ensure the config loads without errors even if keys are missing or mis-typed.app/http.php (1)
314-360:
⚠️ Potential issueRace-condition when creating the screenshots bucket
Between the initial
Authorization::skip(fn () => $db->getDocument(...)->isEmpty())
and the subsequentcreateDocument()call another worker/process could create
the same bucket, leading to aDuplicateexception that bubbles up and aborts server start.Wrap the whole block in a single
Authorization::skip()and handle duplicates:-if (Authorization::skip(fn () => $db->getDocument('buckets','screenshots')->isEmpty())) { - Console::info("Creating screenshots bucket..."); - Authorization::skip(fn() => $db->createDocument(...)); +Authorization::skip(function() use ($dbForPlatform) { + if ($dbForPlatform->getDocument('buckets','screenshots')->isEmpty()) { + Console::info("Creating screenshots bucket..."); + try { + $dbForPlatform->createDocument('buckets', new Document([...])); + } catch (Duplicate) { + Console::info('Skip: screenshots bucket already exists'); + } + } });Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/http.php between lines 314 and 360, the code checks if the screenshots bucket is empty and then creates it, but this can cause a race condition if another process creates the bucket in between, leading to a duplicate exception. To fix this, wrap the entire block that checks for the bucket and creates it inside a single Authorization::skip() call, and add error handling to catch and ignore duplicate creation exceptions so the server start does not abort..github/workflows/benchmark.yml (2)
61-66:
⚠️ Potential issue
apt installhangs without-yin non-interactive runners
sudo apt install ohawill prompt for confirmation and the job will block indefinitely.
Add the non-interactive flags and fail fast on errors:- sudo apt update - sudo apt install oha + sudo apt-get update -y + DEBIAN_FRONTEND=noninteractive sudo apt-get install -y --no-install-recommends ohaAlso consider pinning a specific version to keep future runs reproducible.
📝 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.- name: Install Oha run: | echo "deb [signed-by=/usr/share/keyrings/azlux-archive-keyring.gpg] http://packages.azlux.fr/debian/ stable main" | sudo tee /etc/apt/sources.list.d/azlux.list sudo wget -O /usr/share/keyrings/azlux-archive-keyring.gpg https://azlux.fr/repo.gpg sudo apt-get update -y DEBIAN_FRONTEND=noninteractive sudo apt-get install -y --no-install-recommends oha🤖 Prompt for AI Agents
In .github/workflows/benchmark.yml around lines 61 to 66, the command 'sudo apt install oha' lacks the '-y' flag, causing it to hang waiting for user confirmation in non-interactive CI environments. Modify the install command to include '-y' to automatically confirm installation and add 'set -e' or equivalent to fail fast on errors. Additionally, consider specifying a fixed version of the 'oha' package in the install command to ensure reproducible builds.
56-60: 🛠️ Refactor suggestion
Replace fixed sleep with a health-check loop
docker compose up -d && sleep 10is brittle – startup time varies on CI and might lead to false negatives when the benchmark step hits the endpoint before the containers are healthy.
Recommend polling the health endpoint (with a timeout) instead of a blind sleep:- docker compose up -d - sleep 10 + docker compose up -d +# Wait until Appwrite replies or fail after 90 s + for i in {1..30}; do + if curl -sf http://localhost/v1/health/version >/dev/null 2>&1; then + break + fi + echo "Waiting for Appwrite to become healthy… ($i/30)" + sleep 3 + done + curl -sf http://localhost/v1/health/version >/dev/null + echo "Appwrite is ready"This makes the workflow more deterministic and avoids needless waiting when the service is already up.
📝 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.run: | sed -i 's/traefik/localhost/g' .env docker load --input /tmp/${{ env.IMAGE }}.tar docker compose up -d # Wait until Appwrite replies or fail after 90 s for i in {1..30}; do if curl -sf http://localhost/v1/health/version >/dev/null 2>&1; then break fi echo "Waiting for Appwrite to become healthy… ($i/30)" sleep 3 done curl -sf http://localhost/v1/health/version >/dev/null echo "Appwrite is ready"🤖 Prompt for AI Agents
In .github/workflows/benchmark.yml around lines 56 to 60, replace the fixed sleep command after 'docker compose up -d' with a health-check loop that polls the service's health endpoint until it responds successfully or a timeout is reached. This involves writing a loop that repeatedly sends a request to the health endpoint, waits briefly between attempts, and exits once the service is confirmed healthy or the timeout expires, ensuring the workflow waits only as long as necessary for the containers to be ready.app/config/templates/site.php (4)
29-167: 🛠️ Refactor suggestion
Normalize framework definitions and ensure required keys
TheTEMPLATE_FRAMEWORKSarray is very verbose and inconsistent: some entries (e.g.,ANALOG,VITE,OTHER) lack afallbackFile, while others define it. Missing keys lead to undefined-index issues ingetFramework. We should extract defaults to a shared schema, ensure every framework includes the same key set (installCommand,buildCommand,outputDirectory,buildRuntime,adapter,fallbackFile), and DRY up common values.Would you like assistance extracting these into a reusable config and enforcing defaults?
🤖 Prompt for AI Agents
In app/config/templates/site.php from lines 29 to 167, the TEMPLATE_FRAMEWORKS array has inconsistent keys across framework definitions, causing potential undefined-index errors in getFramework. Refactor by defining a shared default schema with all required keys including fallbackFile, and merge each framework's specific values with these defaults. Also, extract common values like installCommand and buildCommand where possible to reduce repetition and ensure every framework entry consistently includes installCommand, buildCommand, outputDirectory, buildRuntime, adapter, and fallbackFile keys.
169-173:
⚠️ Potential issueValidate framework key in
getFrameworkto avoid undefined index
Right now, a typo in$frameworkEnumwill cause a PHP notice or fatal. Add a guard and throw a clear exception if the framework key is missing.function getFramework(string $frameworkEnum, array $overrides) { + if (!isset(TEMPLATE_FRAMEWORKS[$frameworkEnum])) { + throw new \InvalidArgumentException("Unknown framework '$frameworkEnum'."); + } $settings = \array_merge(TEMPLATE_FRAMEWORKS[$frameworkEnum], $overrides); return $settings; }🤖 Prompt for AI Agents
In app/config/templates/site.php around lines 169 to 173, the function getFramework accesses TEMPLATE_FRAMEWORKS with the key $frameworkEnum without checking if it exists, which can cause a PHP notice or fatal error if the key is invalid. Add a guard clause at the start of the function to check if $frameworkEnum exists in TEMPLATE_FRAMEWORKS; if not, throw a clear exception indicating the invalid framework key. This will prevent undefined index errors and provide clearer error handling.
226-231:
⚠️ Potential issueFix duplicate
outputDirectorykeys in Vitepress override
Your Vitepress template override definesoutputDirectorytwice; the first was likely meant to befallbackFile.- 'outputDirectory' => '404.html', + 'fallbackFile' => '404.html', 'installCommand' => 'npm i vitepress && npm install', 'buildCommand' => 'npm run docs:build', - 'outputDirectory' => './.vitepress/dist', + 'outputDirectory' => './.vitepress/dist',📝 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.'providerRootDirectory' => './vite/vitepress', - 'outputDirectory' => '404.html', + 'fallbackFile' => '404.html', 'installCommand' => 'npm i vitepress && npm install', 'buildCommand' => 'npm run docs:build', - 'outputDirectory' => './.vitepress/dist', + 'outputDirectory' => './.vitepress/dist', ]),🤖 Prompt for AI Agents
In app/config/templates/site.php around lines 226 to 231, there are two keys named outputDirectory in the Vitepress override array, which causes a duplicate key issue. Rename the first outputDirectory key to fallbackFile to correctly represent the fallback file setting and avoid the duplicate key conflict.
249-253:
⚠️ Potential issueFix duplicate
outputDirectorykeys in Vuepress override
Similarly, the Vuepress override block shadows its ownoutputDirectory. The first occurrence should befallbackFile.- 'outputDirectory' => '404.html', + 'fallbackFile' => '404.html', 'installCommand' => 'npm install', 'buildCommand' => 'npm run build', - 'outputDirectory' => './src/.vuepress/dist', + 'outputDirectory' => './src/.vuepress/dist',📝 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.'providerRootDirectory' => './vue/vuepress', 'fallbackFile' => '404.html', 'installCommand' => 'npm install', 'buildCommand' => 'npm run build', 'outputDirectory' => './src/.vuepress/dist',🤖 Prompt for AI Agents
In app/config/templates/site.php around lines 249 to 253, there are duplicate keys named 'outputDirectory' in the Vuepress override array. Rename the first 'outputDirectory' key to 'fallbackFile' to avoid key shadowing and correctly represent its purpose.app/controllers/api/account.php (1)
1185-1186:
⚠️ Potential issueValidator depends on
$devKeythat is never injected – runtime error riskBoth validators reference
$devKey->isEmpty(), but this variable isn’t injected anywhere in the route chain.
During execution the container will passnull, turning the call into a fatal error (call to a member function isEmpty() on null).+ ->inject('devKey') // Ensure the DI container provides the valueor change the closure signature to handle
nullsafely:fn ($clients, ?Document $devKey = null) => $devKey?->isEmpty() ?? true ? new Host($clients) : new URL()Please add the injection (recommended) or guard against
null.🤖 Prompt for AI Agents
In app/controllers/api/account.php around lines 1185 to 1186, the closure validators use $devKey without injecting it, causing a fatal error when $devKey is null. Fix this by either properly injecting $devKey into the route chain so it is never null, or modify the closure signature to accept a nullable $devKey and safely check it using null-safe operators to avoid calling isEmpty() on null.app/controllers/shared/api.php (1)
94-109: 🛠️ Refactor suggestion
DOCUMENTS_*branch usesmodifiedwherecreatedis expectedFor bulk-create events we read:
case Database::EVENT_DOCUMENTS_CREATE: $value = $document->getAttribute('modified', 0);Shouldn’t that be the number of new documents (
created) instead ofmodified?
Using the wrong counter flips the metric sign for “upserts vs creates” and skews usage reports.🤖 Prompt for AI Agents
In app/controllers/shared/api.php around lines 94 to 109, the case for Database::EVENT_DOCUMENTS_CREATE incorrectly uses the 'modified' attribute instead of 'created'. To fix this, change the attribute accessed in this case from 'modified' to 'created' to correctly reflect the number of new documents created and ensure accurate metric reporting..github/workflows/tests 8000 .yml (1)
282-317:
⚠️ Potential issueWorkflow will fail: references
matrix.tables-modebut no matrix is defined
e2e_dev_keysdoesn’t declare astrategy.matrix, yet the step name and script use${{ matrix.tables-mode }}.
actionlintrightfully flags this and the job will error at runtime.Quick fix – drop the placeholder (the job always uses project tables):
- - name: Run Projects tests with dev keys in ${{ matrix.tables-mode }} table mode + - name: Run Projects tests with dev keys (project tables) … - echo "Using project tables" + echo "Using project tables"If you do need multiple table modes here, add the matrix declaration like in
e2e_shared_mode_test.📝 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.e2e_dev_keys: name: E2E Service Test (Dev Keys) runs-on: ubuntu-latest needs: setup strategy: fail-fast: false steps: - name: checkout uses: actions/checkout@v4 - name: Load Cache uses: actions/cache@v4 with: key: ${{ env.CACHE_KEY }} path: /tmp/${{ env.IMAGE }}.tar fail-on-cache-miss: true - name: Load and Start Appwrite run: | docker load --input /tmp/${{ env.IMAGE }}.tar sed -i 's/_APP_OPTIONS_ABUSE=disabled/_APP_OPTIONS_ABUSE=enabled/' .env docker compose up -d sleep 30 - name: Run Projects tests with dev keys (project tables) run: | echo "Using project tables" export _APP_DATABASE_SHARED_TABLES= export _APP_DATABASE_SHARED_TABLES_V1= docker compose exec -T \ -e _APP_DATABASE_SHARED_TABLES \ -e _APP_DATABASE_SHARED_TABLES_V1 \ appwrite test /usr/src/code/tests/e2e/Services/Projects --debug --group=devKeys e2e_dev_keys_shared_mode:🧰 Tools
🪛 actionlint (1.7.7)
306-306: property "tables-mode" is not defined in object type {}
(expression)
🤖 Prompt for AI Agents
In .github/workflows/tests.yml between lines 282 and 317, the job e2e_dev_keys uses the expression ${{ matrix.tables-mode }} in the step name and script, but no strategy.matrix is defined for this job, causing runtime errors. To fix this, either remove all references to ${{ matrix.tables-mode }} from the step name and script since this job always uses project tables, or if multiple table modes are needed, add a strategy.matrix declaration similar to the one in e2e_shared_mode_test to define tables-mode values.app/controllers/api/vcs.php (2)
757-786: 🛠️ Refactor suggestion
Potential “Undefined variable $runtime” notice
$runtimeis declared inside the foreach loop.
If every detection strategy returnsnull, the variable is never defined, yet it is referenced after the loop (if (!empty($runtime))).- $strategies = [ + $runtime = ''; + $strategies = [ ... - if (!\is_null($runtime)) { + if (!\is_null($runtime)) { ... } }Same issue appears in the repository-listing coroutine (lines 907-946). Initialising the variable prevents PHP 8+ warnings in production logs.
🤖 Prompt for AI Agents
In app/controllers/api/vcs.php around lines 757 to 786, the variable $runtime is declared inside the foreach loop and may remain undefined if all detection strategies return null, causing a potential "Undefined variable $runtime" notice when referenced after the loop. To fix this, initialize $runtime to null before the foreach loop to ensure it is always defined regardless of the loop outcome. Apply the same initialization fix in the repository-listing coroutine around lines 907 to 946.
270-276: 🛠️ Refactor suggestion
Empty
_APP_DOMAIN_SITESwill generate invalid preview hostnamesIf
_APP_DOMAIN_SITESis unset,$sitesDomainbecomes an empty string and the generated preview domain ends up as"uuid."(trailing dot).
Down-stream DNS validation and certificate issuance are likely to fail.$sitesDomain = System::getEnv('_APP_DOMAIN_SITES', ''); -if (empty($sitesDomain)) { - $domain = ID::unique() . "." . $sitesDomain; -} else { - // optionally: throw hard error to force explicit configuration -} +if (empty($sitesDomain)) { + throw new Exception(Exception::GENERAL_SERVER_ERROR, + '_APP_DOMAIN_SITES must be configured to enable site preview domains'); +} +$domain = ID::unique() . '.' . $sitesDomain;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/controllers/api/vcs.php around lines 270 to 276, the code assigns $sitesDomain from the environment variable _APP_DOMAIN_SITES without checking if it is empty, which causes invalid preview hostnames with a trailing dot. To fix this, add a validation step after retrieving $sitesDomain to ensure it is not empty; if it is empty, either set a default valid domain or handle the error appropriately to prevent generating invalid domain names.docker-compose.yml (1)
968-971:
⚠️ Potential issueTypo in executor environment variable will disable idle-container reaping
OPR_EXECUTOR_INACTIVE_TRESHOLDis misspelled – the OpenRuntime executor expectsOPR_EXECUTOR_INACTIVE_THRESHOLD(with an “h”).
With the current typo the executor silently falls back to its default, so idle build containers might never get cleaned up.- - OPR_EXECUTOR_INACTIVE_TRESHOLD=$_APP_COMPUTE_INACTIVE_THRESHOLD + - OPR_EXECUTOR_INACTIVE_THRESHOLD=$_APP_COMPUTE_INACTIVE_THRESHOLD📝 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.- OPR_EXECUTOR_IMAGE_PULL=enabled - OPR_EXECUTOR_INACTIVE_THRESHOLD=$_APP_COMPUTE_INACTIVE_THRESHOLD - OPR_EXECUTOR_MAINTENANCE_INTERVAL=$_APP_COMPUTE_MAINTENANCE_INTERVAL - OPR_EXECUTOR_NETWORK=$_APP_COMPUTE_RUNTIMES_NETWORK🤖 Prompt for AI Agents
In docker-compose.yml around lines 968 to 971, the environment variable OPR_EXECUTOR_INACTIVE_TRESHOLD is misspelled and should be corrected to OPR_EXECUTOR_INACTIVE_THRESHOLD. Fix the typo by adding the missing "h" in the variable name to ensure the executor properly reads the intended threshold value for idle-container reaping.app/controllers/general.php (3)
498-523: 🛠️ Refactor suggestion
Duplicate
$entrypointassignment – keep one source of truth
$entrypointis computed twice: lines 493-497 and 519-523.
Remove the second block (or the first), otherwise future edits will easily diverge.🤖 Prompt for AI Agents
In app/controllers/general.php between lines 498 and 523, the variable $entrypoint is assigned twice, once around lines 493-497 and again at lines 519-523. To avoid duplication and potential divergence in future edits, remove the second assignment block at lines 519-523, keeping only the first $entrypoint assignment as the single source of truth.
235-247:
⚠️ Potential issue
$usermay be undefined – preview-auth check can throw notice & skip validation
$useris only initialised when$userIdis non-empty.
If the cookie contains asessionIdbut nouserId, the later$user->find(...)call raises an undefined-variable notice and$sessionExistsis never satisfied, potentially blocking legitimate preview sessions.- $sessionExists = false; - $jwtSessionId = $payload['sessionId'] ?? ''; - if (!empty($jwtSessionId) && !empty($user->find('$id', $jwtSessionId, 'sessions'))) { - $sessionExists = true; - } + $sessionExists = false; + $jwtSessionId = $payload['sessionId'] ?? ''; + if (!empty($jwtSessionId) && isset($user) && !$user->isEmpty() + && !empty($user->find('$id', $jwtSessionId, 'sessions'))) { + $sessionExists = true; + }🤖 Prompt for AI Agents
In app/controllers/general.php around lines 235 to 247, the variable $user is only initialized if $userId is non-empty, but later code uses $user without checking if it is defined, causing a notice when $userId is empty but sessionId is present. To fix this, initialize $user to a default empty or null value before the if block, and add a check to ensure $user is defined and valid before calling $user->find(...) to avoid undefined variable errors and allow proper session validation.
285-289:
⚠️ Potential issueUnreachable branch – singular vs plural “function(s)”
$typeis set to'function'(singular) above, yet the check here uses'functions'(plural).
The “function not found” exception will therefore never fire, while theelsebranch is taken even for missing functions.- if ($resource->isEmpty() || !$resource->getAttribute('enabled')) { - if ($type === 'functions') { + if ($resource->isEmpty() || !$resource->getAttribute('enabled')) { + if ($type === 'function') {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/controllers/general.php around lines 285 to 289, the condition checks if $type is 'functions' (plural), but $type is actually set to 'function' (singular) earlier. This causes the "function not found" exception branch to never execute. To fix this, change the condition to check for 'function' (singular) instead of 'functions' so the correct exception is thrown when a function is not found.app/views/install/compose.phtml (1)
880-882:
⚠️ Potential issueFix typo in environment variable name – executor will ignore the intended setting
OPR_EXECUTOR_INACTIVE_TRESHOLDis miss-spelled (TRESHOLD➜THRESHOLD).
Because the executor image expects the correct key, this line will silently fall back to its default and the inactivity GC threshold won’t be honoured.- - OPR_EXECUTOR_INACTIVE_TRESHOLD=$_APP_COMPUTE_INACTIVE_THRESHOLD + - OPR_EXECUTOR_INACTIVE_THRESHOLD=$_APP_COMPUTE_INACTIVE_THRESHOLD📝 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.- OPR_EXECUTOR_INACTIVE_THRESHOLD=$_APP_COMPUTE_INACTIVE_THRESHOLD - OPR_EXECUTOR_MAINTENANCE_INTERVAL=$_APP_COMPUTE_MAINTENANCE_INTERVAL - OPR_EXECUTOR_NETWORK=$_APP_COMPUTE_RUNTIMES_NETWORK🤖 Prompt for AI Agents
In app/views/install/compose.phtml around lines 880 to 882, fix the typo in the environment variable name by changing OPR_EXECUTOR_INACTIVE_TRESHOLD to OPR_EXECUTOR_INACTIVE_THRESHOLD to ensure the executor recognizes and applies the intended inactivity GC threshold setting.app/config/collections/projects.php (2)
1743-1776: 🛠️ Refactor suggestion
Several new deployment indexes lack a length spec – may fail on MariaDB / MySQL
The new keys (
_key_sourceSize,_key_buildSize,_key_totalSize,_key_resource, …) all omit thelengthsvalue forVARCHARattributes declared withsize≥ 512. This mirrors thedeploymentIdissue above and will likewise break schema deployment under strict SQL modes.Please audit every
indexesentry added in this hunk and supply an appropriate prefix length (oftenDatabase::LENGTH_KEYor a smaller practical value).🤖 Prompt for AI Agents
In app/config/collections/projects.php between lines 1743 and 1776, the new index definitions for keys like _key_sourceSize, _key_buildSize, _key_totalSize, and _key_resource are missing the required prefix length in the 'lengths' array for VARCHAR attributes with size ≥ 512. To fix this, review each index entry and add an appropriate prefix length value such as Database::LENGTH_KEY or a smaller suitable length in the 'lengths' array to ensure compatibility with MariaDB/MySQL strict SQL modes and prevent deployment failures.
846-851:
⚠️ Potential issueMissing prefix length on large VARCHAR index can break MySQL migrations
_key_deploymentIdindexes aVARCHAR(512)column but supplies an emptylengthsarray.
On InnoDB this causesERROR 1170 (42000): BLOB/TEXT column 'deploymentId' used in key specification without a key length, aborting the migration.- 'lengths' => [], + 'lengths' => [Database::LENGTH_KEY], // 512 chars = 512 bytes utf8mb4Apply the same pattern you used for
_key_repositoryId,_key_runtime, etc.📝 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.'$id' => ID::custom('_key_deploymentId'), 'type' => Database::INDEX_KEY, 'attributes' => ['deploymentId'], - 'lengths' => [], + 'lengths' => [Database::LENGTH_KEY], // 512 chars = 512 bytes utf8mb4 'orders' => [Database::ORDER_ASC], ]🤖 Prompt for AI Agents
< 8000 code class="notranslate">In app/config/collections/projects.php around lines 846 to 851, the index definition for '_key_deploymentId' on the VARCHAR(512) column 'deploymentId' lacks a prefix length in the 'lengths' array, causing MySQL migration errors. Fix this by specifying an appropriate prefix length for the 'deploymentId' column in the 'lengths' array, following the pattern used for similar indexes like '_key_repositoryId' and '_key_runtime'.app/config/collections/platform.php (2)
1878-1883:
⚠️ Potential issueMissing third attribute in composite index name
_key_piid_prid_rtonly listsprojectInternalId,providerRepositoryIdbut name ends with_rt. Either addresourceTypeto attributes or fix name to avoid confusion.[tag]:
🤖 Prompt for AI Agents
In app/config/collections/platform.php around lines 1878 to 1883, the composite index name '_key_piid_prid_rt' suggests it includes three attributes, but only 'projectInternalId' and 'providerRepositoryId' are listed in the 'attributes' array. To fix this, either add 'resourceType' as the third attribute in the 'attributes' array to match the index name or rename the index to accurately reflect the two attributes currently included, avoiding confusion.
372-384: 🛠️ Refactor suggestion
Index length omissions may harm MySQL performance
_key_databaseand_key_region_accessed_atomitlengthsfor VARCHAR columns.
If you use MySQL with utf8mb4, indexes on long VARCHARs (>191 chars) will exceed the 767-byte limit and fail. Specify a length or switch to a hash/index prefix.- 'attributes' => ['database'], - 'lengths' => [], + 'attributes' => ['database'], + 'lengths' => [191], // safe prefix for utf8mb4Same for
regionwhen combined withaccessedAt.
[tag]:📝 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.[ '$id' => ID::custom('_key_database'), 'type' => Database::INDEX_KEY, 'attributes'=> ['database'], 'lengths' => [191], // safe prefix for utf8mb4 'orders' => [], ], [ '$id' => ID::custom('_key_region_accessed_at'), 'type' => Database::INDEX_KEY, 'attributes'=> ['region', 'accessedAt'], 'lengths' => [], // consider [191] on `region` if needed 'orders' => [], ],🤖 Prompt for AI Agents
In app/config/collections/platform.php between lines 372 and 384, the index definitions for '_key_database' and '_key_region_accessed_at' omit specifying lengths for VARCHAR columns, which can cause MySQL index size issues with utf8mb4 encoding. To fix this, specify appropriate length prefixes for the VARCHAR attributes in the 'lengths' array for these indexes, ensuring the total indexed length does not exceed MySQL limits, or alternatively use a hash or prefix index to avoid exceeding the 767-byte limit.
✨ Benchmark results
⚡ Benchmark Comparison
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php(5 hunks)tests/e2e/Services/Sites/SitesCustomServerTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Appwrite/Platform/Modules/Functions/Workers/Builds.php
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Messaging)
- GitHub Check: E2E Service Test (Migrations)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (Tokens)
- GitHub Check: E2E Service Test (Users)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: E2E General Test
- GitHub Check: Unit Test
- GitHub Check: scan
Summary by CodeRabbit
New Features
Improvements
Bug Fixes