Playwright UI Element Action pending operation#4107
Conversation
WalkthroughThis pull request introduces new asynchronous operations to enhance UI element interactions. The changes add support for drag-and-drop (by coordinates or via target elements), drawing actions, and multi-element value settings in the UI element handler. The updates extend the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant H as ActUIElementHandler
participant IE as IBrowserElement
participant D as Driver (Selenium/Playwright)
U->>H: Invoke HandleAsync(DragDrop)
H->>H: Determine target method (coordinates vs. element)
alt Coordinates provided
H->>IE: DragDropXYCordinateAsync(x, y)
else Element provided
H->>IE: DragDropAsync(targetElement) or DragDropJSAsync(targetElement)
end
IE->>D: Execute drag-and-drop action via driver
D-->>IE: Return status/result
IE-->>H: Relay operation status
H-->>U: Return final result
sequenceDiagram
participant U as User
participant H as ActUIElementHandler
participant IE as IBrowserElement
U->>H: Invoke HandleAsync(MultiSetValue)
H->>IE: Retrieve all matching elements
loop For each element
IE->>IE: Clear and set new value
end
H-->>U: Return operation outcome
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: 6
🔭 Outside diff range comments (9)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (9)
1-11084: 🛠️ Refactor suggestionConsider breaking down this large file into smaller, more focused components.
This file is over 11,000 lines long, which makes it difficult to maintain and understand. Consider splitting it into smaller, more focused classes following the Single Responsibility Principle:
- Browser management
- Element location
- Element interaction
- Network monitoring
- JavaScript execution
- Screenshot handling
// Example structure: + namespace GingerCore.Drivers { + public class SeleniumBrowserManager { /* Browser lifecycle management */ } + public class SeleniumElementLocator { /* Element location logic */ } + public class SeleniumElementInteractor { /* Element interaction */ } + public class SeleniumNetworkMonitor { /* Network monitoring */ } + public class SeleniumJavaScriptExecutor { /* JS execution */ } + public class SeleniumScreenshotManager { /* Screenshot handling */ } + }
1002-1161: 🛠️ Refactor suggestionImprove error handling in configChromeDriverAndStart method.
The error handling in this method could be improved to provide more specific error messages and proper cleanup:
private void configChromeDriverAndStart(ChromeOptions options) { try { // Existing configuration code } catch (Exception ex) { - if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux) && - ex.Message.Contains("no such file or directory")) + if (ex is WebDriverException webDriverEx) { + HandleWebDriverException(webDriverEx); + } + else if (ex is ChromeNotFoundException) { + HandleChromeNotFoundException(); + } throw; } finally { + CleanupDriverResources(); } }
6789-6814:⚠️ Potential issuePotential security vulnerability in JavaScript execution.
The code executes JavaScript without proper sanitization of input, which could lead to XSS attacks:
- ((IJavaScriptExecutor)Driver).ExecuteScript(script, Params); + // Sanitize script input + if (!IsScriptSafe(script)) { + throw new SecurityException("Potentially unsafe script detected"); + } + ((IJavaScriptExecutor)Driver).ExecuteScript(SanitizeScript(script), Params);
479-496: 🧹 Nitpick (assertive)Add retry mechanism for driver initialization.
The driver initialization lacks a proper retry mechanism for transient failures:
public override void InitDriver(Agent agent) { + int maxRetries = 3; + int retryCount = 0; + while (retryCount < maxRetries) { try { // Existing initialization code + break; } catch (WebDriverException ex) { + retryCount++; + if (retryCount == maxRetries) throw; + Thread.Sleep(1000 * retryCount); // Exponential backoff } } }
10897-10899: 🧹 Nitpick (assertive)Improve error logging with structured logging.
The error logging could be improved with more structured information:
- Reporter.ToLog(eLogLevel.ERROR, $"Method - {MethodBase.GetCurrentMethod().Name}, Error - {ex.Message}", ex); + Reporter.ToLog(eLogLevel.ERROR, new { + Method = MethodBase.GetCurrentMethod().Name, + Error = ex.Message, + StackTrace = ex.StackTrace, + Context = new { BrowserType = mBrowserType, IsHeadless = HeadlessBrowserMode } + });
1379-1415: 🛠️ Refactor suggestionAdd proper resource cleanup in CloseDriver method.
The CloseDriver method should ensure proper cleanup of all resources:
public override void CloseDriver() { try { + // Dispose network monitoring resources + if (interceptor != null) { + interceptor.StopMonitoring().Wait(); + interceptor = null; + } + // Dispose DevTools session + if (devToolsSession != null) { + devToolsSession.Dispose(); + devToolsSession = null; + } // Existing cleanup code } finally { + // Ensure all resources are released + GC.Collect(); + GC.WaitForPendingFinalizers(); } }
6997-7024: 🧹 Nitpick (assertive)Optimize element locator performance.
The element location strategy could be optimized for better performance:
private List<ElementLocator> GetLocatorlistforFriendlyLocator(...) { + // Cache frequently used locators + private static readonly ConcurrentDictionary<string, ElementLocator> _locatorCache + = new ConcurrentDictionary<string, ElementLocator>(); // Existing code + string cacheKey = $"{currentHtmlNode.XPath}_{position}"; + return _locatorCache.GetOrAdd(cacheKey, key => { // Existing locator creation logic }); }
10030-10064: 🛠️ Refactor suggestionAdd input validation for element info updates.
The UpdateElementInfoFields method needs better input validation:
void IWindowExplorer.UpdateElementInfoFields(ElementInfo EI) { + if (EI == null) throw new ArgumentNullException(nameof(EI)); + if (Driver == null) throw new InvalidOperationException("Driver not initialized"); // Validate element object + if (!(EI.ElementObject is IWebElement webElement)) { + throw new ArgumentException("Invalid element object type"); + } // Existing update logic }
1-17: 🧹 Nitpick (assertive)Update license header with current year.
The license header should be updated to reflect the current year:
- Copyright © 2014-2024 European Support Limited + Copyright © 2014-2025 European Support Limited
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActUIElementHandler.cs(4 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/IBrowserElement.cs(2 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserElement.cs(3 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs(1 hunks)Ginger/GingerCoreNET/GeneralLib/JavaScriptHandler.cs(1 hunks)Ginger/GingerCoreNET/GingerCoreNET.csproj(1 hunks)Ginger/GingerCoreNET/Resources/JavaScripts/draganddrop_playwright.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActUIElementHandler.cs (1)
Learnt from: IamRanjeetSingh
PR: Ginger-Automation/Ginger#4058
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActUIElementHandler.cs:648-649
Timestamp: 2025-01-16T04:09:15.662Z
Learning: The Ginger project primarily targets .NET 8.0, which supports C# 12 features including collection expressions with spread operator.
🔇 Additional comments (10)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActUIElementHandler.cs (4)
22-22: New import is consistent with usage.
This added namespace reference appears necessary for accessingJavaScriptHandleror related classes.
194-202: Add appropriate test coverage for new actions.
The newly introducedDragDrop,DrawObject, andMultiSetValuecases look good. Ensure corresponding automated tests are in place to verify their functionality.
244-251: Method is straightforward and clean.
DrawObjectAsync()correctly retrieves the element and callsDrawObjectAsync()on it. If no element is found, an exception is thrown upstream.
883-883: No additional logic to review.
This line merely closes the method block. Nothing else changed.Ginger/GingerCoreNET/Resources/JavaScripts/draganddrop_playwright.js (1)
1-90: 🧹 Nitpick (assertive)Fix copyright symbol and ensure cross-browser compatibility.
The functionality appears solid for a manual drag-and-drop simulation. However, “�” should be replaced with “©”. Also, confirm that all required browsers support the generated events.Proposed fix:
- Copyright � + Copyright ©Likely an incorrect or invalid review comment.
Ginger/GingerCoreNET/GeneralLib/JavaScriptHandler.cs (1)
32-32: New enum entry is well-integrated.
draganddrop_playwrightextends support for the new JS file. No issues found.Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/IBrowserElement.cs (2)
65-67: LGTM! New UI interaction methods are well-defined.The new asynchronous methods for drawing and drag-drop operations follow consistent naming conventions and have clear, descriptive signatures.
76-76: LGTM! ExecuteJavascriptAsync signature enhancement.The addition of an optional nullable args parameter enhances JavaScript execution flexibility while maintaining backward compatibility.
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserElement.cs (1)
813-835: LGTM! JavaScript execution enhancement is well-implemented.The changes to support optional arguments in JavaScript execution are implemented consistently for both locator and element handle versions.
Ginger/GingerCoreNET/GingerCoreNET.csproj (1)
214-214: New Embedded Resource for Playwright Drag-and-Drop Simulation Added.
The addition of the embedded resource line<EmbeddedResource Include="Resources\JavaScripts\draganddrop_playwright.js" />ensures that the new JavaScript file required for simulating drag-and-drop operations via Playwright is included in the assembly. Please verify that the file exists in the specified location and that any corresponding usage in the code (such as in the UI element handler) properly references this resource.
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActUIElementHandler.cs(8 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserElement.cs(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActUIElementHandler.cs (1)
Learnt from: IamRanjeetSingh
PR: Ginger-Automation/Ginger#4058
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActUIElementHandler.cs:648-649
Timestamp: 2025-01-16T04:09:15.662Z
Learning: The Ginger project primarily targets .NET 8.0, which supports C# 12 features including collection expressions with spread operator.
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserElement.cs (1)
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4107
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserElement.cs:622-652
Timestamp: 2025-02-17T12:22:22.732Z
Learning: Playwright's IElementHandle does not support drag-drop operations (DragToAsync). Only ILocator provides drag-drop functionality, so operations should throw InvalidOperationException when attempted with element handles.
🔇 Additional comments (8)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActUIElementHandler.cs (3)
194-202: New UI element actions look consistent.
AddingDragDrop,DrawObject, andMultiSetValuecases helps extend functionality. Ensure that you have corresponding coverage in either automated tests or integration tests to confirm correctness.
244-251: DrawObjectAsync usage is straightforward.
Implementation appears consistent with the rest of the handler. No issues found.
253-299: Validate the drag-and-drop script usage & naming consistency.
The mix of “Selenium” and “JS” references for drag-drop might be confusing in a Playwright-oriented flow. The logic itself seems sound, but ensure all needed references (e.g.,draganddrop_playwrightscript) are tested on all targeted browsers. Also, consider clarifying the naming forDragDropSeleniumto avoid implying Selenium in a Playwright context.Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserElement.cs (5)
20-20: Dependency import is valid.
ImportingAmdocs.Ginger.CoreNET.Drivers.CoreDrivers.Web.Exceptionsappears relevant here for throwingInvalidActionConfigurationException. No issues found.
85-85: New overloaded click method is consistent.
Providing an overload that acceptsSystem.Drawing.Pointis well aligned with the existing pattern of locator vs. element-handle usage. Looks good.
618-623: DragDropAsync method: handling element locator is consistent.
Throwing when_playwrightLocatoris null is consistent with the approach elsewhere. Ensure you have test coverage for the case where_playwrightElementHandlewas used to construct this object (which will raise theInvalidOperationException).
669-709: Coordinate-based drag-and-drop logic is clear.
Manually computing the center X/Y and performing mouse actions is a valid workaround since Playwright’s element-handle drag is not supported. No immediate issues noted.
816-838: JavaScript execution with optional args is versatile.
Be mindful of potential injection risks ifscriptorargsoriginate from untrusted sources. Consider adding a validation step or usage guidelines to avoid accidental security issues.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActUIElementHandler.cs(8 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserElement.cs(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActUIElementHandler.cs (1)
Learnt from: IamRanjeetSingh
PR: Ginger-Automation/Ginger#4058
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActUIElementHandler.cs:648-649
Timestamp: 2025-01-16T04:09:15.662Z
Learning: The Ginger project primarily targets .NET 8.0, which supports C# 12 features including collection expressions with spread operator.
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserElement.cs (1)
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4107
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserElement.cs:622-652
Timestamp: 2025-02-17T12:22:22.732Z
Learning: Playwright's IElementHandle does not support drag-drop operations (DragToAsync). Only ILocator provides drag-drop functionality, so operations should throw InvalidOperationException when attempted with element handles.
🔇 Additional comments (9)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActUIElementHandler.cs (4)
21-21: No concerns regarding this import.
It seems consistent with the existing usage, and there's no indication from static analysis or logs suggesting it's unused.
193-201: New cases look well-structured.
These additional switch cases forDragDrop,DrawObject, andMultiSetValuelogically trigger the corresponding async methods.
243-250: Method call is straightforward.
This direct call toDrawObjectAsyncon the first matching element aligns with the new action introduced for drawing. No issues found.
252-298: Ensure consistent error logging or re-throws.
The logic for drag-and-drop is clear. The method properly handles XY-based and element-based targets. The finalcatchjust rethrows the exception, which is consistent with other patterns in this file.If you’d like more detailed logs, you could optionally catch and log the exception before rethrowing, but that’s not strictly required.
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserElement.cs (5)
20-20: Import usage is consistent.
This import forInvalidActionConfigurationExceptionis relevant to the new drag/drop logic. No issues found.
618-660: Drag-drop feature aligns with Playwright usage.
The method correctly checks for_playwrightLocatorand then delegates toDragDropAsync(IPlaywrightLocator, IBrowserElement), throwing if the locator is null. This is a sensible approach, given that Playwright's element handle does not support direct drag-and-drop.
662-674: Helper method is clear and consistent.
This utility method returns the_playwrightLocatorfrom anotherPlaywrightBrowserElement. Proper fallback is to returnnullotherwise.
676-716: Coordinate-based drag-drop logic appears accurate.
Center calculation via bounding box, then mouseDown → mouseMove → mouseUp is standard. The error message if bounding box is missing is also helpful.
823-845: Extended JavaScript executor with optional arguments.
Adding anargsparameter to the overloadedExecuteJavascriptAsyncmethods expands flexibility. This aligns with the newDragDropJSapproach in the handler.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActUIElementHandler.cs (1)
214-241: 🧹 Nitpick (assertive)Be cautious with rethrowing masked exceptions
Catching all exceptions and throwing a genericInvalidActionConfigurationExceptionmight obscure the real error cause. Consider logging or preserving original exception details for easier troubleshooting.catch (Exception ex) { - throw new InvalidActionConfigurationException($"Multi set value operation cancelled."); + throw new InvalidActionConfigurationException( + $"MultiSetValueAsync failed: {ex.Message}", ex); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActUIElementHandler.cs(8 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/IBrowserElement.cs(2 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserElement.cs(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActUIElementHandler.cs (1)
Learnt from: IamRanjeetSingh
PR: Ginger-Automation/Ginger#4058
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActUIElementHandler.cs:648-649
Timestamp: 2025-01-16T04:09:15.662Z
Learning: The Ginger project primarily targets .NET 8.0, which supports C# 12 features including collection expressions with spread operator.
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserElement.cs (1)
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4107
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserElement.cs:622-652
Timestamp: 2025-02-17T12:22:22.732Z
Learning: Playwright's IElementHandle does not support drag-drop operations (DragToAsync). Only ILocator provides drag-drop functionality, so operations should throw InvalidOperationException when attempted with element handles.
🔇 Additional comments (9)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActUIElementHandler.cs (3)
193-201: New UI element actions are well-defined
The addition ofDragDrop,DrawObject, andMultiSetValuecases improves feature coverage. Ensure unit tests are added for each new action to confirm correct behavior.
243-251: DrawObject operation might need exception handling
IfGetFirstMatchingElementAsync()returns null, callingawait element.DrawObjectAsync()will fail. Validate the element's existence after retrieval.
881-881: Line change confirmation
No functional change appears here; verify that the updated bracket doesn’t affect code flow.Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserElement.cs (4)
617-662: JavaScript-based drag-drop
Using customdispatchEvent(DragEvent, ...)calls is effective. Be sure it aligns with the supported browsers and that fallback logic is present for older browsers if required by your project.
707-719: Clear naming
GetElementLocator(IBrowserElement element)is straightforward. Ensure a helpful error message or logs exist if the returned locator is null to aid debugging.
721-761: Coordinate-based drag-drop
Coordinates for drag-and-drop are computed from the bounding box center. Confirm that fractional offsets (floats) are rounded consistently across browsers.
868-889: Optional arguments in ExecuteJavascriptAsync
The addition ofobject? args = nullis a good extension. Double-check that all call sites properly handle default arguments.Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/IBrowserElement.cs (2)
65-68: New methods for graphical interactions
The additions ofDrawObjectAsync(),DragDropAsync(...),DragDropJSAsync(...), andDragDropXYCordinateAsync(...)boost functionality. Ensure each method is implemented consistently across all driver classes.
77-77: ExecuteJavascriptAsync signature extension
Allowing optional arguments (object? args = null) is a solid improvement. Verify calls across the codebase use the updated signature.
2 participants
Thank you for your contribution.
Before submitting this PR, please make sure:
Summary by CodeRabbit