Repository navigation
Lite: capture PR screenshots in a container - #15381
Closed
PavelLaptev wants to merge 3 commits into
Closed
PavelLaptev wants to merge 3 commits into
PavelLaptev wants to merge 3 commits into
Conversation
PavelLaptev
force-pushed
the
lite-screenshots-container
branch
4 times, most recently
from
August 17, 2026 22:37
c350777 to
0a87e68
Compare
screenshots.spec.ts emptied its output directory in test.beforeAll. Playwright restarts the worker after a failure and runs the hook again, so any failing surface deleted every surface captured before it: a run reporting "3 passed" could leave nothing on disk at all. The intent was right — a PNG left by an earlier run must not survive a rerun in which its own surface failed, or it gets compared as though this run produced it. Only the hook was wrong. Moved to a globalSetup shared by both configs, which runs exactly once, and left the mkdir in beforeAll where repeating is harmless.
PavelLaptev
force-pushed
the
lite-screenshots-container
branch
from
August 17, 2026 23:02
0a87e68 to
4e8edd8
Compare
Capture runs on a developer's machine because it was abandoned on the Linux runners: three of eight surfaces hung under the bare X server, and every log carried "sandboxed_renderer.bundle.js script failed to run". That message is Chromium's setuid sandbox failing to initialise — not something a hosted runner lets you change, and something a container does. Adds a second Playwright config and an image that runs the existing catalogue under Xvfb. The host checkout is mounted read-only and mirrored into a volume, so a macOS node_modules and target/ are never written to; the Linux Electron, `but` and napi SDK live in volumes of their own. The other thing it buys is commensurability. A before/after pair only means something if both halves were rendered the same way, and font hinting and device scale differ between two machines before the UI is even considered. With the OS, font set and rasteriser fixed, the two sides no longer have to be captured by the same person. The switches live in the new config rather than the shared launch, so an ordinary local run still renders the way the developer's machine does.
Delete before this branch merges. The claim under test is specifically about the Linux runners, and there is no container runtime on the machine this was written on — so the only place it can be answered is a runner. Captures one side and uploads it as an artifact; a surface that hangs fails the job on the spec's own 180s budget. Not a before/after run.
PavelLaptev
force-pushed
the
lite-screenshots-container
branch
from
August 17, 2026 23:22
4e8edd8 to
333fd22
Compare
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.
🧢 Changes
Follows up on #15350, and on @samhh's note there that the plan was a second Playwright config running under Docker for cross-platform screenshot support.
apps/lite/e2e/docker/Dockerfile— Playwright'sv1.58.2-noblebase (Chromium's shared libraries, Xvfb, fonts), plus a Rust toolchain forbutand the napi SDK,libxss1,rsync, and a pinned fallback font set.LANG=C.UTF-8,TZ=UTC, and a 1600×1000 Xvfb screen.apps/lite/e2e/docker/run-capture.sh— the entrypoint: mirror the checkout, build, capture.apps/lite/e2e/docker/capture.sh— the host wrapper,capture.sh before/capture.sh after.apps/lite/e2e/playwright.docker.config.ts— the second config. Capture spec only, one worker, two retries, and the Chromium switches.apps/lite/e2e/test.ts— a typedelectronArgsoption, defaulting to empty, spread into the Electron launch..github/workflows/lite-screenshots-container-proof.yml— temporary, delete before merge. Its own commit so it drops cleanly.☕️ Reasoning
The failure that killed CI capture is container-shaped. Every abandoned run logged
sandboxed_renderer.bundle.js script failed to run— Chromium's setuid sandbox failing to initialise. A hosted runner gives no way to change that. A container does, which is why the fix is an image rather than another attempt at the same job.The second thing it buys is commensurability. A before/after pair only means something if both halves were rendered the same way, and font hinting and device scale differ between two contributors' machines before the UI is even considered. With one OS, one font set and one rasteriser, the two halves no longer have to be captured by the same person.
The host checkout is mounted read-only and mirrored into a volume, not built in place. A macOS checkout carries a macOS Electron and macOS native modules under
node_modules, plus a macOStarget/.*.nodeis excluded from the mirror and the napi SDK rebuilt inside, so the Linux artefact replaces the macOS one rather than racing it. Nothing in a developer's tree is written to.The switches are an option on the fixture, not an env var. They live visibly in the config a reader would look in, and the default path stays byte-identical — which is the same reason #15350 dropped the GPU flags rather than keeping them.
The whole
screenshots/directory is mounted, not the one output directory, because the spec clears its own output directory before capturing andrmon a mount point fails withEBUSY.None of this has run. There is no container runtime on the machine it was written on — no Docker, Podman, colima or nerdctl. That is what the temporary workflow is for: the claim is specifically about the Linux runners, so a runner is the only place it can be answered.
What has been checked:
pnpm -F @gitbutler/lite checkpasses and--listFilesconfirms the new config is genuinely in the program;oxlint,oxfmt,prettier,knip:prodandknip:non-prodare clean.Two risks in the proving run, both about the runner rather than the design:
@gitbutler/but-sdk'sbuildis four cargo invocations, so the SDK compiles twice over. If it times out, trimming the graph variant is the first lever.📌 Follow-ups
lite-screenshots-container-proof.ymlbefore merge.but unapplyand so no stacked-branch caveat.Error: Process failed to launch!on this machine, onmasteras well as here. Unrelated to this branch, but it blocks the host capture flow.