Skip to content

[rb] support WebDriver BiDi on Safari Preview and move #bidi onto Driver#17729

Open
titusfortner wants to merge 5 commits into
trunkfrom
safari-bidi-rb
Open

[rb] support WebDriver BiDi on Safari Preview and move #bidi onto Driver#17729
titusfortner wants to merge 5 commits into
trunkfrom
safari-bidi-rb

Conversation

@titusfortner

@titusfortner titusfortner commented Jun 29, 2026

Copy link
Copy Markdown
Member

🔗 Related Issues

💥 What does this PR do?

  • Adds WebDriver BiDi support for Safari in the Ruby bindings
  • Removes HasBiDi extension and puts #bidi on base Driver; improved error messages for bidi not supported
    • Technically not backwards compatible, but functionally nothing can be done with HasBiDi, verified it doesn't affect Appium
  • GitHub Actions tests runs the BiDi specs against Safari Technology Preview only

🔧 Implementation Notes

  • Safari only exposes a usable BiDi websocket when both webSocketUrl and the vendor safari:experimentalWebSocketUrl capabilities are set, so Safari::Options#enable_bidi! now sets both.
  • DriverExtensions::HasBiDi is removed outright (no deprecation shim). It's a public constant, so flagging in case a shim is preferred.
  • The Options constructor for web_socket_url: true uses enable_bidi! to support different subclass behavior.
  • Failing Safari tests are marked pending

🤖 AI assistance

  • No substantial AI assistance used
  • AI assisted (complete below)
    • Tool(s): Claude Code
    • What was generated: implementation, tests, and Bazel wiring
    • I reviewed all AI output and can explain the change

💡 Additional Considerations

  • This approach will scale to new BiDi implementations as we create them.

🔄 Types of changes

  • New feature (non-breaking change which adds functionality and tests!)

@selenium-ci selenium-ci added C-rb Ruby Bindings B-build Includes scripting, bazel and CI integrations B-devtools Includes everything BiDi or Chrome DevTools related labels Jun 29, 2026
@titusfortner titusfortner requested review from aguspe and p0deje June 29, 2026 18:04
@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

[rb] Add WebDriver BiDi support for Safari Preview; move #bidi to base Driver

✨ Enhancement 🧪 Tests ⚙️ Configuration changes 🕐 20-40 Minutes

Grey Divider

AI Description

• Adds WebDriver BiDi support for Safari Technology Preview by setting both webSocketUrl and
 safari:experimentalWebSocketUrl capabilities in Safari::Options#enable_bidi!.
• Removes DriverExtensions::HasBiDi mixin and promotes #bidi directly onto the base Driver
 class, making BiDi accessible from all drivers without a separate extension.
• Adds validated_socket_url to BiDiBridge that raises a clear error when the remote end returns
 a non-ws:///wss:// value instead of silently failing.
• Marks unsupported BiDi specs as pending_if for Safari/Safari Preview (network domain, user
 contexts, log entries, etc.) rather than skipping them entirely.
• Refactors Bazel rb_integration_test to use a first-class bidi parameter instead of a "bidi"
 string tag, and wires safari-preview as a BiDi-capable browser in tests.bzl.
Diagram

graph TD
    Driver["Driver (base)"] -->|delegates| BiDiBridge["BiDiBridge"]
    BiDiBridge -->|validated_socket_url| BiDi["BiDi WebSocket"]
    SafariOptions["Safari::Options"] -->|enable_bidi! sets both caps| Caps["webSocketUrl + safari:experimentalWebSocketUrl"]
    Caps -->|session capabilities| BiDiBridge
    ChromiumDriver["Chromium/Firefox Driver"] -->|inherits #bidi| Driver
    SafariDriver["Safari::Driver"] -->|inherits #bidi| Driver
    HasBiDi["HasBiDi (deleted)"] -. removed .-> Driver

    subgraph Legend
      direction LR
      _mod["Module/Class"] ~~~ _del["Deleted"] ~~~ _data[("Data")]
    end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Deprecation shim for HasBiDi
  • ➕ Zero breaking change for users who reference the constant
  • ➕ Gives one release cycle to migrate
  • ➖ Extra maintenance burden for a single release
  • ➖ Delays the clean removal
2. Keep HasBiDi as an alias/include on Driver
  • ➕ Fully backward-compatible
  • ➖ Perpetuates the mixin pattern the PR is trying to eliminate
  • ➖ Confusing to have both the mixin and the base method

Recommendation: The PR's approach of promoting #bidi to the base Driver class is the right long-term direction — it avoids per-driver mixin bookkeeping and makes BiDi a first-class citizen. The main consideration worth flagging is the hard removal of DriverExtensions::HasBiDi without a deprecation shim: since it's a public constant, any user code that checks driver.is_a?(DriverExtensions::HasBiDi) or includes it explicitly will break silently. A one-release deprecation shim (empty module with a warning) would be safer for a public API.

