perf(review): downscale before/after screenshots before the local VLM - #4687
Merged
Conversation
…#4370) Vision-vision screenshots are captured fullPage, so image HEIGHT scales with a page's full scrollable content even at a fixed 1440px viewport width -- Qwen's dynamic-resolution encoder tokenizes proportional to pixel count, inflating vision prefill cost/latency on long pages. Adds a downscale-to-1280px-longest-edge step scoped ONLY to the bytes sent to the vision model (fetchShotContentBlock), via the same Worker-safe-stub / self-host-real-implementation seam already used for pixel-diff.ts and scroll-gif.ts -- sharp is a native binding and can't run on the Cloudflare Workers runtime, so capture.ts (Worker-reachable) imports only a no-op default; the real resize is swapped in by build-selfhost.mjs's esbuild plugin for the self-host bundle only. Stored/displayed screenshots (the same URL embedded in the PR comment) are completely untouched. Verified with a real local docker build + boot on the target architecture: sharp must be a genuine `dependencies` entry (not devDependencies) since NODE_ENV=production in the runtime image silently skips an explicit `npm install` of a devDependency-classified package -- caught only by actually building and booting the image, not by static review, mirroring how #4354 was diagnosed.
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4687 +/- ##
=======================================
Coverage 94.14% 94.14%
=======================================
Files 436 437 +1
Lines 38530 38531 +1
Branches 14049 14049
=======================================
+ Hits 36273 36274 +1
Misses 1599 1599
Partials 658 658
🚀 New features to boost your workflow:
|
Merged
20 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
shot.ts) are capturedfullPage: true, so image HEIGHT scales with a page's full scrollable content even at a fixed 1440px viewport width — Qwen's dynamic-resolution vision encoder tokenizes proportional to pixel count, inflating vision prefill cost/latency on long pages.fetchShotContentBlockinsrc/review/visual/capture.ts) — the stored/displayed screenshot (the same URL embedded verbatim in the PR comment) is completely untouched.pixel-diff.tsandscroll-gif.ts:sharpis a native binding that can't run on the Cloudflare Workers runtime, socapture.ts(Worker-reachable) imports only a no-op default (src/review/visual/image-downscale.ts); the real resize (src/selfhost/stubs/image-downscale.ts) is swapped in byscripts/build-selfhost.mjs's esbuild plugin only when bundling the self-host entry.sharpis added totest/unit/worker-entry-boundary.test.ts's forbidden-identifier list so a future accidental direct import trips CI.sharpis markedexternalin the--allesbuild bundle (native binary, can't be inlined) and installed separately into the runtime Docker image (mirroring the existingpuppeteer-core/INSTALL_VISUAL_REVIEWpattern, but unconditional — this is a core dependency, not an opt-in sidecar feature).Closes #4370
A real bug caught by empirical verification, not by review
sharpinitially went intopackage.json'sdevDependencies(mirroringpixelmatch/pngjs, which are also self-host-only). That's wrong forsharpspecifically: unlikepixelmatch/pngjs(pure JS, fully bundled intodist/server.mjs, never touch the runtime image'snode_modules),sharphas a native binary that must be installed as a real runtime dependency in the final image. WithsharpunderdevDependencies, the Dockerfile'sRUN npm install sharp@0.34.5 --ignore-scriptsstep silently no-oped —NODE_ENV=production(set earlier inruntime-base) makes npm skip an explicitly-named package that's classified as a devDependency, even though it reports success ("up to date, audited N packages"). The resulting image had nonode_modules/sharpat all; a real vision call would have crashed onERR_MODULE_NOT_FOUNDat the first invocation — no test or typecheck would have caught this, since localnpm installand vitest don't run underNODE_ENV=production.Caught by doing exactly what the issue and #4354 both required: a real local
docker build+ boot, on the target host, followed by an explicit runtime check thatsharpgenuinely resolves and resizes an image inside the built container (not just thatnpm installprinted success). Fixed by movingsharpto regulardependencies. Full before/after verification trail:sharpin devDependencies): image built successfully,npm install sharp@0.34.5 --ignore-scriptsreported "up to date, audited 374 packages... found 0 vulnerabilities" — butnode_modules/sharpwas absent from the final image; a real resize script failed withERR_MODULE_NOT_FOUND.sharptodependencies, rebuilt:node_modules/sharppresent,npm ls sharpconfirmssharp@0.34.5, a real resize script inside the container correctly downscaled a 1600×400 image to 1280×320.dist/server.mjs(not a synthetic script) against real Postgres/Redis on the box's compose network: clean startup,/readyreturns{"ok":true,"checks":{"db":true,"migrations":true,"redis":true}}— no module-resolution errors anywhere in the boot path.All verification ran in an isolated scratch clone + throwaway image tag on edge-nl-01, never touching the live production container; scratch artifacts were removed afterward.
Scope
type(scope): short summaryConventional Commit format, for examplefix(api): restore profile access checks.CONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAME.Closes #123) — a linked open issue is required for every contributor PR.Validation
git diff --checknpm run actionlintnpm run typechecknpm run test:coveragelocally;codecov/patchrequires ≥99% coverage of the lines AND branches you changed (aim for 100% on your diff so CI variance does not fail near the threshold). Global coverage is a non-blocking trend with a loose 90% backstop, not the gate.npm run test:workersnpm run build:mcpnpm run test:mcp-packnpm run ui:openapi:checknpm run ui:lintnpm run ui:typechecknpm run ui:buildnpm audit --audit-level=moderatedocker build+ boot verification on the target architecture (the issue's own explicit requirement, mirroring fix(selfhost): stop .dockerignore from breaking the engine's own build #4354's diagnosis) — see the "real bug" section above.If any required check was skipped, explain why:
actionlint/test:workers/build:mcp/test:mcp-pack/ui:*not applicable).test:coveragewas run targeted on the exact touched test files (image-downscale.test.ts,selfhost-image-downscale-stub.test.ts,worker-entry-boundary.test.ts,visual-capture.test.ts,visual-vision-wiring.test.ts— 133 tests, all green) rather than the full unsharded suite; both new files individually confirmed at ~100% statement coverage via the raw v8 JSON report (the CLI text table silently truncates past a few hundred files in this repo's size, a display quirk unrelated to actual coverage).Safety
UI Evidencesection below. (N/A — no visible UI change; stored/displayed screenshots are explicitly untouched)UI Evidence
N/A — no UI/frontend/docs-visible change. The stored/displayed screenshot bytes are explicitly unaffected by this change (only the bytes sent to the vision model are resized).
Notes