Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChromium launch argument construction centralizes feature-disable flags into two arrays and emits one comma-joined switch for each feature category. ChangesChromium launch argument consolidation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
5be2688 to
e19e702
Compare
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/browser/browser.ts`:
- Around line 100-102: The new `--disable-features` entry in `browser.ts` is
still replacing Chromium’s launcher default instead of extending it when
`launchPersistentContext(..., { args })` is used. Update the argument
construction around the `disableFeatures`/`disableBlinkFeatures` switches so the
launcher defaults are merged with the custom values, or explicitly strip the
default switch first if full replacement is intended.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f5a8c3a5-1b94-4fa6-b383-ed74e07ba9d7
📒 Files selected for processing (1)
src/browser/browser.ts
e19e702 to
84d1da4
Compare
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/browser/browser.ts`:
- Around line 25-29: Update the single-source-of-truth feature switch arrays in
the browser launch configuration to include the defaults added by
launchPersistentContext, ensuring the final --disable-features and
--disable-blink-features values preserve both launcher and custom defaults.
Alternatively, explicitly remove the launcher-provided switches only if
replacement is intentional; do not allow duplicate switches to silently discard
required features.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 79d361fb-d21a-4578-bd77-64c853a18cb6
📒 Files selected for processing (1)
src/browser/browser.ts
84d1da4 to
351a167
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/browser/browser.ts (1)
25-50:⚠️ Potential issue | 🟠 MajorStill merge or explicitly remove launcher feature defaults.
This consolidates only the project’s switches. Playwright retains its own default arguments unless
ignoreDefaultArgsis configured, and its Chromium defaults include a separate--disable-featureslist. The custom switch can therefore replace that list and silently drop launcher-required features. (playwright.dev)Merge the pinned Playwright/CloakBrowser defaults into these arrays, or explicitly filter the default switches if full replacement is intentional. This is the same unresolved issue raised in previous reviews.
#!/usr/bin/env bash set -euo pipefail for dir in node_modules/playwright-core node_modules/@playwright/test node_modules/cloakbrowser; do if [ -d "$dir" ]; then echo "== $dir ==" rg -n -C3 -- '--disable-features|--disable-blink-features|ignoreDefaultArgs|defaultArgs' "$dir" || true fi doneAlso applies to: 102-104
🤖 Prompt for 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. In `@src/browser/browser.ts` around lines 25 - 50, Update the disableFeatures and disableBlinkFeatures arrays to preserve the pinned Playwright/CloakBrowser launcher defaults when constructing Chromium arguments. Merge those default feature values into the existing single-source arrays, or explicitly filter the launcher’s default switches if intentional replacement is required; ensure each switch remains emitted exactly once.
🤖 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.
Duplicate comments:
In `@src/browser/browser.ts`:
- Around line 25-50: Update the disableFeatures and disableBlinkFeatures arrays
to preserve the pinned Playwright/CloakBrowser launcher defaults when
constructing Chromium arguments. Merge those default feature values into the
existing single-source arrays, or explicitly filter the launcher’s default
switches if intentional replacement is required; ensure each switch remains
emitted exactly once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e46313a1-d9b3-438b-95e1-306f3c9f537c
📒 Files selected for processing (1)
src/browser/browser.ts
32553ff to
4210b4b
Compare
…-blink-features Chromium collapses a repeated command-line switch to its LAST value: base::CommandLine stores one value per switch name and base::FeatureList reads it once. sharedArgs passed --disable-features 7 times and --disable-blink-features twice, so only the final occurrence of each took effect and every earlier value was silently dropped. Effect before this fix: - --disable-features: only TrustedHTML applied. Dropped AudioServiceSandbox, the whole sign-in dialog suppression list (SigninInterception, IdentityConsistency, ChromeBrowserCloudManagement, SignInPromo, ChromeWhatsNewUI, AccountConsistency), TranslateUI, AutofillServerCommunication, MediaRouter, TrustedScriptTypes. - --disable-blink-features: only TrustedDOMTypes applied, so AutomationControlled (navigator.webdriver anti-detection) was NOT disabled. This also mattered because CloakBrowser/Playwright inject their own default --disable-features and --disable-blink-features=AutomationControlled BEFORE these args; a trailing single-value switch overrides that whole default list too. Same class of bug fixed in browserless (#5450 / #5452). Fix: fold each switch into one comma-joined value backed by a named array (disableFeatures / disableBlinkFeatures) as the single source of truth, so a future stray duplicate can't silently clobber the list again. No functional values changed — the union is identical to what was intended. Refs: - browserless/browserless#5450 - browserless/browserless#5452 - https://source.chromium.org/chromium/chromium/src/+/main:base/command_line.h Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
4210b4b to
22400d7
Compare
1 participant
Problem
Chromium collapses a repeated command-line switch to its LAST value —
base::CommandLinestores one value per switch name andbase::FeatureListreads it once (command_line.h).sharedArgspassed--disable-features7× and--disable-blink-features2×, so only the final occurrence of each survived; every earlier value was silently discarded.What actually applied before this fix:
--disable-features: onlyTrustedHTML. Dropped:AudioServiceSandbox, the entire sign-in dialog suppression list (SigninInterception,IdentityConsistency,ChromeBrowserCloudManagement,SignInPromo,ChromeWhatsNewUI,AccountConsistency),TranslateUI,AutofillServerCommunication,MediaRouter,TrustedScriptTypes.--disable-blink-features: onlyTrustedDOMTypes→AutomationControlledwas NOT disabled, re-exposing thenavigator.webdriverautomation signal (anti-detection defense).Interleaving with CloakBrowser
CloakBrowser/Playwright inject their own default
--disable-featureslist and--disable-blink-features=AutomationControlledbefore our args. Because Chromium is last-wins, our trailing single-value switch overrode Playwright's whole default list too — so the bug both dropped our values and clobbered the launcher's defaults. Identical class of bug fixed upstream in browserless: #5450, #5452.Fix
Fold each switch into a single comma-joined value backed by a named array (
disableFeatures/disableBlinkFeatures) as the single source of truth, so a future stray duplicate can't silently clobber the list again. No functional values changed — the union is exactly what was intended.Impact / since when
v2.1.12).AutomationControlledclobbered since 2025-12-19; real once CloakBrowser became the detection defense (June 2026).main) has the same--disable-features+AutomationControlleddup (no sign-in list) — follow-up PR tomainrecommended.Test
Not built locally (Node ≤20 gate; this box is Node 22). Change is a template literal +
Array.join— merged string is the verified union of prior values.