Web smart sync action Playwright#4072
Conversation
WalkthroughThis pull request introduces enhancements to web synchronization and element interaction capabilities within the Ginger automation framework. Key changes include the addition of 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
Ginger/GingerCoreCommon/UIElement/ElementInfo.cs(1 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActWebSmartSyncHandler.cs(1 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/IBrowserElement.cs(1 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/IBrowserTab.cs(2 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserElement.cs(2 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs(1 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs(2 hunks)Ginger/GingerCoreNET/GingerCoreNET.csproj(1 hunks)
🧰 Additional context used
📓 Learnings (1)
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-11-12T02:45:14.501Z
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 (15)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActWebSmartSyncHandler.cs (4)
121-132: Ensure boundary handling for visibility checks.
HandleElementIsVisibleAsyncusesToBeVisibleAsync(waitUntilTimeout). IfwaitUntilTimeoutis excessively large or very small, verify that the method handles edge cases gracefully (e.g., zero seconds or large timeouts).
164-175: Verify “selected” vs. “checked”.
HandleElementIsSelectedAsyncaligns with some HTML elements (e.g., radio, checkbox). Ensure the right logic is called to confirm selection or that this method covers all relevant element types (select boxes, toggles, etc.).
195-206: The clickability check is well-structured.
The implementation forHandleElementToBeClickableAsyncis clear and follows the existing pattern. Good job!
280-285: Invisibility polling approach is consistent.
HandleInvisibilityOfAllElementsLocatedByAsyncuses a straightforwardWaitForElementsInvisibleAsyncmethod, consistent with the rest of the file. No issues found here.Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/IBrowserTab.cs (1)
39-39: No functional changes apparent.
Line 39 seems to only adjust indentation forTitleAsync(). Nothing to address here.Ginger/GingerCoreCommon/UIElement/ElementInfo.cs (1)
552-553: Add a quick usage referenceThe new
ByAttributeenum value is consistent with existing patterns. Ensure that test coverage or references toByAttributeusage are in place—line 1176 ofPlaywrightBrowserTab.csindicates usage for this locate type.Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserElement.cs (2)
1082-1102: Consistent fallback handlingReturning false on
TimeoutExceptionorAssertionExceptionaligns with other “WaitFor” patterns. Consider catching only these specific exceptions if that’s the intended logic.
1111-1131: Check overlapping elementsEven if an element is visible and enabled, overlapping elements might prevent real user clicks. Consider verifying pointer events or bounding rectangles to ensure true “clickability.”
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs (3)
1051-1067: Confirm pattern usage
WaitForURLAsync(urlPattern)can be literal or regex. Update the doc string to clarify howurlPatternis interpreted, ensuring consistent usage.
1092-1099: Good approachRelying on
ElementState.Stableis a robust way to ensure presence. No immediate adjustments suggested.
1141-1164: Handle zero matchesWhen
elements.Countis zero, callingWaitForElementStateAsync(...)can’t proceed. Use an early return or an alternative path for the no-element case.Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs (2)
285-326: Augment test coverageIncluding
ActWebSmartSyncis beneficial. Verify how it interacts or overlaps withActSmartSync. Comprehensive tests help confirm each action’s intended usage.
338-338: Adding to supported actionsListing
ActWebSmartSyncamong supported actions is correct. No issues with this addition.Ginger/GingerCoreNET/GingerCoreNET.csproj (2)
315-315: LGTM! Good addition of Playwright NUnit testing support.Adding Microsoft.Playwright.NUnit package will enable better integration with NUnit testing framework. The version matches the core Playwright package which is the recommended approach.
314-314: Verify compatibility with the upgraded Playwright version.The Playwright version has been updated from 1.45.0 to 1.49.0. This is a significant version jump that may include breaking changes.
Run the following script to check the changelog for breaking changes:
Maheshkale447
left a comment
Maheshkale447
left a comment
There was a problem hiding this comment.
Please test the changes with iframe and shadow root elements. also need to check if its supporting POM elements
…/github.com/Ginger-Automation/Ginger into Feature/WebSmartSyncActionPlaywrightSupport
There was a problem hiding this comment.
Actionable comments posted: 10
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActWebSmartSyncHandler.cs(1 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserElement.cs(2 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs(1 hunks)
🧰 Additional context used
📓 Learnings (3)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs (2)
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4072
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs:1084-1091
Timestamp: 2025-01-30T09:08:06.584Z
Learning: In Playwright-based element wait operations (like WaitForElementsInvisibleAsync), use ElementState.Hidden to check for elements that are present in the DOM but not visible, rather than checking if elements are detached/removed from the DOM.
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4072
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs:1075-1083
Timestamp: 2025-01-30T09:07:12.356Z
Learning: In Playwright-based element wait operations (like WaitForElementsVisibleAsync), do not short-circuit when elements are initially absent, as the method is designed to keep polling until timeout to handle dynamic element appearance.
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserElement.cs (2)
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4072
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs:1084-1091
Timestamp: 2025-01-30T09:08:06.584Z
Learning: In Playwright-based element wait operations (like WaitForElementsInvisibleAsync), use ElementState.Hidden to check for elements that are present in the DOM but not visible, rather than checking if elements are detached/removed from the DOM.
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4072
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs:1075-1083
Timestamp: 2025-01-30T09:07:12.356Z
Learning: In Playwright-based element wait operations (like WaitForElementsVisibleAsync), do not short-circuit when elements are initially absent, as the method is designed to keep polling until timeout to handle dynamic element appearance.
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActWebSmartSyncHandler.cs (3)
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4072
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActWebSmartSyncHandler.cs:358-365
Timestamp: 2025-01-30T07:40:08.756Z
Learning: In the Ginger automation framework, when an element is expected to exist but is not found, the code should throw an exception rather than return null, as the element's presence is a precondition for the operation.
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4072
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActWebSmartSyncHandler.cs:137-150
Timestamp: 2025-01-30T07:38:05.310Z
Learning: In the Ginger automation framework, element existence checks using polling with 100ms delay is an acceptable approach, as confirmed by the maintainers.
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4072
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActWebSmartSyncHandler.cs:36-114
Timestamp: 2025-01-30T07:36:32.448Z
Learning: In the Ginger project's web synchronization code, keeping separate implementations for different synchronization operations is preferred over extracting common polling logic, as it improves readability and makes individual cases easier to understand and modify.
🔇 Additional comments (7)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActWebSmartSyncHandler.cs (3)
98-99: Consider verifying element presence before waiting for invisibility
ForHandleInvisibilityOfElementLocatedAsync, you immediately callGetFirstMatchingElementAsync()then check if it “becomes not visible.” If the element never existed in the first place, the logic might pass trivially after finding that single element is missing. You might want to handle the scenario where the element is supposed to exist first or handle the possibility that a brand-new element appears during the wait and becomes invisible.
147-158: Possibility of short-circuiting when elements are clearly missing
Here you poll for existence with aStopwatchand a 100ms delay. Since you’re retrieving the element collection every loop, you allow them to appear dynamically. This is consistent with the existing logic and user preference, so just ensure you’re comfortable with repeatedly enumerating all elements on each pass, especially for very large documents.
190-201: Consider alternatives for page load completeness
InHandlePageHasBeenLoadedAsync, you checkdocument.readyStaterepeatedly. This is a valid pattern, though you might also considerWaitForLoadStateAsyncif you are confident in the correctness of the underlying driver’s load state detection.Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs (1)
1192-1205: Good usage of event unsubscription
WaitForAlertAsyncunsubscribes from_playwrightPage.Dialogafter the wait is done. This approach helps avoid memory leaks. Nicely done.Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserElement.cs (3)
23-23: Confirm necessity of NUnit dependency
You haveusing NUnit.Framework;but are relying on Playwright’sAssertions. Double-check if you still need references toNUnithere. Removing unused references can prevent confusion.
1079-1108: Check for existence prior to checked state
InElementIsSelectedAsync, you callWaitForAsyncfollowed byToBeCheckedAsync. If the element is never found,WaitForAsyncmay or may not throw. Confirm that your logic handles a scenario where no matching element is located.
1209-1239: Consider clarifying “not visible” vs. “hidden in DOM”
InToBeNotVisibleAsync, you rely onWaitForOptions { State = WaitForSelectorState.Hidden }plusNot.ToBeVisibleAsync(...). Hidden elements can remain in the DOM but not displayed. If you also want to handle detached elements, mention this in the docs.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
Ginger/Ginger/Actions/ActionEditPage.xaml.cs (1)
Line range hint
1056-1187: Improve method organization and reduce complexity.The
ColumnMultiSelectComboBox_ItemCheckBoxClickmethod is handling multiple responsibilities and directly modifying global state. Consider the following improvements:
- Extract the "All" checkbox logic into a separate method.
- Create a dedicated method for updating column preferences.
- Use constants for magic strings like "Description", "Path", etc.
- Consider using a dictionary to map column names to their configurations.
Here's a suggested refactoring:
+private const string ALL_COLUMN = "All"; +private const string DESCRIPTION_COLUMN = "Description"; +// ... other column constants +private void HandleAllCheckboxClick(CheckBox checkBox) +{ + bool isChecked = checkBox.IsChecked == true; + for (int i = 1; i < 6; i++) + { + columnMultiSelectComboBox._nodeList[i].IsSelected = isChecked; + } +} +private void UpdateColumnPreferences(string columnName, bool isSelected) +{ + switch (columnName) + { + case DESCRIPTION_COLUMN: + columnPreferences = isSelected ? "Description," : ""; + break; + // ... other cases + } +} private void ColumnMultiSelectComboBox_ItemCheckBoxClick(object sender, EventArgs e) { customDynamicView.GridColsView = []; if (sender is System.Windows.Controls.CheckBox checkBox) { - if (checkBox.Content.ToString() == "All") + if (checkBox.Content.ToString() == ALL_COLUMN) { - if (checkBox.IsChecked == true) - { - for (int i = 1; i < 6; i++) - { - columnMultiSelectComboBox._nodeList[i].IsSelected = true; - } - } - else - { - for (int i = 1; i < 6; i++) - { - columnMultiSelectComboBox._nodeList[i].IsSelected = false; - } - } + HandleAllCheckboxClick(checkBox); } } columnPreferences = ""; columnCount = 0; foreach (Node node in columnMultiSelectComboBox._nodeList) { - switch (node.Title) - { - case "Description": - // ... existing code - case "Path": - // ... existing code - } + UpdateColumnPreferences(node.Title, node.IsSelected); } // ... rest of the method }
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
Ginger/Ginger/Reports/HTMLReportTemplatePage.xaml.cs(1 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs(1 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs(2 hunks)
🧰 Additional context used
📓 Learnings (2)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs (2)
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4072
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs:1084-1091
Timestamp: 2025-01-30T09:08:06.584Z
Learning: In Playwright-based element wait operations (like WaitForElementsInvisibleAsync), use ElementState.Hidden to check for elements that are present in the DOM but not visible, rather than checking if elements are detached/removed from the DOM.
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4072
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs:1075-1083
Timestamp: 2025-01-30T09:07:12.356Z
Learning: In Playwright-based element wait operations (like WaitForElementsVisibleAsync), do not short-circuit when elements are initially absent, as the method is designed to keep polling until timeout to handle dynamic element appearance.
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-11-12T02:45:14.501Z
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 (11)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs (8)
1051-1070: Potential timeout unit mismatch in WaitForUrlMatchAsync
Currently, the method directly passes the floattimeouttoWaitForURLAsync. Playwright’s API expects the timeout in milliseconds, but it’s unclear whether the user is passing seconds or milliseconds.
1080-1086: Straightforward wrapper for visibility
The method simply callsWaitForElementsStateAsyncwithElementState.Visible. Implementation appears correct.
1088-1094: Consistent usage of ElementState.Hidden
UsingHiddenis appropriate for elements present in the DOM but not visible. This aligns well with Playwright-based element wait patterns.
1096-1102: Validate “Stable” state usage for presence
"Stable" ensures the element is not moving. If the intention is to confirm mere presence in DOM, consider usingAttachedinstead to avoid confusion.
1145-1168: Return value for no elements in WaitForElementsStateAsync
When no elements match the selector, the method returnstruewithout waiting. If “presence” is required, consider raising an exception when zero elements are found.
1188-1188: Field for tracking alert detection
TheTaskCompletionSource<bool>usage looks fine and straightforward.
1190-1205: Clarify timeout units in WaitForAlertAsync
Task.Delay((int)timeout)might truncate decimals and interprettimeoutas milliseconds or seconds differently. Ensure consistency with user expectations.
1207-1216: Dialog event handling
Setting the result whenDialogType.Alertis received is correct and succinct.Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs (2)
285-325: Inconsistent timeout usage for ActWebSmartSync
browserOptions.Timeoutis set towaitUntilTime(likely seconds), butactWebSmartSyncHandler.HandleAsyncuseswaitUntilTime * 1000. Confirm that both are intended to be milliseconds or seconds to avoid confusion.
337-337: Newly supported action type
RecognizingActWebSmartSyncinIsActionSupportedbroadens driver coverage.Ginger/Ginger/Reports/HTMLReportTemplatePage.xaml.cs (1)
1034-1034: No functional change
Only a blank line was added. No concerns.
|
in SwitchFrameAsync method its wait until frame gets avilable its not using the time out so need use the stopwatch for specific amt of time |
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActWebSmartSyncHandler.cs(1 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/IBrowserTab.cs(3 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs(4 hunks)
🧰 Additional context used
📓 Learnings (2)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs (3)
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4072
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs:1104-1143
Timestamp: 2025-01-31T13:25:31.910Z
Learning: In Playwright-based element wait operations (like WaitForElementsCheckedAsync), do not throw immediately when elements are initially absent, as the method is designed to keep polling until timeout to handle dynamic element appearance.
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4072
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs:1075-1083
Timestamp: 2025-01-30T09:07:12.356Z
Learning: In Playwright-based element wait operations (like WaitForElementsVisibleAsync), do not short-circuit when elements are initially absent, as the method is designed to keep polling until timeout to handle dynamic element appearance.
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4072
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs:1084-1091
Timestamp: 2025-01-30T09:08:06.584Z
Learning: In Playwright-based element wait operations (like WaitForElementsInvisibleAsync), use ElementState.Hidden to check for elements that are present in the DOM but not visible, rather than checking if elements are detached/removed from the DOM.
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActWebSmartSyncHandler.cs (3)
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4072
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActWebSmartSyncHandler.cs:358-365
Timestamp: 2025-01-30T07:40:08.756Z
Learning: In the Ginger automation framework, when an element is expected to exist but is not found, the code should throw an exception rather than return null, as the element's presence is a precondition for the operation.
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4072
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActWebSmartSyncHandler.cs:137-150
Timestamp: 2025-01-30T07:38:05.310Z
Learning: In the Ginger automation framework, element existence checks using polling with 100ms delay is an acceptable approach, as confirmed by the maintainers.
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4072
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActWebSmartSyncHandler.cs:36-114
Timestamp: 2025-01-30T07:36:32.448Z
Learning: In the Ginger project's web synchronization code, keeping separate implementations for different synchronization operations is preferred over extracting common polling logic, as it improves readability and makes individual cases easier to understand and modify.
🔇 Additional comments (30)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/IBrowserTab.cs (2)
39-39: Method name consistency looks good.
The asynchronous naming convention (“TitleAsync”) aligns well with the rest of the interface methods (e.g.URLAsync).
103-112: Watch out for consistent timeout units.
All these new wait methods (WaitForUrlMatchAsync,WaitForAlertAsync, etc.) accept afloat timeout. Ensure the implementation in the underlying classes consistently interprets the value as seconds or milliseconds. Consider clarifying in documentation or parameter naming (e.g.,timeoutSecondsvstimeoutMs) to avoid confusion.Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActWebSmartSyncHandler.cs (19)
39-44: Constructor logic looks correct.
The constructor properly initializes the handler’s dependencies (_actWebSmartSync,_browserTab,_elementLocator).
50-130: Default case includes a clear error message.
The switch statement covers all sync operations, ending with a default that sets an error message on theactobject. This is good defensive coding.
135-143: Visibility check approach is straightforward.
No issues found; the method callsGetFirstMatchingElementAsyncand checks visibility viaelement.ToBeVisibleAsync().
148-161: Element existence loop is well-structured.
TheStopwatchloop with a 100ms polling cycle is consistent with your chosen approach for waiting.
166-173: Alert presence check delegates to the browser tab.
Clean usage of_browserTab.WaitForAlertAsync, making the code readable and maintainable.
178-186: Element selected check is clear.
The method succinctly checks if the element is selected within the given timeout.
193-199: Page load check.
Relies onWaitTillLoadedAsync. Straightforward implementation; watch out for the meaning of thetimeOutparameter in the underlying driver.
205-212: Element clickable verification.
Your flow to retrieve the element and test clickability is correct; no issues.
217-230: Text matching approach is robust.
You validate the input for text, throw if missing, then check if it matches. Good guard conditions.
235-254: Attribute matching handles invalid inputs.
ThrowingInvalidDataExceptionfor missing attribute name/value helps ensure correct usage.
259-266: Delegate to IBrowserTab for enabling check.
Uses_browserTab.WaitForElementsEnabledAsync—a nice way to keep logic centralized in the driver layer.
272-284: [Duplicate of prior consolidation suggestions]
The method uses a polling loop with aStopwatchandTask.Delay(100)—similar to other checks. Since you’ve expressed a preference for separate implementations, this is fine.
290-296: Simple invisibility check.
Delegates toWaitForElementsInvisibleAsync. The logic is consistent with other methods.
301-309: Single-element invisibility check.
UsesToBeNotVisibleAsyncfor the first matching element. Straightforward logic.
315-321: Presence of elements verification.
Checks all required elements with_browserTab.WaitForElementsPresenceAsync.
327-333: Multiple element selected check.
Uses_browserTab.WaitForElementsCheckedAsyncto verify all are selected.
338-350: URL match operation is comprehensive.
Ensures the input param is valid and delegates to_browserTab.WaitForUrlMatchAsync.
355-362: Visibility of all elements check.
Delegates to_browserTab.WaitForElementsVisibleAsync; consistent with other checks.
367-376: Throws if element is not found.
This matches the preference stated in the learnings: you throw an exception if the element is a precondition for the operation. Good approach.Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs (9)
36-36: Imports for concurrency look fine.
The additionalusing System.Threading; using System.Threading.Tasks;references are necessary for asynchronous tasks.
246-246: Mouse move check.
CallsThrowIfClosed()to ensure the tab is active before proceeding, which is consistent with the rest of the code.
318-328: CatchesExceptionand returns false.
Returningfalseon any exception helps the caller understand load failure. Just ensure any ephemeral errors also get logged, if needed, for debugging.
1082-1088: Double-check if visibility state is mandatory.
WaitForElementsEnabledAsynccallsWaitForElementsStateAsyncwithElementState.VisibleandElementState.Enabled. If only enabled state is needed, consider removing theVisiblerequirement.
1090-1096: Single state usage is straightforward.
WaitForElementsVisibleAsyncuses onlyElementState.Visible, consistent with the method name.
1098-1105: Hidden state usage matches learnings.
WaitForElementsInvisibleAsyncusesElementState.Hiddento detect elements that remain in the DOM but are not visible. This aligns with the preferred approach in the learnings.
1114-1153: Re-check discovered elements repeatedly.
Elements are fetched once, then polled for check status. If new elements might appear later, they won’t be included. Confirm that this behavior is intended.
1180-1197: Selector generation is flexible.
Supports various locator types. No immediate issues as you’re escaping colons for ID-based queries.
1061-1080:⚠️ Potential issuePotential timeout unit mismatch in WaitForUrlMatchAsync.
Playwright’sPageWaitForURLOptions.Timeoutexpects milliseconds, but it looks like thetimeoutparameter is possibly in seconds. Convert or rename to avoid ~1000x discrepancy.- await _playwrightPage.WaitForURLAsync(urlPattern, new PageWaitForURLOptions { Timeout = timeout }); + await _playwrightPage.WaitForURLAsync(urlPattern, new PageWaitForURLOptions { Timeout = timeout * 1000 });Likely invalid or redundant comment.
2 participants
Thank you for your contribution.
Before submitting this PR, please make sure:
Summary by CodeRabbit
New Features
Improvements
Technical Updates