feat: playwright extension #1764
Conversation
|
WalkthroughThis update incorporates a new Playwright extension into the project. The changes update package configuration files to export Playwright modules, include corresponding type definitions, and list the new extension in distribution files. A new TypeScript implementation for the Playwright extension is added, which validates browser options and constructs shell commands for dependency installation and environment setup. Additionally, a new Playwright test task has been introduced along with the integration of the extension into the system’s build configuration. Changes
Suggested Reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
references/v3-catalog/trigger.config.ts (1)
91-91: Consider specifying browser optionsThe playwright extension is added correctly, but you might want to specify which browsers to install, especially if you plan to use browsers other than Chromium as mentioned in the task file comments.
- playwright(), + playwright({ browsers: ["chromium"] }),If you plan to support Firefox and WebKit as mentioned in the task comments, you could use:
- playwright(), + playwright({ browsers: ["chromium", "firefox", "webkit"] }),references/v3-catalog/src/trigger/playwrightTask.ts (1)
42-43: Consider specifying a more robust path for screenshotsSaving the screenshot to a fixed filename in the current working directory might cause issues in production environments or when running multiple tasks.
- const screenshot = await page.screenshot({ path: "screenshot.png" }); + const timestamp = new Date().toISOString().replace(/[:.]/g, '-'); + const path = `./screenshots/google-${timestamp}.png`; + const screenshot = await page.screenshot({ path });You might also need to ensure the directory exists:
import { mkdir } from 'fs/promises'; // In the run function await mkdir('./screenshots', { recursive: true });packages/build/src/extensions/playwright.ts (5)
3-4: Use a custom type alias for better extensibility.While the
PlaywrightBrowserunion type is concise, consider whether you may need more nuanced options (e.g. specific versions, future expansions). Converting this union type into an enum or extending it with additional configuration might offer better extensibility if more browsers or specialized build logic are introduced later.
39-39: Consider dev environment support or warnings.Skipping the entire Playwright layer in the dev environment might simplify local runs, but it also means developers won’t benefit from their chosen browsers or Xvfb setup when testing locally. Consider adding a warning log in dev to make it clear that the Playwright layer is being omitted, or allow an opt-in for local dev scenarios.
45-58: Double-check node installation.The instruction installs
npm, but doesn’t explicitly installnodejs. Depending on the base image,nodejsmight not be installed by default. Consider explicitly installingnodejsto guarantee thatnpmis available in the container.RUN apt-get update && apt-get install -y --no-install-recommends \ + nodejs \ npm \ && apt-get clean && rm -rf /var/lib/apt/lists/*
152-157: Expand OS mapping logic.Currently, macOS-based references (
mac-arm64,mac-15-arm64) are mapped only tolinuxorubuntu-20.04. If Playwright adds new Mac or Linux distributions in the future, or if you need more specific OS handling, this code may break. Consider a more generic mapping or a fallback for unrecognized OS identifiers.
168-177: Headless toggle is well-designed but confirm Xvfb synergy.Granting the user an option for non-headless mode, combined with custom Xvfb commands, is great. However, ensure that Xvfb usage is valid in the final environment (some ephemeral containers or minimal images might lack the necessary capabilities). You might add a note or fallback if Xvfb fails to start.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
packages/build/package.json(3 hunks)packages/build/src/extensions/playwright.ts(1 hunks)references/v3-catalog/package.json(1 hunks)references/v3-catalog/src/trigger/playwrightTask.ts(1 hunks)references/v3-catalog/trigger.config.ts(2 hunks)
🔇 Additional comments (11)
references/v3-catalog/package.json (1)
44-44: Dependency added correctlyThe addition of Playwright as a dependency with version
^1.50.1is correct and allows for compatible updates while maintaining a minimum version requirement.references/v3-catalog/trigger.config.ts (1)
8-8: Import statement looks goodThe import of the playwright extension follows the same pattern as other extensions in this file.
references/v3-catalog/src/trigger/playwrightTask.ts (1)
1-50: Well-documented task implementationThe implementation and documentation look good. The task demonstrates basic Playwright functionality with clear comments on how to extend it.
packages/build/package.json (4)
32-33: Extensions exported correctlyThe addition of the Playwright extension to the tshy exports section looks good.
61-64: Types declaration looks goodThe Playwright extension types are correctly added to the typesVersions section.
181-191: Export configuration is correctThe export configuration for the Playwright extension follows the same pattern as the other extensions.
1-196:✅ Verification successful
Verify the implementation of the playwright extension
The package.json changes look good, but we should verify that the referenced implementation file at
src/extensions/playwright.tsexists and is properly implemented.
🏁 Script executed:
#!/bin/bash # Check if the playwright extension implementation file exists if [ -f "packages/build/src/extensions/playwright.ts" ]; then echo "File exists. Showing its content:" cat "packages/build/src/extensions/playwright.ts" else echo "MISSING: packages/build/src/extensions/playwright.ts does not exist" fiLength of output: 5746
Playwright extension implementation verified
The referenced file at
packages/build/src/extensions/playwright.tsexists and its implementation appears correct. The module exports a factory function that instantiates aPlaywrightExtensionwith proper error handling and build instruction setup (including browser-specific dependencies and environment variable configuration). No changes are needed.packages/build/src/extensions/playwright.ts (4)
31-36: Good constructor validation.The constructor's check ensures that at least one browser is configured, preventing misguided usage. The fallback to
["chromium"]for browsers is also a practical default. Great job!
60-129: Excellent approach to handling duplicate dependencies.Using separate arrays for each browser’s unique system dependencies and then combining them before deduplication ensures minimal wasted space while including all libraries. This is a neat, maintainable pattern. Good work.
138-159: Watch for future Playwright install script changes.The
--dry-runoutput parsing depends on stable script output. If future versions of Playwright change their logging format, thegrep/sedcommands may break. Periodically verify compatibility with newer versions of Playwright or consider more robust parsing (e.g., a JSON-based approach if offered by Playwright).
179-189: Layer-based design is clear and modular.Adding this extension’s instructions and environment variables as a discrete layer (“playwright”) is conceptually clean. It nicely separates responsibilities and ensures the underlying image is extended in a controlled manner.
| for (const browserType of [chromium]) { | ||
| const prefix = (msg: string) => `[${browserType}]: ${msg}`; | ||
|
|
||
| const browser = await browserType.launch(); | ||
| logger.log(prefix("Browser launched")); | ||
|
|
||
| const page = await browser.newPage(); | ||
| logger.log(prefix("New page created")); | ||
|
|
||
| await page.goto("https://google.com"); | ||
| logger.log(prefix("Navigated to google.com")); | ||
|
|
||
| const screenshot = await page.screenshot({ path: "screenshot.png" }); | ||
| logger.log(prefix("Screenshot taken"), { size: screenshot.byteLength }); | ||
|
|
||
| await browser.close(); | ||
| logger.log(prefix("Browser closed")); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to browser operations
The browser operations lack error handling, which could cause the task to fail without proper diagnostics.
- for (const browserType of [chromium]) {
+ for (const browserType of [chromium]) {
const prefix = (msg: string) => `[${browserType}]: ${msg}`;
+ let browser;
- const browser = await browserType.launch();
- logger.log(prefix("Browser launched"));
+ try {
+ browser = await browserType.launch();
+ logger.log(prefix("Browser launched"));
- const page = await browser.newPage();
- logger.log(prefix("New page created"));
+ const page = await browser.newPage();
+ logger.log(prefix("New page created"));
- await page.goto("https://google.com");
- logger.log(prefix("Navigated to google.com"));
+ const response = await page.goto("https://google.com");
+ if (!response || !response.ok()) {
+ throw new Error(`Navigation failed: ${response?.status()}`);
+ }
+ logger.log(prefix("Navigated to google.com"));
- const screenshot = await page.screenshot({ path: "screenshot.png" });
- logger.log(prefix("Screenshot taken"), { size: screenshot.byteLength });
+ const screenshot = await page.screenshot({ path: "screenshot.png" });
+ logger.log(prefix("Screenshot taken"), { size: screenshot.byteLength });
- await browser.close();
- logger.log(prefix("Browser closed"));
+ await browser.close();
+ logger.log(prefix("Browser closed"));
+ } catch (error) {
+ logger.error(prefix("Error during browser automation"), { error });
+ if (browser) {
+ await browser.close().catch(e => logger.error(prefix("Error closing browser"), { error: e }));
+ }
+ throw error;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (const browserType of [chromium]) { | |
| const prefix = (msg: string) => `[${browserType}]: ${msg}`; | |
| const browser = await browserType.launch(); | |
| logger.log(prefix("Browser launched")); | |
| const page = await browser.newPage(); | |
| logger.log(prefix("New page created")); | |
| await page.goto("https://google.com"); | |
| logger.log(prefix("Navigated to google.com")); | |
| const screenshot = await page.screenshot({ path: "screenshot.png" }); | |
| logger.log(prefix("Screenshot taken"), { size: screenshot.byteLength }); | |
| await browser.close(); | |
| logger.log(prefix("Browser closed")); | |
| } | |
| for (const browserType of [chromium]) { | |
| const prefix = (msg: string) => `[${browserType}]: ${msg}`; | |
| let browser; | |
| try { | |
| browser = await browserType.launch(); | |
| logger.log(prefix("Browser launched")); | |
| const page = await browser.newPage(); | |
| logger.log(prefix("New page created")); | |
| const response = await page.goto("https://google.com"); | |
| if (!response || !response.ok()) { | |
| throw new Error(`Navigation failed: ${response?.status()}`); | |
| } | |
| logger.log(prefix("Navigated to google.com")); | |
| const screenshot = await page.screenshot({ path: "screenshot.png" }); | |
| logger.log(prefix("Screenshot taken"), { size: screenshot.byteLength }); | |
| await browser.close(); | |
| logger.log(prefix("Browser closed")); | |
| } catch (error) { | |
| logger.error(prefix("Error during browser automation"), { error }); | |
| if (browser) { | |
| await browser.close().catch(e => logger.error(prefix("Error closing browser"), { error: e })); | |
| } | |
| throw error; | |
| } | |
| } |
|
when will this merge? |
Closes #
✅ Checklist
Testing
Made sure it works with all browsers
Screenshots
Summary by CodeRabbit