Skip to content

internal(browser): Add Select Option action to Browser Test Editor#1164

Merged
going-confetti merged 7 commits intomainfrom
internal/select-option-editor
Apr 15, 2026
Merged

internal(browser): Add Select Option action to Browser Test Editor#1164
going-confetti merged 7 commits intomainfrom
internal/select-option-editor

Conversation

@going-confetti
Copy link
Copy Markdown
Collaborator

@going-confetti going-confetti commented Apr 13, 2026

Description

  • Add support for the Select option action
  • ⚠️ This PR doesn't include any action options
grafik

How to Test

  • Add a new Select option action
  • Configure the locator and add some options to be selected
  • Verify that the code is properly generated

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)


Note

Medium Risk
Touches the browser test editor, action-to-test conversion, and codegen IR/emitter types, so regressions could impact generated scripts for select actions. Changes are additive and scoped, with minimal impact outside select-option handling.

Overview
Adds a new Select option action to the Browser Test Editor, including UI to configure one or more option matchers by value, label, or index.

Updates the browser test graph/types and action-to-test conversion to carry structured select values (deduped) and set multiple automatically.

Extends the browser codegen intermediate AST and scenario emitter with a new SelectOptionValueExpression so generated locator.selectOption(...) calls can emit either strings or { value, label, index } objects, and wires this through variable substitution and browser-scenario detection.

Reviewed by Cursor Bugbot for commit 51ba3a7. Bugbot is set up for automated code reviews on this repo. Configure here.

Comment on lines +35 to +40
Select
<SelectOptionValuesForm
values={action.values}
onChange={handleChangeValues}
/>
in
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This order is different from other actions, but feels more natural/easier to read IMO

@going-confetti going-confetti marked this pull request as ready for review April 13, 2026 15:06
@going-confetti going-confetti requested a review from a team as a code owner April 13, 2026 15:06
Copy link
Copy Markdown
Collaborator

@allansson allansson left a comment

Choose a reason for hiding this comment

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

Works great! UI feels good.

I think we should allow empty strings for value because it is valid HTML, but I'm happy to hear counter arguments.

Comment thread src/codegen/browser/test.ts Outdated
Comment on lines +427 to +432
const nonEmpty = action.values.filter(
(v) =>
(v.value !== undefined && v.value !== '') ||
(v.label !== undefined && v.label !== '') ||
v.index !== undefined
)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

value is allowed to be an empty string according to the HTML specification (but label is not).

Suggested change
const nonEmpty = action.values.filter(
(v) =>
(v.value !== undefined && v.value !== '') ||
(v.label !== undefined && v.label !== '') ||
v.index !== undefined
)
const nonEmpty = action.values.filter(
(v) =>
(v.label !== undefined && v.label !== '') ||
v.value !== undefined ||
v.index !== undefined
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Maybe this is a dumb optimization, but we could dedupe the values for the user:

function getKey(v) {
  if (v.value !== undefined) {
    return `value:${v.value}`
  }

  if (v.label !== undefined) {
    return `label:${v.label}`
  }

  return `index:${v.index}`
}

const deduped = Object.values(keyBy(nonEmpty, getKey))

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

No, I think it's a good suggestion 👍

overflow: hidden;
`}
>
<SelectOptions options={values} />
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

As noted by @allansson, empty values are not necessarily invalid, so SelectOptions needs to handle those in a better way. There should probably also be a better distinction between labels, values, and indexes. I'll try to create a separate PR for that change

Image

Copy link
Copy Markdown
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix prepared a fix for the issue found in the latest run.

  • ✅ Fixed: Missing nonEmpty filter before dedup allows invalid codegen
    • I filtered locator.selectOption entries to keep values/indexes and only non-empty labels before keyBy dedup so empty-label objects are no longer emitted in generated code.

Create PR

Or push these changes by commenting:

@cursor push 8e43b1c30c
Preview (8e43b1c30c)
diff --git a/src/codegen/browser/test.ts b/src/codegen/browser/test.ts
--- a/src/codegen/browser/test.ts
+++ b/src/codegen/browser/test.ts
@@ -434,8 +434,14 @@
           },
         }
       case 'locator.selectOption': {
+        const nonEmpty = action.values.filter((v) => {
+          if (v.value !== undefined) return true
+          if (v.index !== undefined) return true
+          return v.label !== undefined && v.label.trim() !== ''
+        })
+
         const deduped = Object.values(
-          keyBy(action.values, (v) => {
+          keyBy(nonEmpty, (v) => {
             if (v.value !== undefined) return `value:${v.value}`
             if (v.label !== undefined) return `label:${v.label}`
             return `index:${v.index}`

You can send follow-ups to the cloud agent here.

Reviewed by Cursor Bugbot for commit 51ba3a7. Configure here.

Comment thread src/codegen/browser/test.ts
Copy link
Copy Markdown
Collaborator

@allansson allansson left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

One small thing: maybe we should render value and label with quotes to make it clear if it's an empty value. We could also maybe use icons to indicate whether its a label, value or index.

But I think it's fine as it is and we can add polish later.

@going-confetti
Copy link
Copy Markdown
Collaborator Author

One small thing: maybe we should render value and label with quotes to make it clear if it's an empty value. We could also maybe use icons to indicate whether its a label, value or index.

But I think it's fine as it is and we can add polish later.

Yes, I did mention this implicitly in a comment above. I'll try to open a PR later today.

@going-confetti going-confetti merged commit 36b41da into main Apr 15, 2026
14 checks passed
@going-confetti going-confetti deleted the internal/select-option-editor branch April 15, 2026 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants