Repository navigation
Get Playwright from the venetian gem - #190
Merged
Merged
Conversation
Upright depended on playwright-ruby-client and installed the Playwright driver from npm. The two had to be the same version, and the version was set in four places: Upright::PLAYWRIGHT_VERSION, the gemspec constraint, package.json and the generated Dockerfile. A Dependabot update that changed only the gemspec constraint left a 1.63 client talking to a 1.59 driver, and the Playwright probe tests failed. The venetian gem includes the Playwright driver and Node.js for each platform and pins the matching playwright-ruby-client, so Gemfile.lock now sets one Playwright version for both. Upright requires venetian/playwright, which does not load Capybara, and starts the driver with Venetian.start. config.playwright_cli_path still overrides the driver. The generated Dockerfile no longer installs Node.js or npm. The build stage copies the venetian gem to /venetian, and the final stage bind-mounts it to install Chromium, so the browser layer is rebuilt only when the venetian version changes. Upright::PLAYWRIGHT_VERSION is removed. CI, bin/setup and bin/smoke_test install Chromium with bundle exec playwright. Playwright moves from 1.59 to 1.63. Also removes the Playwright server container from the dummy app's docker-compose.yml and the README's deploy example, and the dummy app's config/playwright.yml. Nothing has connected to that server since 0.4.
There was a problem hiding this comment.
🟡 Changes recommended
The dependency range still permits a client/driver mismatch, and the upgrade checklist leaves an obsolete Docker COPY that breaks builds.
2 open findings
What changed in this PR
Moves Upright’s browser probes to Venetian’s bundled Playwright driver and Node.js, reducing client/driver version drift.
Changes:
- Replaces the direct Playwright dependency and updates driver startup.
- Uses Venetian for browser installation in Docker, CI, and setup.
- Documents migration steps and removes unused Playwright server configuration.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
| File | Description |
|---|---|
| upright.gemspec | Replaces the Playwright dependency with Venetian. |
| UPGRADING.md | Adds browser and Docker migration instructions. |
| test/dummy/docker-compose.yml | Removes the unused Playwright server. |
| test/dummy/config/playwright.yml | Deletes obsolete server configuration. |
| RELEASING.md | Updates browser compatibility checks. |
| README.md | Updates deployment and Playwright guidance. |
| lib/upright/version.rb | Removes the separate Playwright version constant. |
| lib/upright/configuration.rb | Defaults to the bundled driver. |
| lib/generators/upright/install/templates/upright.rb | Documents the new driver default. |
| lib/generators/upright/install/templates/Dockerfile | Installs Chromium through Venetian. |
| Gemfile.lock | Locks Venetian and matching client dependencies. |
| CHANGELOG.md | Records migration and cleanup changes. |
| bin/smoke_test | Removes the npm driver override. |
| bin/setup | Installs browsers through Bundler. |
| bin/screenshots | Clarifies npm prerequisites in a comment. |
| app/models/upright/probes/playwright/base.rb | Requires Venetian’s Playwright entry point. |
| app/models/concerns/upright/playwright/lifecycle.rb | Starts browsers through Venetian. |
| .github/workflows/ci.yml | Installs Chromium through Bundler. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
venetian 0.3.0 required playwright-ruby-client >= 1.63.0, so an app that kept 0.3.0 could resolve a 1.64 client against the 1.63 driver. 0.3.1 pins ~> 1.63.0, so the gemspec now requires it. The UPGRADING steps did not say to remove the final stage's COPY of /tmp/playwright_version. Once the build stage stops writing that file, the COPY fails the image build.
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.
Upright depended on
playwright-ruby-clientand installed the Playwright driver from npm. The two have to be the same version, and that version was set in four places:Upright::PLAYWRIGHT_VERSION, the gemspec constraint,package.jsonand the generated Dockerfile. #153 changed only the gemspec constraint, which left a 1.63 client talking to a 1.59 driver, and the Playwright probe tests failed.venetian publishes a gem for each platform that includes the Playwright driver and Node.js, and pins the matching
playwright-ruby-client.Gemfile.locknow sets one Playwright version for both.Changes
venetian ~> 0.3in place ofplaywright-ruby-client. Playwright moves from 1.59 to 1.63.venetian/playwrightand start the driver withVenetian.start. That entry point does not load Capybara.capybara-playwright-driveris installed as a dependency of venetian but is never required.config.playwright_cli_path/PLAYWRIGHT_CLI_PATHstill override the driver./venetian. The final stage bind-mounts it and runs/venetian/exe/playwright install --with-deps chromium, so the browser layer is rebuilt only when the venetian version changes, as with the previous npm step.Upright::PLAYWRIGHT_VERSION: a Dockerfile generated by 0.4 reads it, so UPGRADING.md has a step for updating the Dockerfile.bin/setupandbin/smoke_testinstall Chromium withbundle exec playwright.package.jsonstays forbin/screenshotsonly.docker-compose.ymland the README's deploy example, and the dummy app'sconfig/playwright.yml. Nothing has connected to that server since 0.4.Testing
bin/rails test: 490 runs, 0 failures, including the Playwright probe tests against the bundled 1.63 driver.bin/rubocopandbin/brakemanare clean.bin/smoke_test: a newly generated app runs a Playwright probe end to end and records the trace artifact.Venetian.startlaunched Chromium and rendered a page, with nonodeornpmon the PATH and Capybara not loaded. After a Gemfile change unrelated to venetian, the browser install layer stayed cached. The venetian gem adds 135 MB to the image; the Node.js and npm packages it replaces are no longer installed.venetian's release task does not yet verify checksums of the Node.js and
playwright-coredownloads; that is tracked in jmalcic/venetian#21.Closes #153.