Skip to content

Optimize: size #6

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Optimize: size #6

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 38 additions & 14 deletions Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,43 @@
FROM node:22-slim AS base
ENV PNPM_HOME="/pnpm"
ENV PATH="$PNPM_HOME:$PATH"
RUN corepack enable
# install and optimize dependencies
FROM zenika/alpine-chrome:with-node AS base

WORKDIR /app

# copy package files
COPY package.json pnpm-lock.yaml ./
RUN corepack prepare

FROM base AS prod-deps
RUN --mount=type=cache,id=pnpm,target=/pnpm/store pnpm install --prod --frozen-lockfile
# install pnpm as root,
# then switch back to chrome user
USER root
RUN npm install -g pnpm@10
Copy link

@coderabbitai coderabbitai bot Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.1

Option 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.

Suggested change
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.

Copy link
Contributor

@Meldiron Meldiron Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets use specific version, with all 3 numbers. prevents unwanted breaks in future


# install dependencies as normal user
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 caches
rm -rf ~/.pnpm ~/.npm /tmp/* /var/cache/apk/*
Comment on lines +16 to +24
Copy link

@coderabbitai coderabbitai bot Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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.

Comment on lines +19 to +24
Copy link
Contributor

@Meldiron Meldiron Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this, we can try and use tool we use in Open Runtimes; it knows a bit more files and paths:

https://github.com/open-runtimes/open-runtimes/blob/main/runtimes/node/versions/latest/helpers/sveltekit/bundle.sh#L17


# production stage
FROM zenika/alpine-chrome:with-node
Copy link
Contributor

@Meldiron Meldiron Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit worryin to use non-official image without specifying version. Lets stick to node alpine image, and install specific version of chromium

Copy link
Member Author

@ItzNotABug ItzNotABug Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its OSS -
https://github.com/Zenika/alpine-chrome

using this and then node creates issues.


# environment configuration
ENV PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD=1 \
PLAYWRIGHT_CHROMIUM_EXECUTABLE_PATH=/usr/bin/chromium-browser \
NODE_ENV=production

WORKDIR /app
USER chrome

FROM base AS build
COPY . .
# copy application files
COPY package.json ./
COPY --from=base /app/node_modules ./node_modules
COPY src/index.js ./src/
Copy link
Contributor

@Meldiron Meldiron Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets copy entire src directory


FROM base
COPY --from=prod-deps /app/node_modules /app/node_modules
RUN pnpm playwright install --with-deps chromium
COPY --from=build /app/src/index.js /app/src/index.js
CMD ["pnpm", "start"]
# start the application
CMD ["node", "src/index.js"]
10 changes: 10 additions & 0 deletions docker-compose.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
services:
appwrite-browser:
build:
context: .
dockerfile: Dockerfile
image: appwrite/browser:local
ports:
- "3000:3000"
environment:
- PORT=3000
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
"dependencies": {
"h3": "^1.13.0",
"lighthouse": "^12.2.1",
"playwright": "^1.52.0",
"playwright-core": "^1.52.0",
"playwright-lighthouse": "^4.0.0",
"zod": "^3.23.8"
},
Expand Down
35 changes: 8 additions & 27 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 4 additions & 2 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
import { readValidatedBody } from "h3";
import lighthouseDesktopConfig from "lighthouse/core/config/lr-desktop-config.js";
import lighthouseMobileConfig from "lighthouse/core/config/lr-mobile-config.js";
import { chromium } from "playwright";
import { chromium } from "playwright-core";
import { playAudit } from "playwright-lighthouse";
import { z } from "zod";

Expand All @@ -20,6 +20,7 @@ const router = createRouter();
console.log("Chromium starting...");
const browser = await chromium.launch({
args: ["--remote-debugging-port=9222"],
executablePath: process.env.PLAYWRIGHT_CHROMIUM_EXECUTABLE_PATH,
});
console.log("Chromium started!");

Expand All @@ -36,9 +37,10 @@ const defaultContext = {
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),
});
Comment on lines 37 to 42
Copy link

@coderabbitai coderabbitai bot Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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.


router.post(
"/v1/screenshots",
defineEventHandler(async (event) => {
Expand Down