Files changed (20) +196 / -46

Enhancement (3) +11 / -1
options.rbCall enable_bidi! from Options constructor when web_socket_url is set +2/-0

Call enable_bidi! from Options constructor when web_socket_url is set

• The base 'Options#initialize' now calls 'enable_bidi!' when 'web_socket_url: true' is passed, allowing subclasses (e.g. Safari) to override 'enable_bidi!' and set additional vendor capabilities automatically.

rb/lib/selenium/webdriver/common/options.rb

options.rbAdd Safari::Options#enable_bidi! to set vendor WebSocket capability +7/-1

Add Safari::Options#enable_bidi! to set vendor WebSocket capability

• Adds 'safari:experimentalWebSocketUrl' to the 'CAPABILITIES' map and overrides 'enable_bidi!' to set it alongside the standard 'webSocketUrl'. This is required for Safari Technology Preview to expose a usable BiDi WebSocket.

rb/lib/selenium/webdriver/safari/options.rb

driver.rbsAdd bidi method signature to Driver RBS type definition +2/-0

Add bidi method signature to Driver RBS type definition

• Adds 'def bidi: () -> BiDi' to the RBS type signature for 'Driver', keeping the type system in sync with the new method.

rb/sig/lib/selenium/webdriver/common/driver.rbs

Bug fix (1) +9 / -2
bidi_bridge.rbValidate webSocketUrl before creating BiDi connection +9/-2

Validate webSocketUrl before creating BiDi connection

• Extracts 'validated_socket_url' private method that checks the capability value is a 'ws://' or 'wss://' string, raising a descriptive 'WebDriverError' if not. This prevents silent failures when a browser returns a boolean or HTTP URL.

rb/lib/selenium/webdriver/remote/bidi_bridge.rb

Refactor (4) +11 / -3
driver.rbPromote #bidi method onto base Driver class +11/-0

Promote #bidi method onto base Driver class

• Adds a '#bidi' method directly to 'WebDriver::Driver' that delegates to '@bridge.bidi'. This replaces the 'DriverExtensions::HasBiDi' mixin that was previously included in individual browser drivers.

rb/lib/selenium/webdriver/common/driver.rb

driver.rbRemove HasBiDi from Chromium driver EXTENSIONS +0/-1

Remove HasBiDi from Chromium driver EXTENSIONS

• Drops 'DriverExtensions::HasBiDi' from the Chromium driver's extension list since '#bidi' is now provided by the base 'Driver' class.

rb/lib/selenium/webdriver/chromium/driver.rb

driver.rbRemove HasBiDi from Firefox driver EXTENSIONS +0/-1

Remove HasBiDi from Firefox driver EXTENSIONS

• Drops 'DriverExtensions::HasBiDi' from the Firefox driver's extension list since '#bidi' is now provided by the base 'Driver' class.

rb/lib/selenium/webdriver/firefox/driver.rb

common.rbRemove require for has_bidi driver extension file +0/-1

Remove require for has_bidi driver extension file

• Removes the 'require' for the now-deleted 'has_bidi' extension file from the common loader.

rb/lib/selenium/webdriver/common.rb

Tests (9) +152 / -27
bidi_bridge_spec.rbAdd unit tests for BiDiBridge webSocketUrl validation +53/-0

Add unit tests for BiDiBridge webSocketUrl validation

• New unit spec for 'BiDiBridge#create_session' that verifies a clear 'WebDriverError' is raised when the remote end returns a boolean 'true' or an HTTP URL instead of a valid 'ws://'/'wss://' URL.

rb/spec/unit/selenium/webdriver/remote/bidi_bridge_spec.rb

options_spec.rbAdd unit tests for Safari::Options#enable_bidi! and web_socket_url constructor +24/-0

Add unit tests for Safari::Options#enable_bidi! and web_socket_url constructor

• Adds specs verifying that 'enable_bidi!' sets both 'web_socket_url' and 'experimental_web_socket_url', that passing 'web_socket_url: true' to the constructor triggers the same, and that 'as_json' serializes both capabilities correctly.

rb/spec/unit/selenium/webdriver/safari/options_spec.rb

bidi_spec.rbMark session.status BiDi specs pending for Safari +4/-2

Mark session.status BiDi specs pending for Safari

• Adds 'pending_if' guards for Safari and Safari Preview on tests that check 'session.status.ready' because Safari always reports 'ready: true'.

rb/spec/integration/selenium/webdriver/bidi_spec.rb

browser_spec.rbMark BiDi browser user-context and window specs pending for Safari +15/-5

Mark BiDi browser user-context and window specs pending for Safari

