Feature - 39263 Playwright New POM Support #3811
Conversation
WalkthroughThe recent changes to the Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 13
Outside diff range and nitpick comments (11)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/IBrowserShadowRoot.cs (1)
9-12: Interface Definition: Add XML Documentation CommentsAdding XML documentation comments to the interface and its method will improve code readability and maintainability.
+ /// <summary> + /// Interface for handling browser shadow root elements. + /// </summary> internal interface IBrowserShadowRoot { + /// <summary> + /// Retrieves the HTML content of the shadow root. + /// </summary> public Task<string> HTML(); }Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserShadowRoot.cs (2)
10-17: Class Definition: Add XML Documentation CommentsAdding XML documentation comments to the class and its constructor will improve code readability and maintainability.
+ /// <summary> + /// Represents a Playwright browser shadow root element. + /// </summary> internal sealed class PlaywrightBrowserShadowRoot : IBrowserShadowRoot { private readonly IPlaywrightLocator _playwrightLocator; + /// <summary> + /// Initializes a new instance of the <see cref="PlaywrightBrowserShadowRoot"/> class. + /// </summary> + /// <param name="playwrightLocator">The Playwright locator for the shadow root element.</param> internal PlaywrightBrowserShadowRoot(IPlaywrightLocator playwrightLocator) { _playwrightLocator = playwrightLocator; } }
29-32: MethodShadowRootExists: Add XML Documentation CommentsAdding XML documentation comments to the method will improve code readability and maintainability.
+ /// <summary> + /// Checks if the shadow root exists for the element. + /// </summary> private Task<bool> ShadowRootExists() { return _playwrightLocator.EvaluateAsync<bool>("element => element.shadowRoot != null"); }Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/IBrowserElement.cs (2)
24-24: MethodPositionAsync: Add XML Documentation CommentsAdding XML documentation comments to the method will improve code readability and maintainability.
+ /// <summary> + /// Gets the position of the browser element. + /// </summary> public Task<Point> PositionAsync();
72-72: MethodShadowRootAsync: Add XML Documentation CommentsAdding XML documentation comments to the method will improve code readability and maintainability.
+ /// <summary> + /// Gets the shadow root of the browser element. + /// </summary> public Task<IBrowserShadowRoot?> ShadowRootAsync();Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMLearner.cs (2)
1-20: Remove unnecessary using directives.The using directives for
DocumentFormat.OpenXml.Office2010.ExcelAc,Microsoft.Graph,Microsoft.VisualStudio.Services.Common, andNPOI.OpenXmlFormatsseem unnecessary and can be removed to clean up the code.-using DocumentFormat.OpenXml.Office2010.ExcelAc; -using Microsoft.Graph; -using Microsoft.VisualStudio.Services.Common; -using NPOI.OpenXmlFormats;
569-569: Remove unnecessaryToString()call.There's no need to call
ToString()on a string.-strList.Add(item.ToString().Remove(0, 1)); +strList.Add(item.Remove(0, 1));Tools
GitHub Check: Codacy Static Code Analysis
[notice] 569-569: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMLearner.cs#L569
There's no need to call 'ToString()' on a string.Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserElement.cs (2)
19-20: Rename_playwrightElementHandleto maintain naming consistency.To maintain naming consistency, rename
_playwrightElementHandleto_elementHandle.-private readonly IPlaywrightElementHandle? _playwrightElementHandle; +private readonly IPlaywrightElementHandle? _elementHandle;
569-569: Remove unnecessaryToString()call.There's no need to call
ToString()on a string.-strList.Add(item.ToString().Remove(0, 1)); +strList.Add(item.Remove(0, 1));Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs (2)
Line range hint
375-407: Refactor asynchronous handling inGetScreenShot.Using
Task.Runwith.Wait()can lead to potential deadlocks and reduces readability. Consider usingasync/awaitdirectly within the method for better performance and readability.[SupportedOSPlatform("windows")] public async Task<Bitmap?> GetScreenShotAsync(Tuple<int, int>? size = null, bool fullPage = false) { ThrowIfClosed(); IBrowserTab tab = _browser!.CurrentWindow.CurrentTab; if (size != null) { await tab.SetViewportSizeAsync(new Size(width: size.Item1, height: size.Item2)); } byte[] screenshot; if (fullPage) { screenshot = await tab.ScreenshotAsync(); } else { screenshot = await tab.ScreenshotFullPageAsync(); } return BytesToBitmap(screenshot); }
Line range hint
407-443: Refactor asynchronous handling inGetElementScreenshot.Using
Task.Runwith.Wait()can lead to potential deadlocks and reduces readability. Consider usingasync/awaitdirectly within the method for better performance and readability.[SupportedOSPlatform("windows")] public async Task<Bitmap?> GetElementScreenshotAsync(Act act) { ThrowIfClosed(); eLocateBy locateBy; string locateValue; if (act is ActUIElement actUIElement) { locateBy = actUIElement.ElementLocateBy; locateValue = actUIElement.ElementLocateValueForDriver; } else { locateBy = act.LocateBy; locateValue = act.LocateValueCalculated; } IBrowserElement? element = (await _browser! .CurrentWindow .CurrentTab .GetElementsAsync(locateBy, locateValue)) .FirstOrDefault(); if (element == null) { return null; } byte[] screenshot = await element.ScreenshotAsync(); return BytesToBitmap(screenshot); }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (14)
- Ginger/Ginger/AutomatePageLib/AddActionMenu/WindowExplorer/Common/WindowExplorerPage.xaml.cs (1 hunks)
- Ginger/GingerCoreCommon/UIElement/AppWindow.cs (1 hunks)
- Ginger/GingerCoreCommon/UIElement/ElementInfo.cs (1 hunks)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActGotoURLHandler.cs (1 hunks)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/GingerWebDriver.cs (3 hunks)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/IBrowserElement.cs (3 hunks)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/IBrowserShadowRoot.cs (1 hunks)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/IBrowserTab.cs (2 hunks)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMLearner.cs (1 hunks)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserElement.cs (8 hunks)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserShadowRoot.cs (1 hunks)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs (5 hunks)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs (6 hunks)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (2 hunks)
Files skipped from review due to trivial changes (1)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActGotoURLHandler.cs
Additional context used
Learnings (3)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs (1)
Learnt from: IamRanjeetSingh PR: Ginger-Automation/Ginger#3783 File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs:129-129 Timestamp: 2024-06-24T08:39:59.351Z Learning: User IamRanjeetSingh has indicated that the `RunAction` method in the `PlaywrightDriver` class should remain synchronous due to current constraints in the codebase.Ginger/Ginger/AutomatePageLib/AddActionMenu/WindowExplorer/Common/WindowExplorerPage.xaml.cs (4)
Learnt from: prashelke PR: Ginger-Automation/Ginger#3429 File: Ginger/Ginger/AutomatePageLib/AddActionMenu/WindowExplorer/Common/WindowExplorerPage.xaml.cs:1139-1142 Timestamp: 2024-01-05T14:03:05.043Z Learning: The user has updated the code by removing or addressing the commented code as suggested in the review.Learnt from: prashelke PR: Ginger-Automation/Ginger#3429 File: Ginger/Ginger/AutomatePageLib/AddActionMenu/WindowExplorer/Common/WindowExplorerPage.xaml.cs:1139-1142 Timestamp: 2024-01-05T14:03:05.043Z Learning: The user has updated the code by removing or addressing the commented code as suggested in the review.Learnt from: prashelke PR: Ginger-Automation/Ginger#3429 File: Ginger/Ginger/AutomatePageLib/AddActionMenu/WindowExplorer/Common/WindowExplorerPage.xaml.cs:1123-1126 Timestamp: 2024-01-05T14:01:37.634Z Learning: The user has removed an unnecessary empty line at 1123 in `WindowExplorerPage.xaml.cs` for better code compactness.Learnt from: prashelke PR: Ginger-Automation/Ginger#3429 File: Ginger/Ginger/AutomatePageLib/AddActionMenu/WindowExplorer/Common/WindowExplorerPage.xaml.cs:1123-1126 Timestamp: 2024-01-05T14:01:37.634Z Learning: The user has removed an unnecessary empty line at 1123 in `WindowExplorerPage.xaml.cs` for better code compactness.Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (2)
Learnt from: manas-droid PR: Ginger-Automation/Ginger#3436 File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs:6702-6749 Timestamp: 2024-01-10T08:58:17.226Z Learning: The user has indicated that the commented `CheckifPageLoaded` method is needed for later use, suggesting a requirement to retain the code within the source files for reference or potential re-implementation.Learnt from: manas-droid PR: Ginger-Automation/Ginger#3436 File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs:6702-6749 Timestamp: 2024-01-10T08:58:17.226Z Learning: The user has indicated that the commented `CheckifPageLoaded` method is needed for later use, suggesting a requirement to retain the code within the source files for reference or potential re-implementation.
GitHub Check: Codacy Static Code Analysis
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/GingerWebDriver.cs
[warning] 284-284: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/GingerWebDriver.cs#L284
Either remove or fill this block of code.
[notice] 284-284: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/GingerWebDriver.cs#L284
Handle the exception or explain in a comment why it can be ignored.
[warning] 307-307: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/GingerWebDriver.cs#L307
Remove this commented out code.Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMLearner.cs
[failure] 302-302: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMLearner.cs#L302
Add a 'default' clause to this 'switch' statement.
[notice] 569-569: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMLearner.cs#L569
There's no need to call 'ToString()' on a string.Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserElement.cs
[warning] 361-361: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserElement.cs#L361
'System.Exception' should not be thrown by user code.
[warning] 372-372: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserElement.cs#L372
'System.Exception' should not be thrown by user code.
[warning] 376-376: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserElement.cs#L376
'System.Exception' should not be thrown by user code.Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs
[warning] 775-775: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs#L775
'System.Exception' should not be thrown by user code.
[failure] 957-957: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs#L957
Add a 'default' clause to this 'switch' statement.
[notice] 1281-1281: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs#L1281
Remove this empty statement.
[warning] 1326-1326: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs#L1326
Remove this useless assignment to local variable 'elemNode'.
[notice] 1350-1350: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs#L1350
Use a StringBuilder instead.
[warning] 1382-1382: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs#L1382
Remove this commented out code.
Additional comments not posted (28)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserShadowRoot.cs (1)
19-27: MethodHTML: Ensure Proper Error HandlingThe method throws an
InvalidOperationExceptionif no shadow root exists. Ensure that this exception is handled appropriately where this method is called.Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/IBrowserTab.cs (2)
35-35: Addition ofInjectJavascriptAsyncmethod approved.This method enhances the interface's capability to inject JavaScript into the browser tab, which is useful for browser automation.
66-66: Addition ofGetElementsAsyncmethod approved.This method enhances the interface by allowing element retrieval using JavaScript, providing flexibility for complex element retrieval scenarios.
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs (2)
88-92: Addition ofInjectJavascriptAsyncmethod approved.This method enhances the class's capability to inject JavaScript into the browser tab, which is useful for browser automation.
271-281: Addition ofGetElementsAsyncmethod approved.This method enhances the class by allowing element retrieval using JavaScript, providing flexibility for complex element retrieval scenarios.
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/GingerWebDriver.cs (16)
54-69: Modification toPomCategoryproperty approved.The new getter and setter logic ensures that the
PomCategoryis set toePomElementCategory.Webif it is not already set.
87-87: Addition ofGetBrowsermethod approved.This abstract method is essential for retrieving the browser instance, which is a fundamental part of the web driver functionality.
89-111: Addition ofGetAppWindowsAsyncmethod approved.This method asynchronously retrieves the list of application windows, which is useful for managing multiple windows in browser automation.
113-143: Addition ofSwitchTabAsyncmethod approved.This method provides a flexible way to switch between tabs, which is useful for scenarios where specific tabs need to be targeted.
145-157: Addition ofHighlightElementAsyncandUnhighlightElementAsyncmethods approved.These methods are useful for visually identifying elements during automation, which can help with debugging and validation.
159-163: Addition ofSwitchToFramemethod approved.This method is essential for switching frames during browser automation.
165-189: Addition ofSwitchToFrameOfElementAsyncmethod approved.This method is useful for switching to the frame of a specific element, which is a common requirement in browser automation.
191-225: Addition ofFindBrowserElementAsyncmethod approved.This method is essential for finding elements during browser automation.
227-264: Addition ofTestElementLocatorsAsyncmethod approved.This method is useful for testing element locators, which is important for ensuring the reliability of browser automation scripts.
272-287: Addition ofIsLiveSpyScriptInjectedAsyncmethod approved.This method is useful for checking if the Live Spy script is injected, which is important for certain automation tasks.
Tools
GitHub Check: Codacy Static Code Analysis
[warning] 284-284: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/GingerWebDriver.cs#L284
Either remove or fill this block of code.
[notice] 284-284: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/GingerWebDriver.cs#L284
Handle the exception or explain in a comment why it can be ignored.
289-302: Addition ofInjectLiveSpyScriptAsyncmethod approved.This method is essential for injecting the Live Spy script, which is important for certain automation tasks.
304-309: Addition ofInjectScriptAsyncmethod approved.This method is useful for injecting scripts into the browser, which is a common requirement in browser automation.
Tools
GitHub Check: Codacy Static Code Analysis
[warning] 307-307: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/GingerWebDriver.cs#L307
Remove this commented out code.
323-323: Addition ofFindBrowserElementAsyncmethod approved.This abstract method is essential for finding elements during browser automation.
325-377: Addition ofGenerateXPathFromHTMLElementInfomethod approved.This method is useful for generating XPath from HTML element information, which is important for locating elements during browser automation.
379-409: Addition ofGenerateXPathFromBrowserElementAsyncmethod approved.This method is useful for generating XPath from browser elements, which is important for locating elements during browser automation.
411-490: Addition ofCreateHtmlElementAsyncmethod approved.This method is useful for creating HTML elements, which is important for interacting with elements during browser automation.
Ginger/GingerCoreCommon/UIElement/ElementInfo.cs (1)
363-364: Addition ofParentElementandChildElementsproperties approved.These properties enhance the class's ability to manage element hierarchies, which is useful for complex UI interactions.
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMLearner.cs (1)
26-33: InterfaceIBrowserElementProvidermethods look good.The methods added to the
IBrowserElementProviderinterface are well-defined and cover necessary interactions with browser elements, frames, and shadow DOM.Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs (2)
375-375: LGTM!The method
ThrowIfClosedis correctly implemented to ensure that operations are only performed when the browser is running.
872-888: LGTM!The method
GetPageSourceDocumentis correctly implemented to handle the retrieval and caching of the page source document.Ginger/Ginger/AutomatePageLib/AddActionMenu/WindowExplorer/Common/WindowExplorerPage.xaml.cs (1)
557-557: > [!TIP]Codebase Verification
Update Commented-Out Enum Value
There is still a commented-out instance of
AppWindow.eWindowType.SeleniumWebPagein the codebase.
Ginger/Ginger/UserControlsLib/UCWindowsGrid.xaml.csEnsure this is updated to
AppWindow.eWindowType.WebPageor removed if it is no longer needed.Analysis chain
LGTM! But verify the enum usage in the codebase.
The code change from
AppWindow.eWindowType.SeleniumWebPagetoAppWindow.eWindowType.WebPageis approved.However, ensure that all instances of
AppWindow.eWindowType.SeleniumWebPagehave been updated toAppWindow.eWindowType.WebPagein the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all instances of `AppWindow.eWindowType.SeleniumWebPage` have been updated to `AppWindow.eWindowType.WebPage`. # Test: Search for the old enum value. Expect: No occurrences of the old enum value. rg --type cs $'AppWindow.eWindowType.SeleniumWebPage'Length of output: 166
Skipped due to learnings
Learnt from: prashelke PR: Ginger-Automation/Ginger#3429 File: Ginger/Ginger/AutomatePageLib/AddActionMenu/WindowExplorer/Common/WindowExplorerPage.xaml.cs:1139-1142 Timestamp: 2024-01-05T14:03:05.043Z Learning: The user has updated the code by removing or addressing the commented code as suggested in the review.Learnt from: prashelke PR: Ginger-Automation/Ginger#3429 File: Ginger/Ginger/AutomatePageLib/AddActionMenu/WindowExplorer/Common/WindowExplorerPage.xaml.cs:1139-1142 Timestamp: 2024-01-05T14:03:05.043Z Learning: The user has updated the code by removing or addressing the commented code as suggested in the review.Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (2)
5011-5011: LGTM!The
WindowTypeproperty update toAppWindow.eWindowType.WebPageis consistent with the provided context.
10951-10961: Verify exception handling for NotImplementedException.The methods
GetBrowserandFindBrowserElementAsyncare placeholders and throwNotImplementedException. Ensure that these exceptions are properly handled in the calling code to avoid runtime issues.
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserElement.cs
Show resolved
Hide resolved
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs
Show resolved
Hide resolved
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs
Outdated
Show resolved
Hide resolved
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs
Outdated
Show resolved
Hide resolved
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs
Outdated
Show resolved
Hide resolved
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/GingerWebDriver.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs
There was a problem hiding this comment.
Actionable comments posted: 21
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/GingerWebDriver.cs (3 hunks)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/IBrowserElement.cs (4 hunks)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/IBrowserTab.cs (2 hunks)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMLearner.cs (1 hunks)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserElement.cs (9 hunks)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs (5 hunks)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs (6 hunks)
Files skipped from review as they are similar to previous changes (3)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/IBrowserElement.cs
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/IBrowserTab.cs
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs
Additional context used
Learnings (2)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserElement.cs (1)
Learnt from: IamRanjeetSingh PR: Ginger-Automation/Ginger#3811 File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserElement.cs:392-407 Timestamp: 2024-07-08T13:53:26.335Z Learning: When suggesting to avoid throwing `System.Exception` directly, if the user defers the change, acknowledge their decision and note that the change might be considered in future revisions.Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs (1)
Learnt from: IamRanjeetSingh PR: Ginger-Automation/Ginger#3783 File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs:129-129 Timestamp: 2024-06-24T08:39:59.351Z Learning: User IamRanjeetSingh has indicated that the `RunAction` method in the `PlaywrightDriver` class should remain synchronous due to current constraints in the codebase.
GitHub Check: Codacy Static Code Analysis
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/GingerWebDriver.cs
[warning] 284-284: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/GingerWebDriver.cs#L284
Either remove or fill this block of code.
[notice] 284-284: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/GingerWebDriver.cs#L284
Handle the exception or explain in a comment why it can be ignored.
[warning] 307-307: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/GingerWebDriver.cs#L307
Remove this commented out code.Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserElement.cs
[warning] 392-392: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserElement.cs#L392
'System.Exception' should not be thrown by user code.
[warning] 403-403: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserElement.cs#L403
'System.Exception' should not be thrown by user code.
[warning] 407-407: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserElement.cs#L407
'System.Exception' should not be thrown by user code.Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMLearner.cs
[failure] 302-302: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMLearner.cs#L302
Add a 'default' clause to this 'switch' statement.
[warning] 406-406: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMLearner.cs#L406
Either remove or fill this block of code.
[notice] 632-632: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMLearner.cs#L632
There's no need to call 'ToString()' on a string.Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs
[warning] 550-550: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs#L550
Either remove or fill this block of code.
[notice] 550-550: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs#L550
Handle the exception or explain in a comment why it can be ignored.
[warning] 799-799: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs#L799
'System.Exception' should not be thrown by user code.
[failure] 1004-1004: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs#L1004
Add a 'default' clause to this 'switch' statement.
[notice] 1336-1336: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs#L1336
Remove this empty statement.
[warning] 1381-1381: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs#L1381
Remove this useless assignment to local variable 'elemNode'.
[notice] 1405-1405: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs#L1405
Use a StringBuilder instead.
[warning] 1437-1437: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs#L1437
Remove this commented out code.
Additional comments not posted (33)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/GingerWebDriver.cs (12)
145-150: LGTM!The code changes are approved.
152-157: LGTM!The code changes are approved.
159-163: LGTM!The code changes are approved.
165-189: LGTM!The code changes are approved.
191-225: LGTM!The code changes are approved.
227-263: LGTM!The code changes are approved.
289-302: LGTM!The code changes are approved.
311-321: LGTM!The code changes are approved.
323-323: LGTM!The code changes are approved.
325-377: LGTM!The code changes are approved.
379-409: LGTM!The code changes are approved.
411-489: LGTM!The code changes are approved.
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserElement.cs (12)
20-29: LGTM!The code changes are approved.
34-43: LGTM!The code changes are approved.
65-73: LGTM!The code changes are approved.
105-113: LGTM!The code changes are approved.
135-143: LGTM!The code changes are approved.
175-183: LGTM!The code changes are approved.
199-207: LGTM!The code changes are approved.
223-231: LGTM!The code changes are approved.
245-255: LGTM!The code changes are approved.
281-311: LGTM!The code changes are approved.
313-336: LGTM!The code changes are approved.
338-361: LGTM!The code changes are approved.
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs (9)
124-152: Refactor asynchronous handling inRunAction.Using
Task.Runwith.Wait()can lead to potential deadlocks and reduces readability. Consider usingasync/awaitdirectly within the method for better performance and readability.public override async Task RunActionAsync(Act act) { ThrowIfClosed(); switch (act) { case ActBrowserElement actBrowserElement: ActBrowserElementHandler actBrowserElementHandler = new(actBrowserElement, _browser, new ActBrowserElementHandler.Context { BusinessFlow = BusinessFlow, Environment = Environment, }); await actBrowserElementHandler.HandleAsync(); break; case ActUIElement actUIElement: ActUIElementHandler actUIElementHandler = new(actUIElement, _browser, BusinessFlow, Environment); await actUIElementHandler.HandleAsync(); break; case ActScreenShot actScreenShot: ActScreenShotHandler actScreenShotHandler = new(actScreenShot, _browser); await actScreenShotHandler.HandleAsync(); break; case ActGotoURL actGotoURL: ActGotoURLHandler actGotoURLHandler = new(actGotoURL, _browser); await actGotoURLHandler.HandleAsync(); break; default: act.Error = $"Run Action Failed due to unrecognized action type - {act.GetType().Name}"; break; } }
460-465: Refactor asynchronous handling inGetAppWindows.Using
Task.Runwith.Wait()can lead to potential deadlocks and reduces readability. Consider usingasync/awaitdirectly within the method for better performance and readability.public async Task<List<AppWindow>> GetAppWindowsAsync() { //TODO: unhighlight last highlighted element IEnumerable<AppWindow> appWindows = await GetAppWindowsAsync(); return new List<AppWindow>(appWindows); }
467-477: Refactor asynchronous handling inSwitchWindow.Using
Task.Runwith.Wait()can lead to potential deadlocks and reduces readability. Consider usingasync/awaitdirectly within the method for better performance and readability.public async Task SwitchWindowAsync(string tabTitle) { await SwitchTabAsync(filter: async (_, tab) => { string title = await tab.TitleAsync(); return string.Equals(title, tabTitle); }); }
479-504: Refactor asynchronous handling inHighLightElement.Using
Task.Runwith.Wait()can lead to potential deadlocks and reduces readability. Consider usingasync/await` directly within the method for better performance and readability.public async Task HighLightElementAsync(ElementInfo element, bool locateElementByItLocators = false, IList<ElementInfo> MappedUIElements = null) { if (_lastHighlightedElement != null) { await UnhighlightElementAsync(_lastHighlightedElement); } await SwitchToFrameOfElementAsync(element); IBrowserElement? browserElement = await FindBrowserElementAsync(element); if (browserElement == null) { return; } _lastHighlightedElement = browserElement; await HighlightElementAsync(browserElement); }
1349-1450: Refactor asynchronous handling inGetElementAtPoint.Using
Task.Runwith.Wait()can lead to potential deadlocks and reduces readability. Consider usingasync/await` directly within the method for better performance and readability.public async Task<ElementInfo?> GetElementAtPointAsync(long ptX, long ptY) { ThrowIfClosed(); try { HTMLElementInfo? elemInfo = null; string iframeXPath = string.Empty; Point parentElementLocation = new Point(0, 0); while (true) { string s_Script = $"return document.elementFromPoint({ptX}, {ptY});"; IBrowserElement? ele = (await _browser.CurrentWindow.CurrentTab.GetElementsAsync(s_Script)).FirstOrDefault(); if (ele == null) { return null; } else { HtmlNode? elemNode = null; string elemId; try { elemId = await ele.AttributeValueAsync("id"); if (_currentPageDocument == null) { _currentPageDocument = new HtmlDocument(); _currentPageDocument.LoadHtml(GetCurrentPageSourceString()); } elemNode = _currentPageDocument.DocumentNode.Descendants().FirstOrDefault(x => x.Id.Equals(elemId)); } catch (Exception) { elemId = ""; } elemInfo = await CreateHtmlElementAsync(ele); elemInfo.Path = iframeXPath; } if (elemInfo.ElementTypeEnum != eElementType.Iframe) { await _browser.CurrentWindow.CurrentTab.SwitchToMainFrameAsync(); break; } if (string.IsNullOrEmpty(iframeXPath)) { iframeXPath = elemInfo.XPath; } else { iframeXPath += "," + elemInfo.XPath; } parentElementLocation.X += elemInfo.X; parentElementLocation.Y += elemInfo.Y; String s_Script2 = "var X, Y; " + "if (window.pageYOffset) " + "{ " + " X = window.pageXOffset; " + " Y = window.pageYOffset; " + "} " + "else " + "{ " + " var Elem = document.documentElement; " + " if (!Elem.clientHeight) Elem = document.body; " + " X = Elem.scrollLeft; " + " Y = Elem.scrollTop; " + "} " + "return new Array(X, Y);"; var i_Coord = (await _browser.CurrentWindow.CurrentTab.ExecuteJavascriptAsync(s_Script2)).ToList(); int s32_ScrollX = Convert.ToInt32(i_Coord[0]); int s32_ScrollY = Convert.ToInt32(i_Coord[1]); Point elePosition = await ele.PositionAsync(); Point p_Pos = new Point(elePosition.X - s32_ScrollX, elePosition.Y - s32_ScrollY); ptX -= p_Pos.X; ptY -= p_Pos.Y; } elemInfo.X += parentElementLocation.X; elemInfo.Y += parentElementLocation.Y; return elemInfo; } catch (Exception ex) { Reporter.ToLog(eLogLevel.ERROR, "Failed to Get Element At Point", ex); return null; } }Tools
GitHub Check: Codacy Static Code Analysis
[warning] 1381-1381: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs#L1381
Remove this useless assignment to local variable 'elemNode'.
[notice] 1405-1405: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs#L1405
Use a StringBuilder instead.
[warning] 1437-1437: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs#L1437
Remove this commented out code.
1276-1340: Refactor asynchronous handling inGetVisualElementsInfo.Using
Task.Runwith.Wait()can lead to potential deadlocks and reduces readability. Consider usingasync/await` directly within the method for better performance and readability.[SupportedOSPlatform("windows")] public async Task<VisualElementsInfo> GetVisualElementsInfoAsync() { ThrowIfClosed(); VisualElementsInfo visualElementsInfo = new(); visualElementsInfo.Bitmap = await GetScreenShotAsync(); //TODO: add function to get all tags available - below is missing some... List<IBrowserElement> visualBrowserElements = new(); visualBrowserElements.AddRange(await _browser.CurrentWindow.CurrentTab.GetElementsAsync(eLocateBy.ByTagName, "a")); visualBrowserElements.AddRange(await _browser.CurrentWindow.CurrentTab.GetElementsAsync(eLocateBy.ByTagName, "input")); visualBrowserElements.AddRange(await _browser.CurrentWindow.CurrentTab.GetElementsAsync(eLocateBy.ByTagName, "select")); visualBrowserElements.AddRange(await _browser.CurrentWindow.CurrentTab.GetElementsAsync(eLocateBy.ByTagName, "label")); visualBrowserElements.AddRange(await _browser.CurrentWindow.CurrentTab.GetElementsAsync(eLocateBy.ByTagName, "H1")); visualBrowserElements.AddRange(await _browser.CurrentWindow.CurrentTab.GetElementsAsync(eLocateBy.ByTagName, "H2")); visualBrowserElements.AddRange(await _browser.CurrentWindow.CurrentTab.GetElementsAsync(eLocateBy.ByTagName, "H3")); visualBrowserElements.AddRange(await _browser.CurrentWindow.CurrentTab.GetElementsAsync(eLocateBy.ByTagName, "H4")); visualBrowserElements.AddRange(await _browser.CurrentWindow.CurrentTab.GetElementsAsync(eLocateBy.ByTagName, "H5")); visualBrowserElements.AddRange(await _browser.CurrentWindow.CurrentTab.GetElementsAsync(eLocateBy.ByTagName, "H6")); List<IBrowserElement> visibleVisualBrowserElements = new(); foreach (IBrowserElement visualBrowserElement in visualBrowserElements) { bool isVisible = await visualBrowserElement.IsVisibleAsync(); Size size = await visualBrowserElement.SizeAsync(); if (isVisible && size.Width > 0 && size.Height > 0) { visibleVisualBrowserElements.Add(visualBrowserElement); } } List<VisualElement> visualElements = new(); foreach (IBrowserElement visibleVisualBrowserElement in visibleVisualBrowserElements) { string tag = await visibleVisualBrowserElement.TagNameAsync(); string text = await visibleVisualBrowserElement.TextContentAsync(); if (string.IsNullOrEmpty(text) && string.Equals(tag, "input", StringComparison.OrdinalIgnoreCase)) { text = await visibleVisualBrowserElement.AttributeValueAsync("value"); if (string.IsNullOrEmpty(text)) { text = await visibleVisualBrowserElement.AttributeValueAsync("outerHTML"); } } Size size = await visibleVisualBrowserElement.SizeAsync(); Point position = await visibleVisualBrowserElement.PositionAsync(); VisualElement VE = new() { ElementType = tag, Text = text, X = position.X, Y = position.Y, Width = size.Width, Height = size.Height }; visualElements.Add(VE); } visualElementsInfo.Elements = visualElements; return visualElementsInfo; }Tools
GitHub Check: Codacy Static Code Analysis
[notice] 1336-1336: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs#L1336
Remove this empty statement.
1371-1381: Remove commented-out code and unnecessary assignment.The commented-out line of code and the assignment to
elemNodeare unnecessary and should be removed to improve code readability.- HtmlNode? elemNode = null; - // ele.TagName != "frame" && ele.TagName != "iframe")Tools
GitHub Check: Codacy Static Code Analysis
[warning] 1381-1381: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs#L1381
Remove this useless assignment to local variable 'elemNode'.
656-702: Refactor asynchronous handling inGetElementChildren.Using
Task.Runwith.Wait()can lead to potential deadlocks and reduces readability. Consider usingasync/await` directly within the method for better performance and readability.public async Task<List<ElementInfo>> GetElementChildrenAsync(ElementInfo elementInfo) { ThrowIfClosed(); if (elementInfo == null || elementInfo is not HTMLElementInfo htmlElementInfo) { return []; } await SwitchToFrameOfElementAsync(elementInfo); string xpath = GenerateXPathFromHTMLElementInfo(htmlElementInfo); IEnumerable<IBrowserElement> browserElements = await _browser.CurrentWindow.CurrentTab.GetElementsAsync(eLocateBy.ByXPath, xpath); List<HTMLElementInfo> htmlElements = []; foreach (IBrowserElement browserElement in browserElements) { HTMLElementInfo newHtmlElement = await CreateHtmlElementAsync(browserElement); if (string.IsNullOrEmpty(newHtmlElement.ID)) { newHtmlElement.ID = htmlElementInfo.HTMLElementObject.Id; } string elementPath = htmlElementInfo.Path; if (string.IsNullOrEmpty(htmlElementInfo.XPath)) { string[] xpathSegments = htmlElementInfo.XPath.Split('/', StringSplitOptions.RemoveEmptyEntries); if (xpathSegments.Length > 0) { string lastXPathSegment = xpathSegments[^1].TrimStart('[').TrimEnd(']'); if (string.Equals(lastXPathSegment, "frame", StringComparison.OrdinalIgnoreCase) || string.Equals(lastXPathSegment, "iframe", StringComparison.OrdinalIgnoreCase)) { elementPath = $"{htmlElementInfo.Path},{htmlElementInfo.XPath}"; } elementPath = elementPath.TrimStart(','); } } newHtmlElement.Path = elementPath; newHtmlElement.WindowExplorer = this; newHtmlElement.RelXpath = POMLearner.GenerateRelativeXPathFromHTMLElementInfo(newHtmlElement, xpathImpl: this, pomSetting: null); htmlElements.Add(newHtmlElement); } return htmlElements.Cast<ElementInfo>().ToList(); }
587-611: Refactor asynchronous handling inGetVisibleControls.Using
Task.Runwith.Wait()can lead to potential deadlocks and reduces readability. Consider usingasync/await` directly within the method for better performance and readability.public async Task<List<ElementInfo>> GetVisibleControlsAsync(PomSetting pomSetting, ObservableList<ElementInfo> foundElementsList = null, ObservableList<POMPageMetaData> PomMetaData = null) { ThrowIfClosed(); if (_lastHighlightedElement != null) { await UnhighlightElementAsync(_lastHighlightedElement); } IBrowserTab currentTab = _browser.CurrentWindow.CurrentTab; await currentTab.SwitchToMainFrameAsync(); string pageSource = await _browser.CurrentWindow.CurrentTab.PageSourceAsync(); POMLearner pomLearner = POMLearner.Create(pageSource, new PlaywrightBrowserElementProvider(currentTab), pomSetting, xpathImpl: this); IEnumerable<HTMLElementInfo> htmlElements = await pomLearner.LearnElementsAsync(); if (foundElementsList != null) { foundElementsList.AddRange(htmlElements.Cast<ElementInfo>()); } return new(htmlElements); }
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/GingerWebDriver.cs
Outdated
Show resolved
Hide resolved
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserElement.cs
Show resolved
Hide resolved
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserElement.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 15
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/GingerWebDriver.cs (3 hunks)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMLearner.cs (1 hunks)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs (6 hunks)
Additional context used
Learnings (3)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/GingerWebDriver.cs (2)
Learnt from: IamRanjeetSingh PR: Ginger-Automation/Ginger#3811 File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserElement.cs:386-410 Timestamp: 2024-07-08T14:02:08.377Z Learning: When suggesting to avoid throwing `System.Exception` directly, if the user defers the change, acknowledge their decision and note that the change might be considered in future revisions.Learnt from: IamRanjeetSingh PR: Ginger-Automation/Ginger#3811 File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserElement.cs:392-407 Timestamp: 2024-07-08T13:53:26.335Z Learning: When suggesting to avoid throwing `System.Exception` directly, if the user defers the change, acknowledge their decision and note that the change might be considered in future revisions.Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMLearner.cs (4)
Learnt from: IamRanjeetSingh PR: Ginger-Automation/Ginger#3811 File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMLearner.cs:551-551 Timestamp: 2024-07-08T14:00:54.980Z Learning: In C# 9.0 and later versions, the `[]` syntax can be used for list initialization to create an empty list of the inferred type.Learnt from: IamRanjeetSingh PR: Ginger-Automation/Ginger#3811 File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMLearner.cs:171-172 Timestamp: 2024-07-08T14:00:35.641Z Learning: In C#, the correct way to initialize a list is using the `List<T>` constructor, not `[]`.Learnt from: IamRanjeetSingh PR: Ginger-Automation/Ginger#3811 File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMLearner.cs:679-679 Timestamp: 2024-07-08T14:03:28.207Z Learning: The syntax `Stack<HtmlNode> nodes = [];` is unconventional and likely invalid in C#. The standard way to initialize a list or stack in C# is using the `new` keyword.Learnt from: IamRanjeetSingh PR: Ginger-Automation/Ginger#3811 File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMLearner.cs:54-55 Timestamp: 2024-07-08T13:53:40.645Z Learning: In C#, the correct syntax for initializing a list is `new List<string> { "POMLearner", "Web" };`.Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs (2)
Learnt from: IamRanjeetSingh PR: Ginger-Automation/Ginger#3738 File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserWindow.cs:30-67 Timestamp: 2024-06-07T21:51:52.559Z Learning: User IamRanjeetSingh prefers keeping certain operations synchronous in the context of the `PlaywrightBrowserWindow` class due to specific requirements or constraints.Learnt from: IamRanjeetSingh PR: Ginger-Automation/Ginger#3783 File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs:129-129 Timestamp: 2024-06-24T08:39:59.351Z Learning: User IamRanjeetSingh has indicated that the `RunAction` method in the `PlaywrightDriver` class should remain synchronous due to current constraints in the codebase.
GitHub Check: Codacy Static Code Analysis
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/GingerWebDriver.cs
[warning] 284-284: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/GingerWebDriver.cs#L284
Either remove or fill this block of code.
[notice] 284-284: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/GingerWebDriver.cs#L284
Handle the exception or explain in a comment why it can be ignored.Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMLearner.cs
[failure] 297-297: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMLearner.cs#L297
Add a 'default' clause to this 'switch' statement.
[warning] 401-401: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMLearner.cs#L401
Either remove or fill this block of code.
[notice] 627-627: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMLearner.cs#L627
There's no need to call 'ToString()' on a string.Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs
[warning] 801-801: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs#L801
'System.Exception' should not be thrown by user code.
[failure] 1006-1006: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs#L1006
Add a 'default' clause to this 'switch' statement.
[notice] 1338-1338: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs#L1338
Remove this empty statement.
[warning] 1383-1383: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs#L1383
Remove this useless assignment to local variable 'elemNode'.
[notice] 1407-1407: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs#L1407
Use a StringBuilder instead.
[warning] 1439-1439: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs#L1439
Remove this commented out code.
Additional comments not posted (21)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/GingerWebDriver.cs (9)
145-150: LGTM!The method for highlighting an element by changing its style looks good.
152-157: LGTM!The method for unhighlighting an element by resetting its style looks good.
159-163: LGTM!The method for switching to a frame within the current tab looks good.
165-189: LGTM!The method for switching to a frame of a specified element looks good.
191-224: LGTM!The method for finding a browser element based on the locators provided in the
ElementInfolooks good.
227-264: LGTM!The method for testing the locators of an element looks good.
289-302: LGTM!The method for injecting the LiveSpy script into a browser tab looks good.
304-308: LGTM!The method for injecting a script into a browser tab looks good.
54-69: LGTM! But verify the usage ofPomCategory.The changes to the getter and setter logic for
PomCategorylook good. Ensure that all usages ofPomCategoryare consistent with the new logic.Verification successful
LGTM! But verify the usage of
PomCategory.The changes to the getter and setter logic for
PomCategorylook good. Ensure that all usages ofPomCategoryare consistent with the new logic.
- Ginger/GingerCore/Drivers/WindowsLib/WindowsDriver.cs
- Ginger/GingerCore/Drivers/PBDriver/PBDriver.cs
- Ginger/GingerCore/Drivers/MainFrame/MainFrameDriver.cs
- Ginger/GingerCore/Drivers/JavaDriverLib/JavaDriver.cs
- Ginger/GingerCoreNET/RunLib/DriverBase.cs
- Ginger/GingerCoreNET/RecordingLib/RecordingManager.cs
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/GingerWebDriver.cs
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of `PomCategory` to ensure consistency with the new logic. # Test: Search for the usage of `PomCategory`. Expect: Consistent usage. rg --type csharp $'PomCategory'Length of output: 5579
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMLearner.cs (11)
52-56: LGTM!The method for initializing a list of important properties using the
[]syntax looks good.
75-116: LGTM!The method for initializing a list of
HTMLElementInfousing the[]syntax looks good.
147-190: LGTM!The method for initializing lists of locators and properties using the
[]syntax looks good.
378-404: LGTM!The method for initializing a list of optional values using the
[]syntax looks good.Tools
GitHub Check: Codacy Static Code Analysis
[warning] 401-401: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMLearner.cs#L401
Either remove or fill this block of code.
407-539: LGTM!The method for initializing a list of properties using the
[]syntax looks good.
542-601: LGTM!The method for initializing a list of locators using the
[]syntax looks good.
603-652: LGTM!The method for initializing a list of locators using the
[]syntax looks good.Tools
GitHub Check: Codacy Static Code Analysis
[notice] 627-627: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMLearner.cs#L627
There's no need to call 'ToString()' on a string.
655-707: LGTM!The method for initializing a stack of
HtmlNodeusing the[]syntax looks good.
732-762: LGTM!The method for initializing a list of
HTMLElementInfousing the[]syntax looks good.
764-793: LGTM!The method for initializing a list of
HTMLElementInfousing the[]syntax looks good.
720-730: LGTM!The method for capturing a screenshot of an element and returning it as a base64 string looks good.
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs (1)
804-840: Refactor asynchronous handling inUpdateElementInfoFields.Using
Task.Runwith.Wait()can lead to potential deadlocks and reduces readability. Consider usingasync/awaitdirectly within the method for better performance and readability.public async Task UpdateElementInfoFieldsAsync(ElementInfo elementInfo) { ThrowIfClosed(); if (elementInfo == null || elementInfo is not HTMLElementInfo htmlElementInfo) { return; } IBrowserElement? browserElement = (IBrowserElement)elementInfo.ElementObject; if (browserElement == null) { return; } if (string.IsNullOrEmpty(elementInfo.XPath)) { elementInfo.XPath </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs
Outdated
Show resolved
Hide resolved
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs
Outdated
Show resolved
Hide resolved
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs
Outdated
Show resolved
Hide resolved
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 8
Outside diff range and nitpick comments (2)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs (2)
Line range hint
375-407: Refactor asynchronous handling inGetScreenShot.Using
Task.Runwith.Resultcan lead to potential deadlocks and reduces readability. Consider usingasync/awaitdirectly within the method for better performance and readability.[SupportedOSPlatform("windows")] public async Task<Bitmap?> GetScreenShotAsync(Tuple<int, int>? size = null, bool fullPage = false) { ThrowIfClosed(); IBrowserTab tab = _browser!.CurrentWindow.CurrentTab; if (size != null) { await tab.SetViewportSizeAsync(new Size(width: size.Item1, height: size.Item2)); } byte[] screenshot; if (fullPage) { screenshot = await tab.ScreenshotAsync(); } else { screenshot = await tab.ScreenshotFullPageAsync(); } return BytesToBitmap(screenshot); }
Line range hint
407-443: Refactor asynchronous handling inGetElementScreenshot.Using
Task.Runwith.Resultcan lead to potential deadlocks and reduces readability. Consider usingasync/awaitdirectly within the method for better performance and readability.[SupportedOSPlatform("windows")] public async Task<Bitmap?> GetElementScreenshotAsync(Act act) { ThrowIfClosed(); eLocateBy locateBy; string locateValue; if (act is ActUIElement actUIElement) { locateBy = actUIElement.ElementLocateBy; locateValue = actUIElement.ElementLocateValueForDriver; } else { locateBy = act.LocateBy; locateValue = act.LocateValueCalculated; } IBrowserElement? element = (await _browser! .CurrentWindow .CurrentTab .GetElementsAsync(locateBy, locateValue)) .FirstOrDefault(); if (element == null) { return null; } byte[] screenshot = await element.ScreenshotAsync(); return BytesToBitmap(screenshot); }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/GingerWebDriver.cs (3 hunks)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/IBrowserTab.cs (2 hunks)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMLearner.cs (1 hunks)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs (5 hunks)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs (6 hunks)
Files skipped from review as they are similar to previous changes (3)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/GingerWebDriver.cs
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/IBrowserTab.cs
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMLearner.cs
Additional context used
Learnings (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs (2)
Learnt from: IamRanjeetSingh PR: Ginger-Automation/Ginger#3738 File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserWindow.cs:30-67 Timestamp: 2024-06-07T21:51:52.559Z Learning: User IamRanjeetSingh prefers keeping certain operations synchronous in the context of the `PlaywrightBrowserWindow` class due to specific requirements or constraints.Learnt from: IamRanjeetSingh PR: Ginger-Automation/Ginger#3783 File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs:129-129 Timestamp: 2024-06-24T08:39:59.351Z Learning: User IamRanjeetSingh has indicated that the `RunAction` method in the `PlaywrightDriver` class should remain synchronous due to current constraints in the codebase.
Additional comments not posted (8)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs (6)
11-12: LGTM!The new using statements for
IPlaywrightJSHandleandIPlaywrightElementHandleare appropriate and necessary for the added functionalities.
27-30: LGTM!The additions of
eLocateBy.ByName,eLocateBy.ByTagName, andeLocateBy.ByRelXPathto theSupportedElementLocatorslist enhance the functionality of the class by supporting more locator types.
88-92: LGTM!The new overload of
ExecuteJavascriptAsyncthat takes an additionalobject argparameter enhances the flexibility of JavaScript execution.
94-101: LGTM!The new
InjectJavascriptAsyncmethod is a useful addition for injecting JavaScript code into the page, enhancing automation capabilities.
280-290: LGTM!The new
GetElementAsyncmethod allows for retrieving elements using custom JavaScript, which is useful for complex element retrieval scenarios.
362-370: LGTM!The additions of cases for
eLocateBy.ByNameandeLocateBy.ByTagNameto theGetElementLocatormethod enhance its versatility by supporting more locator types.Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs (2)
582-606: LGTM!The
GetVisibleControlsmethod correctly usesawaitfor asynchronous operations.
1344-1444: LGTM!The
GetElementAtPointmethod correctly usesawaitfor asynchronous operations.
| Task.Run(() => | ||
| { | ||
| case ActBrowserElement actBrowserElement: | ||
| ActBrowserElementHandler actBrowserElementHandler = new(actBrowserElement, _browser!, new ActBrowserElementHandler.Context | ||
| { | ||
| BusinessFlow = BusinessFlow, | ||
| Environment = Environment, | ||
| }); | ||
| actBrowserElementHandler.HandleAsync().Wait(); | ||
| break; | ||
| case ActUIElement actUIElement: | ||
| ActUIElementHandler actUIElementHandler = new(actUIElement, _browser!, BusinessFlow, Environment); | ||
| actUIElementHandler.HandleAsync().Wait(); | ||
| break; | ||
| case ActScreenShot actScreenShot: | ||
| ActScreenShotHandler actScreenShotHandler = new(actScreenShot, _browser!); | ||
| actScreenShotHandler.HandleAsync().Wait(); | ||
| break; | ||
| default: | ||
| act.Error = $"Run Action Failed due to unrecognized action type - {act.GetType().Name}"; | ||
| break; | ||
| } | ||
| switch (act) | ||
| { | ||
| case ActBrowserElement actBrowserElement: | ||
| ActBrowserElementHandler actBrowserElementHandler = new(actBrowserElement, _browser, new ActBrowserElementHandler.Context | ||
| { | ||
| BusinessFlow = BusinessFlow, | ||
| Environment = Environment, | ||
| }); | ||
| actBrowserElementHandler.HandleAsync().Wait(); | ||
| break; | ||
| case ActUIElement actUIElement: | ||
| ActUIElementHandler actUIElementHandler = new(actUIElement, _browser, BusinessFlow, Environment); | ||
| actUIElementHandler.HandleAsync().Wait(); | ||
| break; | ||
| case ActScreenShot actScreenShot: | ||
| ActScreenShotHandler actScreenShotHandler = new(actScreenShot, _browser); | ||
| actScreenShotHandler.HandleAsync().Wait(); | ||
| break; | ||
| case ActGotoURL actGotoURL: | ||
| ActGotoURLHandler actGotoURLHandler = new(actGotoURL, _browser); | ||
| actGotoURLHandler.HandleAsync().Wait(); | ||
| break; | ||
| default: | ||
| act.Error = $"Run Action Failed due to unrecognized action type - {act.GetType().Name}"; | ||
| break; | ||
| } | ||
| }).Wait(); |
There was a problem hiding this comment.
Refactor asynchronous handling in RunAction.
Using Task.Run with .Wait() can lead to potential deadlocks and reduces readability. Consider using async/await directly within the method for better performance and readability.
public override async Task RunActionAsync(Act act)
{
ThrowIfClosed();
switch (act)
{
case ActBrowserElement actBrowserElement:
ActBrowserElementHandler actBrowserElementHandler = new(actBrowserElement, _browser, new ActBrowserElementHandler.Context
{
BusinessFlow = BusinessFlow,
Environment = Environment,
});
await actBrowserElementHandler.HandleAsync();
break;
case ActUIElement actUIElement:
ActUIElementHandler actUIElementHandler = new(actUIElement, _browser, BusinessFlow, Environment);
await actUIElementHandler.HandleAsync();
break;
case ActScreenShot actScreenShot:
ActScreenShotHandler actScreenShotHandler = new(actScreenShot, _browser);
await actScreenShotHandler.HandleAsync();
break;
case ActGotoURL actGotoURL:
ActGotoURLHandler actGotoURLHandler = new(actGotoURL, _browser);
await actGotoURLHandler.HandleAsync();
break;
default:
act.Error = $"Run Action Failed due to unrecognized action type - {act.GetType().Name}";
break;
}
}| public List<AppWindow> GetAppWindows() | ||
| { | ||
| //TODO: unhighlight last highlighted element | ||
| IEnumerable<AppWindow> appWindows = Task.Run(() => GetAppWindowsAsync().Result).Result; | ||
| return new List<AppWindow>(appWindows); | ||
| } |
There was a problem hiding this comment.
Refactor asynchronous handling in GetAppWindows.
Using Task.Run with .Result can lead to potential deadlocks and reduces readability. Consider using async/await directly within the method for better performance and readability.
public async Task<List<AppWindow>> GetAppWindowsAsync()
{
//TODO: unhighlight last highlighted element
IEnumerable<AppWindow> appWindows = await GetAppWindowsAsync();
return new List<AppWindow>(appWindows);
}| public void SwitchWindow(string tabTitle) | ||
| { | ||
| TypeConverter typeConverter = TypeDescriptor.GetConverter(typeof(Bitmap)); | ||
| return (Bitmap?)typeConverter.ConvertFrom(bytes); | ||
| Task.Run(async () => | ||
| { | ||
| await SwitchTabAsync(filter: async (_, tab) => | ||
| { | ||
| string title = await tab.TitleAsync(); | ||
| return string.Equals(title, tabTitle); | ||
| }); | ||
| }).Wait(); | ||
| } |
There was a problem hiding this comment.
Refactor asynchronous handling in SwitchWindow.
Using Task.Run with .Wait() can lead to potential deadlocks and reduces readability. Consider using async/await directly within the method for better performance and readability.
public async Task SwitchWindowAsync(string tabTitle)
{
await SwitchTabAsync(filter: async (_, tab) =>
{
string title = await tab.TitleAsync();
return string.Equals(title, tabTitle);
});
}| public void HighLightElement(ElementInfo element, bool locateElementByItLocators = false, IList<ElementInfo> MappedUIElements = null) | ||
| { | ||
| Task.Run(async () => | ||
| { | ||
| if (_lastHighlightedElement != null) | ||
| { | ||
| await UnhighlightElementAsync(_lastHighlightedElement); | ||
| } | ||
| await SwitchToFrameOfElementAsync(element); | ||
| IBrowserElement? browserElement = null; | ||
| if (element.ElementObject is IBrowserElement) | ||
| { | ||
| browserElement = (IBrowserElement)element.ElementObject; | ||
| } | ||
| if (browserElement == null) | ||
| { | ||
| browserElement = await FindBrowserElementAsync(element); | ||
| } | ||
| if (browserElement == null) | ||
| { | ||
| return; | ||
| } | ||
| _lastHighlightedElement = browserElement; | ||
| await HighlightElementAsync(browserElement); | ||
| }).Wait(); | ||
| } |
There was a problem hiding this comment.
Refactor asynchronous handling in HighLightElement.
Using Task.Run with .Wait() can lead to potential deadlocks and reduces readability. Consider using async/await directly within the method for better performance and readability.
public async Task HighLightElementAsync(ElementInfo element, bool locateElementByItLocators = false, IList<ElementInfo> MappedUIElements = null)
{
if (_lastHighlightedElement != null)
{
await UnhighlightElementAsync(_lastHighlightedElement);
}
await SwitchToFrameOfElementAsync(element);
IBrowserElement? browserElement = null;
if (element.ElementObject is IBrowserElement)
{
browserElement = (IBrowserElement)element.ElementObject;
}
if (browserElement == null)
{
browserElement = await FindBrowserElementAsync(element);
}
if (browserElement == null)
{
return;
}
_lastHighlightedElement = browserElement;
await HighlightElementAsync(browserElement);
}| public void UnHighLightElements() | ||
| { | ||
| Task.Run(async () => | ||
| { | ||
| if (_lastHighlightedElement != null) | ||
| { | ||
| await UnhighlightElementAsync(_lastHighlightedElement); | ||
| } | ||
| }).Wait(); | ||
| } |
There was a problem hiding this comment.
Refactor asynchronous handling in UnHighLightElements.
Using Task.Run with .Wait() can lead to potential deadlocks and reduces readability. Consider using async/await directly within the method for better performance and readability.
public async Task UnHighLightElementsAsync()
{
if (_lastHighlightedElement != null)
{
await UnhighlightElementAsync(_lastHighlightedElement);
}
}| public List<ElementInfo> GetElementChildren(ElementInfo elementInfo) | ||
| { | ||
| ThrowIfClosed(); | ||
| if (elementInfo == null || elementInfo is not HTMLElementInfo htmlElementInfo) | ||
| { | ||
| return []; | ||
| } | ||
|
|
||
| return Task.Run(async () => | ||
| { | ||
| await SwitchToFrameOfElementAsync(elementInfo); | ||
| string xpath = GenerateXPathFromHTMLElementInfo(htmlElementInfo); | ||
| IEnumerable<IBrowserElement> browserElements = await _browser.CurrentWindow.CurrentTab.GetElementsAsync(eLocateBy.ByXPath, xpath); | ||
| List<HTMLElementInfo> htmlElements = []; | ||
| foreach (IBrowserElement browserElement in browserElements) | ||
| { | ||
| HTMLElementInfo newHtmlElement = await CreateHtmlElementAsync(browserElement); | ||
|
|
||
| if (string.IsNullOrEmpty(newHtmlElement.ID)) | ||
| { | ||
| newHtmlElement.ID = htmlElementInfo.HTMLElementObject.Id; | ||
| } | ||
|
|
||
| string elementPath = htmlElementInfo.Path; | ||
| if (string.IsNullOrEmpty(htmlElementInfo.XPath)) | ||
| { | ||
| string[] xpathSegments = htmlElementInfo.XPath.Split('/', StringSplitOptions.RemoveEmptyEntries); | ||
| if (xpathSegments.Length > 0) | ||
| { | ||
| string lastXPathSegment = xpathSegments[^1].TrimStart('[').TrimEnd(']'); | ||
| if (string.Equals(lastXPathSegment, "frame", StringComparison.OrdinalIgnoreCase) || | ||
| string.Equals(lastXPathSegment, "iframe", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| elementPath = $"{htmlElementInfo.Path},{htmlElementInfo.XPath}"; | ||
| } | ||
| elementPath = elementPath.TrimStart(','); | ||
| } | ||
| } | ||
| newHtmlElement.Path = elementPath; | ||
| newHtmlElement.WindowExplorer = this; | ||
| newHtmlElement.RelXpath = POMLearner.GenerateRelativeXPathFromHTMLElementInfo(newHtmlElement, xpathImpl: this, pomSetting: null); | ||
|
|
||
| htmlElements.Add(newHtmlElement); | ||
| } | ||
| return htmlElements.Cast<ElementInfo>().ToList(); | ||
| }).Result; | ||
| } |
There was a problem hiding this comment.
Refactor asynchronous handling in GetElementChildren.
Using Task.Run with .Result can lead to potential deadlocks and reduces readability. Consider using async/await directly within the method for better performance and readability.
public async Task<List<ElementInfo>> GetElementChildrenAsync(ElementInfo elementInfo)
{
ThrowIfClosed();
if (elementInfo == null || elementInfo is not HTMLElementInfo htmlElementInfo)
{
return [];
}
await SwitchToFrameOfElementAsync(elementInfo);
string xpath = GenerateXPathFromHTMLElementInfo(htmlElementInfo);
IEnumerable<IBrowserElement> browserElements = await _browser.CurrentWindow.CurrentTab.GetElementsAsync(eLocateBy.ByXPath, xpath);
List<HTMLElementInfo> htmlElements = [];
foreach (IBrowserElement browserElement in browserElements)
{
HTMLElementInfo newHtmlElement = await CreateHtmlElementAsync(browserElement);
if (string.IsNullOrEmpty(newHtmlElement.ID))
{
newHtmlElement.ID = htmlElementInfo.HTMLElementObject.Id;
}
string elementPath = htmlElementInfo.Path;
if (string.IsNullOrEmpty(htmlElementInfo.XPath))
{
string[] xpathSegments = htmlElementInfo.XPath.Split('/', StringSplitOptions.RemoveEmptyEntries);
if (xpathSegments.Length > 0)
{
string lastXPathSegment = xpathSegments[^1].TrimStart('[').TrimEnd(']');
if (string.Equals(lastXPathSegment, "frame", StringComparison.OrdinalIgnoreCase) ||
string.equals(lastXPathSegment, "iframe", StringComparison.OrdinalIgnoreCase))
{
elementPath = $"{htmlElementInfo.Path},{htmlElementInfo.XPath}";
}
elementPath = elementPath.TrimStart(',');
}
}
newHtmlElement.Path = elementPath;
newHtmlElement.WindowExplorer = this;
newHtmlElement.RelXpath = POMLearner.GenerateRelativeXPathFromHTMLElementInfo(newHtmlElement, xpathImpl: this, pomSetting: null);
htmlElements.Add(newHtmlElement);
}
return htmlElements.Cast<ElementInfo>().ToList();
}| public ObservableList<ControlProperty> GetElementProperties(ElementInfo elementInfo) | ||
| { | ||
| if (elementInfo is not HTMLElementInfo htmlElementInfo) | ||
| { | ||
| return []; | ||
| } | ||
|
|
||
| if (htmlElementInfo.ElementObject == null) | ||
| { | ||
| htmlElementInfo.ElementObject = Task.Run(async () => await FindBrowserElementAsync(htmlElementInfo)).Result; | ||
| } | ||
|
|
||
| if (htmlElementInfo.OptionalValuesObjectsList == null) | ||
| { | ||
| htmlElementInfo.OptionalValuesObjectsList = []; | ||
| } | ||
| htmlElementInfo.OptionalValuesObjectsList.AddRange(Task.Run(() => POMLearner.GetOptionalValuesAsync(htmlElementInfo).Result).Result); | ||
|
|
||
| return new(Task.Run(() => POMLearner.GetPropertiesAsync(htmlElementInfo).Result).Result); | ||
| } |
There was a problem hiding this comment.
Refactor asynchronous handling in GetElementProperties.
Using Task.Run with .Result can lead to potential deadlocks and reduces readability. Consider using async/await directly within the method for better performance and readability.
public async Task<ObservableList<ControlProperty>> GetElementPropertiesAsync(ElementInfo elementInfo)
{
if (elementInfo is not HTMLElementInfo htmlElementInfo)
{
return [];
}
if (htmlElementInfo.ElementObject == null)
{
htmlElementInfo.ElementObject = await FindBrowserElementAsync(htmlElementInfo);
}
if (htmlElementInfo.OptionalValuesObjectsList == null)
{
htmlElementInfo.OptionalValuesObjectsList = [];
}
htmlElementInfo.OptionalValuesObjectsList.AddRange(await POMLearner.GetOptionalValuesAsync(htmlElementInfo));
return new(await POMLearner.GetPropertiesAsync(htmlElementInfo));
}| public ObservableList<ElementLocator> GetElementLocators(ElementInfo elementInfo, PomSetting pomSetting = null) | ||
| { | ||
| if (elementInfo is not HTMLElementInfo htmlElementInfo) | ||
| { | ||
| return []; | ||
| } | ||
|
|
||
| IEnumerable<ElementLocator> locators = Task.Run(() => POMLearner.GenerateLocatorsAsync(htmlElementInfo, pomSetting).Result).Result; | ||
| return new(locators); |
There was a problem hiding this comment.
Refactor asynchronous handling in GetElementLocators.
Using Task.Run with .Result can lead to potential deadlocks and reduces readability. Consider using async/await directly within the method for better performance and readability.
public async Task<ObservableList<ElementLocator>> GetElementLocatorsAsync(ElementInfo elementInfo, PomSetting pomSetting = null)
{
if (elementInfo is not HTMLElementInfo htmlElementInfo)
{
return [];
}
IEnumerable<ElementLocator> locators = await POMLearner.GenerateLocatorsAsync(htmlElementInfo, pomSetting);
return new(locators);
}
Thank you for your contribution.
Before submitting this PR, please make sure:
Summary by CodeRabbit
New Features
IBrowserTab.POMLearner.PlaywrightBrowserTab.Improvements
PlaywrightDriver.