Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the build/test tooling around Lexxy’s browser tests and asset bundling, and adds a precompressed Brotli asset for the minified bundle.
Changes:
- Configure Playwright to use system Chromium/Firefox executables when present.
- Mark
@rails/activestorageas an external dependency in the Rollup build. - Add a Playwright npm script intended to run browser tests without WebKit, and add
lexxy.min.js.brto assets.
Reviewed changes
Copilot reviewed 3 out of 9 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| test/browser/playwright.config.js | Adds executable path detection for system-installed Chromium/Firefox and passes it via launchOptions. |
| rollup.config.mjs | Treats @rails/activestorage as an external module for the Rails asset build output. |
| package.json | Adds a “no webkit” Playwright test script. |
| app/assets/javascript/lexxy.min.js.br | Adds Brotli-compressed minified asset output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| { name: "chromium", use: { ...devices["Desktop Chrome"], | ||
| launchOptions: chromiumPath ? { executablePath: chromiumPath } : {} } }, | ||
| { name: "firefox", use: { ...devices["Desktop Firefox"], | ||
| launchOptions: firefoxPath ? { executablePath: firefoxPath } : {} } }, |
There was a problem hiding this comment.
launchOptions is always being set, and when the executable path isn't present it becomes an empty object. That can unintentionally override any default/device launchOptions (or any future shared use.launchOptions) with {}. Prefer omitting launchOptions entirely when no override is needed (e.g., set it to undefined or conditionally spread it).
| { name: "chromium", use: { ...devices["Desktop Chrome"], | |
| launchOptions: chromiumPath ? { executablePath: chromiumPath } : {} } }, | |
| { name: "firefox", use: { ...devices["Desktop Firefox"], | |
| launchOptions: firefoxPath ? { executablePath: firefoxPath } : {} } }, | |
| { name: "chromium", use: { | |
| ...devices["Desktop Chrome"], | |
| ...(chromiumPath && { launchOptions: { executablePath: chromiumPath } }), | |
| } }, | |
| { name: "firefox", use: { | |
| ...devices["Desktop Firefox"], | |
| ...(firefoxPath && { launchOptions: { executablePath: firefoxPath } }), | |
| } }, |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 9 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
- skip webkit - use local chromium
f6bd49c to
3c26404
Compare
Looks like there may be some challenges getting
playwrightworking nicely on Archchromiumfirefox