• Adds 'pending_if' for Safari/Safari Preview on all user-context and 'getClientWindows' specs since Safari does not support those BiDi commands.

rb/spec/integration/selenium/webdriver/bidi/browser_spec.rb

browsing_context_spec.rbMark browsing context specs pending for Safari where unsupported +4/-2

Mark browsing context specs pending for Safari where unsupported

• Adds Safari/Safari Preview to the 'pending_if' list for 'errors on unknown type' and 'activates a browser context' specs.

rb/spec/integration/selenium/webdriver/bidi/browsing_context_spec.rb

network_spec.rbMark all BiDi network intercept specs pending for Safari +34/-12

Mark all BiDi network intercept specs pending for Safari

• Adds 'pending_if' for Safari/Safari Preview on all network intercept specs since Safari does not support the BiDi network domain.

rb/spec/integration/selenium/webdriver/bidi/network_spec.rb

script_spec.rbMark BiDi script/log specs pending for Safari +12/-4

Mark BiDi script/log specs pending for Safari

• Adds 'pending_if' for Safari/Safari Preview on console message and JavaScript error log specs since Safari does not deliver BiDi log entries.

rb/spec/integration/selenium/webdriver/bidi/script_spec.rb

navigation_spec.rbMark navigation back/forward spec pending for Safari in BiDi mode +3/-1

Mark navigation back/forward spec pending for Safari in BiDi mode

• Adds 'pending_if' for Safari/Safari Preview when BiDi is enabled, as Safari does not support 'browsingContext.traverseHistory'.

rb/spec/integration/selenium/webdriver/navigation_spec.rb

network_spec.rbMark Network describe block pending for Safari +3/-1

Mark Network describe block pending for Safari

• Adds a top-level 'pending_if' on the 'Network' describe block for Safari/Safari Preview since Safari does not support the BiDi network domain.

rb/spec/integration/selenium/webdriver/network_spec.rb

Other (3) +13 / -13
tests.bzlRefactor Bazel bidi parameter from tag string to first-class boolean +9/-10

Refactor Bazel bidi parameter from tag string to first-class boolean

• Replaces the '"bidi"' string tag control signal with a proper 'bidi' boolean parameter on 'rb_integration_test'. The macro now iterates all 'BROWSERS' and generates BiDi targets for any browser with 'bidi: True' in its config. Removes the '_CONTROL_TAGS' filtering mechanism.

rb/spec/tests.bzl

BUILD.bazelSwitch BiDi integration test list from tags to bidi=True parameter +1/-1

Switch BiDi integration test list from tags to bidi=True parameter

• Updates the BiDi integration test list comprehension to use 'bidi = True' instead of 'tags = ["bidi"]', matching the new 'rb_integration_test' API.

rb/spec/integration/selenium/webdriver/BUILD.bazel

test_environment.rbEnable BiDi in safari_preview_options when WEBDRIVER_BIDI env var is set +3/-2

Enable BiDi in safari_preview_options when WEBDRIVER_BIDI env var is set

• Updates 'safari_preview_options' to set 'web_socket_url: true' when the 'WEBDRIVER_BIDI' environment variable is present, matching the pattern used by Chrome, Edge, and Firefox option helpers.

rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb

@qodo-code-review

qodo-code-review Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (3) 📜 Skill insights (0)

Context used
✅ Compliance rules (platform): 19 rules

Grey Divider


Action required

1. Leaked BiDi session ✓ Resolved 🐞 Bug ☼ Reliability
Description
Remote::BiDiBridge#create_session calls super to create a remote session and then can raise while
validating webSocketUrl, but Driver#create_bridge/Driver#initialize do not rescue/cleanup, leaving
an orphaned session on the remote end. This can leak resources and cause flaky subsequent runs due
to leftover sessions.
Code

rb/lib/selenium/webdriver/remote/bidi_bridge.rb[R26-29]

        def create_session(capabilities)
          super
-          socket_url = @capabilities[:web_socket_url]
-          @bidi = Selenium::WebDriver::BiDi.new(url: socket_url)
+          @bidi = Selenium::WebDriver::BiDi.new(url: validated_socket_url)
        end
Evidence
BiDiBridge#create_session now validates and may raise after session creation; Driver initialization
does not rescue create_bridge failures, so there is no code path that issues delete_session when
this exception occurs.

rb/lib/selenium/webdriver/remote/bidi_bridge.rb[26-67]
rb/lib/selenium/webdriver/common/driver.rb[71-76]
rb/lib/selenium/webdriver/common/driver.rb[333-338]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`Remote::BiDiBridge#create_session` now raises if the returned `webSocketUrl` is invalid, but this happens **after** `super` has already created a WebDriver session. Because `Driver#create_bridge`/`Driver#initialize` don't rescue around `bridge.create_session`, the newly-created session is never deleted when this error is raised.

