Bugfix/Headless Execution Default Size #4292
Merged
Merged
Conversation
Contributor
WalkthroughPlaywrightDriver now builds launch args dynamically, adding a headless window-size flag when HeadlessBrowserMode is enabled. SeleniumDriver's StartDriver sets a deterministic 1080x1920 window when running headless (or on Linux) and no explicit BrowserWidth/BrowserHeight are provided. No public APIs changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as Test Runner
participant PD as PlaywrightDriver
participant SD as SeleniumDriver
participant B as Browser
rect rgba(220,235,255,0.25)
note over PD: Playwright start flow
T->>PD: StartDriver()
PD->>PD: args = ["--start-maximized"]
alt Headless
PD->>PD: args += ["--window-size=1920,1080"]
end
PD->>B: Launch with Options.Args = args
end
rect rgba(235,255,220,0.25)
note over SD: Selenium start flow
T->>SD: StartDriver()
alt Headless or Linux AND no explicit size
SD->>B: launch/set window 1080x1920
else size provided or not headless
SD->>B: maximize or use provided size
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs (2)
329-355: Units bug: seconds vs. milliseconds mix leads to 1000x timeoutsHere
browserOptions.Timeoutis treated as ms elsewhere, but this block interprets/assigns it as seconds and then multiplies again when invoking the handler, causing excessive waits.- float? driverDefaultTimeout1 = browserOptions.Timeout; - float waitUntilTime; - if (act.Timeout > 0) - { - waitUntilTime = act.Timeout.GetValueOrDefault(); - } - else if(browserOptions.Timeout>0) - { - waitUntilTime = browserOptions.Timeout.Value; - } - else - { - waitUntilTime = 5; - } - browserOptions.Timeout = waitUntilTime; + float? driverDefaultTimeout1 = browserOptions.Timeout; // stored in ms + float waitMs; + if (act.Timeout > 0) + { + // act.Timeout is in seconds → convert to ms + waitMs = act.Timeout.Value * 1000f; + } + else if (browserOptions.Timeout > 0) + { + // already ms + waitMs = browserOptions.Timeout.Value; + } + else + { + waitMs = 5000f; // default 5s in ms + } + browserOptions.Timeout = waitMs; try { - actWebSmartSyncHandler.HandleAsync(act, waitUntilTime*1000).Wait(); + actWebSmartSyncHandler.HandleAsync(act, (int)waitMs).Wait(); }Please confirm elsewhere that
browserOptions.Timeoutis consistently treated as milliseconds.
614-621: fullPage flag inverted in screenshot logic
fullPage=trueshould call the full-page API; it is currently swapped.- if (fullPage) - { - screenshot = await tab.ScreenshotAsync(); - } - else - { - screenshot = await tab.ScreenshotFullPageAsync(); - } + if (fullPage) + { + screenshot = await tab.ScreenshotFullPageAsync(); + } + else + { + screenshot = await tab.ScreenshotAsync(); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs(2 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowser.cs (3)
PlaywrightBrowser(24-88)PlaywrightBrowser(55-63)Options(29-42)
🔇 Additional comments (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs (1)
143-146: Args/Timeout wiring looks goodArgs now flow from the built list; Timeout conversion to ms is consistent with other usages. No concerns here.
Maheshkale447
approved these changes
Sep 3, 2025
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (2)
740-744: Trim trailing spaces and set Firefox headless size via args.
- Trailing whitespace after the headless arg.
- Also ensure deterministic headless viewport on CI by setting width/height flags when no explicit size is provided.
Apply for the whitespace:
- FirefoxOption.AddArgument("--headless"); + FirefoxOption.AddArgument("--headless");Additionally (outside this hunk), consider:
// After adding --headless for Firefox if ((HeadlessBrowserMode || RuntimeInformation.IsOSPlatform(OSPlatform.Linux)) && string.IsNullOrEmpty(BrowserHeight) && string.IsNullOrEmpty(BrowserWidth)) { FirefoxOption.AddArgument("--width=1920"); FirefoxOption.AddArgument("--height=1080"); }
1124-1127: Broaden condition and inject Chromium window-size to avoid 800×600 in headless/Linux.This branch misses Linux runs where headless is forced earlier; and Convert.ToInt32 is unnecessary. Also add options-level window-size for Chromium to ensure the viewport matches.
- else if (HeadlessBrowserMode) - { - Driver.Manage().Window.Size = new Size() { Height = Convert.ToInt32(1080), Width = Convert.ToInt32(1920) }; - } + else if (HeadlessBrowserMode || RuntimeInformation.IsOSPlatform(OSPlatform.Linux)) + { + Driver.Manage().Window.Size = new Size(width: 1920, height: 1080); + }Additionally (outside this hunk):
- Chrome/Brave (in configChromeDriverAndStart, after adding headless):
if ((HeadlessBrowserMode || RuntimeInformation.IsOSPlatform(OSPlatform.Linux)) && string.IsNullOrEmpty(BrowserHeight) && string.IsNullOrEmpty(BrowserWidth)) { options.AddArgument("--window-size=1920,1080"); }
- Edge (in Edge options block, after adding headless):
if ((HeadlessBrowserMode || RuntimeInformation.IsOSPlatform(OSPlatform.Linux)) && string.IsNullOrEmpty(BrowserHeight) && string.IsNullOrEmpty(BrowserWidth)) { EDOpts.AddArgument("--window-size=1920,1080"); }Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs (1)
134-140: Headless: drop ineffective --start-maximized; align default size and gate --window-size by engine
- In headless, "--start-maximized" is ignored; keep it only for headed launches.
- Current default "1920,1080" diverges from Selenium’s reported "1080,1920". Standardize to avoid visual/regression baseline drift.
- "--window-size" is Chromium/Edge-specific; Firefox ignores it in headless—set viewport via Playwright context for cross-browser consistency.
Apply this diff locally; I can follow up with a viewport patch for Firefox/WebKit in PlaywrightBrowser if desired.
- var args = new List<string> { "--start-maximized" }; - - if (HeadlessBrowserMode) - { - args.Add("--window-size=1920,1080"); - } + var args = new List<string>(); + if (HeadlessBrowserMode) + { + // Align with Selenium default and restrict to Chromium-based browsers + if (BrowserType is WebBrowserType.Chrome or WebBrowserType.Edge) + { + args.Add("--window-size=1080,1920"); + } + // For Firefox/WebKit, set viewport in the Playwright context (not via args). + } + else + { + args.Add("--start-maximized"); + }Validation script to confirm Selenium’s headless default and avoid regressions:
#!/usr/bin/env bash set -euo pipefail # Find Selenium driver sizing logic and print nearby context fd -t f -i 'SeleniumDriver.cs' | while read -r f; do echo "==> $f" rg -n -C3 -e 'window[- ]?size|SetWindowSize|Viewport|Width|Height|1080|1920|Headless' "$f" || true done
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs(2 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-04T11:19:44.679Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4294
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs:10651-10658
Timestamp: 2025-09-04T11:19:44.679Z
Learning: In Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs, maintainer prefers not to keep the helper IsSvgElementByWebElement(IWebElement); rely on the existing enum-based IsSvgElement(eElementType) helper instead. Future reviews should not reintroduce IsSvgElementByWebElement.
Applied to files:
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs
📚 Learning: 2025-09-04T09:55:20.522Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4294
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs:5789-5796
Timestamp: 2025-09-04T09:55:20.522Z
Learning: In Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (around line 5786), maintainer prashelke prefers to keep the helper method `IsSvgElement(eElementType)` as-is (no inlining or renaming), even though it currently checks only for `eElementType.Svg`. Future reviews should not suggest removing or renaming this helper.
Applied to files:
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs
🧬 Code graph analysis (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowser.cs (3)
PlaywrightBrowser(24-88)PlaywrightBrowser(55-63)Options(29-42)
🔇 Additional comments (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs (1)
143-143: Args hookup looks goodPassing the dynamically built args array into options is correct.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (1)
740-744: Trim trailing whitespace and set headless defaults for Firefox.
- Trailing spaces after the headless argument. Same nit as earlier.
- While here, set width/height when running headless and no explicit size is provided to make the viewport deterministic.
- FirefoxOption.AddArgument("--headless"); + FirefoxOption.AddArgument("--headless"); + if (string.IsNullOrEmpty(BrowserHeight) && string.IsNullOrEmpty(BrowserWidth)) + { + FirefoxOption.AddArgument("--width=1920"); + FirefoxOption.AddArgument("--height=1080"); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs(2 hunks)
Maheshkale447
approved these changes
Sep 4, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thank you for your contribution.
Before submitting this PR, please make sure:
Summary by CodeRabbit
New Features
Style