Optimize: size #6
Conversation
WalkthroughThe Dockerfile now uses zenika/alpine-chrome:with-node as base, removes Corepack, installs pnpm globally, sets NODE_ENV=production, configures Playwright to skip browser download, sets PLAYWRIGHT_CHROMIUM_EXECUTABLE_PATH to /usr/bin/chromium-browser, performs cleanup to reduce image size, and changes the startup command to node src/index.js. docker-compose.yml adds an appwrite-browser service built from the Dockerfile, exposing port 3000 with PORT=3000. package.json replaces the playwright dependency with playwright-core at the same version. src/index.js switches imports to playwright-core, uses the executable path from the environment, and sets the request schema headers field to default to an empty object. ✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/index.js (1)
101-129: Lighthouse response handling breaks when json=false
JSON.parse(results.report)will throw for HTML/CSV reports.-const results = await playAudit({ +const results = await playAudit({ /* ... */ -}); +}); - await context.close(); - return JSON.parse(results.report); + await context.close(); + if (body.json) { + event.node.res.setHeader("Content-Type", "application/json"); + return JSON.parse(results.report); + } + if (body.html) { + event.node.res.setHeader("Content-Type", "text/html; charset=utf-8"); + return results.report; + } + if (body.csv) { + event.node.res.setHeader("Content-Type", "text/csv; charset=utf-8"); + return results.report; + } + return { ok: true };
🧹 Nitpick comments (9)
src/index.js (5)
29-31: JSDoc type import points to 'playwright', not 'playwright-core'Keep types consistent to avoid editor/IDE confusion.
-/** @type {Partial<import('playwright').BrowserContext>} defaultContext */ +/** @type {Partial<import('playwright-core').BrowserContext>} defaultContext */
57-70: Sanitize/normalize override headers for the appwrite domainNormalize keys and prevent overriding sensitive hop-by-hop headers.
- return await route.continue({ - headers: { - ...request.headers(), - ...body.headers, - }, - }); + const base = request.headers(); + const safe = Object.fromEntries( + Object.entries(body.headers).map(([k, v]) => [k.toLowerCase(), v]), + ); + // Disallow overriding dangerous headers + for (const h of ["host", "content-length", "connection", "transfer-encoding"]) { + delete safe[h]; + } + return await route.continue({ headers: { ...base, ...safe } });
80-86: Return correct content-type for screenshotsHelps clients treat the response as PNG.
-const screen = await page.screenshot(); +const screen = await page.screenshot(); @@ -await context.close(); -return screen; +await context.close(); +event.node.res.setHeader("Content-Type", "image/png"); +return screen;
131-138: Graceful shutdown to avoid orphaned ChromiumAdd signal handlers to close the shared browser on exit.
router.get( @@ ); createServer(toNodeListener(app)).listen(port); console.log(`Server running on port http://0.0.0.0:${port}`); + +for (const sig of ["SIGTERM", "SIGINT"]) { + process.on(sig, async () => { + try { await browser.close(); } catch {} + process.exit(0); + }); +}
37-42: SSRF hardening (optional, but recommended)The service fetches arbitrary URLs. Consider an allowlist (e.g., only appwrite network) or DNS/IP blocking (RFC1918).
I can propose a small URL allowlist+CIDR block check if desired.
Also applies to: 57-70, 72-79
Dockerfile (2)
27-33: Pin base image for reproducibility
zenika/alpine-chrome:with-nodeis moving. Pin a specific tag or digest to avoid surprises.Example:
-FROM zenika/alpine-chrome:with-node +FROM zenika/alpine-chrome:with-node-22@sha256:<digest>
42-43: Document port exposureNot required, but helpful for users and tooling.
+# document the listening port +EXPOSE 3000 # start the application CMD ["node", "src/index.js"]docker-compose.yml (2)
1-10: Add newline at EOF to satisfy yamllintMinor formatting fix.
services: appwrite-browser: build: context: . dockerfile: Dockerfile image: appwrite/browser:local ports: - "3000:3000" environment: - PORT=3000 +
2-10: Optional: improve stability and operability
- Increase shared memory for Chromium; or keep
--disable-dev-shm-usage.- Add a healthcheck.
services: appwrite-browser: @@ ports: - "3000:3000" environment: - PORT=3000 + shm_size: "512m" + healthcheck: + test: ["CMD", "wget", "-qO-", "http://localhost:3000/v1/health"] + interval: 10s + timeout: 5s + retries: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
Dockerfile(1 hunks)docker-compose.yml(1 hunks)package.json(1 hunks)src/index.js(3 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
docker-compose.yml
[error] 10-10: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (3)
package.json (1)
21-21: Remove lingering “playwright” import and verify Lighthouse integration
- src/index.js (around line 29): update the JSDoc
import('playwright')to referenceplaywright-coreor remove it entirely.- Confirm that playwright-lighthouse ^4.0.0 peer-dependencies are satisfied by playwright-core ^1.52.0.
src/index.js (2)
44-87: Concurrency on a single debug port (9222)Concurrent /v1/reports calls may contend on the shared CDP port. Consider a queue or dedicated Chromium for LH runs.
Would you like a lightweight in-process queue (p-limit) around playAudit calls?
Also applies to: 101-129
11-12: System Chrome via playwright-core: add container stability flags and pin executable pathAdd the following flags to improve Chromium stability in containers and explicitly set the binary path:
-import { chromium } from "playwright-core"; +import { chromium } from "playwright-core"; @@ -const browser = await chromium.launch({ - args: ["--remote-debugging-port=9222"], - executablePath: process.env.PLAYWRIGHT_CHROMIUM_EXECUTABLE_PATH, -}); +const browser = await chromium.launch({ + args: [ + "--remote-debugging-port=9222", + "--disable-dev-shm-usage", + "--no-first-run", + "--no-default-browser-check", + ], + executablePath: process.env.PLAYWRIGHT_CHROMIUM_EXECUTABLE_PATH, +});Ensure that
PLAYWRIGHT_CHROMIUM_EXECUTABLE_PATHpoints to an installed Chromium binary in your container.
| # install pnpm as root, | ||
| # then switch back to chrome user | ||
| USER root | ||
| RUN npm install -g pnpm@10 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align PNPM version with packageManager field
package.json pins packageManager: pnpm@9.12.1 but Docker installs pnpm@10. This may desync lockfile semantics.
Option A (keep lockfile as-is):
-RUN npm install -g pnpm@10
+RUN npm install -g pnpm@9.12.1Option B (if you prefer pnpm 10): bump package.json’s packageManager and regenerate lockfile.
📝 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 npm install -g pnpm@10 | |
| RUN npm install -g pnpm@9.12.1 |
🤖 Prompt for AI Agents
In Dockerfile at line 12, the RUN installs pnpm@10 while package.json declares
packageManager: pnpm@9.12.1 causing mismatch; fix by making versions consistent:
either change the Dockerfile to RUN npm install -g pnpm@9.12.1 to match
package.json and keep the existing lockfile, or update
package.json.packageManager to pnpm@10 and regenerate the pnpm lockfile (pnpm
install) and commit the updated lockfile so Docker and repo use the same pnpm
version.
| RUN pnpm install --prod --frozen-lockfile && \ | ||
| pnpm prune --prod && \ | ||
| # remove unnecessary files to reduce image size | ||
| find /app/node_modules -name "*.md" -delete && \ | ||
| find /app/node_modules -type d -name "test*" -exec rm -rf {} + 2>/dev/null || true && \ | ||
| find /app/node_modules -type d -name "doc*" -exec rm -rf {} + 2>/dev/null || true && \ | ||
| find /app/node_modules -name ".cache" -type d -exec rm -rf {} + 2>/dev/null || true && \ | ||
| # clean up caches | ||
| rm -rf ~/.pnpm ~/.npm /tmp/* /var/cache/apk/* |
There was a problem hiding this comment.
Build may fail: removing /var/cache/apk as non-root
This rm -rf /var/cache/apk/* runs as USER chrome and can exit non‑zero.
Move privileged cleanup under root (or ignore errors):
USER chrome
-RUN pnpm install --prod --frozen-lockfile && \
+RUN pnpm install --prod --frozen-lockfile && \
pnpm prune --prod && \
# remove unnecessary files to reduce image size
find /app/node_modules -name "*.md" -delete && \
find /app/node_modules -type d -name "test*" -exec rm -rf {} + 2>/dev/null || true && \
find /app/node_modules -type d -name "doc*" -exec rm -rf {} + 2>/dev/null || true && \
- find /app/node_modules -name ".cache" -type d -exec rm -rf {} + 2>/dev/null || true && \
- # clean up caches
- rm -rf ~/.pnpm ~/.npm /tmp/* /var/cache/apk/*
+ find /app/node_modules -name ".cache" -type d -exec rm -rf {} + 2>/dev/null || true && \
+ # clean up user caches
+ rm -rf ~/.pnpm ~/.npm /tmp/*
+
+# remove apk cache as root
+USER root
+RUN rm -rf /var/cache/apk/* || true
+USER chrome📝 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 pnpm install --prod --frozen-lockfile && \ | |
| pnpm prune --prod && \ | |
| # remove unnecessary files to reduce image size | |
| find /app/node_modules -name "*.md" -delete && \ | |
| find /app/node_modules -type d -name "test*" -exec rm -rf {} + 2>/dev/null || true && \ | |
| find /app/node_modules -type d -name "doc*" -exec rm -rf {} + 2>/dev/null || true && \ | |
| find /app/node_modules -name ".cache" -type d -exec rm -rf {} + 2>/dev/null || true && \ | |
| # clean up caches | |
| rm -rf ~/.pnpm ~/.npm /tmp/* /var/cache/apk/* | |
| USER chrome | |
| RUN pnpm install --prod --frozen-lockfile && \ | |
| pnpm prune --prod && \ | |
| # remove unnecessary files to reduce image size | |
| find /app/node_modules -name "*.md" -delete && \ | |
| find /app/node_modules -type d -name "test*" -exec rm -rf {} + 2>/dev/null || true && \ | |
| find /app/node_modules -type d -name "doc*" -exec rm -rf {} + 2>/dev/null || true && \ | |
| find /app/node_modules -name ".cache" -type d -exec rm -rf {} + 2>/dev/null || true && \ | |
| # clean up user caches | |
| rm -rf ~/.pnpm ~/.npm /tmp/* | |
| # remove apk cache as root | |
| USER root | |
| RUN rm -rf /var/cache/apk/* || true | |
| USER chrome |
🤖 Prompt for AI Agents
In Dockerfile around lines 16-24 the cleanup command rm -rf /var/cache/apk/* can
fail because the image may be running as USER chrome (non-root); fix by moving
privileged cleanup into a root-run step (perform the /var/cache/apk/* removal in
a RUN executed while still root or before switching to USER chrome) or make the
command tolerant of permission errors by appending || true to that specific rm
invocation so the build won’t fail when run as non-root.
| const screenshotSchema = z.object({ | ||
| url: z.string().url(), | ||
| theme: z.enum(["light", "dark"]).default("light"), | ||
| headers: z.record(z.string(), z.any()), | ||
| headers: z.record(z.string(), z.any()).default({}), | ||
| sleep: z.number().min(0).max(60000).default(3000), | ||
| }); |
There was a problem hiding this comment.
Header schema allows non-string values; Playwright expects strings
Current z.any() can pass numbers/objects and break route.continue.
-headers: z.record(z.string(), z.any()).default({}),
+headers: z.record(z.string(), z.string()).default({}),If you must allow booleans/numbers, coerce:
-headers: z.record(z.string(), z.any()).default({}),
+headers: z
+ .record(z.string(), z.union([z.string(), z.number(), z.boolean()]))
+ .default({})
+ .transform((h) =>
+ Object.fromEntries(Object.entries(h).map(([k, v]) => [k, String(v)])),
+ ),📝 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.
| const screenshotSchema = z.object({ | |
| url: z.string().url(), | |
| theme: z.enum(["light", "dark"]).default("light"), | |
| headers: z.record(z.string(), z.any()), | |
| headers: z.record(z.string(), z.any()).default({}), | |
| sleep: z.number().min(0).max(60000).default(3000), | |
| }); | |
| const screenshotSchema = z.object({ | |
| url: z.string().url(), | |
| theme: z.enum(["light", "dark"]).default("light"), | |
| headers: z.record(z.string(), z.string()).default({}), | |
| sleep: z.number().min(0).max(60000).default(3000), | |
| }); |
🤖 Prompt for AI Agents
In src/index.js around lines 37-42, the headers schema currently uses
z.record(z.string(), z.any()) which permits non-string values that break
Playwright's route.continue; change the schema to require string values (e.g.,
z.record(z.string(), z.string()).default({})) or alternatively accept
numbers/booleans but coerce them to strings (use z.preprocess or a transform
that converts values to String) so all header values are guaranteed to be
strings before being passed to Playwright.
| # install pnpm as root, | ||
| # then switch back to chrome user | ||
| USER root | ||
| RUN npm install -g pnpm@10 |
There was a problem hiding this comment.
Lets use specific version, with all 3 numbers. prevents unwanted breaks in future
| rm -rf ~/.pnpm ~/.npm /tmp/* /var/cache/apk/* | ||
|
|
||
| # production stage | ||
| FROM zenika/alpine-chrome:with-node |
There was a problem hiding this comment.
A bit worryin to use non-official image without specifying version. Lets stick to node alpine image, and install specific version of chromium
There was a problem hiding this comment.
its OSS -
https://github.com/Zenika/alpine-chrome
using this and then node creates issues.
| # copy application files | ||
| COPY package.json ./ | ||
| COPY --from=base /app/node_modules ./node_modules | ||
| COPY src/index.js ./src/ |
There was a problem hiding this comment.
lets copy entire src directory
| find /app/node_modules -name "*.md" -delete && \ | ||
| find /app/node_modules -type d -name "test*" -exec rm -rf {} + 2>/dev/null || true && \ | ||
| find /app/node_modules -type d -name "doc*" -exec rm -rf {} + 2>/dev/null || true && \ | ||
| find /app/node_modules -name ".cache" -type d -exec rm -rf {} + 2>/dev/null || true && \ | ||
| # clean up caches | ||
| rm -rf ~/.pnpm ~/.npm /tmp/* /var/cache/apk/* |
There was a problem hiding this comment.
Instead of this, we can try and use tool we use in Open Runtimes; it knows a bit more files and paths:
What does this PR do?
Should be around 1.04/1.05GB now.
Test Plan
Manual.
Related PRs and Issues
N/A.
Have you read the Contributing Guidelines on issues?
Yes.
Summary by CodeRabbit
New Features
Refactor
Chores