### Issue Context
This is a failure-path regression: it only triggers when a remote end returns an invalid `webSocketUrl` (e.g., boolean `true`), but when it does it can leave orphan sessions on the remote server.

### Fix Focus Areas
- rb/lib/selenium/webdriver/remote/bidi_bridge.rb[26-67]
- rb/lib/selenium/webdriver/common/driver.rb[71-76]
- rb/lib/selenium/webdriver/common/driver.rb[333-338]

### Suggested fix
Wrap the post-`super` validation/BiDi initialization in a rescue block that deletes the newly-created session (and closes the HTTP client) before re-raising. For example:
- In `BiDiBridge#create_session`, after `super`, attempt to compute `socket_url = validated_socket_url`.
- If validation fails, call `execute(:delete_session)` (guarded by presence of a session id) and `http.close` (rescuing the same QUIT_ERRORS patterns), then re-raise.
Optionally also harden `BiDiBridge#quit` to handle `@bidi` being nil (e.g., `@bidi&.close`) so cleanup paths don’t crash when initialization fails early.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. experimental_web_socket_url missing RBS ✗ Dismissed 📘 Rule violation ≡ Correctness
Description
Adding experimental_web_socket_url to Safari::Options::CAPABILITIES creates new public accessor
methods via Options.set_capabilities, but the corresponding .rbs file does not declare them.
This can break type checking and makes the published Ruby signatures drift from the actual public
API.
Code

rb/lib/selenium/webdriver/safari/options.rb[R27-29]

        CAPABILITIES = {automatic_inspection: 'safari:automaticInspection',
-                        automatic_profiling: 'safari:automaticProfiling'}.freeze
+                        automatic_profiling: 'safari:automaticProfiling',
+                        experimental_web_socket_url: 'safari:experimentalWebSocketUrl'}.freeze
Evidence
The diff adds experimental_web_socket_url to Safari::Options::CAPABILITIES, which
Options.set_capabilities turns into public methods, but
rb/sig/lib/selenium/webdriver/safari/options.rbs does not include experimental_web_socket_url /
experimental_web_socket_url= declarations, violating the requirement to keep .rbs signatures
synchronized with public API changes.

Rule 389239: Keep Ruby .rbs signatures in sync with public API changes
rb/lib/selenium/webdriver/safari/options.rb[26-30]
rb/lib/selenium/webdriver/common/options.rb[54-65]
rb/sig/lib/selenium/webdriver/safari/options.rbs[19-36]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`Selenium::WebDriver::Safari::Options` now defines a new capability alias `experimental_web_socket_url`, which generates public getter/setter methods, but `rb/sig/lib/selenium/webdriver/safari/options.rbs` was not updated to include these new public methods.

## Issue Context
`Selenium::WebDriver::Options.set_capabilities` defines methods for each key in `self::CAPABILITIES`. By adding `experimental_web_socket_url` to Safari's `CAPABILITIES`, the Ruby surface area changes and the `.rbs` signatures should reflect it.

## Fix Focus Areas
- rb/lib/selenium/webdriver/safari/options.rb[26-30]
- rb/lib/selenium/webdriver/common/options.rb[54-65]
- rb/sig/lib/selenium/webdriver/safari/options.rbs[19-36]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Bazel tag filter matches none ✗ Dismissed 🐞 Bug ≡ Correctness
Description
The macOS os-tests-full job sets --test_tag_filters to safari-local,safari-preview-bidi, which
requires both tags on the same Bazel test target and will therefore select neither Safari classic
local tests nor Safari Preview BiDi tests. This can make the macOS full job run zero tests and
silently drop Safari CI coverage.
Code

.github/workflows/ci-ruby.yml[93]

+            tag_filters: safari-local,safari-preview-bidi
Evidence
The workflow passes matrix.tag_filters directly to Bazel as --test_tag_filters. The Bazel rule
generator applies "{browser}-local" tags to classic local targets and "{browser}-bidi" tags to
BiDi targets, so safari-local and safari-preview-bidi are attached to different targets and
won’t co-occur, making the comma-separated filter match nothing.

.github/workflows/ci-ruby.yml[82-109]
rb/spec/tests.bzl[211-221]
rb/spec/tests.bzl[251-260]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The macOS include sets `tag_filters: safari-local,safari-preview-bidi`, but Bazel `--test_tag_filters` uses comma as an AND, so no target will match both tags.

### Issue Context
In `rb/spec/tests.bzl`, classic local targets get a `"{browser}-local"` tag, while BiDi targets get a `"{browser}-bidi"` tag. For Safari this produces `safari-local` on classic Safari targets and `safari-preview-bidi` on Safari Technology Preview BiDi targets, with no overlap.

### Fix Focus Areas
- .github/workflows/ci-ruby.yml[88-109]

