feat(cli): add --no-browser-deps to skip browser OS dependency install - #1400
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (18)
WalkthroughAdds Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant CommandHandler
participant installBrowsers
participant installBrowserList
participant Playwright
CLI->>CommandHandler: Parse --no-browser-deps
CommandHandler->>installBrowsers: Pass browserDeps
installBrowsers->>installBrowserList: Forward browserDeps
installBrowserList->>Playwright: Run install with or without --with-deps
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/install/browserDepsFlag.test.ts`:
- Around line 49-51: Add a Commander integration test alongside the direct
mergedBrowserDeps tests that calls parseAsync with ["--no-browser-deps",
"browsers"] and asserts the browsers command action receives false. Keep the
existing helper-level test unchanged and configure the command hierarchy so the
flag is parsed on the parent before the subcommand.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: eeee8246-c0c7-4d4d-813c-74aa6ce31453
⛔ Files ignored due to path filters (1)
src/commands/__snapshots__/help.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (18)
.changeset/no-browser-deps-flag.mdsrc/commands/flows/buildFlowsRunDeps.tssrc/commands/flows/hybridRun.test.tssrc/commands/flows/run.register.tssrc/commands/flows/runDefaults.handle.test.tssrc/commands/flows/runDefaults.reporterWiring.test.tssrc/commands/flows/runStagedFlows.test.tssrc/commands/install/all.tssrc/commands/install/browserDepsFlag.test.tssrc/commands/install/browserDepsFlag.tssrc/commands/install/browsers.fixtures.tssrc/commands/install/browsers.test.tssrc/commands/install/browsers.tssrc/commands/install/index.tssrc/commands/program.test.tssrc/domains/install/browsers.tssrc/domains/runner/run.fixtures.tssrc/domains/runner/runInternals.ts
796d51d to
65779e5
Compare
65779e5 to
7dcf05a
Compare
Note
PR body AI drafted & edited as needed
Overview of Changes
On Linux the CLI unconditionally runs
playwright install --with-deps <browser>before web flows. The--with-depsstep shells out toapt-get, which requires root even when every system library is already installed (apt needs its lock). On non-root machines without sudo, Playwright falls back tosu root -c "apt-get …", which either aborts (playwright install chromium failed: Password:) or hangs indefinitely waiting for a password — there is no timeout. This makesflows rununusable on non-root CI runners regardless of how well-provisioned the image is.--no-browser-deps(onflows run,install, andinstall browsers): drops--with-depsfrom the Playwright invocation, so only the browsers themselves are installed — no apt, no root. Commander's negated-boolean handling keeps the default (browserDeps: true) unchanged for everyone else. Intended for environments where system libraries are preinstalled (e.g. images baked withplaywright install-deps); if they aren't, the failure moves to browser launch, where Playwright names the exact missing packages.browserDepsFlag.ts): bothinstallandinstall browsersregister the flag, and in Commander's default parsing mode the parent consumes it even when written after the subcommand name — leaving the subcommand's opts at the default.mergedBrowserDepsANDs both scopes (exact, since the flag is negation-only). Found via runtime verification; covered by a regression test reproducing the topology.Testing
bun run test bun run typecheck bun run lint bun run format:check bun run knipbuildArgsomits--with-depson Linux whenbrowserDepsis false; flag registered with defaulttrueon all three commands;mergedBrowserDepsparent-consumption regression.ubuntu:24.04container as a non-root user with no sudo (realplaywright@1.58.2+@qawolf/flows@0.1.4, real browser download):--no-browser-depsflows run, bare Ubuntu 24.04, non-root, no sudosu root -c apt-get…, thenplaywright install chromium failed: Password:suspawnedinstall browsers,install,flows run× flag)install --with-deps chromiuminstall chromiumon all three commands--no-browser-dep→(Did you mean --no-browser-deps?); flag twice → idempotentChecklist