Selenium upgrade#4461
Merged
Merged
Conversation
Contributor
WalkthroughConverted driver update/retrieval APIs to async, updated callers (UI and tests) to await GetDriverPath/UpdateDriver, bumped Selenium to 4.41.0, changed DevTools alias to V145, commented out FTP proxy assignments, and removed one unused DevTools using. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as "UI (EditPage)"
participant SeleniumDrv as "SeleniumDriver"
participant DriverFinder as "DriverFinder / Selenium Manager"
participant FS as "FileSystem"
UI->>SeleniumDrv: GetDriverPath(browserType) (await)
SeleniumDrv->>DriverFinder: GetDriverPathAsync(browserType)
DriverFinder-->>SeleniumDrv: driverPath (async)
SeleniumDrv-->>UI: driverPath (awaited result)
UI->>FS: copy driver files to node folder
FS-->>UI: copy result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ❌ 5❌ Failed checks (3 warnings, 2 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
![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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (2)
1322-1378:⚠️ Potential issue | 🔴 CriticalUse
browserTypehere;mBrowserTypemakes the new API return the wrong driver.
GetDriverPath(eBrowserType browserType)now accepts an explicit browser, butUpdateDriver()still switches and reports on the instance field. A default-constructedSeleniumDriverwill therefore treat every request asIEand return""for Chrome/Edge/Firefox.🛠️ Suggested fix
private async Task<string> UpdateDriver(eBrowserType browserType) { try { - Reporter.ToLog(eLogLevel.INFO, $"Failed to Download latest {mBrowserType} driver. Attempting to Update {mBrowserType} driver to latest using System Proxy Settings...."); + Reporter.ToLog(eLogLevel.INFO, $"Failed to download latest {browserType} driver. Attempting to update {browserType} driver to latest using System Proxy Settings...."); DriverOptions driverOptions = null; - driverOptions = mBrowserType switch + driverOptions = browserType switch { eBrowserType.Chrome => new ChromeOptions(), eBrowserType.Edge => new EdgeOptions(), eBrowserType.FireFox => new FirefoxOptions(), _ => null @@ if (!WorkSpace.Instance.RunningInExecutionMode && !WorkSpace.Instance.RunningFromUnitTest) { - Reporter.ToUser(eUserMsgKey.FailedToDownloadDriver, mBrowserType); + Reporter.ToUser(eUserMsgKey.FailedToDownloadDriver, browserType); } - Reporter.ToLog(eLogLevel.ERROR, string.Format(Reporter.UserMsgsPool[eUserMsgKey.FailedToDownloadDriver].Message, mBrowserType), ex); + Reporter.ToLog(eLogLevel.ERROR, string.Format(Reporter.UserMsgsPool[eUserMsgKey.FailedToDownloadDriver].Message, browserType), ex); throw; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs` around lines 1322 - 1378, The issue is UpdateDriver uses the instance field mBrowserType instead of the method parameter browserType causing wrong behavior; update all references so the method operates on the passed-in browserType: change the switch that builds driverOptions to use browserType (not mBrowserType), update Reporter.ToLog messages to use browserType, and ensure SetBrowserVersion is called with the explicit browserType (or use an overload that accepts browserType) instead of relying on mBrowserType; also update the error handling Reporter.ToUser and the formatted error message (Reporter.UserMsgsPool[eUserMsgKey.FailedToDownloadDriver].Message) to supply browserType so logs and user messages refer to the correct browser.
505-511:⚠️ Potential issue | 🟠 MajorImplement
IDisposablepattern; do not call blocking cleanup from the finalizer.The finalizer invokes
CloseDriver(), which blocks on async operations via.GetAwaiter().GetResult()(line 1554). Running blocking/async teardown on the finalizer thread can stall garbage collection and cause resource leaks. Instead, implementIDisposableonSeleniumDriverand move deterministic cleanup there, keeping the finalizer minimal or removing it entirely. Callers should be responsible for callingDispose()explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs` around lines 505 - 511, The finalizer (~SeleniumDriver) currently calls CloseDriver() which blocks on async teardown; change SeleniumDriver to implement IDisposable and move all blocking/async cleanup calls out of the finalizer into a Dispose implementation: add a public void Dispose() that calls a private Dispose(bool disposing) which performs CloseDriver() (safely awaiting or calling the synchronous equivalent) when disposing is true, call GC.SuppressFinalize(this) in Dispose(), and remove or make the finalizer non-blocking (or remove it entirely) so that CloseDriver() is no longer invoked on the finalizer thread; reference the SeleniumDriver class, the existing ~SeleniumDriver(), CloseDriver(), and implement IDisposable and Dispose(bool) pattern accordingly.Ginger/Ginger/Drivers/DriversConfigsEditPages/SeleniumRemoteWebDriverEditPage.xaml.cs (2)
186-186: 🧹 Nitpick | 🔵 TrivialRemove unused variable
driversSourcePath.This variable is assigned but never used. It appears to be leftover from before the refactoring to use
GetDriverPath()which now returns the full path directly.♻️ Proposed fix
- string driversSourcePath = assemblyLocation;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Ginger/Ginger/Drivers/DriversConfigsEditPages/SeleniumRemoteWebDriverEditPage.xaml.cs` at line 186, Remove the unused local variable driversSourcePath that is assigned from assemblyLocation in SeleniumRemoteWebDriverEditPage.xaml.cs; the code now uses GetDriverPath() which returns the full path, so delete the declaration/assignment "string driversSourcePath = assemblyLocation;" and ensure no other code references driversSourcePath (if any do, replace with GetDriverPath() or the correct variable).
162-256:⚠️ Potential issue | 🟠 MajorWrap async operations in try-catch to prevent unhandled exceptions.
The
async voidevent handler lacks exception handling. If any of the awaitedGetDriverPathcalls or subsequent file operations fail, the exception will propagate unhandled to the synchronization context, potentially crashing the application or leaving the UI in an inconsistent state.🛡️ Proposed fix to add exception handling
private async void GetNodeFilesButton_Click(object sender, RoutedEventArgs e) { + try + { //Getting the Ginger execution path string? assemblyLocation = Path.GetDirectoryName(System.Reflection.Assembly.GetExecutingAssembly().Location); // ... existing code ... //Open Dialog folder Process.Start(new ProcessStartInfo() { FileName = targetZipPath, UseShellExecute = true }); + } + catch (Exception ex) + { + Reporter.ToLog(eLogLevel.ERROR, "Failed to get node files", ex); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Ginger/Ginger/Drivers/DriversConfigsEditPages/SeleniumRemoteWebDriverEditPage.xaml.cs` around lines 162 - 256, GetNodeFilesButton_Click contains awaited calls (SeleniumDriver.GetDriverPath) and file IO that can throw; wrap the entire method body in a try-catch to prevent unhandled exceptions: surround the existing code in GetNodeFilesButton_Click with try { ... } catch (Exception ex) { Reporter.ToLog(eLogLevel.ERROR, $"GetNodeFilesButton_Click failed: {ex}"); /* optionally show a user-friendly MessageBox */ } so all exceptions from await seleniumDriver.GetDriverPath(...) and System.IO operations are caught and logged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs`:
- Around line 1382-1385: GetDriverPath and UpdateDriver were converted to async
Task<string> but callers remain synchronous; update all callers to await the
async results or revert signatures. Specifically, in tests (GetDriverPathTest.cs
where lines assign string DriverPath = driver.GetDriverPath(...)) change those
to await the async call and make the test methods async, and in SeleniumDriver
exception path (where UpdateDriver(mBrowserType) is invoked before
StartDriver()) await UpdateDriver(mBrowserType) so the driver update completes
before calling StartDriver(); ensure any call sites that expect a string now use
await and propagate async up the call chain or change the methods back to
synchronous signatures consistently.
In `@Ginger/GingerCoreNET/GingerCoreNET.csproj`:
- Around line 335-336: Update the Selenium.Support package reference to match
Selenium.WebDriver's version: change the PackageReference for "Selenium.Support"
(the element with Include="Selenium.Support") from 4.35.0 to 4.41.0 so both
PackageReference entries use version 4.41.0, ensuring dependency compatibility
between Selenium.Support and Selenium.WebDriver.
---
Outside diff comments:
In
`@Ginger/Ginger/Drivers/DriversConfigsEditPages/SeleniumRemoteWebDriverEditPage.xaml.cs`:
- Line 186: Remove the unused local variable driversSourcePath that is assigned
from assemblyLocation in SeleniumRemoteWebDriverEditPage.xaml.cs; the code now
uses GetDriverPath() which returns the full path, so delete the
declaration/assignment "string driversSourcePath = assemblyLocation;" and ensure
no other code references driversSourcePath (if any do, replace with
GetDriverPath() or the correct variable).
- Around line 162-256: GetNodeFilesButton_Click contains awaited calls
(SeleniumDriver.GetDriverPath) and file IO that can throw; wrap the entire
method body in a try-catch to prevent unhandled exceptions: surround the
existing code in GetNodeFilesButton_Click with try { ... } catch (Exception ex)
{ Reporter.ToLog(eLogLevel.ERROR, $"GetNodeFilesButton_Click failed: {ex}"); /*
optionally show a user-friendly MessageBox */ } so all exceptions from await
seleniumDriver.GetDriverPath(...) and System.IO operations are caught and
logged.
In `@Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs`:
- Around line 1322-1378: The issue is UpdateDriver uses the instance field
mBrowserType instead of the method parameter browserType causing wrong behavior;
update all references so the method operates on the passed-in browserType:
change the switch that builds driverOptions to use browserType (not
mBrowserType), update Reporter.ToLog messages to use browserType, and ensure
SetBrowserVersion is called with the explicit browserType (or use an overload
that accepts browserType) instead of relying on mBrowserType; also update the
error handling Reporter.ToUser and the formatted error message
(Reporter.UserMsgsPool[eUserMsgKey.FailedToDownloadDriver].Message) to supply
browserType so logs and user messages refer to the correct browser.
- Around line 505-511: The finalizer (~SeleniumDriver) currently calls
CloseDriver() which blocks on async teardown; change SeleniumDriver to implement
IDisposable and move all blocking/async cleanup calls out of the finalizer into
a Dispose implementation: add a public void Dispose() that calls a private
Dispose(bool disposing) which performs CloseDriver() (safely awaiting or calling
the synchronous equivalent) when disposing is true, call
GC.SuppressFinalize(this) in Dispose(), and remove or make the finalizer
non-blocking (or remove it entirely) so that CloseDriver() is no longer invoked
on the finalizer thread; reference the SeleniumDriver class, the existing
~SeleniumDriver(), CloseDriver(), and implement IDisposable and Dispose(bool)
pattern accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1cd0c843-824a-4d33-93dc-c0db1ad0fe9b
📒 Files selected for processing (4)
Ginger/Ginger/Drivers/DriversConfigsEditPages/SeleniumRemoteWebDriverEditPage.xaml.csGinger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.csGinger/GingerCoreNET/GeneralLib/GingerPlayUtils.csGinger/GingerCoreNET/GingerCoreNET.csproj
💤 Files with no reviewable changes (1)
- Ginger/GingerCoreNET/GeneralLib/GingerPlayUtils.cs
Comment on lines
+1382
to
1385
| public async Task<string> GetDriverPath(eBrowserType browserType) | ||
| { | ||
| string DriverPath = string.Empty; | ||
| DriverPath = UpdateDriver(browserType); | ||
| string DriverPath = await UpdateDriver(browserType); | ||
| return DriverPath; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# List all current call sites for the newly-async APIs and highlight usages that are not obviously awaited.
rg -n -C2 '\b(GetDriverPath|UpdateDriver)\s*\(' --glob '*.cs'
printf '\nPotentially synchronous call sites:\n'
rg -nP --glob '*.cs' '^(?!.*\b(await|GetAwaiter\s*\(\)\.GetResult\s*\(\)|\.Result\b)).*\b(GetDriverPath|UpdateDriver)\s*\('Repository: Ginger-Automation/Ginger
Length of output: 8176
Fix incomplete async migration: GetDriverPath and UpdateDriver have synchronous callers that will fail to compile and cause runtime races.
GetDriverPath and UpdateDriver are now async (Task<string>), but two categories of callers were not updated:
-
Test compilation errors in
GetDriverPathTest.cs(lines 39, 47, 55): Test methods assign the result directly tostringwithoutawait:string DriverPath = driver.GetDriverPath(SeleniumDriver.eBrowserType.Chrome);
-
Fire-and-forget race condition in
SeleniumDriver.cs(line 1163): Within an exception handler before retry,UpdateDriver(mBrowserType)is called withoutawait:UpdateDriver(mBrowserType); StartDriver();
This will not wait for the driver update to complete before attempting to start the driver.
Either revert these methods to synchronous signatures or update all callers to properly await the results.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs`
around lines 1382 - 1385, GetDriverPath and UpdateDriver were converted to async
Task<string> but callers remain synchronous; update all callers to await the
async results or revert signatures. Specifically, in tests (GetDriverPathTest.cs
where lines assign string DriverPath = driver.GetDriverPath(...)) change those
to await the async call and make the test methods async, and in SeleniumDriver
exception path (where UpdateDriver(mBrowserType) is invoked before
StartDriver()) await UpdateDriver(mBrowserType) so the driver update completes
before calling StartDriver(); ensure any call sites that expect a string now use
await and propagate async up the call chain or change the methods back to
synchronous signatures consistently.
ravirk91
previously approved these changes
Mar 11, 2026
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Ginger/Ginger/Drivers/DriversConfigsEditPages/SeleniumRemoteWebDriverEditPage.xaml.cs (1)
163-243:⚠️ Potential issue | 🟠 MajorPrevent overlapping package generations in this async handler.
After Line 163, the dispatcher can accept another click while the first run is awaiting
GetDriverPath(...). This method still writes to the shared...\Temp\GingerTemp/...\Temp\Gingerfolders and deletes them at the end, so concurrent runs can race on file copy, zip creation, and directory deletion and leave a broken package.🛠️ Suggested fix
+ private static readonly System.Threading.SemaphoreSlim _nodePackageLock = new(1, 1); + private async void GetNodeFilesButton_Click(object sender, RoutedEventArgs e) { - //Getting the Ginger execution path - string? assemblyLocation = Path.GetDirectoryName(System.Reflection.Assembly.GetExecutingAssembly().Location); - string userFolder = Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData); - string sourcePath = Path.Combine(assemblyLocation, "StaticDrivers"); - string targetPath = userFolder + "\\Temp\\GingerTemp"; - string jarFileName = "selenium-server-standalone.jar"; - string sourceFile = System.IO.Path.Combine(sourcePath, jarFileName); - string destFile = System.IO.Path.Combine(targetPath, jarFileName); + await _nodePackageLock.WaitAsync(); + try + { + //Getting the Ginger execution path + string? assemblyLocation = Path.GetDirectoryName(System.Reflection.Assembly.GetExecutingAssembly().Location); + string userFolder = Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData); + string sourcePath = Path.Combine(assemblyLocation, "StaticDrivers"); + string targetPath = Path.Combine(userFolder, "Temp", "GingerTemp", Guid.NewGuid().ToString("N")); + string jarFileName = "selenium-server-standalone.jar"; + string sourceFile = System.IO.Path.Combine(sourcePath, jarFileName); + string destFile = System.IO.Path.Combine(targetPath, jarFileName); - // existing body + // existing body + } + finally + { + _nodePackageLock.Release(); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Ginger/Ginger/Drivers/DriversConfigsEditPages/SeleniumRemoteWebDriverEditPage.xaml.cs` around lines 163 - 243, GetNodeFilesButton_Click can be re-entered while awaiting GetDriverPath which causes race conditions on the shared temp folder (targetPath) and zip operations; prevent overlapping runs by gating the handler with a reentrancy guard: add a private bool or SemaphoreSlim (e.g., _isGettingNodeFiles or _getNodeFilesLock) checked at the start of GetNodeFilesButton_Click to return immediately if already running, set it before any await and clear it in a finally block (or Release the semaphore), and also optionally disable/enable the UI button control within the same try/finally so concurrent clicks cannot start a second execution while file copy/zip/delete operations (targetPath, chromeDestFile, fireFoxDestFile, IEDestFile, edgeSourceFile, and any subsequent zip/delete code) are in progress.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@Ginger/Ginger/Drivers/DriversConfigsEditPages/SeleniumRemoteWebDriverEditPage.xaml.cs`:
- Around line 163-243: GetNodeFilesButton_Click can be re-entered while awaiting
GetDriverPath which causes race conditions on the shared temp folder
(targetPath) and zip operations; prevent overlapping runs by gating the handler
with a reentrancy guard: add a private bool or SemaphoreSlim (e.g.,
_isGettingNodeFiles or _getNodeFilesLock) checked at the start of
GetNodeFilesButton_Click to return immediately if already running, set it before
any await and clear it in a finally block (or Release the semaphore), and also
optionally disable/enable the UI button control within the same try/finally so
concurrent clicks cannot start a second execution while file copy/zip/delete
operations (targetPath, chromeDestFile, fireFoxDestFile, IEDestFile,
edgeSourceFile, and any subsequent zip/delete code) are in progress.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 514f85e6-e1a6-4f3c-bd0b-0cc0bcfa736e
📒 Files selected for processing (2)
Ginger/Ginger/Drivers/DriversConfigsEditPages/SeleniumRemoteWebDriverEditPage.xaml.csGinger/GingerCoreNETUnitTest/SeleniumDriverTest/GetDriverPathTest.cs
Contributor
There was a problem hiding this comment.
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/Selenium/SeleniumDriver.cs (2)
1326-1335:⚠️ Potential issue | 🟠 MajorUse the
browserTypeargument insideUpdateDriver().This method now accepts the target browser, but the actual selection and failure reporting still use
mBrowserType. If the instance state does not match the requested browser,GetDriverPath(browserType)can update the wrong driver or return""entirely. That is user-visible now thatSeleniumRemoteWebDriverEditPage.xaml.cs:161-165explicitly callsGetDriverPath(SeleniumDriver.eBrowserType.Chrome).🛠️ Proposed fix
- Reporter.ToLog(eLogLevel.INFO, $"Failed to Download latest {mBrowserType} driver. Attempting to Update {mBrowserType} driver to latest using System Proxy Settings...."); + Reporter.ToLog(eLogLevel.INFO, $"Failed to download latest {browserType} driver. Attempting to update {browserType} driver to latest using system proxy settings...."); DriverOptions driverOptions = null; - driverOptions = mBrowserType switch + driverOptions = browserType switch { eBrowserType.Chrome => new ChromeOptions(), eBrowserType.Edge => new EdgeOptions(), eBrowserType.FireFox => new FirefoxOptions(), _ => null @@ - Reporter.ToUser(eUserMsgKey.FailedToDownloadDriver, mBrowserType); + Reporter.ToUser(eUserMsgKey.FailedToDownloadDriver, browserType); } - Reporter.ToLog(eLogLevel.ERROR, string.Format(Reporter.UserMsgsPool[eUserMsgKey.FailedToDownloadDriver].Message, mBrowserType), ex); + Reporter.ToLog(eLogLevel.ERROR, string.Format(Reporter.UserMsgsPool[eUserMsgKey.FailedToDownloadDriver].Message, browserType), ex);Also applies to: 1373-1377
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs` around lines 1326 - 1335, Update the UpdateDriver() implementation to use the method parameter browserType instead of the instance field mBrowserType: replace references to mBrowserType in the Reporter.ToLog call and the switch that builds driverOptions (currently returning ChromeOptions/EdgeOptions/FirefoxOptions) so they switch on browserType; also update any subsequent similar blocks (around the second block at lines 1373-1377) to use browserType and ensure calls to GetDriverPath(browserType) and any error messages reference the passed-in browserType so the requested browser is updated/logged correctly.
505-511:⚠️ Potential issue | 🟠 MajorRemove the finalizer and implement a proper
IDisposablepattern.The finalizer
~SeleniumDriver()callsCloseDriver(), which is unsafe for multiple reasons:
- It accesses managed state (POMUtils, Driver, BMPClient, BMPServer)
- It blocks on async code via
StopNetworkLog().GetAwaiter().GetResult()— this can deadlock when called from the finalizer thread- Finalizers are nondeterministic and run on the GC thread, not the main application thread
The class should implement
IDisposablewith a publicDispose()method for explicit cleanup instead of relying on the finalizer.Current unsafe code
- ~SeleniumDriver() - { - if (Driver != null) - { - CloseDriver(); - } - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs` around lines 505 - 511, Remove the finalizer ~SeleniumDriver() and implement the standard IDisposable pattern on SeleniumDriver: add a public Dispose() that calls a protected virtual Dispose(bool disposing), call GC.SuppressFinalize(this), and move all managed resource cleanup (accessing POMUtils, Driver, BMPClient, BMPServer) into Dispose(true) so CloseDriver is invoked deterministically from Dispose rather than a finalizer. Refactor CloseDriver/StopNetworkLog so Dispose does not block on async code: either provide an async CloseDriverAsync/StopNetworkLogAsync and implement IAsyncDisposable (DisposeAsync) to await them, or make CloseDriver perform only synchronous safe teardown and kick off StopNetworkLog asynchronously without blocking (e.g., fire-and-forget with proper error handling). Ensure no managed state is accessed from a finalizer and remove the existing ~SeleniumDriver() finalizer reference.
♻️ Duplicate comments (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (1)
1158-1164:⚠️ Potential issue | 🔴 CriticalThis retry still does not wait for
UpdateDriver().
ConfigureAwait(true)only changes how an awaited continuation resumes; because nothing awaits the returned awaitable here, the update runs fire-and-forget andStartDriver()retries immediately. The restart can still happen against the stale driver. The updated UI caller inSeleniumRemoteWebDriverEditPage.xaml.cs:161-165awaits this API correctly; this path still does not.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs` around lines 1158 - 1164, The retry path is firing UpdateDriver(mBrowserType).ConfigureAwait(true) without awaiting it, so UpdateDriver runs fire-and-forget and StartDriver() runs against the stale driver; change the method containing the RestartRetry logic to be async and await the UpdateDriver call (e.g. await UpdateDriver(mBrowserType).ConfigureAwait(false or true as appropriate) ) before calling StartDriver(), and remove the unawaited ConfigureAwait usage; update any callers if necessary to handle the now-async method so the restart truly waits for UpdateDriver to complete (symbols: RestartRetry, UpdateDriver, StartDriver in SeleniumDriver).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs`:
- Around line 1326-1335: Update the UpdateDriver() implementation to use the
method parameter browserType instead of the instance field mBrowserType: replace
references to mBrowserType in the Reporter.ToLog call and the switch that builds
driverOptions (currently returning ChromeOptions/EdgeOptions/FirefoxOptions) so
they switch on browserType; also update any subsequent similar blocks (around
the second block at lines 1373-1377) to use browserType and ensure calls to
GetDriverPath(browserType) and any error messages reference the passed-in
browserType so the requested browser is updated/logged correctly.
- Around line 505-511: Remove the finalizer ~SeleniumDriver() and implement the
standard IDisposable pattern on SeleniumDriver: add a public Dispose() that
calls a protected virtual Dispose(bool disposing), call
GC.SuppressFinalize(this), and move all managed resource cleanup (accessing
POMUtils, Driver, BMPClient, BMPServer) into Dispose(true) so CloseDriver is
invoked deterministically from Dispose rather than a finalizer. Refactor
CloseDriver/StopNetworkLog so Dispose does not block on async code: either
provide an async CloseDriverAsync/StopNetworkLogAsync and implement
IAsyncDisposable (DisposeAsync) to await them, or make CloseDriver perform only
synchronous safe teardown and kick off StopNetworkLog asynchronously without
blocking (e.g., fire-and-forget with proper error handling). Ensure no managed
state is accessed from a finalizer and remove the existing ~SeleniumDriver()
finalizer reference.
---
Duplicate comments:
In `@Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs`:
- Around line 1158-1164: The retry path is firing
UpdateDriver(mBrowserType).ConfigureAwait(true) without awaiting it, so
UpdateDriver runs fire-and-forget and StartDriver() runs against the stale
driver; change the method containing the RestartRetry logic to be async and
await the UpdateDriver call (e.g. await
UpdateDriver(mBrowserType).ConfigureAwait(false or true as appropriate) ) before
calling StartDriver(), and remove the unawaited ConfigureAwait usage; update any
callers if necessary to handle the now-async method so the restart truly waits
for UpdateDriver to complete (symbols: RestartRetry, UpdateDriver, StartDriver
in SeleniumDriver).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fb567737-cf91-4fbb-8c31-a5510d710c5b
📒 Files selected for processing (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs
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.
Description
Type of Change
Checklist
[IsSerializedForLocalRepository]where neededReporter.ToLog()patternSummary by CodeRabbit
Dependencies
Improvements
Changes
Tests
Chores