Suggested approach:
- Change the matrix `include` to create **two** macOS entries:
 - one with `tag_filters: safari-local`
 - one with `tag_filters: safari-preview-bidi`

(Alternatively, run two separate `bazel test` commands in the job, one per tag filter.)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


View more (5)
4. warn uses symbol id: ✗ Dismissed 📘 Rule violation ✧ Quality
Description
WebDriver.logger.warn is called with id: :safari_bidi (a Symbol) rather than a non-empty string
identifier as required. This can break standardized log/deprecation tooling that expects string IDs
for deduping and filtering.
Code

rb/lib/selenium/webdriver/safari/options.rb[R51-52]

+          WebDriver.logger.warn("Safari's WebDriver BiDi support is experimental and may be incomplete",
+                                id: :safari_bidi)
Evidence
PR Compliance ID 389238 requires all WebDriver.logger.warn calls to include an id keyword
argument as a non-empty string (or named constant). The new enable_bidi! warning uses `id:
:safari_bidi` (Symbol), and the unit spec asserts the same Symbol.

Rule 389238: Require id keyword for WebDriver.logger.warn calls
rb/lib/selenium/webdriver/safari/options.rb[51-52]
rb/spec/unit/selenium/webdriver/safari/options_spec.rb[77-82]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`WebDriver.logger.warn` must include an `id:` keyword that is a non-empty string identifier, but the new code uses a Symbol (`:safari_bidi`).

## Issue Context
This warning is part of the new Safari BiDi support messaging. The unit test also asserts the Symbol value, so it should be updated alongside the implementation.

## Fix Focus Areas
- rb/lib/selenium/webdriver/safari/options.rb[51-52]
- rb/spec/unit/selenium/webdriver/safari/options_spec.rb[77-82]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


5. HasBiDi removed without deprecation 📘 Rule violation ☼ Reliability
Description
This change set removes the public API entry point Selenium::WebDriver::DriverExtensions::HasBiDi
without a prior deprecation period or in-code migration guidance, introducing a
backward-incompatible break for downstream consumers. Existing client code that includes or
references this constant/module may fail at load time or when mixing in the extension module even
though a new Selenium::WebDriver::Driver#bidi API is being added.
Code

rb/lib/selenium/webdriver/common/driver_extensions/has_bidi.rb[L20-33]

-module Selenium
-  module WebDriver
-    module DriverExtensions
-      module HasBiDi
-        #
-        # Retrieves WebDriver BiDi connection.
-        #
-        # @return [BiDi]
-        #
-
-        def bidi
-          @bridge.bidi
-        end
-      end # HasBiDi
Evidence
The stated policy requires deprecating public APIs (with guidance) before removal, but the cited
changes introduce the new Driver#bidi surface while eliminating the prior extension-based
exposure: the extension is no longer required/loaded via common.rb, and it is no longer listed in
the driver extensions mechanism, meaning the public constant/module is effectively deleted rather
than deprecated. This combination of adding Driver#bidi and removing DriverExtensions::HasBiDi
demonstrates a direct breaking change with no deprecation mechanism visible in the impacted code
paths.

Rule 389271: Deprecate public APIs with guidance before removal
Rule 389266: Maintain backward-compatible public API and ABI
rb/lib/selenium/webdriver/common/driver.rb[93-103]
rb/lib/selenium/webdriver/common.rb[76-82]
rb/lib/selenium/webdriver/chromium/driver.rb[28-33]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`Selenium::WebDriver::DriverExtensions::HasBiDi` appears to have been removed outright and replaced by `Selenium::WebDriver::Driver#bidi`, which creates a backward-incompatible public API break. Reintroduce the old extension entry point as a deprecated shim (with clear migration guidance) so existing users do not break immediately.

## Issue Context
The new preferred API surface is `Selenium::WebDriver::Driver#bidi`, but existing client code may still include or reference `Selenium::WebDriver::DriverExtensions::HasBiDi`. Removing the extension module/constant, its require path, and its inclusion in the extensions listing can cause failures at load time or when mixing in the extension module; policy requires a deprecation window with explicit guidance before deletion.

## Fix Focus Areas
- rb/lib/selenium/webdriver/common/driver_extensions/has_bidi.rb[20-33]
- rb/lib/selenium/webdriver/common.rb[76-82]
- rb/lib/selenium/webdriver/chromium/driver.rb[28-33]
- rb/lib/selenium/webdriver/common/driver.rb[93-103]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


6. Options#initialize missing YARD 📘 Rule violation ✧ Quality
Description
Selenium::WebDriver::Options#initialize is modified to enable BiDi based on web_socket_url but
has no required YARD documentation for this public API method. This hides a user-visible behavior
change from generated docs.
Code

