Skip to content

Conversation

ywang1110
Copy link
Contributor

@ywang1110 ywang1110 commented Jul 27, 2025

  • Added support for native dropdowns in DoAction when action type is DropDown Extracted handling logic into a private method HandleSelectDropDownAsync
  • Automatically compares current selected value before selecting new option
  • Waits for NetworkIdle after postback-triggered change
  • Logs both skip and change cases
  • Fallbacks to div-based dropdown selection when element is not a User description Added support for native dropdowns in DoAction when action type is DropDown
  • Extracted handling logic into a private method HandleSelectDropDownAsync Automatically compares current selected value before selecting new option Waits for NetworkIdle after postback-triggered change Logs both skip and change cases Fallbacks to div-based dropdown selection when element is not a
Snipaste_2025-07-27_14-08-03

PR Type

Enhancement


Description

  • Added native <select> dropdown support in Playwright WebDriver

  • Extracted select handling logic into dedicated method

  • Added current value comparison to avoid unnecessary changes

  • Implemented network idle waiting after selection


Diagram Walkthrough

flowchart LR
  A["DropDown Action"] --> B["Check Element Type"]
  B --> C["<select> Element?"]
  C -->|Yes| D["HandleSelectDropDownAsync"]
  C -->|No| E["Existing div-based Logic"]
  D --> F["Compare Current Value"]
  F --> G["Select Option if Different"]
  G --> H["Wait for NetworkIdle"]
Loading

File Walkthrough

Relevant files
Enhancement
PlaywrightWebDriver.DoAction.cs
Enhanced dropdown handling with native select support       

src/Plugins/BotSharp.Plugin.WebDriver/Drivers/PlaywrightDriver/PlaywrightWebDriver.DoAction.cs

  • Added tag name detection to differentiate from div dropdowns Extracted HandleSelectDropDownAsync method for native select handling Added current value comparison before selection Implemented network idle waiting after option selection
+40/-7   

Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Exception Handling

The method HandleSelectDropDownAsync can throw exceptions when getting selected text from option:checked if no option is selected, but this exception is not caught in the calling code. This could cause the entire action to fail unexpectedly.

var selectedText = await locator.Locator("option:checked").InnerTextAsync();
Selection Logic

The code selects options by label but doesn't handle cases where the option text might not match exactly due to whitespace, special characters, or when multiple options have similar text. The XPath fallback for div elements is more flexible than the select option matching.

await locator.SelectOptionAsync(new SelectOptionValue
{
    Label = action.Content
});

Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Handle missing selected option safely

The InnerTextAsync() call may throw an exception if no option is currently
selected in the dropdown. Add error handling or use a safer approach to get the
selected value.

src/Plugins/BotSharp.Plugin.WebDriver/Drivers/PlaywrightDriver/PlaywrightWebDriver.DoAction.cs [266]

-var selectedText = await locator.Locator("option:checked").InnerTextAsync();
+var selectedText = await locator.EvaluateAsync<string>("el => el.value") ?? string.Empty;
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that InnerTextAsync can throw an exception if no option is selected, and proposes a safer alternative, which improves the robustness of the code.

Medium
General
Add fallback for option selection

Using Label for option selection may fail if the option text doesn't match
exactly. Consider using Value or implementing fallback logic to try both
approaches.

src/Plugins/BotSharp.Plugin.WebDriver/Drivers/PlaywrightDriver/PlaywrightWebDriver.DoAction.cs [270-273]

-await locator.SelectOptionAsync(new SelectOptionValue
+try
 {
-    Label = action.Content
-});
+    await locator.SelectOptionAsync(new SelectOptionValue { Label = action.Content });
+}
+catch
+{
+    await locator.SelectOptionAsync(new SelectOptionValue { Value = action.Content });
+}
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly points out that selecting by label might fail and proposes a reasonable fallback to select by value, making the selection logic more robust.

Low
  • More

@Oceania2018 Oceania2018 requested a review from Copilot July 28, 2025 15:18
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR enhances the Playwright WebDriver's dropdown handling by adding native <select> element support to the DoAction method. Previously, the implementation only handled div-based dropdowns, but now it can intelligently detect and handle both native HTML select elements and custom div-based dropdowns.

Key changes:

  • Added automatic detection of element type (select vs div) using tag name evaluation
  • Extracted select-specific logic into a dedicated HandleSelectDropDownAsync method
  • Implemented current value comparison to avoid unnecessary selections and network calls

Comment on lines +266 to +267
var selectedText = await locator.Locator("option:checked").InnerTextAsync();

Copy link

Copilot AI Jul 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line will throw an exception if no option is currently selected in the select element. Consider using FirstOrDefaultAsync() or checking if any options are selected before calling InnerTextAsync().

Suggested change
var selectedText = await locator.Locator("option:checked").InnerTextAsync();
var selectedOptionLocator = locator.Locator("option:checked");
var selectedOptionCount = await selectedOptionLocator.CountAsync();
string selectedText = null;
if (selectedOptionCount > 0)
{
selectedText = await selectedOptionLocator.InnerTextAsync();
}
else
{
_logger.LogWarning("No option is currently selected in the dropdown.");
}

Copilot uses AI. Check for mistakes.

Comment on lines +270 to +274
await locator.SelectOptionAsync(new SelectOptionValue
{
Label = action.Content
});

Copy link

Copilot AI Jul 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Selecting by label may fail if the option text doesn't exactly match. Consider also trying to select by value if label selection fails, or use a more robust selection strategy that handles both scenarios.

Suggested change
await locator.SelectOptionAsync(new SelectOptionValue
{
Label = action.Content
});
var selectionResult = await locator.SelectOptionAsync(new SelectOptionValue
{
Label = action.Content
});
if (selectionResult == null || selectionResult.Length == 0)
{
_logger.LogWarning($"Failed to select dropdown option by label: {action.Content}. Attempting to select by value.");
selectionResult = await locator.SelectOptionAsync(new SelectOptionValue
{
Value = action.Content
});
if (selectionResult == null || selectionResult.Length == 0)
{
throw new InvalidOperationException($"Failed to select dropdown option by both label and value: {action.Content}");
}
}

Copilot uses AI. Check for mistakes.

Label = action.Content
});

await page.WaitForLoadStateAsync(LoadState.NetworkIdle);
Copy link

Copilot AI Jul 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waiting for NetworkIdle after every select option change may cause unnecessary delays, especially if the select doesn't trigger network requests. Consider making this configurable or adding a timeout to prevent indefinite waiting.

Copilot uses AI. Check for mistakes.

@Oceania2018 Oceania2018 merged commit 37158d3 into SciSharp:master Jul 28, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants