BugFix - 40345 - Browser Launched In Private Mode#3753
Conversation
…ad of IBrowser since, when we launch a persistent (non-private) window from playwright, it directly returns a IBrowserContext instead of IBrowser.
WalkthroughIn the recent update 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: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Exceptions/EntityNotFoundException.cs (1 hunks)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/ActionHandlers/ActUIElementHandler.cs (1 hunks)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowser.cs (4 hunks)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs (7 hunks)
Files skipped from review due to trivial changes (1)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Exceptions/EntityNotFoundException.cs
Additional context used
Learnings (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowser.cs (1)
User: 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.
Additional comments not posted (8)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs (3)
174-178: The methodCanStartAnotherInstancehas been moved down in the class definition. This change is purely organizational and does not affect functionality.
35-36: The instantiation of_browsernow uses the new constructor signature. Ensure that all dependencies and usages of_browserthroughout the codebase are updated to align with this change.#!/bin/bash # Description: Verify all usages of `_browser` are updated to the new constructor signature. # Test: Search for `_browser` instantiation. Expect: Only occurrences of the new signature. rg --type python $'_browser = new'
Line range hint
48-65: Refactored methodBuildPlaywrightBrowserOptionsnow correctly initializes browser options including proxy settings. Ensure that the proxy functionality is thoroughly tested, especially since proxy handling can be sensitive to syntax and configuration errors.#!/bin/bash # Description: Verify the proxy settings are correctly applied in the browser. # Test: Search for proxy usage in the browser settings. Expect: Correct application of proxy settings. rg --type python $'options.Proxy'Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowser.cs (4)
17-26: The addition of theOptionsclass encapsulates browser configuration settings, enhancing modularity and maintainability of the code. Ensure that all properties of this class are properly documented to facilitate ease of use and maintenance.
93-118: The methodBuildPlaywrightBrowserContextLaunchOptionsnow correctly constructs launch options based on the browser type. This is a crucial part of ensuring that browser-specific features and quirks are correctly handled.
205-209: TheClosePlaywrightBrowsermethod now disposes of the Playwright instance and handles the browser close event. This is an important update for resource management, ensuring that resources are properly cleaned up when the browser is closed.
42-70: The constructor ofPlaywrightBrowserhas been refactored to accept new parameters and handle the creation of a new window asynchronously. This change aligns with modern asynchronous programming practices but ensure that error handling is robust, especially since browser initialization can be prone to errors.#!/bin/bash # Description: Verify robust error handling in the new window creation process. # Test: Search for error handling patterns in new window creation. Expect: Proper error handling mechanisms. rg --type python $'catch \(Exception ex\)'Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/ActionHandlers/ActUIElementHandler.cs (1)
169-169: Updated the exception thrown inGetFirstMatchingElementAsynctoEntityNotFoundException. This change improves the specificity of the exception, making error handling more precise and informative.
…ithub.com/Ginger-Automation/Ginger into BugFix/40345-BrowserLaunchedInPrivateMode
There was a problem hiding this comment.
Actionable comments posted: 9
Outside diff range and nitpick comments (3)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs (1)
105-105: Consider adding detailed logging for console messages retrieval.Adding detailed logging could help in debugging and maintaining a clear operational trail.
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs (2)
Line range hint
49-66: Refactor proxy configuration to a separate method for clarity.- if (!string.IsNullOrEmpty(Proxy)) + ConfigureProxy(options);And then define
ConfigureProxymethod to handle proxy settings.
175-179: Clarify the purpose ofCanStartAnotherInstancemethod.The method's purpose and usage should be clearly documented to avoid confusion.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs (5 hunks)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs (7 hunks)
Files not summarized due to errors (1)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs: Error: Server error. Please try again later.
Additional comments not posted (2)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs (2)
111-111: Refactor the JavaScript execution to a separate method for better modularity.
[REFACTOR_SUGGESTion]- string script = "var performance = window.performance || window.mozPerformance || window.msPerformance || window.webkitPerformance || {}; var network = performance.getEntries() || {}; network;"; + string script = GeneratePerformanceScript();And then define
GeneratePerformanceScriptmethod to encapsulate the JavaScript code.
190-190: Ensure that switching to the main frame resets the state appropriately.Verification successful
The code snippets around
_playwrightPage.MainFrameindicate that the state is being reset by assigning_currentFrameto_playwrightPage.MainFramein multiple methods, includingSwitchToMainFrameAsync. This suggests that the state is indeed being reset appropriately when switching to the main frame.
PlaywrightBrowserTabconstructor:_currentFrameis initialized to_playwrightPage.MainFrame.GoToURLAsyncmethod:_currentFrameis reset to_playwrightPage.MainFrameafter navigating to a URL.SwitchToMainFrameAsyncmethod:_currentFrameis reset to_playwrightPage.MainFrame.These instances confirm that the state reset logic is present and functioning as expected.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that switching to the main frame resets the state as expected. # Test: Search for state reset logic after switching frames. rg 'MainFrame' 'PlaywrightBrowserTab.cs'Length of output: 102
Script:
#!/bin/bash # Locate the PlaywrightBrowserTab.cs file fd PlaywrightBrowserTab.cs # If the file is found, search for state reset logic after switching frames if [ -f "$(fd PlaywrightBrowserTab.cs)" ]; then rg 'MainFrame' "$(fd PlaywrightBrowserTab.cs)" fiLength of output: 566
Script:
#!/bin/bash # Extract and display lines around occurrences of '_playwrightPage.MainFrame' in PlaywrightBrowserTab.cs rg -C 5 '_playwrightPage.MainFrame' Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.csLength of output: 1070
|
|
||
| public async Task<IEnumerable<IBrowserElement>> GetElementsAsync(eLocateBy locateBy, string value) | ||
| { | ||
| ThrowIfClosed(); |
There was a problem hiding this comment.
Optimize element retrieval by caching frequently accessed elements.
Consider implementing a caching mechanism for elements that are frequently accessed to improve performance.
|
|
||
| public Task SwitchToParentFrameAsync() | ||
| { | ||
| ThrowIfClosed(); |
There was a problem hiding this comment.
Add error handling for null parent frames when switching to parent frame.
- if (parentFrame != null)
+ if (parentFrame == null)
+ {
+ throw new InvalidOperationException("No parent frame available.");
+ }
+ elseThis ensures robustness by handling cases where no parent frame is available.
Committable suggestion was skipped due to low confidence.
| public async Task<IBrowserElement?> GetFocusedElement() | ||
| { | ||
| ThrowIfClosed(); | ||
| IPlaywrightLocator locator = _currentFrame.Locator("css=*:focus"); | ||
| int matchedElementCount = await locator.CountAsync(); | ||
| if (matchedElementCount <= 0) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| return new PlaywrightBrowserElement(locator); | ||
| } |
There was a problem hiding this comment.
Ensure null checks and error handling for focused elements retrieval.
- if (matchedElementCount <= 0)
+ if (matchedElementCount <= 0 || locator == null)
{
return null;
}This change adds an additional safety check for null locators.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public async Task<IBrowserElement?> GetFocusedElement() | |
| { | |
| ThrowIfClosed(); | |
| IPlaywrightLocator locator = _currentFrame.Locator("css=*:focus"); | |
| int matchedElementCount = await locator.CountAsync(); | |
| if (matchedElementCount <= 0) | |
| { | |
| return null; | |
| } | |
| return new PlaywrightBrowserElement(locator); | |
| } | |
| public async Task<IBrowserElement?> GetFocusedElement() | |
| { | |
| ThrowIfClosed(); | |
| IPlaywrightLocator locator = _currentFrame.Locator("css=*:focus"); | |
| int matchedElementCount = await locator.CountAsync(); | |
| if (matchedElementCount <= 0 || locator == null) | |
| { | |
| return null; | |
| } | |
| return new PlaywrightBrowserElement(locator); | |
| } |
| public override Act GetCurrentElement() | ||
| { | ||
| ThrowIfClosed(); | ||
|
|
||
| async Task<Act?> getCurrentElementAsync() | ||
| { | ||
| IBrowserElement? element = await ((PlaywrightBrowserTab)_browser!.CurrentWindow.CurrentTab).GetFocusedElement(); | ||
| if (element == null) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| string tagName = await element.TagNameAsync(); | ||
| Act? act = null; | ||
| switch (tagName) | ||
| { | ||
| case "text": | ||
| act = new ActTextBox() | ||
| { | ||
| TextBoxAction = ActTextBox.eTextBoxAction.SetValue, | ||
| }; | ||
| await SetElementLocatorToActionAsync(element, act); | ||
| act.AddOrUpdateInputParamValue("Value", await element.AttributeValueAsync(name: "value")); | ||
| act.AddOrUpdateReturnParamActual("Actual", $"Tag Name = {tagName}"); | ||
| break; | ||
| case "button": | ||
| string idAttrValue = await element.AttributeValueAsync(name: "id"); | ||
|
|
||
| act = new ActButton() | ||
| { | ||
| LocateBy = eLocateBy.ByID, | ||
| LocateValue = idAttrValue | ||
| }; | ||
| break; | ||
| case "submit": | ||
| idAttrValue = await element.AttributeValueAsync(name: "id"); | ||
|
|
||
| act = new ActButton() | ||
| { | ||
| LocateBy = eLocateBy.ByID, | ||
| LocateValue = idAttrValue | ||
| }; | ||
| break; | ||
| case "reset": | ||
| //TODO: add missing Act get() method | ||
| break; | ||
| case "file": | ||
| //TODO: add missing Act get() method | ||
| break; | ||
| case "hidden": // does type this apply? | ||
| //TODO: add missing Act get() method | ||
| break; | ||
| case "password": | ||
| act = new ActPassword() | ||
| { | ||
| PasswordAction = ActPassword.ePasswordAction.SetValue, | ||
| }; | ||
| await SetElementLocatorToActionAsync(element, act); | ||
| act.AddOrUpdateInputParamValue("Value", await element.AttributeValueAsync(name: "value")); | ||
| act.AddOrUpdateReturnParamActual("Actual", $"Tag Name = {tagName}"); | ||
| break; | ||
| case "checkbox": | ||
| idAttrValue = await element.AttributeValueAsync(name: "id"); | ||
| act = new ActCheckbox() | ||
| { | ||
| LocateBy = eLocateBy.ByID, | ||
| LocateValue = idAttrValue, | ||
| }; | ||
| break; | ||
| case "radio": | ||
| idAttrValue = await element.AttributeValueAsync(name: "id"); | ||
| act = new ActRadioButton() | ||
| { | ||
| LocateBy = eLocateBy.ByID, | ||
| LocateValue = idAttrValue | ||
| }; | ||
| break; | ||
|
|
||
| } | ||
| return act; | ||
| } | ||
|
|
||
| return Task.Run(getCurrentElementAsync).Result!; | ||
|
|
||
| } |
There was a problem hiding this comment.
Refactor GetCurrentElement to improve readability and maintainability.
Consider breaking down this large method into smaller, more manageable methods, each handling a specific part of the element retrieval process.
| PlaywrightBrowser.Options browserOptions = BuildPlaywrightBrowserOptions(); | ||
| _browser = new(playwright, BrowserType, browserOptions, OnBrowserClose); |
There was a problem hiding this comment.
Ensure proper asynchronous handling in StartDriver.
Consider using await instead of .Result to avoid potential deadlocks and improve performance.
| ViewportSize = ViewportSize.NoViewport, | ||
| }; | ||
|
|
||
| if (_browserType == WebBrowserType.Chrome) |
There was a problem hiding this comment.
what about firefox browsertype, is it missed or default is set to firefox?
| public async Task<string> GetBrowserLogsAsync() | ||
| { | ||
| ThrowIfClosed(); | ||
| string script = "var performance = window.performance || window.mozPerformance || window.msPerformance || window.webkitPerformance || {}; var network = performance.getEntries() || {}; network;"; |
There was a problem hiding this comment.
define const string for it and keep it in base driver class which then can be used for selenium and playwright
There was a problem hiding this comment.
the script is slightly different compared to Selenium.
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/ActionHandlers/ActUIElementHandler.cs (1 hunks)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowser.cs (4 hunks)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs (5 hunks)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs (9 hunks)
Files skipped from review as they are similar to previous changes (1)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/ActionHandlers/ActUIElementHandler.cs
Additional context used
Learnings (2)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowser.cs (1)
User: 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.Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs (2)
User: IamRanjeetSingh PR: Ginger-Automation/Ginger#3753 File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs:99-99 Timestamp: 2024-06-12T12:54:44.221Z Learning: User IamRanjeetSingh prefers exceptions to propagate rather than being caught and handled locally within methods in `PlaywrightBrowserTab.cs`.User: IamRanjeetSingh PR: Ginger-Automation/Ginger#3738 File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs:72-77 Timestamp: 2024-06-07T20:51:31.445Z Learning: Error handling for the `GoToURLAsync` method in the `PlaywrightBrowserTab` class is managed in the parent method or surrounding context.
Additional comments not posted (10)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowser.cs (6)
8-8: ReplacedIPlaywrightBrowserwithIPlaywrightBrowserTypeto align with updated Playwright interfaces.
19-28: IntroducedOptionsclass for better encapsulation of browser configuration settings.
93-113: Enhanced error handling inLaunchBrowserContextWithInstallationAsyncto manage browser installation if executable is not found.
124-145:ExecutePlaywrightInstallationCommandmethod added to handle browser installation based on the browser type.
147-173:BuildBrowserContextLaunchOptionsmethod updated to include additional options such as proxy settings and viewport size.
259-263: Enhanced theClosePlaywrightBrowsermethod to handle disposal of Playwright objects and invoke browser close callbacks.Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs (1)
115-115: AddedThrowIfClosed()checks in several methods to ensure operations are not performed on a closed tab, enhancing error robustness.Also applies to: 121-121, 127-127, 159-159, 211-211, 218-218, 230-230, 279-290
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs (3)
36-37: RefactoredStartDriverandBuildPlaywrightBrowserOptionsto handle browser options more effectively, including proxy settings.Also applies to: 49-66
165-165: Improved action support checking inIsActionSupportedto handle frame locators and operation types more robustly.
197-281:GetCurrentElementmethod refactored to asynchronously fetch the current element and its attributes, enhancing performance and maintainability.
| internal PlaywrightBrowser(IPlaywright playwright, WebBrowserType browserType, Options? options = null, IBrowser.OnBrowserClose? onBrowserClose = null) | ||
| { | ||
| _playwrightBrowser = playwrightBrowser; | ||
| _playwright = playwright; | ||
| _browserType = browserType; | ||
| _options = options; | ||
| _onBrowserClose = onBrowserClose; | ||
|
|
||
| List<IPlaywrightBrowserContext> contexts = new(_playwrightBrowser.Contexts); | ||
| foreach (IPlaywrightBrowserContext context in contexts) | ||
| { | ||
| PlaywrightBrowserWindow window = new(context, OnWindowClose); | ||
| _windows.AddLast(window); | ||
| } | ||
| _windows = []; | ||
|
|
||
| if (_windows.Count > 0) | ||
| { | ||
| _currentWindow = _windows.Last!.Value; | ||
| } | ||
| else | ||
| //Remove synchronous object creation if it is not possible. instead have a async Creator method | ||
| IBrowserWindow? newWindow = Task.Run(() => | ||
| { | ||
| IBrowserWindow? newWindow = Task.Run(() => | ||
| try | ||
| { | ||
| try | ||
| { | ||
| //this code needs to be executed in a separate Task otherwise, it will cause a deadlock and freeze the calling thread | ||
| //check this for an example https://stackoverflow.com/a/43912280/12190808 | ||
| return NewWindowAsync().Result; | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Reporter.ToLog(eLogLevel.ERROR, $"Error occurred while creating {nameof(IBrowserWindow)}", ex); | ||
| return null!; | ||
| } | ||
| }).Result; | ||
|
|
||
| if (newWindow == null) | ||
| //this code needs to be executed in a separate Task otherwise, it will cause a deadlock and freeze the calling thread | ||
| //check this for an example https://stackoverflow.com/a/43912280/12190808 | ||
| return NewWindowAsync(); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| throw new Exception($"Error occurred while creating {nameof(IBrowserWindow)}"); | ||
| Reporter.ToLog(eLogLevel.ERROR, $"Error occurred while creating {nameof(IBrowserWindow)}", ex); | ||
| return null!; | ||
| } | ||
| _currentWindow = newWindow; | ||
| }).Result; | ||
|
|
||
| if (newWindow == null) | ||
| { | ||
| throw new Exception($"Error occurred while creating {nameof(IBrowserWindow)}"); | ||
| } | ||
| _currentWindow = newWindow; |
There was a problem hiding this comment.
Refactored constructor to handle asynchronous window creation and error handling more robustly.
Consider creating an asynchronous factory method to handle window creation instead of using .Result which can lead to potential deadlocks.
- IBrowserWindow? newWindow = Task.Run(() => { /* existing code */ }).Result;
+ public static async Task<PlaywrightBrowser> CreateAsync(IPlaywright playwright, WebBrowserType browserType, Options? options = null, IBrowser.OnBrowserClose? onBrowserClose = null) {
+ PlaywrightBrowser browser = new PlaywrightBrowser(playwright, browserType, options, onBrowserClose);
+ browser._currentWindow = await browser.NewWindowAsync();
+ return browser;
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| internal PlaywrightBrowser(IPlaywright playwright, WebBrowserType browserType, Options? options = null, IBrowser.OnBrowserClose? onBrowserClose = null) | |
| { | |
| _playwrightBrowser = playwrightBrowser; | |
| _playwright = playwright; | |
| _browserType = browserType; | |
| _options = options; | |
| _onBrowserClose = onBrowserClose; | |
| List<IPlaywrightBrowserContext> contexts = new(_playwrightBrowser.Contexts); | |
| foreach (IPlaywrightBrowserContext context in contexts) | |
| { | |
| PlaywrightBrowserWindow window = new(context, OnWindowClose); | |
| _windows.AddLast(window); | |
| } | |
| _windows = []; | |
| if (_windows.Count > 0) | |
| { | |
| _currentWindow = _windows.Last!.Value; | |
| } | |
| else | |
| //Remove synchronous object creation if it is not possible. instead have a async Creator method | |
| IBrowserWindow? newWindow = Task.Run(() => | |
| { | |
| IBrowserWindow? newWindow = Task.Run(() => | |
| try | |
| { | |
| try | |
| { | |
| //this code needs to be executed in a separate Task otherwise, it will cause a deadlock and freeze the calling thread | |
| //check this for an example https://stackoverflow.com/a/43912280/12190808 | |
| return NewWindowAsync().Result; | |
| } | |
| catch (Exception ex) | |
| { | |
| Reporter.ToLog(eLogLevel.ERROR, $"Error occurred while creating {nameof(IBrowserWindow)}", ex); | |
| return null!; | |
| } | |
| }).Result; | |
| if (newWindow == null) | |
| //this code needs to be executed in a separate Task otherwise, it will cause a deadlock and freeze the calling thread | |
| //check this for an example https://stackoverflow.com/a/43912280/12190808 | |
| return NewWindowAsync(); | |
| } | |
| catch (Exception ex) | |
| { | |
| throw new Exception($"Error occurred while creating {nameof(IBrowserWindow)}"); | |
| Reporter.ToLog(eLogLevel.ERROR, $"Error occurred while creating {nameof(IBrowserWindow)}", ex); | |
| return null!; | |
| } | |
| _currentWindow = newWindow; | |
| }).Result; | |
| if (newWindow == null) | |
| { | |
| throw new Exception($"Error occurred while creating {nameof(IBrowserWindow)}"); | |
| } | |
| _currentWindow = newWindow; | |
| internal PlaywrightBrowser(IPlaywright playwright, WebBrowserType browserType, Options? options = null, IBrowser.OnBrowserClose? onBrowserClose = null) | |
| { | |
| _playwright = playwright; | |
| _browserType = browserType; | |
| _options = options; | |
| _onBrowserClose = onBrowserClose; | |
| _windows = []; | |
| } | |
| public static async Task<PlaywrightBrowser> CreateAsync(IPlaywright playwright, WebBrowserType browserType, Options? options = null, IBrowser.OnBrowserClose? onBrowserClose = null) { | |
| PlaywrightBrowser browser = new PlaywrightBrowser(playwright, browserType, options, onBrowserClose); | |
| browser._currentWindow = await browser.NewWindowAsync(); | |
| return browser; | |
| } |
|
|
||
| public async Task<bool> SwitchFrameAsync(eLocateBy locateBy, string value) | ||
| { | ||
| ThrowIfClosed(); |
There was a problem hiding this comment.
Updated SwitchFrameAsync to include better error handling and support checks for frame locators. Consider using a dictionary or a strategy pattern for cleaner implementation of locator strategies.
- switch (locateBy) {
- case eLocateBy.ByID:
- frameLocator = _currentFrame.FrameLocator($"css=#{value}");
- break;
- // other cases
- }
+ private static readonly Dictionary<eLocateBy, string> frameLocatorStrategies = new Dictionary<eLocateBy, string> {
+ { eLocateBy.ByID, "css=#{value}" },
+ // other mappings
+ };
+ frameLocator = _currentFrame.FrameLocator(frameLocatorStrategies[locateBy].Replace("{value}", value));Also applies to: 164-164
| private async Task SetElementLocatorToActionAsync(IBrowserElement element, Act act) | ||
| { | ||
| //order by priority | ||
|
|
||
| // By ID | ||
| string locatorValue = await element.AttributeValueAsync(name: "id"); | ||
| if (locatorValue != "") | ||
| { | ||
| act.LocateBy = eLocateBy.ByID; | ||
| act.LocateValue = locatorValue; | ||
| return; | ||
| } | ||
|
|
||
| // By name | ||
| locatorValue = await element.AttributeValueAsync(name: "name"); | ||
| if (locatorValue != "") | ||
| { | ||
| act.LocateBy = eLocateBy.ByName; | ||
| act.LocateValue = locatorValue; | ||
| return; | ||
| } | ||
|
|
||
| //TODO: CSS.... | ||
|
|
||
| //By href | ||
| locatorValue = await element.AttributeValueAsync(name: "href"); | ||
| if (locatorValue != "") | ||
| { | ||
| act.LocateBy = eLocateBy.ByHref; | ||
| act.LocateValue = locatorValue; | ||
| return; | ||
| } | ||
|
|
||
| //By Value | ||
| locatorValue = await element.AttributeValueAsync(name: "value"); | ||
| if (locatorValue != "") | ||
| { | ||
| act.LocateBy = eLocateBy.ByValue; | ||
| act.LocateValue = locatorValue; | ||
| return; | ||
| } | ||
|
|
||
| // by text | ||
| locatorValue = await element.TextContentAsync(); | ||
| if (locatorValue != "") | ||
| { | ||
| act.LocateBy = eLocateBy.ByLinkText; | ||
| act.LocateValue = locatorValue; | ||
| return; | ||
| } | ||
| //TODO: add XPath | ||
| } |
There was a problem hiding this comment.
Refactored SetElementLocatorToActionAsync to prioritize element locators. Consider adding missing CSS and XPath support as indicated by the TODO comments.
- //TODO: CSS....
- //TODO: add XPath
+ locatorValue = await element.AttributeValueAsync(name: "class");
+ if (locatorValue != "") {
+ act.LocateBy = eLocateBy.ByCSS;
+ act.LocateValue = locatorValue;
+ return;
+ }
+ locatorValue = await element.XPath;
+ if (locatorValue != "") {
+ act.LocateBy = eLocateBy.ByXPath;
+ act.LocateValue = locatorValue;
+ return;
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private async Task SetElementLocatorToActionAsync(IBrowserElement element, Act act) | |
| { | |
| //order by priority | |
| // By ID | |
| string locatorValue = await element.AttributeValueAsync(name: "id"); | |
| if (locatorValue != "") | |
| { | |
| act.LocateBy = eLocateBy.ByID; | |
| act.LocateValue = locatorValue; | |
| return; | |
| } | |
| // By name | |
| locatorValue = await element.AttributeValueAsync(name: "name"); | |
| if (locatorValue != "") | |
| { | |
| act.LocateBy = eLocateBy.ByName; | |
| act.LocateValue = locatorValue; | |
| return; | |
| } | |
| //TODO: CSS.... | |
| //By href | |
| locatorValue = await element.AttributeValueAsync(name: "href"); | |
| if (locatorValue != "") | |
| { | |
| act.LocateBy = eLocateBy.ByHref; | |
| act.LocateValue = locatorValue; | |
| return; | |
| } | |
| //By Value | |
| locatorValue = await element.AttributeValueAsync(name: "value"); | |
| if (locatorValue != "") | |
| { | |
| act.LocateBy = eLocateBy.ByValue; | |
| act.LocateValue = locatorValue; | |
| return; | |
| } | |
| // by text | |
| locatorValue = await element.TextContentAsync(); | |
| if (locatorValue != "") | |
| { | |
| act.LocateBy = eLocateBy.ByLinkText; | |
| act.LocateValue = locatorValue; | |
| return; | |
| } | |
| //TODO: add XPath | |
| } | |
| private async Task SetElementLocatorToActionAsync(IBrowserElement element, Act act) | |
| { | |
| //order by priority | |
| // By ID | |
| string locatorValue = await element.AttributeValueAsync(name: "id"); | |
| if (locatorValue != "") | |
| { | |
| act.LocateBy = eLocateBy.ByID; | |
| act.LocateValue = locatorValue; | |
| return; | |
| } | |
| // By name | |
| locatorValue = await element.AttributeValueAsync(name: "name"); | |
| if (locatorValue != "") | |
| { | |
| act.LocateBy = eLocateBy.ByName; | |
| act.LocateValue = locatorValue; | |
| return; | |
| } | |
| // By CSS | |
| locatorValue = await element.AttributeValueAsync(name: "class"); | |
| if (locatorValue != "") | |
| { | |
| act.LocateBy = eLocateBy.ByCSS; | |
| act.LocateValue = locatorValue; | |
| return; | |
| } | |
| //By href | |
| locatorValue = await element.AttributeValueAsync(name: "href"); | |
| if (locatorValue != "") | |
| { | |
| act.LocateBy = eLocateBy.ByHref; | |
| act.LocateValue = locatorValue; | |
| return; | |
| } | |
| //By Value | |
| locatorValue = await element.AttributeValueAsync(name: "value"); | |
| if (locatorValue != "") | |
| { | |
| act.LocateBy = eLocateBy.ByValue; | |
| act.LocateValue = locatorValue; | |
| return; | |
| } | |
| // by text | |
| locatorValue = await element.TextContentAsync(); | |
| if (locatorValue != "") | |
| { | |
| act.LocateBy = eLocateBy.ByLinkText; | |
| act.LocateValue = locatorValue; | |
| return; | |
| } | |
| // By XPath | |
| locatorValue = await element.XPath; | |
| if (locatorValue != "") | |
| { | |
| act.LocateBy = eLocateBy.ByXPath; | |
| act.LocateValue = locatorValue; | |
| return; | |
| } | |
| } |
2 participants
Thank you for your contribution.
Before submitting this PR, please make sure:
Summary by CodeRabbit
Refactor
Optionsclass for browser configuration.Bug Fixes
Chores
NotFoundExceptiontoEntityNotFoundExceptionfor better semantic clarity.