rb/lib/selenium/webdriver/common/options.rb[R75-80]

        @options = opts
        @options[:browser_name] = self.class::BROWSER
+
+        enable_bidi! if @options[:web_socket_url]
      end
Evidence
The checklist requires YARD tags for public API methods that are new or modified.
Options#initialize is modified in this PR, but there is no YARD doc block above the method
describing the parameters and new behavior.

Rule 389240: Document public API methods with YARD tags
rb/lib/selenium/webdriver/common/options.rb[71-80]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Modified public API methods must be documented with YARD tags, including behavior changes and parameters.

## Issue Context
`Options#initialize(**opts)` now calls `enable_bidi!` when `web_socket_url` is set, which is user-visible but undocumented.

## Fix Focus Areas
- rb/lib/selenium/webdriver/common/options.rb[71-80]

## Suggested approach
Add a YARD doc block above `def initialize(**opts)` including:
- Short description
- `@param` tags describing `opts` and relevant keys (including `web_socket_url` / `bidi` behavior)
- `@return` (if your project documents constructors)
- Any relevant `@raise` (if applicable)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


7. Safari BiDi breaks back/forward 🐞 Bug ≡ Correctness
Description
Enabling BiDi for Safari sessions causes the session to use Remote::BiDiBridge, whose
go_back/go_forward are implemented via browsingContext.traverse_history; Safari does not support
that BiDi command, so Navigation#back/#forward fail whenever BiDi is enabled for Safari.
Code

rb/lib/selenium/webdriver/safari/options.rb[R48-51]

+        def enable_bidi!
+          super
+          @options[:experimental_web_socket_url] = true
+        end
Evidence
Safari BiDi is enabled by setting webSocketUrl plus Safari’s experimental capability; sessions with
webSocketUrl requested are created with Remote::BiDiBridge. Navigation#back/#forward delegate to
bridge.go_back/go_forward, and BiDiBridge implements those via browsingContext.traverse_history,
which the updated integration spec explicitly notes Safari does not support in BiDi mode.

rb/lib/selenium/webdriver/safari/options.rb[48-51]
rb/lib/selenium/webdriver/common/options.rb[71-80]
rb/lib/selenium/webdriver/common/driver.rb[333-337]
rb/lib/selenium/webdriver/common/navigation.rb[31-49]
rb/lib/selenium/webdriver/remote/bidi_bridge.rb[35-41]
rb/spec/integration/selenium/webdriver/navigation_spec.rb[23-49]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
With BiDi enabled, `Remote::BiDiBridge` overrides `#go_back`/`#go_forward` to call BiDi `browsingContext.traverse_history`. Safari BiDi does not implement `traverseHistory`, so `driver.navigate.back` / `driver.navigate.forward` break for Safari BiDi sessions.

### Issue Context
This PR makes it easy to enable BiDi for Safari by setting both `webSocketUrl` and `safari:experimentalWebSocketUrl`. Once `webSocketUrl` is requested, the Ruby driver selects `Remote::BiDiBridge` for the session, meaning core navigation methods are routed through BiDi.

### Fix Focus Areas
- Implement a safe fallback in `Remote::BiDiBridge#go_back` and `#go_forward`: try BiDi traversal, but if the remote end reports an unknown/unsupported command (e.g., `UnknownCommandError` / `UnknownMethodError` / `UnsupportedOperationError`), call the classic WebDriver HTTP command via `super` (or `execute :back` / `execute :forward`).
- Optionally restrict fallback to Safari only (based on `@capabilities[:browser_name]`) if you want to preserve strict BiDi coverage for other browsers.
- Update/adjust the Safari BiDi pending in the navigation integration spec if the fallback makes the test pass.

Fix Focus Areas (code references)
- rb/lib/selenium/webdriver/remote/bidi_bridge.rb[31-41]
- rb/spec/integration/selenium/webdriver/navigation_spec.rb[23-49]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


8. enable_bidi! missing YARD docs ✓ Resolved 📘 Rule violation ✧ Quality
Description
Safari::Options#enable_bidi! is a newly added public method but lacks the required YARD doc block
with tags. This makes the public API change harder to discover and use correctly.
Code

rb/lib/selenium/webdriver/safari/options.rb[R48-51]

+        def enable_bidi!
+          super
+          @options[:experimental_web_socket_url] = true
+        end
Evidence
The checklist requires public API methods to have YARD documentation with tags. The added
enable_bidi! method has no YARD doc block immediately above it.

Rule 389240: Document public API methods with YARD tags
rb/lib/selenium/webdriver/safari/options.rb[48-51]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
New/modified public API methods must be documented with YARD tags.

## Issue Context
`Safari::Options#enable_bidi!` was added and changes Safari capability behavior, but it has no doc block.

## Fix Focus Areas
- rb/lib/selenium/webdriver/safari/options.rb[48-51]

