Skip to content

[rb] route BiDiBridge navigation through the generated Protocol::BrowsingContext#17785

Open
titusfortner wants to merge 1 commit into
SeleniumHQ:trunkfrom
titusfortner:bidi-bridge-navigation
Open

[rb] route BiDiBridge navigation through the generated Protocol::BrowsingContext#17785
titusfortner wants to merge 1 commit into
SeleniumHQ:trunkfrom
titusfortner:bidi-bridge-navigation

Conversation

@titusfortner

@titusfortner titusfortner commented Jul 15, 2026

Copy link
Copy Markdown
Member

🔗 Related Issues

Part of the Ruby BiDi protocol generator rollout (foundation: #17731).

💥 What does this PR do?

Routes the Ruby BiDiBridge's own navigation — get, back, forward, refresh — through the generated BiDi::Protocol::BrowsingContext instead of the hand-written BiDi::BrowsingContext. This makes driver navigation the first production consumer of the generated BiDi protocol layer, rather than it being reachable only through integration specs. No user-facing behavior change.

🔧 Implementation Notes

The generated layer never reads capabilities, so the bridge supplies the two things it omits: the context (current window handle) and the wait readiness mapped from page_load_strategy.

One wire nuance: when page_load_strategy is absent the bridge now defaults to normal (→ complete) rather than the previous wait: null — the generated readiness enum is non-nullable. The common normal/eager/none cases are unchanged.

refresh omits ignoreCache — a plain reload doesn't need it, and Firefox rejects the (correctly-named) param as unimplemented. The old path sent it mis-cased, so Firefox ignored it anyway; behavior is unchanged.

🤖 AI assistance

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

💡 Additional Considerations

  • Deprecating the hand-written BrowsingContext methods is for a future PR.

🔄 Types of changes

  • Bug fix (backwards compatible)

@selenium-ci selenium-ci added the C-rb Ruby Bindings label Jul 15, 2026
@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Route BiDiBridge navigation through generated BiDi::Protocol::BrowsingContext

✨ Enhancement 🐞 Bug fix 🕐 10-20 Minutes

Grey Divider

AI Description

• Switch BiDiBridge navigation commands to the generated BiDi protocol BrowsingContext domain.
• Explicitly pass current window handle and readiness wait derived from page_load_strategy.
• Default missing page_load_strategy to normal/complete readiness to satisfy non-nullable enum.
Diagram

graph TD
  A["Driver navigation API"] --> B["Remote::BiDiBridge"] --> C["BiDi::Protocol::BrowsingContext"] --> D["BiDi::Transport"] --> E["WebSocket connection"] --> F["Browser BiDi endpoint"]
  B --> G["Readiness mapping"] --> C
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Omit `wait` when page_load_strategy is unset
  • ➕ Preserves previous wire behavior of sending wait: null/omitting readiness semantics when not configured
  • ➕ Avoids imposing a default that could theoretically change timing edge-cases
  • ➖ May diverge from typical WebDriver expectations where unset implies normal
  • ➖ Would require plumbing Serialization::UNSET (or similar) from the bridge, coupling bridge logic to protocol serialization details
2. Teach the generated layer to infer `context`/`wait`
  • ➕ Reduces duplication of context/readiness handling at call sites
  • ➕ Centralizes navigation defaults
  • ➖ Couples generated protocol code to driver capabilities/window-handle concepts
  • ➖ Adds complexity to the generator and reduces separation between transport/protocol and bridge layers

Recommendation: Current approach (bridge supplies context and a concrete readiness derived from page_load_strategy) is the cleanest separation of concerns: the generated protocol stays capability-agnostic while the bridge owns WebDriver semantics. The only nuance to double-check is the intentional default to normal when page_load_strategy is absent; if that aligns with the Ruby driver's existing implied default, the change is appropriate.

Files changed (1) +17 / -5

Enhancement (1) +17 / -5
bidi_bridge.rbRoute navigation via generated Protocol::BrowsingContext with readiness mapping +17/-5

Route navigation via generated Protocol::BrowsingContext with readiness mapping

• Adds the generated BiDi protocol require and replaces the hand-written BiDi::BrowsingContext usage with BiDi::Protocol::BrowsingContext over the shared Transport. Updates get/back/forward/refresh to pass explicit 'context' (window_handle) and 'wait' readiness derived from page_load_strategy, defaulting missing strategy to 'normal'/':complete'.

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

@qodo-code-review

qodo-code-review Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

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

Context used
✅ Compliance rules (platform): 19 rules

Grey Divider


Action required

1. BiDiBridge navigation lacks tests 📘 Rule violation ▣ Testability
Description
The PR changes the core navigation implementation (get, go_back, go_forward, refresh) to use
generated BiDi protocol calls, but the change set includes no corresponding test additions/updates
to cover the new argument wiring and readiness mapping. This increases regression risk for a
critical user path (driver navigation).
Code

rb/lib/selenium/webdriver/remote/bidi_bridge.rb[R45-59]

        def get(url)
-          browsing_context.navigate(url)
+          browsing_context.navigate(context: window_handle, url: url, wait: readiness_state)
        end

        def go_back
-          browsing_context.traverse_history(-1)
+          browsing_context.traverse_history(context: window_handle, delta: -1)
        end

        def go_forward
-          browsing_context.traverse_history(1)
+          browsing_context.traverse_history(context: window_handle, delta: 1)
        end

        def refresh
-          browsing_context.reload
+          browsing_context.reload(context: window_handle, ignore_cache: false, wait: readiness_state)
        end
Evidence
PR Compliance ID 389273 requires tests for new functionality/bug fixes. The diff changes BiDiBridge
navigation methods to call generated protocol APIs with new parameters, but no tests were
added/updated in this change set to validate the new behavior.

Rule 389273: Require tests for all new functionality and bug fixes
rb/lib/selenium/webdriver/remote/bidi_bridge.rb[45-59]

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 PR changes navigation to go through `BiDi::Protocol::BrowsingContext`, but there are no accompanying test changes to ensure the new protocol calls and parameters (`context`, `delta`, `ignore_cache`, `wait`) are correctly passed.

## Issue Context
This touches high-traffic public behavior (`driver.navigate.to/back/forward/refresh`) and also introduces a new readiness-state mapping used by the generated protocol layer.

## Fix Focus Areas
- rb/lib/selenium/webdriver/remote/bidi_bridge.rb[45-81]
- rb/spec/integration/selenium/webdriver/navigation_spec.rb[23-60]

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



Remediation recommended

2. BiDiBridge missing @api private ✗ Dismissed 📘 Rule violation ✧ Quality
Description
The newly added internal helper readiness_state (and related READINESS_STATE constant) is not
marked with a YARD @api private tag, even though Selenium::WebDriver::Remote internals are
typically annotated as private API. This can accidentally expose internal implementation details in
generated docs and mislead downstream users about support/compatibility guarantees.
Code

rb/lib/selenium/webdriver/remote/bidi_bridge.rb[R92-94]

+        def readiness_state
+          READINESS_STATE.fetch(capabilities[:page_load_strategy] || 'normal')
        end
Evidence
PR Compliance ID 389237 requires internal Ruby APIs to be marked with # @api private in YARD docs.
The new private method readiness_state was added without any adjacent YARD doc block containing
@api private.

Rule 389237: Mark internal Ruby APIs with @api private in YARD docs
rb/lib/selenium/webdriver/remote/bidi_bridge.rb[88-94]

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::Remote::BiDiBridge` gained new internal implementation (`READINESS_STATE` and `readiness_state`) but the file/method lacks a YARD `# @api private` annotation, which this project uses to mark internal Ruby APIs.

## Issue Context
Other `Selenium::WebDriver::Remote` internals (e.g., `Remote::Driver`) include `# @api private` to avoid documenting/advertising internal APIs.

## Fix Focus Areas
- rb/lib/selenium/webdriver/remote/bidi_bridge.rb[24-35]
- rb/lib/selenium/webdriver/remote/bidi_bridge.rb[88-94]

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


3. readiness_state defaults to normal 📘 Rule violation ≡ Correctness
Description
The new readiness_state logic forces page_load_strategy to default to normal (:complete)
when absent, changing the wire behavior from previously passing a nil readiness (wait: null). This
is a user-visible behavioral default and should be compared against at least one other language
binding and documented if intentionally divergent.
Code

rb/lib/selenium/webdriver/remote/bidi_bridge.rb[R79-80]

+        def readiness_state
+          READINESS_STATE.fetch(capabilities[:page_load_strategy] || 'normal')
Evidence
PR Compliance ID 389265 requires a cross-language comparison when changing user-visible behavior.
The new code defaults absent page_load_strategy to normal, whereas the prior implementation
derived readiness from a hash lookup that could return nil, changing the resulting wait
parameter semantics.

Rule 389265: Compare cross-language bindings when changing user-visible behavior
rb/lib/selenium/webdriver/remote/bidi_bridge.rb[79-80]
rb/lib/selenium/webdriver/bidi/browsing_context.rb[39-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
A default behavior change was introduced for navigation readiness when `page_load_strategy` is absent (`nil`), and the PR should include evidence of cross-binding comparison or document any intentional divergence.

## Issue Context
Previously the handwritten BiDi browsing context used `@readiness = READINESS_STATE[page_load_strategy]`, which yielded `nil` when the capability was absent; the new code uses `fetch(... || 'normal')`, which always forces a non-nil readiness (`:complete`).

## Fix Focus Areas
- rb/lib/selenium/webdriver/remote/bidi_bridge.rb[79-80]
- rb/lib/selenium/webdriver/bidi/browsing_context.rb[39-51]

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


4. Readiness mapping raises KeyError 🐞 Bug ☼ Reliability
Description
BiDiBridge#readiness_state uses READINESS_STATE.fetch(...), so an unexpected non-nil
capabilities[:page_load_strategy] will raise KeyError and break get/refresh before any BiDi command
is sent. Previously, the hand-written BiDi::BrowsingContext used a tolerant lookup
(READINESS_STATE[...]) and would pass wait: nil when the strategy was missing/unknown.
Code

rb/lib/selenium/webdriver/remote/bidi_bridge.rb[R79-80]

+        def readiness_state
+          READINESS_STATE.fetch(capabilities[:page_load_strategy] || 'normal')
Evidence
The new code uses READINESS_STATE.fetch(...), which raises on unknown keys; get and refresh
call readiness_state as part of argument evaluation, so they will error out before calling into
the protocol layer. The previous BiDi browsing context mapping used READINESS_STATE[...] and
passed the resulting (possibly nil) readiness to the wire command instead of raising.

rb/lib/selenium/webdriver/remote/bidi_bridge.rb[30-35]
rb/lib/selenium/webdriver/remote/bidi_bridge.rb[45-59]
rb/lib/selenium/webdriver/remote/bidi_bridge.rb[79-81]
rb/lib/selenium/webdriver/bidi/browsing_context.rb[28-41]
rb/lib/selenium/webdriver/bidi/browsing_context.rb[48-52]
rb/lib/selenium/webdriver/bidi/browsing_context.rb[69-73]
rb/lib/selenium/webdriver/bidi/protocol/browsing_context.rb[47-51]
rb/lib/selenium/webdriver/bidi/protocol/browsing_context.rb[555-559]

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

### Issue description
`BiDiBridge#readiness_state` currently uses `READINESS_STATE.fetch(...)`, which raises `KeyError` for any unexpected non-nil `page_load_strategy`. This makes navigation methods (`get`, `refresh`) fail synchronously before issuing any BiDi command.

### Issue Context
The prior hand-written implementation effectively allowed unknown/missing values by producing `nil` readiness and sending `wait: nil`.

### Fix Focus Areas
- rb/lib/selenium/webdriver/remote/bidi_bridge.rb[30-35]
- rb/lib/selenium/webdriver/remote/bidi_bridge.rb[79-81]

### Suggested fix
Change `readiness_state` to either:
- Provide a safe fallback (e.g., default to `'normal'`/`:complete` when the value is unknown), or
- Raise a clearer Selenium error (e.g., `Error::WebDriverError`) with an explicit message listing allowed values, rather than bubbling up a `KeyError`.

Example approach (fallback):
```ruby
strategy = capabilities[:page_load_strategy] || 'normal'
READINESS_STATE[strategy] || READINESS_STATE['normal']
```

Example approach (clear error):
```ruby
strategy = capabilities[:page_load_strategy] || 'normal'
READINESS_STATE.fetch(strategy) do
 raise Error::WebDriverError, "Unsupported page_load_strategy=#{strategy.inspect} (expected one of: none, eager, normal)"
end
```

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


Grey Divider

Previous review results

Review updated until commit c63ced9

Results up to commit 42247c3


🐞 Bugs (1) 📘 Rule violations (2) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)


Action required
1. BiDiBridge navigation lacks tests 📘 Rule violation ▣ Testability
Description
The PR changes the core navigation implementation (get, go_back, go_forward, refresh) to use
generated BiDi protocol calls, but the change set includes no corresponding test additions/updates
to cover the new argument wiring and readiness mapping. This increases regression risk for a
critical user path (driver navigation).
Code

rb/lib/selenium/webdriver/remote/bidi_bridge.rb[R45-59]

        def get(url)
-          browsing_context.navigate(url)
+          browsing_context.navigate(context: window_handle, url: url, wait: readiness_state)
        end

        def go_back
-          browsing_context.traverse_history(-1)
+          browsing_context.traverse_history(context: window_handle, delta: -1)
        end

        def go_forward
-          browsing_context.traverse_history(1)
+          browsing_context.traverse_history(context: window_handle, delta: 1)
        end

        def refresh
-          browsing_context.reload
+          browsing_context.reload(context: window_handle, ignore_cache: false, wait: readiness_state)
        end
Evidence
PR Compliance ID 389273 requires tests for new functionality/bug fixes. The diff changes BiDiBridge
navigation methods to call generated protocol APIs with new parameters, but no tests were
added/updated in this change set to validate the new behavior.

Rule 389273: Require tests for all new functionality and bug fixes
rb/lib/selenium/webdriver/remote/bidi_bridge.rb[45-59]

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 PR changes navigation to go through `BiDi::Protocol::BrowsingContext`, but there are no accompanying test changes to ensure the new protocol calls and parameters (`context`, `delta`, `ignore_cache`, `wait`) are correctly passed.

## Issue Context
This touches high-traffic public behavior (`driver.navigate.to/back/forward/refresh`) and also introduces a new readiness-state mapping used by the generated protocol layer.

## Fix Focus Areas
- rb/lib/selenium/webdriver/remote/bidi_bridge.rb[45-81]
- rb/spec/integration/selenium/webdriver/navigation_spec.rb[23-60]

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



Remediation recommended
2. readiness_state defaults to normal 📘 Rule violation ≡ Correctness
Description
The new readiness_state logic forces page_load_strategy to default to normal (:complete)
when absent, changing the wire behavior from previously passing a nil readiness (wait: null). This
is a user-visible behavioral default and should be compared against at least one other language
binding and documented if intentionally divergent.
Code

rb/lib/selenium/webdriver/remote/bidi_bridge.rb[R79-80]

+        def readiness_state
+          READINESS_STATE.fetch(capabilities[:page_load_strategy] || 'normal')
Evidence
PR Compliance ID 389265 requires a cross-language comparison when changing user-visible behavior.
The new code defaults absent page_load_strategy to normal, whereas the prior implementation
derived readiness from a hash lookup that could return nil, changing the resulting wait
parameter semantics.

Rule 389265: Compare cross-language bindings when changing user-visible behavior
rb/lib/selenium/webdriver/remote/bidi_bridge.rb[79-80]
rb/lib/selenium/webdriver/bidi/browsing_context.rb[39-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
A default behavior change was introduced for navigation readiness when `page_load_strategy` is absent (`nil`), and the PR should include evidence of cross-binding comparison or document any intentional divergence.

## Issue Context
Previously the handwritten BiDi browsing context used `@readiness = READINESS_STATE[page_load_strategy]`, which yielded `nil` when the capability was absent; the new code uses `fetch(... || 'normal')`, which always forces a non-nil readiness (`:complete`).

## Fix Focus Areas
- rb/lib/selenium/webdriver/remote/bidi_bridge.rb[79-80]
- rb/lib/selenium/webdriver/bidi/browsing_context.rb[39-51]

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


3. Readiness mapping raises KeyError 🐞 Bug ☼ Reliability
Description
BiDiBridge#readiness_state uses READINESS_STATE.fetch(...), so an unexpected non-nil
capabilities[:page_load_strategy] will raise KeyError and break get/refresh before any BiDi command
is sent. Previously, the hand-written BiDi::BrowsingContext used a tolerant lookup
(READINESS_STATE[...]) and would pass wait: nil when the strategy was missing/unknown.
Code

rb/lib/selenium/webdriver/remote/bidi_bridge.rb[R79-80]

+        def readiness_state
+          READINESS_STATE.fetch(capabilities[:page_load_strategy] || 'normal')
Evidence
The new code uses READINESS_STATE.fetch(...), which raises on unknown keys; get and refresh
call readiness_state as part of argument evaluation, so they will error out before calling into
the protocol layer. The previous BiDi browsing context mapping used READINESS_STATE[...] and
passed the resulting (possibly nil) readiness to the wire command instead of raising.

rb/lib/selenium/webdriver/remote/bidi_bridge.rb[30-35]
rb/lib/selenium/webdriver/remote/bidi_bridge.rb[45-59]
rb/lib/selenium/webdriver/remote/bidi_bridge.rb[79-81]
rb/lib/selenium/webdriver/bidi/browsing_context.rb[28-41]
rb/lib/selenium/webdriver/bidi/browsing_context.rb[48-52]
rb/lib/selenium/webdriver/bidi/browsing_context.rb[69-73]
rb/lib/selenium/webdriver/bidi/protocol/browsing_context.rb[47-51]
rb/lib/selenium/webdriver/bidi/protocol/browsing_context.rb[555-559]

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

### Issue description
`BiDiBridge#readiness_state` currently uses `READINESS_STATE.fetch(...)`, which raises `KeyError` for any unexpected non-nil `page_load_strategy`. This makes navigation methods (`get`, `refresh`) fail synchronously before issuing any BiDi command.

### Issue Context
The prior hand-written implementation effectively allowed unknown/missing values by producing `nil` readiness and sending `wait: nil`.

### Fix Focus Areas
- rb/lib/selenium/webdriver/remote/bidi_bridge.rb[30-35]
- rb/lib/selenium/webdriver/remote/bidi_bridge.rb[79-81]

### Suggested fix
Change `readiness_state` to either:
- Provide a safe fallback (e.g., default to `'normal'`/`:complete` when the value is unknown), or
- Raise a clearer Selenium error (e.g., `Error::WebDriverError`) with an explicit message listing allowed values, rather than bubbling up a `KeyError`.

Example approach (fallback):
```ruby
strategy = capabilities[:page_load_strategy] || 'normal'
READINESS_STATE[strategy] || READINESS_STATE['normal']
```

Example approach (clear error):
```ruby
strategy = capabilities[:page_load_strategy] || 'normal'
READINESS_STATE.fetch(strategy) do
 raise Error::WebDriverError, "Unsupported page_load_strategy=#{strategy.inspect} (expected one of: none, eager, normal)"
end
```

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


Results up to commit 66a391d


🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)


Remediation recommended
1. BiDiBridge missing @api private ✗ Dismissed 📘 Rule violation ✧ Quality
Description
The newly added internal helper readiness_state (and related READINESS_STATE constant) is not
marked with a YARD @api private tag, even though Selenium::WebDriver::Remote internals are
typically annotated as private API. This can accidentally expose internal implementation details in
generated docs and mislead downstream users about support/compatibility guarantees.
Code

rb/lib/selenium/webdriver/remote/bidi_bridge.rb[R92-94]

+        def readiness_state
+          READINESS_STATE.fetch(capabilities[:page_load_strategy] || 'normal')
        end
Evidence
PR Compliance ID 389237 requires internal Ruby APIs to be marked with # @api private in YARD docs.
The new private method readiness_state was added without any adjacent YARD doc block containing
@api private.

Rule 389237: Mark internal Ruby APIs with @api private in YARD docs
rb/lib/selenium/webdriver/remote/bidi_bridge.rb[88-94]

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::Remote::BiDiBridge` gained new internal implementation (`READINESS_STATE` and `readiness_state`) but the file/method lacks a YARD `# @api private` annotation, which this project uses to mark internal Ruby APIs.

## Issue Context
Other `Selenium::WebDriver::Remote` internals (e.g., `Remote::Driver`) include `# @api private` to avoid documenting/advertising internal APIs.

## Fix Focus Areas
- rb/lib/selenium/webdriver/remote/bidi_bridge.rb[24-35]
- rb/lib/selenium/webdriver/remote/bidi_bridge.rb[88-94]

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


Qodo Logo

Comment thread rb/lib/selenium/webdriver/remote/bidi_bridge.rb
Comment thread rb/lib/selenium/webdriver/remote/bidi_bridge.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 5bd9a18

@titusfortner
titusfortner force-pushed the bidi-bridge-navigation branch from 5bd9a18 to 66a391d Compare July 15, 2026 22:04
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 66a391d

@titusfortner
titusfortner force-pushed the bidi-bridge-navigation branch from 66a391d to c63ced9 Compare July 15, 2026 23:18
@qodo-code-review

Copy link
Copy Markdown
Contributor

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

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

Labels

C-rb Ruby Bindings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants