chore: release sdks for adding getScreenshot method to Avatars service#10882
chore: release sdks for adding getScreenshot method to Avatars service#10882
Conversation
📝 WalkthroughWalkthroughThis PR updates SDK version strings in app/config/platforms.php; expands and tightens parameter validators and examples for the /v1/avatars/screenshots endpoint in app/controllers/api/avatars.php, builds a structured browser capture config (headers object, viewport object, sleep converted to ms, optional userAgent/locale/timezone/geolocation/hasTouch/permissions), posts that config to a browser service, and adds conditional post-fetch image processing (crop/convert/quality/output). It also updates SDK specification formatting in src/Appwrite/SDK/Specification/Format.php, OpenAPI3.php, and Swagger2.php to map the Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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 |
ChiragAgg5k
changed the title
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
✨ Benchmark results
⚡ Benchmark Comparison
|
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/controllers/api/avatars.php (1)
649-649: Cache key is missing critical parameters that affect output.The cache resource key is missing
headers,viewportWidth, andviewportHeightparameters. Since these parameters affect the screenshot output:
- Different
headers(e.g., auth tokens) could result in different page content- Different viewport dimensions produce different screenshots
This could lead to incorrect cached responses being served.
- ->label('cache.resource', 'screenshot/{request.url}/{request.width}/{request.height}/{request.scale}/{request.theme}/{request.userAgent}/{request.fullpage}/{request.locale}/{request.timezone}/{request.latitude}/{request.longitude}/{request.accuracy}/{request.touch}/{request.permissions}/{request.sleep}/{request.quality}/{request.output}') + ->label('cache.resource', 'screenshot/{request.url}/{request.headers}/{request.viewportWidth}/{request.viewportHeight}/{request.width}/{request.height}/{request.scale}/{request.theme}/{request.userAgent}/{request.fullpage}/{request.locale}/{request.timezone}/{request.latitude}/{request.longitude}/{request.accuracy}/{request.touch}/{request.permissions}/{request.sleep}/{request.quality}/{request.output}')
🧹 Nitpick comments (3)
app/controllers/api/avatars.php (3)
673-673: Consider increasing locale text length limit.The
Text(10)validator may be too restrictive for valid BCP 47 locale tags. For example,zh-Hans-CNis exactly 10 characters, andsr-Latn-RSis also 10 characters, meaning there's no room for edge cases. Consider increasing toText(35)to accommodate the full BCP 47 specification.- ->param('locale', '', new Text(10), 'Browser locale (e.g., "en-US", "fr-FR"). Defaults to browser default.', true, example: 'en-US') + ->param('locale', '', new Text(35), 'Browser locale (e.g., "en-US", "fr-FR"). Defaults to browser default.', true, example: 'en-US')
750-757: Consider explicit geolocation flag or tolerance for coordinate check.The condition
$latitude != 0 || $longitude != 0may not reliably detect when geolocation is intentionally set, as:
- The (0, 0) coordinate ("Null Island" in the Gulf of Guinea) is a valid location
- Floating-point comparisons to 0 can be unreliable
If a user genuinely wants to capture a screenshot with geolocation set to (0, 0), it would be ignored. Consider adding an explicit flag parameter or using a different sentinel value (e.g.,
nullif the framework supports it).
817-819: Consider sanitizing error messages from external service.The error message from the browser service is passed directly to the user via
$th->getMessage(). This could potentially expose internal service details or stack traces. Consider providing a more generic error message while logging the full details server-side.} catch (\Throwable $th) { - throw new Exception(Exception::AVATAR_REMOTE_URL_FAILED, 'Screenshot generation failed: ' . $th->getMessage()); + // Log the full error for debugging + // Consider adding: $logger?->addError('Screenshot generation failed', ['error' => $th->getMessage()]); + throw new Exception(Exception::AVATAR_REMOTE_URL_FAILED, 'Screenshot generation failed'); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (30)
app/config/specs/open-api3-1.8.x-client.jsonis excluded by!app/config/specs/**app/config/specs/open-api3-1.8.x-console.jsonis excluded by!app/config/specs/**app/config/specs/open-api3-1.8.x-server.jsonis excluded by!app/config/specs/**app/config/specs/open-api3-latest-client.jsonis excluded by!app/config/specs/**app/config/specs/open-api3-latest-console.jsonis excluded by!app/config/specs/**app/config/specs/open-api3-latest-server.jsonis excluded by!app/config/specs/**app/config/specs/swagger2-1.8.x-client.jsonis excluded by!app/config/specs/**app/config/specs/swagger2-1.8.x-console.jsonis excluded by!app/config/specs/**app/config/specs/swagger2-1.8.x-server.jsonis excluded by!app/config/specs/**app/config/specs/swagger2-latest-client.jsonis excluded by!app/config/specs/**app/config/specs/swagger2-latest-console.jsonis excluded by!app/config/specs/**app/config/specs/swagger2-latest-server.jsonis excluded by!app/config/specs/**docs/examples/1.8.x/client-android/java/avatars/get-screenshot.mdis excluded by!docs/examples/**docs/examples/1.8.x/client-android/kotlin/avatars/get-screenshot.mdis excluded by!docs/examples/**docs/examples/1.8.x/client-apple/examples/avatars/get-screenshot.mdis excluded by!docs/examples/**docs/examples/1.8.x/client-flutter/examples/avatars/get-screenshot.mdis excluded by!docs/examples/**docs/examples/1.8.x/client-react-native/examples/avatars/get-screenshot.mdis excluded by!docs/examples/**docs/examples/1.8.x/client-web/examples/avatars/get-screenshot.mdis excluded by!docs/examples/**docs/examples/1.8.x/console-web/examples/avatars/get-screenshot.mdis excluded by!docs/examples/**docs/examples/1.8.x/console-web/examples/vcs/list-repositories.mdis excluded by!docs/examples/**docs/examples/1.8.x/server-dart/examples/avatars/get-screenshot.mdis excluded by!docs/examples/**docs/examples/1.8.x/server-dotnet/examples/avatars/get-screenshot.mdis excluded by!docs/examples/**docs/examples/1.8.x/server-go/examples/avatars/get-screenshot.mdis excluded by!docs/examples/**docs/examples/1.8.x/server-kotlin/java/avatars/get-screenshot.mdis excluded by!docs/examples/**docs/examples/1.8.x/server-kotlin/kotlin/avatars/get-screenshot.mdis excluded by!docs/examples/**docs/examples/1.8.x/server-nodejs/examples/avatars/get-screenshot.mdis excluded by!docs/examples/**docs/examples/1.8.x/server-php/examples/avatars/get-screenshot.mdis excluded by!docs/examples/**docs/examples/1.8.x/server-python/examples/avatars/get-screenshot.mdis excluded by!docs/examples/**docs/examples/1.8.x/server-ruby/examples/avatars/get-screenshot.mdis excluded by!docs/examples/**docs/examples/1.8.x/server-swift/examples/avatars/get-screenshot.mdis excluded by!docs/examples/**
📒 Files selected for processing (1)
app/controllers/api/avatars.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/controllers/api/avatars.php (1)
src/Appwrite/GraphQL/Types/Mapper.php (1)
param(255-363)
⏰ 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: Unit Test
- GitHub Check: Benchmark
- GitHub Check: Setup & Build SDK
- GitHub Check: scan
🔇 Additional comments (2)
app/controllers/api/avatars.php (2)
687-687: LGTM!The action function signature correctly aligns with the parameter definitions, with appropriate types for each parameter (float for scale/coordinates, bool for fullpage/touch, array for headers/permissions).
728-731: LGTM!The mapping from user-facing
scaleparameter todeviceScaleFactoraligns with browser automation APIs (Puppeteer/Playwright convention). The conditional addition avoids sending default values.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/Appwrite/SDK/Specification/Format/OpenAPI3.php (1)
604-661: Consider extracting the blacklist check into a helper method.The blacklist iteration logic is duplicated between the array and non-array branches (lines 618-627 and 643-652). While correct, extracting this into a private method would reduce duplication.
+ private function isEnumBlacklisted(string $namespace, string $method, string $param): bool + { + foreach ($this->enumBlacklist as $blacklist) { + if ( + $blacklist['namespace'] == $namespace + && $blacklist['method'] == $method + && $blacklist['parameter'] == $param + ) { + return true; + } + } + return false; + }app/controllers/api/avatars.php (1)
768-779: Consider sanitizing error messages from external service.The error message on line 778 includes the raw response body from the browser service:
'Screenshot service failed: ' . $fetchResponse->getBody(). This could potentially expose internal implementation details to API consumers.if ($fetchResponse->getStatusCode() >= 400) { - throw new Exception(Exception::AVATAR_REMOTE_URL_FAILED, 'Screenshot service failed: ' . $fetchResponse->getBody()); + throw new Exception(Exception::AVATAR_REMOTE_URL_FAILED, 'Screenshot service failed'); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (12)
app/config/specs/open-api3-1.8.x-client.jsonis excluded by!app/config/specs/**app/config/specs/open-api3-1.8.x-console.jsonis excluded by!app/config/specs/**app/config/specs/open-api3-1.8.x-server.jsonis excluded by!app/config/specs/**app/config/specs/open-api3-latest-client.jsonis excluded by!app/config/specs/**app/config/specs/open-api3-latest-console.jsonis excluded by!app/config/specs/**app/config/specs/open-api3-latest-server.jsonis excluded by!app/config/specs/**app/config/specs/swagger2-1.8.x-client.jsonis excluded by!app/config/specs/**app/config/specs/swagger2-1.8.x-console.jsonis excluded by!app/config/specs/**app/config/specs/swagger2-1.8.x-server.jsonis excluded by!app/config/specs/**app/config/specs/swagger2-latest-client.jsonis excluded by!app/config/specs/**app/config/specs/swagger2-latest-console.jsonis excluded by!app/config/specs/**app/config/specs/swagger2-latest-server.jsonis excluded by!app/config/specs/**
📒 Files selected for processing (4)
app/controllers/api/avatars.php(1 hunks)src/Appwrite/SDK/Specification/Format.php(1 hunks)src/Appwrite/SDK/Specification/Format/OpenAPI3.php(7 hunks)src/Appwrite/SDK/Specification/Format/Swagger2.php(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-05T15:30:59.312Z
Learnt from: ArnabChatterjee20k
Repo: appwrite/appwrite PR: 10443
File: src/Appwrite/SDK/Specification/Format/OpenAPI3.php:450-458
Timestamp: 2025-09-05T15:30:59.312Z
Learning: In OpenAPI 3.0.x, using oneOf: [{'type': 'array'}] for array items is the correct approach for spatial data validation, as it ensures each element is specifically an array type. Using new \stdClass() or empty objects {} for items can cause compatibility issues with OpenAPI 3.0.x specification.
Applied to files:
src/Appwrite/SDK/Specification/Format/Swagger2.phpsrc/Appwrite/SDK/Specification/Format/OpenAPI3.php
🧬 Code graph analysis (3)
src/Appwrite/SDK/Specification/Format/Swagger2.php (2)
src/Appwrite/Template/Template.php (2)
Template(8-178)fromCamelCaseToSnake(156-165)src/Appwrite/SDK/Specification/Format.php (2)
getRequestEnumName(115-469)getRequestEnumKeys(471-562)
src/Appwrite/SDK/Specification/Format/OpenAPI3.php (3)
src/Appwrite/GraphQL/Types/Mapper.php (1)
param(255-363)src/Appwrite/Template/Template.php (2)
Template(8-178)fromCamelCaseToSnake(156-165)src/Appwrite/SDK/Specification/Format.php (2)
getRequestEnumName(115-469)getRequestEnumKeys(471-562)
app/controllers/api/avatars.php (1)
src/Appwrite/GraphQL/Types/Mapper.php (1)
param(255-363)
⏰ 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: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build SDK
- GitHub Check: scan
🔇 Additional comments (10)
src/Appwrite/SDK/Specification/Format.php (1)
174-179: LGTM!The new
getScreenshotcase correctly maps thepermissionsparameter to theBrowserPermissionenum, following the established pattern for request enum name mappings in this file.src/Appwrite/SDK/Specification/Format/OpenAPI3.php (2)
442-445: LGTM!Adding
Utopia\Validator\WhiteListto the subclass switch allows proper handling of array-wrapped whitelist validators, enabling correct enum generation for array parameters likepermissions.
514-516: Consistent conditional example handling.For
ArrayList, the conditional!empty($param['example'])pattern correctly avoids overwriting with null/empty values while preserving explicit examples.src/Appwrite/SDK/Specification/Format/Swagger2.php (2)
447-450: LGTM!Adding WhiteList to the subclass switch enables proper array-wrapped whitelist handling, consistent with OpenAPI3.php changes.
593-641: LGTM with optional refactor note.The WhiteList handling correctly differentiates between array and non-array cases, properly assigns enum metadata, and handles integer formatting. The blacklist check duplication could be extracted (same suggestion as OpenAPI3.php).
app/controllers/api/avatars.php (5)
665-684: Well-structured parameter definitions.The new parameters are thoroughly documented with validators, examples, and appropriate defaults. The use of
Rangevalidators withRange::TYPE_FLOATfor scale/coordinates andWhiteListfor theme/timezone provides good input validation.
703-712: Defensive headers handling.The indexed array check and stdClass conversion ensure proper JSON serialization (
{}vs[]), correctly handling edge cases from the Assoc validator.
750-757: Verify: Geolocation at (0, 0) coordinates.The condition
$latitude != 0 || $longitude != 0means the exact coordinates (0, 0) — a valid point in the Gulf of Guinea — cannot be explicitly set. While likely intentional to avoid sending defaults, consider using a separate flag if this edge case matters.
720-720: LGTM!The sleep conversion from seconds to milliseconds is correct. With a max sleep of 10 seconds and a 30-second client timeout, there's adequate headroom for the screenshot capture.
787-803: Callingcrop(0, 0)when width and height are both zero is unnecessary and potentially problematic.The
needsProcessingcondition on line 788 evaluates totrueeven when bothwidthandheightare 0, as long asquality !== -1oroutputis not empty. This causes line 793 to execute$image->crop(0, 0), which is semantically incorrect—cropping to 0x0 dimensions has no meaningful effect and wastes processing.Consider adding an explicit dimension check in the condition to prevent crop from being called with invalid dimensions, or add a guard within the processing block to skip the crop operation when both dimensions are zero:
$needsProcessing = ($width > 0 || $height > 0) || $quality !== -1 || !empty($output); if ($needsProcessing) { $image = new Image($screenshot); if ($width > 0 || $height > 0) { $image->crop($width, $height); } $output = $output ?: 'png'; $resizedScreenshot = $image->output($output, $quality); unset($image);
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Appwrite/SDK/Specification/Format/Swagger2.php (1)
605-632: Consider extracting blacklist check logic.The blacklist iteration is duplicated between array (lines 606-612) and non-array (lines 626-632) branches. This could be extracted into a helper method.
Example extraction:
private function isParameterBlacklisted(string $namespace, string $methodName, string $name): bool { foreach ($this->enumBlacklist as $blacklist) { if ($blacklist['namespace'] == $namespace && $blacklist['method'] == $methodName && $blacklist['parameter'] == $name) { return true; } } return false; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Appwrite/SDK/Specification/Format/Swagger2.php(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-05T15:30:59.312Z
Learnt from: ArnabChatterjee20k
Repo: appwrite/appwrite PR: 10443
File: src/Appwrite/SDK/Specification/Format/OpenAPI3.php:450-458
Timestamp: 2025-09-05T15:30:59.312Z
Learning: In OpenAPI 3.0.x, using oneOf: [{'type': 'array'}] for array items is the correct approach for spatial data validation, as it ensures each element is specifically an array type. Using new \stdClass() or empty objects {} for items can cause compatibility issues with OpenAPI 3.0.x specification.
Applied to files:
src/Appwrite/SDK/Specification/Format/Swagger2.php
🧬 Code graph analysis (1)
src/Appwrite/SDK/Specification/Format/Swagger2.php (2)
src/Appwrite/Template/Template.php (2)
Template(8-178)fromCamelCaseToSnake(156-165)src/Appwrite/SDK/Specification/Format.php (2)
getRequestEnumName(115-469)getRequestEnumKeys(471-562)
⏰ 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 (Webhooks)
- GitHub Check: E2E Service Test (VCS)
- GitHub Check: E2E Service Test (Tokens)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Teams)
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: E2E General Test
- GitHub Check: Unit Test
- GitHub Check: Setup & Build SDK
- GitHub Check: scan
🔇 Additional comments (7)
src/Appwrite/SDK/Specification/Format/Swagger2.php (7)
446-450: LGTM!Adding
WhiteListto the array subclass switch correctly enables the downstream WhiteList case to handle array-wrapped validators.
454-500: Consistent x-example pattern looks good.The
($param['example'] ?? '') ?: <default>pattern correctly prioritizes per-parameter examples while preserving backward-compatible defaults across all these validator types.
501-511: LGTM!Conditionally setting
x-exampleonly when provided is appropriate forArrayListsince there's no sensible universal default for array contents.
539-570: LGTM!The example preference pattern is consistently applied with appropriate defaults for each validator type.
571-591: LGTM!Conditionally setting
x-examplefor numeric and length validators is appropriate given there's no universal default.
643-646: LGTM!Consistent application of the example preference pattern.
662-685: LGTM!The Operation case correctly defers to
param['example']when provided, and the default case appropriately avoids generating placeholder examples.
3 participants
No description provided.