## Suggested approach
Add a YARD doc block above `def enable_bidi!` including:
- A short description line
- `@return` (likely `[void]` or `[self]` depending on convention)
- Any relevant `@raise` (if applicable)
- Any behavioral notes (e.g., also enables `safari:experimentalWebSocketUrl`)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

9. Safari preview classic disabled ✗ Dismissed 🐞 Bug ≡ Correctness
Description
The BROWSERS entry for safari-preview sets classic: False, which prevents rb_integration_test
from generating any classic targets for Safari Technology Preview. This silently drops
safari-preview coverage and can break expected Bazel targets under suites that explicitly include
safari-preview (e.g., the Safari integration suite).
Code

rb/spec/tests.bzl[R163-164]

+        "classic": False,
+        "bidi": True,
Evidence
rb_integration_test computes generate_classic from the BROWSERS entry and only emits classic
targets when both classic and generate_classic are true; since safari-preview is set to
classic: False, no classic targets will be produced. This is problematic because the Safari
integration BUILD explicitly requests safari-preview as a browser, implying classic targets for it
are expected.

rb/spec/tests.bzl[156-165]
rb/spec/tests.bzl[200-212]
rb/spec/integration/selenium/webdriver/safari/BUILD.bazel[4-11]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`safari-preview` is configured with `classic: False` in `BROWSERS`, so `rb_integration_test` will never generate classic test targets for Safari Technology Preview. This contradicts BUILD files that explicitly include `"safari-preview"` and unintentionally removes coverage / breaks previously-generated target names.

### Issue Context
`rb_integration_test` gates classic target generation on both the macro arg `classic` and per-browser `generate_classic = BROWSERS[browser].get("classic", True)`. With `classic: False` on `safari-preview`, even explicit `browsers = ["safari", "safari-preview"]` suites won’t emit any `*-safari-preview` classic targets.

### Fix Focus Areas
- rb/spec/tests.bzl[156-165]
- rb/spec/tests.bzl[200-212]

### Suggested fix
- Remove (or set to `True`) the `"classic": False` flag for `safari-preview` in `BROWSERS`.
- If the intent is to avoid running the *general* classic suite on Safari Preview, exclude `safari-preview` at the call sites that shouldn’t generate it (e.g., by passing an explicit `browsers = [...]` list for those suites), rather than disabling classic generation globally for the browser.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


10. allow(http).to receive used 📘 Rule violation ▣ Testability
Description
The new unit test uses RSpec mocking (allow(...).to receive(...)) to simulate an external/remote
HTTP interaction without a contract-verified stub. This violates the requirement to avoid mocks in
favor of real or contract-driven integrations.
Code

rb/spec/unit/selenium/webdriver/remote/bidi_bridge_spec.rb[R29-33]

+        def stub_new_session(web_socket_url)
+          capabilities = {'browserName' => 'safari', 'webSocketUrl' => web_socket_url}
+          allow(http).to receive(:request)
+            .and_return('value' => {'sessionId' => 'foo', 'capabilities' => capabilities})
+        end
Evidence
The checklist disallows use of mocking frameworks for external/service interactions unless backed by
a contract. The added test stubs the HTTP client's request method using RSpec mocking to emulate a
remote end response.

Rule 389270: Avoid mocks in tests; use real or contract-driven integrations
rb/spec/unit/selenium/webdriver/remote/bidi_bridge_spec.rb[29-33]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Tests should avoid mocking frameworks for external API/service behavior unless the mock is generated/validated by a machine-checked contract.

## Issue Context
`BiDiBridge` session creation is being tested by mocking `http.request` via RSpec (`allow(...).to receive`).

## Fix Focus Areas
- rb/spec/unit/selenium/webdriver/remote/bidi_bridge_spec.rb[29-33]

## Suggested approach
- Replace the RSpec mock with a small in-memory fake HTTP client object that implements the same interface (`#request`) and returns the desired response.
- If a contract-driven stub exists for the WebDriver protocol responses, use that instead and reference the contract source in the test.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread rb/lib/selenium/webdriver/safari/options.rb
Comment thread rb/lib/selenium/webdriver/common/options.rb
Comment thread rb/lib/selenium/webdriver/safari/options.rb
Comment thread rb/lib/selenium/webdriver/common/driver_extensions/has_bidi.rb
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 1eac820

Comment thread rb/lib/selenium/webdriver/safari/options.rb
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 3aad87f

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 0bcc302

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds Safari Technology Preview coverage for WebDriver BiDi in the Ruby bindings by enabling Safari’s vendor BiDi capability, moving #bidi onto the base Driver, and wiring Bazel/RSpec so BiDi specs can run (with Safari-specific pendings where domains are unsupported).

Changes:

  • Enable Safari BiDi by setting both webSocketUrl and safari:experimentalWebSocketUrl, with an “experimental” warning.
  • Expose Driver#bidi directly (and validate returned webSocketUrl for remote BiDi sessions).
  • Update Bazel/RSpec integration to generate/run BiDi variants and mark known Safari gaps as pending.

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
rb/spec/unit/selenium/webdriver/safari/options_spec.rb Adds unit coverage for Safari BiDi capability enablement and warning behavior.
rb/spec/unit/selenium/webdriver/remote/bidi_bridge_spec.rb New unit tests for clearer failures when remote webSocketUrl is invalid.
rb/spec/tests.bzl Adds a bidi flag and Safari Preview BiDi metadata for generating BiDi test targets.
rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb Enables web_socket_url automatically for Safari Preview when WEBDRIVER_BIDI is set.
rb/spec/integration/selenium/webdriver/network_spec.rb Marks Network specs pending on Safari where the BiDi network domain is unsupported.
rb/spec/integration/selenium/webdriver/navigation_spec.rb Marks BiDi traverseHistory-dependent navigation test pending on Safari.
rb/spec/integration/selenium/webdriver/BUILD.bazel Switches BiDi wiring from a tag to the new bidi = True flag.
rb/spec/integration/selenium/webdriver/bidi/script_spec.rb Marks console log-entry BiDi tests pending on Safari due to missing event delivery.
rb/spec/integration/selenium/webdriver/bidi/network_spec.rb Marks BiDi network-domain tests pending on Safari and updates combined pending conditions.
rb/spec/integration/selenium/webdriver/bidi/browsing_context_spec.rb Expands pending coverage for Safari-specific BiDi behavior differences.
rb/spec/integration/selenium/webdriver/bidi/browser_spec.rb Marks user-context and client-window BiDi tests pending on Safari.
rb/spec/integration/selenium/webdriver/bidi_spec.rb Marks session.status-dependent tests pending on Safari due to differing readiness reporting.
rb/sig/lib/selenium/webdriver/remote/bidi_bridge.rbs Adds RBS signature for validated_socket_url.
rb/sig/lib/selenium/webdriver/common/driver.rbs Adds RBS signature for Driver#bidi.
rb/sig/lib/selenium/webdriver/common/driver_extensions/has_bidi.rbs Removes the HasBiDi extension signature.
rb/lib/selenium/webdriver/safari/options.rb Implements Safari-specific BiDi enablement (vendor capability + warning).
rb/lib/selenium/webdriver/remote/bidi_bridge.rb Validates remote webSocketUrl and raises a clearer error for invalid values.
rb/lib/selenium/webdriver/firefox/driver.rb Removes HasBiDi from Firefox driver extension list now that Driver#bidi exists.
rb/lib/selenium/webdriver/common/options.rb Auto-calls enable_bidi! during options initialization when web_socket_url is set.
rb/lib/selenium/webdriver/common/driver.rb Adds Driver#bidi delegating to the underlying bridge.
rb/lib/selenium/webdriver/common/driver_extensions/has_bidi.rb Removes the HasBiDi extension implementation.
rb/lib/selenium/webdriver/common.rb Drops the require for the removed HasBiDi extension file.
rb/lib/selenium/webdriver/chromium/driver.rb Removes HasBiDi from Chromium driver extension list now that Driver#bidi exists.

Comment thread rb/spec/unit/selenium/webdriver/safari/options_spec.rb
Comment thread rb/lib/selenium/webdriver/common.rb
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 5b27335

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 8f5d12e

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit d2787e5

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated 2 comments.

Comment thread rb/lib/selenium/webdriver/common.rb
Comment thread rb/spec/tests.bzl Outdated
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 417ee5a

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit b781119

Comment thread rb/spec/tests.bzl
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 8303927

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 29 out of 29 changed files in this pull request and generated 3 comments.

Comment thread rb/lib/selenium/webdriver/common.rb
Comment thread rb/spec/integration/selenium/webdriver/bidi/browser_spec.rb Outdated
Comment thread rb/spec/integration/selenium/webdriver/bidi/browser_spec.rb Outdated
Comment thread .github/workflows/ci-ruby.yml
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit e032a44

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 584be91

Comment thread rb/lib/selenium/webdriver/safari/options.rb
Comment thread rb/lib/selenium/webdriver/remote/bidi_bridge.rb
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit fe95e14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 30 out of 30 changed files in this pull request and generated 2 comments.

Comment thread rb/spec/tests.bzl
Comment thread rb/lib/selenium/webdriver/common.rb
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 5aa169f

@diemol diemol left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If we are sure that removing HasBiDi won't break anything because in theory is not being used, then we should go for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-build Includes scripting, bazel and CI integrations B-devtools Includes everything BiDi or Chrome DevTools related C-rb Ruby Bindings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants