feat: add builds/driver for playwright#884
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds Playwright driver artifacts under Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Adapter as PlaywrightBinary
participant Builds as /builds/
Client->>Adapter: GET /builds/
Adapter->>Adapter: fetch package versions (recent 20)
loop each version
Adapter->>Adapter: determine isBeta(version)
loop each arch in PLAYWRIGHT_DRIVER_ARCHS
Adapter->>Builds: add "playwright-<version>-<arch>.zip" (url, size '-', date)
end
alt isBeta
Adapter->>Builds: also add same items under /builds/driver/next/
end
end
Adapter->>Client: return builds tree (includes driver/ and optional driver/next/)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
Summary of ChangesHello @akkuman, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Playwright binary adapter by integrating support for Playwright driver builds. It allows for the direct listing and download of specific Playwright driver versions across multiple operating systems and architectures, including a mechanism to handle beta releases. This feature streamlines the process of acquiring Playwright drivers, making them more accessible for various development and testing environments. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for mirroring Playwright driver builds. The implementation correctly adds the driver directory, but there's a logic issue in how beta and stable versions are organized into subdirectories. I've provided a suggestion to fix this. Additionally, I've pointed out an opportunity to improve code maintainability by extracting a hardcoded array into a constant. Finally, I've recommended adding more comprehensive tests for the new functionality to ensure it works as expected.
| assert.equal(result.items[5].name, 'ffmpeg/'); | ||
| assert.equal(result.items[6].name, 'winldd/'); | ||
| assert.equal(result.items[7].name, 'android/'); | ||
| assert.equal(result.items[8].name, 'driver/'); |
There was a problem hiding this comment.
While this assertion correctly checks for the presence of the driver/ directory, the new functionality for populating this directory and its potential next/ subdirectory is not tested. It would be beneficial to add more tests to verify that:
- The
/builds/driver/directory contains the correct stable driver files. - If beta versions are present in the mock data, the
next/directory is created under/builds/driver/and contains the beta driver files. - The URLs for the driver files are constructed correctly.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/common/adapter/binary/PlaywrightBinary.test.ts (1)
74-95: Consider adding test coverage for the driver directory.The test correctly verifies that
driver/appears in the/builds/listing. However, unlike other directories (chromium, firefox, etc.), thedriverdirectory is not included in thenamesarray and therefore isn't tested for navigation in the loop at lines 96-158.Consider adding test coverage similar to the existing browser directories to ensure the driver directory structure works correctly, especially for the beta/next subdirectory handling.
app/common/adapter/binary/PlaywrightBinary.ts (1)
420-437: Use consistent string quotes.Lines 420, 422, and 437 use double quotes (
"), while the rest of the file consistently uses single quotes (').Consider changing to single quotes for consistency:
- this.dirItems["/builds/driver/"] = [] + this.dirItems['/builds/driver/'] = [] if (packageVersions.some(version => version.includes('-beta-'))) { - this.dirItems["/builds/driver/"].push({ + this.dirItems['/builds/driver/'].push({ name: 'next/',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/common/adapter/binary/PlaywrightBinary.ts(2 hunks)test/common/adapter/binary/PlaywrightBinary.test.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/common/adapter/binary/PlaywrightBinary.test.ts (1)
app/common/adapter/binary/PrismaBinary.ts (1)
result(85-144)
🔇 Additional comments (2)
app/common/adapter/binary/PlaywrightBinary.ts (2)
385-391: LGTM!The driver directory entry is added correctly using the same pattern as browser directories.
432-432: Architecture names verified as correct; no changes needed.The verification confirms all specified architectures are valid and available from Playwright's official driver repository. While the naming convention does mix underscores (
win32_x64) and hyphens (mac-arm64,linux-arm64), this reflects Playwright's own driver artifact naming—not an error in the code.
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for Playwright driver downloads organized by version and architecture. The implementation follows the pattern used in playwright-python and playwright-go projects to expose driver binaries for direct download without requiring Node.js installation.
- Adds a new
/builds/driver/directory structure with multi-architecture support (Windows x64, macOS ARM64/x64, Linux ARM64/x64) - Beta versions are exposed under a
nextsubdirectory for preview access - Generates driver listings for the most recent 20 package versions
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 14 comments.
| File | Description |
|---|---|
| app/common/adapter/binary/PlaywrightBinary.ts | Implements driver directory structure with architecture-specific download URLs and beta version handling |
| test/common/adapter/binary/PlaywrightBinary.test.ts | Updates test assertions to account for the new driver directory in builds listing |
Comments suppressed due to low confidence (2)
app/common/adapter/binary/PlaywrightBinary.ts:392
- Avoid automated semicolon insertion (94% of all statements in the enclosing function have an explicit semicolon).
})
app/common/adapter/binary/PlaywrightBinary.ts:430
- Avoid automated semicolon insertion (94% of all statements in the enclosing function have an explicit semicolon).
})
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/common/adapter/binary/PlaywrightBinary.ts (2)
16-16: Consider using single quotes for consistency.The array elements use double quotes, but the project's Prettier configuration specifies
singleQuote: true. While the linter should catch this, consider applying the style directly.Apply this diff:
-const PLAYWRIGHT_DRIVER_ARCHS = ["win32_x64", "mac-arm64", "mac", "linux-arm64", "linux"]; +const PLAYWRIGHT_DRIVER_ARCHS = ['win32_x64', 'mac-arm64', 'mac', 'linux-arm64', 'linux'];
433-453: Consider refactoring for improved code quality.Several optional improvements to enhance maintainability:
- The
driverFileNamevariable is defined but its value is repeated in the loop body. Consider computing it once per version iteration.- The
driverURLvariable usesletbut could beconstwith a ternary operator.- The beta version check
version.includes('-beta-')is called twice and could be cached asisBeta.- String literals should use single quotes for consistency with the project's Prettier configuration.
Apply this diff to address these improvements:
packageVersions.forEach(version => { // https://github.com/playwright-community/playwright-go/blob/56e30d60f8b42785982469eaca6ad969bc2e1946/run.go#L341-L374 + const isBeta = version.includes('-beta-'); PLAYWRIGHT_DRIVER_ARCHS.forEach(arch => { - const driverFileName = `playwright-${version}-${arch}.zip` - let driverURL = DOWNLOAD_HOST + `builds/driver/${driverFileName}` - if (version.includes('-beta-')) { - driverURL = DOWNLOAD_HOST + `builds/driver/next/${driverFileName}` - } + const driverFileName = `playwright-${version}-${arch}.zip`; + const driverURL = isBeta + ? DOWNLOAD_HOST + `builds/driver/next/${driverFileName}` + : DOWNLOAD_HOST + `builds/driver/${driverFileName}`; const driverItem = { name: driverFileName, isDir: false, url: driverURL, size: '-', date: version, - } - this.dirItems["/builds/driver/"].push(driverItem) - if (version.includes('-beta-')) { - this.dirItems["/builds/driver/next/"].push(driverItem) + }; + this.dirItems['/builds/driver/'].push(driverItem); + if (isBeta) { + this.dirItems['/builds/driver/next/'].push(driverItem); } }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/common/adapter/binary/PlaywrightBinary.ts(3 hunks)test/common/adapter/binary/PlaywrightBinary.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/common/adapter/binary/PlaywrightBinary.test.ts
🔇 Additional comments (2)
app/common/adapter/binary/PlaywrightBinary.ts (2)
386-392: LGTM!The driver directory entry follows the same pattern as the browser directories and integrates cleanly with the existing structure.
420-453: Review comment is incorrect.The driver logic at lines 420-453 is already covered by existing test cases in
test/common/adapter/binary/PlaywrightBinary.test.ts(lines 86-103). The tests verify:
- Driver directory structure creation (
/builds/driver/)- Separation of stable and beta versions (
/builds/driver/next/)- File naming and URL generation with the expected pattern
- The
next/subdirectory entry creation when beta versions existNo action is required; the code changes are already tested.
Likely an incorrect or invalid review comment.
akkuman
force-pushed
the
feat-playwright-driver
branch
from
6179697 to
e2ddf9f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
app/common/adapter/binary/PlaywrightBinary.ts (2)
420-432: Add missing semicolon for consistency.The beta version detection and
next/directory initialization logic is correct. However, the statement on line 430 is missing a trailing semicolon.Apply this diff:
this.dirItems['/builds/driver/'].push({ name: 'next/', isDir: true, url: '', size: '-', date: 'next', - }); + }); this.dirItems['/builds/driver/next/'] = []; }Wait, looking more carefully:
- }); + });Actually on line 430, it shows
});which already has the semicolon closing the push statement. Let me re-examine... The issue is on line 430 the closing should be});not just}). Let me check the annotated code again:Line 424-430 shows:
...If line 430 shows
});then it already has the semicolon. Let me not comment on this unless I'm certain there's an issue.
[scratchpad_end] -->
433-452: Separate beta and stable driver files into correct directories.Beta driver files are incorrectly being added to both
/builds/driver/and/builds/driver/next/. This means/builds/driver/contains both stable and beta releases, when it should contain only stable releases.Expected behavior:
/builds/driver/should contain only stable driver files/builds/driver/next/should contain only beta driver filesBased on learnings
Apply this diff to properly separate the files:
packageVersions.forEach(version => { PLAYWRIGHT_DRIVER_ARCHS.forEach(arch => { + const isBeta = version.includes('-beta-'); const driverFileName = `playwright-${version}-${arch}.zip`; - const driverURL = version.includes('-beta-') + const driverURL = isBeta ? DOWNLOAD_HOST + `builds/driver/next/playwright-${version}-${arch}.zip` : DOWNLOAD_HOST + `builds/driver/playwright-${version}-${arch}.zip`; const driverItem = { name: driverFileName, isDir: false, url: driverURL, size: '-', date: version, }; - this.dirItems['/builds/driver/'].push(driverItem); - if (version.includes('-beta-')) { - this.dirItems['/builds/driver/next/'].push(driverItem); - } + const targetDir = isBeta ? '/builds/driver/next/' : '/builds/driver/'; + this.dirItems[targetDir].push(driverItem); }); });
🧹 Nitpick comments (2)
test/common/adapter/binary/PlaywrightBinary.test.ts (1)
86-107: Fix string quote style for consistency.The test logic correctly validates driver directory contents, including file names, URLs, and metadata. However, string literals use double quotes instead of single quotes, inconsistent with the project's Prettier configuration (
singleQuote: true).Apply this diff to fix the quote style:
const driverDirs = [ - "driver/", - "driver/next/" + 'driver/', + 'driver/next/', ]; for (const dirname of driverDirs) { result = await binary.fetch(`/builds/${dirname}`); for (const item of result.items) { if (item.isDir) { - assert.ok(item.name === "next/"); + assert.ok(item.name === 'next/'); } else {app/common/adapter/binary/PlaywrightBinary.ts (1)
386-392: Add missing semicolon for consistency.The logic correctly adds the
driver/directory entry to the builds listing. However, the statement is missing a trailing semicolon, which is inconsistent with the coding style used throughout this file.Apply this diff:
buildDirs.push({ name: 'driver/', isDir: true, url: '', size: '-', date: nowDateISO, - }) + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/common/adapter/binary/PlaywrightBinary.ts(3 hunks)test/common/adapter/binary/PlaywrightBinary.test.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/common/adapter/binary/PlaywrightBinary.test.ts (1)
app/common/adapter/binary/PrismaBinary.ts (1)
result(85-144)
🔇 Additional comments (3)
test/common/adapter/binary/PlaywrightBinary.test.ts (2)
74-74: LGTM - Correct item count adjustment.The expected count properly reflects the addition of the
driver/directory to the builds listing.
83-83: LGTM - Validates new driver directory.The assertion correctly checks for the presence of the
driver/directory in the builds listing.app/common/adapter/binary/PlaywrightBinary.ts (1)
16-16: LGTM - Well-defined constant.The
PLAYWRIGHT_DRIVER_ARCHSconstant properly extracts the supported architectures, improving maintainability and readability.
akkuman
force-pushed
the
feat-playwright-driver
branch
from
e2ddf9f to
b32d6e4
Compare
|
@elrrrrrrr 下次部署这个也带上😁 |
Head branch was pushed to by a user without write access
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/common/adapter/binary/PlaywrightBinary.ts (1)
448-450: Verify the intended directory structure for beta drivers.The current implementation adds all drivers (both stable and beta) to
/builds/driver/(line 448), and additionally adds beta drivers to/builds/driver/next/(line 450). This means beta drivers appear in both directories.Previous review comments suggested that beta drivers should only appear in
/builds/driver/next/and stable drivers only in/builds/driver/(similar to the suggested fix by coderabbitai[bot] and gemini-code-assist[bot]). Those comments were marked as addressed, but the current code doesn't match that structure.If the current behavior is intentional (all drivers in the main directory, with a filtered beta view in
next/), then this is fine. Otherwise, consider whether beta drivers should be exclusive to thenext/directory.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/common/adapter/binary/PlaywrightBinary.ts(3 hunks)test/common/adapter/binary/PlaywrightBinary.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/common/adapter/binary/PlaywrightBinary.test.ts
🔇 Additional comments (2)
app/common/adapter/binary/PlaywrightBinary.ts (2)
16-17: LGTM!Good practice extracting the architecture list as a named constant. This improves maintainability and follows the pattern suggested in previous reviews.
387-393: LGTM!The driver directory entry is properly added to the builds structure, consistent with the existing browser directory entries.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #884 +/- ##
==========================================
- Coverage 95.35% 95.19% -0.17%
==========================================
Files 197 197
Lines 23856 23948 +92
Branches 2504 2495 -9
==========================================
+ Hits 22749 22797 +48
- Misses 1107 1151 +44 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Head branch was pushed to by a user without write access
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/common/adapter/binary/PlaywrightBinary.ts (2)
437-440: Consider simplifying URL construction to reduce duplication.The URL construction repeats
builds/driver/in both branches of the ternary. This can be consolidated for better maintainability.Apply this diff to simplify:
const isBetaVersion = version.includes('-beta-'); const driverFileName = `playwright-${version}-${arch}.zip`; - const driverURL = isBetaVersion - ? DOWNLOAD_HOST + `builds/driver/next/${driverFileName}` - : DOWNLOAD_HOST + `builds/driver/${driverFileName}`; + const driverPath = `builds/driver/${isBetaVersion ? 'next/' : ''}${driverFileName}`; + const driverURL = DOWNLOAD_HOST + driverPath;
422-450: Consider using single quotes for string consistency.Several string literals use double quotes (e.g., lines 422, 425, 432, 448, 450) while the rest of the file uses single quotes. For consistency with the project's style conventions, consider using single quotes throughout.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/common/adapter/binary/PlaywrightBinary.ts(3 hunks)
🔇 Additional comments (2)
app/common/adapter/binary/PlaywrightBinary.ts (2)
16-17: LGTM: Architecture constant is well-defined.The
PLAYWRIGHT_DRIVER_ARCHSconstant appropriately centralizes the supported architectures and aligns with the Playwright Go reference implementation.
387-393: LGTM: Driver directory entry follows existing pattern.The driver directory entry is correctly added to
buildDirsand follows the same structure as browser directories.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/common/adapter/binary/PlaywrightBinary.ts (1)
16-17: Consider documenting architecture list maintenance.The architecture list is derived from the Playwright Go implementation. Consider adding a note or TODO to periodically verify this list against Playwright's official supported platforms, especially when upgrading to new Playwright versions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/common/adapter/binary/PlaywrightBinary.ts(3 hunks)
🔇 Additional comments (4)
app/common/adapter/binary/PlaywrightBinary.ts (4)
16-17: LGTM: Constant extraction improves maintainability.The architecture list is now properly extracted to a named constant at the top level, which addresses previous feedback and makes it easier to maintain.
387-393: LGTM: Directory entry follows established pattern.The new 'driver/' directory entry is consistent with the existing browser directory structure.
434-451: LGTM: Directory separation logic is correct.The critical issue from previous reviews has been properly fixed. Each driver file is now pushed to exactly one directory:
- Stable versions →
/builds/driver/- Beta versions →
/builds/driver/next/The code correctly computes
targetDirbased onisBetaVersionand pushes each item only once, eliminating the previous duplication problem.
421-451: Verify test coverage for driver logic.Per fengmk2's earlier request, ensure that unit tests cover the driver directory logic, including:
- Stable versions appearing only in
/builds/driver/- Beta versions appearing only in
/builds/driver/next/- Correct URL construction for both paths
- Proper initialization of the
next/subdirectory when beta versions existThe AI summary mentions test updates, but please confirm tests are comprehensive.
[skip ci] ## 4.12.0 (2025-11-18) * feat: add builds/driver for playwright (#884) ([93034bf](93034bf)), closes [#884](#884) [#883](#883) * feat: add ffmpeg-builds binary mirror (#879) ([d7de1cd](d7de1cd)), closes [#879](#879) * feat: try to avoid recalculating integrity (#880) ([4859ce8](4859ce8)), closes [#880](#880) * chore: add deployment test (#874) ([0d32146](0d32146)), closes [#874](#874) * chore: Configure Renovate (#845) ([f3f3584](f3f3584)), closes [#845](#845) * chore(deps): update actions/attest-build-provenance action to v3 (#847) ([957f43d](957f43d)), closes [#847](#847) * chore(deps): update actions/checkout action to v5 (#849) ([e8a3ee5](e8a3ee5)), closes [#849](#849) * chore(deps): update actions/first-interaction action to v3 (#850) ([35a7be3](35a7be3)), closes [#850](#850) * chore(deps): update actions/setup-node action to v6 (#851) ([3671c52](3671c52)), closes [#851](#851) * chore(deps): update dependency lint-staged to v16 (#853) ([e1848c7](e1848c7)), closes [#853](#853) * chore(deps): update dependency mocha to v11.7.4 (#846) ([4213bd8](4213bd8)), closes [#846](#846) * chore(deps): update dependency oxlint-tsgolint to ^0.3.0 (#863) ([0c987a7](0c987a7)), closes [#863](#863) * chore(deps): update dependency oxlint-tsgolint to ^0.5.0 (#875) ([95543b1](95543b1)), closes [#875](#875) * chore(deps): update dependency oxlint-tsgolint to ^0.7.0 (#885) ([06a3ae1](06a3ae1)), closes [#885](#885) * chore(deps): update dependency type-fest to v5 [skip ci] (#871) ([01385f4](01385f4)), closes [#871](#871) * fix(deps): update dependency npm-package-arg to v13 (#866) ([c3059c7](c3059c7)), closes [#866](#866) * fix(deps): update dependency read-env-value to v2 (#869) ([098c75a](098c75a)), closes [#869](#869) * fix(deps): update dependency ssri to v13 (#870) ([0f11e77](0f11e77)), closes [#870](#870) * fix(deps): update dependency validate-npm-package-name to v7 [skip ci] (#873) ([6c29f08](6c29f08)), closes [#873](#873) * refactor: use all in one egg v4 (#855) ([89f1250](89f1250)), closes [#855](#855)
|
🎉 This PR is included in version 4.12.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
发现一个长期问题，playwright 的这种不按目录分版本的更新速度，30 年后这个 html 页面是没法打开了(;¬_¬) |
这种在 s3 上都不是一个好的目录结构，list 这个目录的压力会很大，也不知道上游是为什么搞成这样 |
我猜访问都是直接拼接 url 的，所以没有考虑 list 的方式。 |
3 participants
https://playwright-ruby-client.vercel.app/docs/article/guides/download_playwright_driver#directly-download-driver-without-nodejs-installation
https://github.com/microsoft/playwright-python/blob/75ceca04b7f4b29c1c026130696568fca68c9e88/setup.py#L100-L117
https://github.com/playwright-community/playwright-go/blob/56e30d60f8b42785982469eaca6ad969bc2e1946/run.go#L341-L374
close #883
Summary by CodeRabbit
New Features
Tests