Remote web Driver support added for all type of browser#4089
Conversation
WalkthroughThis pull request introduces a new configuration option for specifying a Remote Web Driver URL. A new UI panel is added to the WebAgentConfigEditPage.xaml, which includes a label and an input control. The code-behind in WebAgentConfigEditPage.xaml.cs initializes a new configuration parameter and binds it to the UI. Additionally, the SeleniumDriver class now defines a RemoteWebDriverUrl property and adjusts its driver initialization logic to create a remote driver instance when a valid URL is provided. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant A as WebAgentConfigEditPage
participant C as ConfigHandler
participant S as SeleniumDriver
participant R as RemoteWebDriver
participant L as LocalDriver
U->>A: Enter Remote Web Driver URL
A->>C: Update configuration parameter
C->>S: Pass RemoteWebDriverUrl value
S->>S: Check if URL is provided
alt Remote URL provided
S->>R: Initialize RemoteWebDriver with URL
else
S->>L: Initialize LocalDriver
end
S-->>C: Return driver instance
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 3
🔭 Outside diff range comments (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (1)
80-81: Consider breaking down large driver classThe SeleniumDriver class is over 11,000 lines and handles multiple responsibilities including browser automation, element location, recording, visual testing etc. Consider breaking it down into smaller focused classes following Single Responsibility Principle.
Some suggestions:
- Extract element location logic into separate ElementLocator class
- Move recording functionality into separate Recorder class
- Create dedicated classes for visual testing and browser management
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
Ginger/Ginger/Drivers/DriversConfigsEditPages/WebAgentConfigEditPage.xaml(1 hunks)Ginger/Ginger/Drivers/DriversConfigsEditPages/WebAgentConfigEditPage.xaml.cs(1 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs(4 hunks)
🔇 Additional comments (3)
Ginger/Ginger/Drivers/DriversConfigsEditPages/WebAgentConfigEditPage.xaml (1)
212-215: LGTM!The new remote web driver URL configuration panel follows the established pattern and maintains consistency with other configuration panels in terms of styling and layout.
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (2)
397-401: New property added for Remote WebDriver supportThe new property
RemoteWebDriverUrlis added with appropriate user configuration attributes to allow specifying a remote WebDriver URL.
1119-1127: Remote WebDriver support added for Chrome browserThe code now checks for RemoteWebDriverUrl and creates either a RemoteWebDriver or local ChromeDriver based on the URL configuration. The implementation follows good practices with proper error handling.
| //Remote Web Driver Url | ||
| DriverConfigParam RemoteWebDriverUrl = mAgent.GetOrCreateParam(nameof(SeleniumDriver.RemoteWebDriverUrl)); | ||
| xRemoteWebDriverUrlVE.Init(null, RemoteWebDriverUrl, nameof(DriverConfigParam.Value)); | ||
| BindingHandler.ObjFieldBinding(xRemoteWebDriverUrlVE, TextBox.ToolTipProperty, RemoteWebDriverUrl, nameof(DriverConfigParam.Description)); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Follow C# naming conventions for local variables.
The local variable RemoteWebDriverUrl should be in camelCase to follow C# naming conventions.
Apply this diff to fix the naming:
- DriverConfigParam RemoteWebDriverUrl = mAgent.GetOrCreateParam(nameof(SeleniumDriver.RemoteWebDriverUrl));
- xRemoteWebDriverUrlVE.Init(null, RemoteWebDriverUrl, nameof(DriverConfigParam.Value));
- BindingHandler.ObjFieldBinding(xRemoteWebDriverUrlVE, TextBox.ToolTipProperty, RemoteWebDriverUrl, nameof(DriverConfigParam.Description));
+ DriverConfigParam remoteWebDriverUrl = mAgent.GetOrCreateParam(nameof(SeleniumDriver.RemoteWebDriverUrl));
+ xRemoteWebDriverUrlVE.Init(null, remoteWebDriverUrl, nameof(DriverConfigParam.Value));
+ BindingHandler.ObjFieldBinding(xRemoteWebDriverUrlVE, TextBox.ToolTipProperty, remoteWebDriverUrl, nameof(DriverConfigParam.Description));There was a problem hiding this comment.
@prashelke Understood, I'll move on from the naming suggestion.
| if (!string.IsNullOrEmpty(RemoteWebDriverUrl)) | ||
| { | ||
| Driver = new RemoteWebDriver(new Uri(RemoteWebDriverUrl), ieOptions); | ||
| } | ||
| else | ||
| { | ||
| Driver = new InternetExplorerDriver((InternetExplorerDriverService)driverService, ieOptions, TimeSpan.FromSeconds(Convert.ToInt32(HttpServerTimeOut))); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remote WebDriver support added for Edge browser with code duplication
The RemoteWebDriver initialization logic is duplicated for both IE mode and regular Edge mode. Consider refactoring to extract the common RemoteWebDriver initialization code into a separate method.
// Create a helper method
+ private IWebDriver CreateRemoteOrLocalDriver(Uri remoteUrl, DriverOptions options, DriverService service, int timeoutSeconds) {
+ return !string.IsNullOrEmpty(RemoteWebDriverUrl)
+ ? new RemoteWebDriver(remoteUrl, options)
+ : new EdgeDriver((EdgeDriverService)service, options, TimeSpan.FromSeconds(timeoutSeconds));
+ }Also applies to: 836-843
There was a problem hiding this comment.
no need of refactoring
There was a problem hiding this comment.
@prashelke Understood, I'll respect your decision to keep the current implementation as-is. The code works correctly in its current form.
(_/)/
(•-•)
🌟
|
|
||
| [UserConfigured] | ||
| [UserConfiguredDescription("Remote Web Driver Url")] | ||
| public string RemoteWebDriverUrl { get; set; } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Overall assessment of Remote WebDriver implementation
The changes successfully add Remote WebDriver support across Chrome, Edge and IE browsers. While the core functionality is implemented correctly, there are opportunities for improvement:
- Extract duplicated RemoteWebDriver initialization logic into helper methods
- Consider breaking down the large SeleniumDriver class into smaller focused classes
- Add XML documentation comments for the new RemoteWebDriverUrl property
The implementation is functional and can be merged, with the suggested improvements handled as follow-up refactoring tasks.
Also applies to: 795-802, 836-843, 1119-1127
There was a problem hiding this comment.
will handle it later
There was a problem hiding this comment.
@prashelke Understood, I'll add this to our knowledge base. Would you like me to create an issue to track these refactoring improvements for later?
✏️ Learnings added
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4089
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs:397-401
Timestamp: 2025-02-07T12:11:25.466Z
Learning: The developer prefers to handle code refactoring and documentation improvements in separate PRs rather than as part of the initial implementation PR.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
2 participants
Thank you for your contribution.
Before submitting this PR, please make sure:
Summary by CodeRabbit