tests/e2e/Services/Avatars/AvatarsBase.php (1)

575-579: Nice: body size assertion added

This addresses the earlier feedback to assert a minimum payload size.

app/config/variables.php (1)

955-964: Add basic URL validation filter for _APP_BROWSER_HOST

Recommend setting filter to 'url' to catch obvious misconfigurations early. Keeps behavior but adds guardrails.

Apply:

-                'filter' => ''
+                'filter' => 'url'

tests/e2e/Services/Avatars/AvatarsBase.php (3)

600-699: Avoid testing with sensitive headers (Authorization) or assert they’re blocked

Passing Authorization to arbitrary sites can normalize risky patterns. Prefer benign headers (e.g., X-Test-Header) or add a negative test asserting security-sensitive headers (Host, Cookie, Authorization, X-Forwarded-*) are rejected/stripped by the service.

If the service blocks these, add:

// Expect 400 when sending disallowed headers
$response = $this->client->call(Client::METHOD_GET, '/avatars/screenshots', [
  'x-appwrite-project' => $this->getProject()['$id'],
], [
  'url' => 'https://appwrite.io',
  'headers' => ['Host' => 'evil.test', 'Cookie' => 'a=b', 'Authorization' => 'Bearer X'],
]);
$this->assertEquals(400, $response['headers']['status-code']);

---

728-741: Duplicate success case for max viewport

This repeats the earlier 1920x1080 success path (Lines 700–709). Consider removing one to shave test time.

---

877-889: Add positive cases for output and quality

You test invalid output/quality but not valid combinations. Add a couple of happy paths to assert correct content-type/encoding.

Apply after this block:

+        // SUCCESS: output=jpg with quality
+        $response = $this->client->call(Client::METHOD_GET, '/avatars/screenshots', [
+            'x-appwrite-project' => $this->getProject()['$id'],
+        ], [
+            'url' => 'https://appwrite.io?x=' . time() . rand(1000, 9999),
+            'width' => 800,
+            'height' => 600,
+            'output' => 'jpg',
+            'quality' => 80,
+        ]);
+        $this->assertEquals(200, $response['headers']['status-code']);
+        $this->assertStringContainsString('image/jpeg', $response['headers']['content-type']);
+
+        // SUCCESS: output=webp
+        $response = $this->client->call(Client::METHOD_GET, '/avatars/screenshots', [
+            'x-appwrite-project' => $this->getProject()['$id'],
+        ], [
+            'url' => 'https://appwrite.io?x=' . time() . rand(1000, 9999),
+            'width' => 800,
+            'height' => 600,
+            'output' => 'webp',
+        ]);
+        $this->assertEquals(200, $response['headers']['status-code']);
+        $this->assertStringContainsString('image/webp', $response['headers']['content-type']);

tests/e2e/Services/GraphQL/AvatarsTest.php (2)

198-203: Remove noisy debug echos from passing path.

Echoing headers/body on non-image responses adds noise to CI logs and can leak data. Prefer asserting and letting the failure output surface, or only print on assertion failure via PHPUnit messages.

Apply this diff:

-        // Debug: Print the actual response if it's not an image
-        if (!str_contains($screenshot['headers']['content-type'], 'image/')) {
-            echo "Response content-type: " . $screenshot['headers']['content-type'] . "\n";
-            echo "Response body: " . print_r($screenshot['body'], true) . "\n";
-        }

Also applies to: 229-234, 281-286, 345-350

---

240-270: Exercise output/quality variants as well.

Consider adding cases for output ("webp", "jpeg") and quality boundaries to cover format negotiation through GraphQL too.

app/controllers/api/avatars.php (2)

699-713: Over-complicated headers serialization; avoid stdClass and duplicate config building.

json_encode will serialize assoc arrays as JSON objects; forcing stdClass and rebuilding $finalConfig is unnecessary and error-prone.

Apply this diff:

-        // Create a new object to ensure proper JSON serialization
-        $headersObject = new \stdClass();
-        foreach ($headers as $key => $value) {
-            $headersObject->$key = $value;
-        }
-
-        // Create the config with headers as an object
+        // Build request config; assoc arrays serialize to JSON objects
         $config = [
             'url' => $url,
             'theme' => $theme,
-            'headers' => $headersObject,
+            'headers' => $headers,
             'sleep' => $sleep * 1000, // Convert seconds to milliseconds
             'waitUntil' => 'load',
             'viewport' => [
                 'width' => $viewportWidth,
                 'height' => $viewportHeight
             ]
         ];
@@
-        // Manually handle the config to ensure headers is an object but arrays remain arrays
-        $finalConfig = [
-            'url' => $config['url'],
-            'theme' => $config['theme'],
-            'headers' => $config['headers'], // Keep as object
-            'sleep' => $config['sleep'],
-            'viewport' => $config['viewport'] // Keep as object
-        ];
-        // Add scale if not default
-        if ($scale != 1) {
-            $finalConfig['deviceScaleFactor'] = $scale;
-        }
-        // Add optional parameters that were set, preserving arrays as arrays
-        if (!empty($userAgent)) {
-            $finalConfig['userAgent'] = $userAgent;
-        }
-        if ($fullpage) {
-            $finalConfig['fullPage'] = true;
-        }
-        if (!empty($locale)) {
-            $finalConfig['locale'] = $locale;
-        }
-        if (!empty($timezone)) {
-            $finalConfig['timezoneId'] = $timezone;
-        }
-        // Add geolocation if any coordinates are provided
-        if ($latitude != 0 || $longitude != 0) {
-            $finalConfig['geolocation'] = [
-                'latitude' => $latitude,
-                'longitude' => $longitude,
-                'accuracy' => $accuracy
-            ];
-        }
-        if ($touch) {
-            $finalConfig['hasTouch'] = true;
-        }
-        // Add permissions if provided (preserve as array)
-        if (!empty($permissions)) {
-            $finalConfig['permissions'] = $permissions; // Keep as array
-        }
-        $config = $finalConfig;
+        // Optional params
+        if ($scale != 1) $config['deviceScaleFactor'] = $scale;
+        if (!empty($userAgent)) $config['userAgent'] = $userAgent;
+        if ($fullpage) $config['fullPage'] = true;
+        if (!empty($locale)) $config['locale'] = $locale;
+        if (!empty($timezone)) $config['timezoneId'] = $timezone;
+        if ($latitude != 0 || $longitude != 0) {
+            $config['geolocation'] = ['latitude' => $latitude, 'longitude' => $longitude, 'accuracy' => $accuracy];
+        }
+        if ($touch) $config['hasTouch'] = true;
+        if (!empty($permissions)) $config['permissions'] = $permissions;

Also applies to: 773-823

---

843-860: Processing path and content-type are sound; minor polish.

Logic correctly skips processing for original images and maps output to storage-outputs. Consider defaulting output based on actual screenshot mime if the service returns non-PNG (future-proofing).

Also applies to: 861-871

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

app/controllers/api/avatars.php (2)

700-700: Timeout may be insufficient for fullpage + sleep scenarios.

The 30-second timeout can be exceeded when:

• fullpage=true on long pages requiring scroll and multiple renders
• sleep is set to 10 seconds (the maximum)
• Page has slow-loading resources

Consider increasing to 60 seconds or making it proportional: setTimeout(30 + $sleep + ($fullpage ? 20 : 0))

---

817-819: Generic exception handling loses error context.

The catch block swallows all Throwable types and converts them to a generic AVATAR_REMOTE_URL_FAILED exception. This loses important context:

• Network timeouts vs. browser service errors vs. image processing failures
• HTTP status codes from the browser service
• Specific Imagick errors

Consider preserving more context:

 } catch (\Throwable $th) {
-    throw new Exception(Exception::AVATAR_REMOTE_URL_FAILED, 'Screenshot generation failed: ' . $th->getMessage());
+    $message = 'Screenshot generation failed: ' . $th->getMessage();
+    if ($th instanceof \ImagickException) {
+        throw new Exception(Exception::GENERAL_SERVER_ERROR, 'Image processing failed: ' . $th->getMessage());
+    }
+    throw new Exception(Exception::AVATAR_REMOTE_URL_FAILED, $message, $th->getCode(), $th);
 }

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro